Merge pull request #716 from TheBlueMatt/2020-09-fee-tests

(Fix and) Test that txn pay at least a minimum relay fee in functional tests

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -1054,8 +1054,30 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	}
 
 	#[inline]
-	fn get_closing_transaction_weight(a_scriptpubkey: &Script, b_scriptpubkey: &Script) -> u64 {
-		(4 + 1 + 36 + 4 + 1 + 1 + 2*(8+1) + 4 + a_scriptpubkey.len() as u64 + b_scriptpubkey.len() as u64)*4 + 2 + 1 + 1 + 2*(1 + 72)
+	fn get_closing_transaction_weight(&self, a_scriptpubkey: Option<&Script>, b_scriptpubkey: Option<&Script>) -> u64 {
+		let mut ret =
+		(4 +                                           // version
+		 1 +                                           // input count
+		 36 +                                          // prevout
+		 1 +                                           // script length (0)
+		 4 +                                           // sequence
+		 1 +                                           // output count
+		 4                                             // lock time
+		 )*4 +                                         // * 4 for non-witness parts
+		2 +                                            // witness marker and flag
+		1 +                                            // witness element count
+		4 +                                            // 4 element lengths (2 sigs, multisig dummy, and witness script)
+		self.get_funding_redeemscript().len() as u64 + // funding witness script
+		2*(1 + 71);                                    // two signatures + sighash type flags
+		if let Some(spk) = a_scriptpubkey {
+			ret += ((8+1) +                            // output values and script length
+				spk.len() as u64) * 4;                 // scriptpubkey and witness multiplier
+		}
+		if let Some(spk) = b_scriptpubkey {
+			ret += ((8+1) +                            // output values and script length
+				spk.len() as u64) * 4;                 // scriptpubkey and witness multiplier
+		}
+		ret
 	}
 
 	#[inline]
@@ -2880,13 +2902,14 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		if self.feerate_per_kw > proposed_feerate {
 			proposed_feerate = self.feerate_per_kw;
 		}
-		let tx_weight = Self::get_closing_transaction_weight(&self.get_closing_scriptpubkey(), self.counterparty_shutdown_scriptpubkey.as_ref().unwrap());
+		let tx_weight = self.get_closing_transaction_weight(Some(&self.get_closing_scriptpubkey()), Some(self.counterparty_shutdown_scriptpubkey.as_ref().unwrap()));
 		let proposed_total_fee_satoshis = proposed_feerate as u64 * tx_weight / 1000;
 
 		let (closing_tx, total_fee_satoshis) = self.build_closing_transaction(proposed_total_fee_satoshis, false);
 		let sig = self.holder_keys
 			.sign_closing_transaction(&closing_tx, &self.secp_ctx)
 			.ok();
+		assert!(closing_tx.get_weight() as u64 <= tx_weight);
 		if sig.is_none() { return None; }
 
 		self.last_sent_closing_fee = Some((proposed_feerate, total_fee_satoshis, sig.clone().unwrap()));
@@ -3007,7 +3030,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		if !self.pending_inbound_htlcs.is_empty() || !self.pending_outbound_htlcs.is_empty() {
 			return Err(ChannelError::Close("Remote end sent us a closing_signed while there were still pending HTLCs".to_owned()));
 		}
-		if msg.fee_satoshis > 21000000 * 10000000 { //this is required to stop potential overflow in build_closing_transaction
+		if msg.fee_satoshis > 21_000_000 * 1_0000_0000 { //this is required to stop potential overflow in build_closing_transaction
 			return Err(ChannelError::Close("Remote tried to send us a closing tx with > 21 million BTC fee".to_owned()));
 		}
 
@@ -3031,9 +3054,14 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			},
 		};
 
+		let closing_tx_max_weight = self.get_closing_transaction_weight(
+			if let Some(oup) = closing_tx.output.get(0) { Some(&oup.script_pubkey) } else { None },
+			if let Some(oup) = closing_tx.output.get(1) { Some(&oup.script_pubkey) } else { None });
 		if let Some((_, last_fee, sig)) = self.last_sent_closing_fee {
 			if last_fee == msg.fee_satoshis {
 				self.build_signed_closing_transaction(&mut closing_tx, &msg.signature, &sig);
+				assert!(closing_tx.get_weight() as u64 <= closing_tx_max_weight);
+				debug_assert!(closing_tx.get_weight() as u64 >= closing_tx_max_weight - 2);
 				self.channel_state = ChannelState::ShutdownComplete as u32;
 				self.update_time_counter += 1;
 				return Ok((None, Some(closing_tx)));
@@ -3042,11 +3070,12 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 		macro_rules! propose_new_feerate {
 			($new_feerate: expr) => {
-				let closing_tx_max_weight = Self::get_closing_transaction_weight(&self.get_closing_scriptpubkey(), self.counterparty_shutdown_scriptpubkey.as_ref().unwrap());
-				let (closing_tx, used_total_fee) = self.build_closing_transaction($new_feerate as u64 * closing_tx_max_weight / 1000, false);
+				let tx_weight = self.get_closing_transaction_weight(Some(&self.get_closing_scriptpubkey()), Some(self.counterparty_shutdown_scriptpubkey.as_ref().unwrap()));
+				let (closing_tx, used_total_fee) = self.build_closing_transaction($new_feerate as u64 * tx_weight / 1000, false);
 				let sig = self.holder_keys
 					.sign_closing_transaction(&closing_tx, &self.secp_ctx)
 					.map_err(|_| ChannelError::Close("External signer refused to sign closing transaction".to_owned()))?;
+				assert!(closing_tx.get_weight() as u64 <= tx_weight);
 				self.last_sent_closing_fee = Some(($new_feerate, used_total_fee, sig.clone()));
 				return Ok((Some(msgs::ClosingSigned {
 					channel_id: self.channel_id,
@@ -3056,10 +3085,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			}
 		}
 
-		let proposed_sat_per_kw = msg.fee_satoshis  * 1000 / closing_tx.get_weight() as u64;
+		let mut min_feerate = 253;
 		if self.channel_outbound {
 			let max_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);
-			if (proposed_sat_per_kw as u32) > max_feerate {
+			if (msg.fee_satoshis as u64) > max_feerate as u64 * closing_tx_max_weight / 1000 {
 				if let Some((last_feerate, _, _)) = self.last_sent_closing_fee {
 					if max_feerate <= last_feerate {
 						return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wanted something higher ({}) than our Normal feerate ({})", last_feerate, max_feerate)));
@@ -3068,21 +3097,23 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 				propose_new_feerate!(max_feerate);
 			}
 		} else {
-			let min_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
-			if (proposed_sat_per_kw as u32) < min_feerate {
-				if let Some((last_feerate, _, _)) = self.last_sent_closing_fee {
-					if min_feerate >= last_feerate {
-						return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wanted something lower ({}) than our Background feerate ({}).", last_feerate, min_feerate)));
-					}
+			min_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
+		}
+		if (msg.fee_satoshis as u64) < min_feerate as u64 * closing_tx_max_weight / 1000 {
+			if let Some((last_feerate, _, _)) = self.last_sent_closing_fee {
+				if min_feerate >= last_feerate {
+					return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wanted something lower ({}) than our Background feerate ({}).", last_feerate, min_feerate)));
 				}
-				propose_new_feerate!(min_feerate);
 			}
+			propose_new_feerate!(min_feerate);
 		}
 
 		let sig = self.holder_keys
 			.sign_closing_transaction(&closing_tx, &self.secp_ctx)
 			.map_err(|_| ChannelError::Close("External signer refused to sign closing transaction".to_owned()))?;
 		self.build_signed_closing_transaction(&mut closing_tx, &msg.signature, &sig);
+		assert!(closing_tx.get_weight() as u64 <= closing_tx_max_weight);
+		debug_assert!(closing_tx.get_weight() as u64 >= closing_tx_max_weight - 2);
 
 		self.channel_state = ChannelState::ShutdownComplete as u32;
 		self.update_time_counter += 1;

lightning/src/ln/functional_test_utils.rs

@@ -505,14 +505,26 @@ pub fn create_announced_chan_between_nodes_with_value<'a, 'b, 'c, 'd>(nodes: &'a
 macro_rules! check_spends {
 	($tx: expr, $($spends_txn: expr),*) => {
 		{
-			$tx.verify(|out_point| {
+			let get_output = |out_point: &bitcoin::blockdata::transaction::OutPoint| {
 				$(
 					if out_point.txid == $spends_txn.txid() {
 						return $spends_txn.output.get(out_point.vout as usize).cloned()
 					}
 				)*
 				None
-			}).unwrap();
+			};
+			let mut total_value_in = 0;
+			for input in $tx.input.iter() {
+				total_value_in += get_output(&input.previous_output).unwrap().value;
+			}
+			let mut total_value_out = 0;
+			for output in $tx.output.iter() {
+				total_value_out += output.value;
+			}
+			let min_fee = ($tx.get_weight() as u64 + 3) / 4; // One sat per vbyte (ie per weight/4, rounded up)
+			// Input amount - output amount = fee, so check that out + min_fee is smaller than input
+			assert!(total_value_out + min_fee <= total_value_in);
+			$tx.verify(get_output).unwrap();
 		}
 	}
 }

lightning/src/ln/functional_tests.rs

@@ -4708,6 +4708,7 @@ macro_rules! check_spendable_outputs {
 										input: vec![input],
 										output: vec![outp],
 									};
+									spend_tx.output[0].value -= (spend_tx.get_weight() + 2 + 1 + 73 + 35 + 3) as u64 / 4; // (Max weight + 3 (to round up)) / 4
 									let secp_ctx = Secp256k1::new();
 									let keys = $keysinterface.derive_channel_keys($chan_value, key_derivation_params.0, key_derivation_params.1);
 									let remotepubkey = keys.pubkeys().payment_point;
@@ -4742,6 +4743,7 @@ macro_rules! check_spendable_outputs {
 
 										let delayed_payment_pubkey = PublicKey::from_secret_key(&secp_ctx, &delayed_payment_key);
 										let witness_script = chan_utils::get_revokeable_redeemscript(revocation_pubkey, *to_self_delay, &delayed_payment_pubkey);
+										spend_tx.output[0].value -= (spend_tx.get_weight() + 2 + 1 + 73 + 1 + witness_script.len() + 1 + 3) as u64 / 4; // (Max weight + 3 (to round up)) / 4
 										let sighash = Message::from_slice(&bip143::SigHashCache::new(&spend_tx).signature_hash(0, &witness_script, output.value, SigHashType::All)[..]).unwrap();
 										let local_delayedsig = secp_ctx.sign(&sighash, &delayed_payment_key);
 										spend_tx.input[0].witness.push(local_delayedsig.serialize_der().to_vec());
@@ -4769,6 +4771,7 @@ macro_rules! check_spendable_outputs {
 										input: vec![input],
 										output: vec![outp.clone()],
 									};
+									spend_tx.output[0].value -= (spend_tx.get_weight() + 2 + 1 + 73 + 35 + 3) as u64 / 4; // (Max weight + 3 (to round up)) / 4
 									let secret = {
 										match ExtendedPrivKey::new_master(Network::Testnet, &$node.node_seed) {
 											Ok(master_key) => {