Merge pull request #2212 from wpaulino/off-by-one-locktime

Fix off-by-one finalized transaction locktime

Author: TheBlueMatt

lightning-background-processor/src/lib.rs

@@ -1033,12 +1033,12 @@ mod tests {
 	}
 
 	fn create_nodes(num_nodes: usize, persist_dir: String) -> Vec<Node> {
+		let network = Network::Testnet;
 		let mut nodes = Vec::new();
 		for i in 0..num_nodes {
-			let tx_broadcaster = Arc::new(test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new()), blocks: Arc::new(Mutex::new(Vec::new()))});
+			let tx_broadcaster = Arc::new(test_utils::TestBroadcaster::new(network));
 			let fee_estimator = Arc::new(test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) });
 			let logger = Arc::new(test_utils::TestLogger::with_id(format!("node {}", i)));
-			let network = Network::Testnet;
 			let genesis_block = genesis_block(network);
 			let network_graph = Arc::new(NetworkGraph::new(network, logger.clone()));
 			let scorer = Arc::new(Mutex::new(TestScorer::new()));

lightning/src/chain/channelmonitor.rs

@@ -4169,7 +4169,7 @@ mod tests {
 		replay_update.updates.push(ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage: payment_preimage_1 });
 		replay_update.updates.push(ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage: payment_preimage_2 });
 
-		let broadcaster = TestBroadcaster::new(Arc::clone(&nodes[1].blocks));
+		let broadcaster = TestBroadcaster::with_blocks(Arc::clone(&nodes[1].blocks));
 		assert!(
 			pre_update_monitor.update_monitor(&replay_update, &&broadcaster, &chanmon_cfgs[1].fee_estimator, &nodes[1].logger)
 			.is_err());
@@ -4195,10 +4195,7 @@ mod tests {
 	fn test_prune_preimages() {
 		let secp_ctx = Secp256k1::new();
 		let logger = Arc::new(TestLogger::new());
-		let broadcaster = Arc::new(TestBroadcaster {
-			txn_broadcasted: Mutex::new(Vec::new()),
-			blocks: Arc::new(Mutex::new(Vec::new()))
-		});
+		let broadcaster = Arc::new(TestBroadcaster::new(Network::Testnet));
 		let fee_estimator = TestFeeEstimator { sat_per_kw: Mutex::new(253) };
 
 		let dummy_key = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());

lightning/src/chain/onchaintx.rs

@@ -748,8 +748,8 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 			preprocessed_requests.push(req);
 		}
 
-		// Claim everything up to and including cur_height + 1
-		let remaining_locked_packages = self.locktimed_packages.split_off(&(cur_height + 2));
+		// Claim everything up to and including `cur_height`
+		let remaining_locked_packages = self.locktimed_packages.split_off(&(cur_height + 1));
 		for (pop_height, mut entry) in self.locktimed_packages.iter_mut() {
 			log_trace!(logger, "Restoring delayed claim of package(s) at their timelock at {}.", pop_height);
 			preprocessed_requests.append(&mut entry);
@@ -1036,8 +1036,10 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 			}
 		}
 		for ((_package_id, _), ref mut request) in bump_candidates.iter_mut() {
+			// `height` is the height being disconnected, so our `current_height` is 1 lower.
+			let current_height = height - 1;
 			if let Some((new_timer, new_feerate, bump_claim)) = self.generate_claim(
-				height, &request, true /* force_feerate_bump */, fee_estimator, &&*logger
+				current_height, &request, true /* force_feerate_bump */, fee_estimator, &&*logger
 			) {
 				request.set_timer(new_timer);
 				request.set_feerate(new_feerate);

lightning/src/chain/package.rs

@@ -460,13 +460,13 @@ impl PackageSolvingData {
 		}
 	}
 	fn absolute_tx_timelock(&self, current_height: u32) -> u32 {
-		// We use `current_height + 1` as our default locktime to discourage fee sniping and because
+		// We use `current_height` as our default locktime to discourage fee sniping and because
 		// transactions with it always propagate.
 		let absolute_timelock = match self {
-			PackageSolvingData::RevokedOutput(_) => current_height + 1,
-			PackageSolvingData::RevokedHTLCOutput(_) => current_height + 1,
-			PackageSolvingData::CounterpartyOfferedHTLCOutput(_) => current_height + 1,
-			PackageSolvingData::CounterpartyReceivedHTLCOutput(ref outp) => cmp::max(outp.htlc.cltv_expiry, current_height + 1),
+			PackageSolvingData::RevokedOutput(_) => current_height,
+			PackageSolvingData::RevokedHTLCOutput(_) => current_height,
+			PackageSolvingData::CounterpartyOfferedHTLCOutput(_) => current_height,
+			PackageSolvingData::CounterpartyReceivedHTLCOutput(ref outp) => cmp::max(outp.htlc.cltv_expiry, current_height),
 			// HTLC timeout/success transactions rely on a fixed timelock due to the counterparty's
 			// signature.
 			PackageSolvingData::HolderHTLCOutput(ref outp) => {
@@ -475,7 +475,7 @@ impl PackageSolvingData {
 				}
 				outp.cltv_expiry
 			},
-			PackageSolvingData::HolderFundingOutput(_) => current_height + 1,
+			PackageSolvingData::HolderFundingOutput(_) => current_height,
 		};
 		absolute_timelock
 	}

lightning/src/ln/channelmanager.rs

@@ -3026,10 +3026,11 @@ where
 		}
 		{
 			let height = self.best_block.read().unwrap().height();
-			// Transactions are evaluated as final by network mempools at the next block. However, the modules
-			// constituting our Lightning node might not have perfect sync about their blockchain views. Thus, if
-			// the wallet module is in advance on the LDK view, allow one more block of headroom.
-			if !funding_transaction.input.iter().all(|input| input.sequence == Sequence::MAX) && LockTime::from(funding_transaction.lock_time).is_block_height() && funding_transaction.lock_time.0 > height + 2 {
+			// Transactions are evaluated as final by network mempools if their locktime is strictly
+			// lower than the next block height. However, the modules constituting our Lightning
+			// node might not have perfect sync about their blockchain views. Thus, if the wallet
+			// module is ahead of LDK, only allow one more block of headroom.
+			if !funding_transaction.input.iter().all(|input| input.sequence == Sequence::MAX) && LockTime::from(funding_transaction.lock_time).is_block_height() && funding_transaction.lock_time.0 > height + 1 {
 				return Err(APIError::APIMisuseError {
 					err: "Funding transaction absolute timelock is non-final".to_owned()
 				});
@@ -9054,7 +9055,7 @@ pub mod bench {
 		// calls per node.
 		let network = bitcoin::Network::Testnet;
 
-		let tx_broadcaster = test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new()), blocks: Arc::new(Mutex::new(Vec::new()))};
+		let tx_broadcaster = test_utils::TestBroadcaster::new(network);
 		let fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) };
 		let logger_a = test_utils::TestLogger::with_id("node a".to_owned());
 		let scorer = Mutex::new(test_utils::TestScorer::new());

lightning/src/ln/functional_test_utils.rs

@@ -230,6 +230,9 @@ fn do_connect_block<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, block: Block, sk
 	#[cfg(feature = "std")] {
 		eprintln!("Connecting block using Block Connection Style: {:?}", *node.connect_style.borrow());
 	}
+	// Update the block internally before handing it over to LDK, to ensure our assertions regarding
+	// transaction broadcast are correct.
+	node.blocks.lock().unwrap().push((block.clone(), height));
 	if !skip_intermediaries {
 		let txdata: Vec<_> = block.txdata.iter().enumerate().collect();
 		match *node.connect_style.borrow() {
@@ -279,7 +282,6 @@ fn do_connect_block<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, block: Block, sk
 	}
 	call_claimable_balances(node);
 	node.node.test_process_background_events();
-	node.blocks.lock().unwrap().push((block, height));
 }
 
 pub fn disconnect_blocks<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, count: u32) {
@@ -2435,10 +2437,7 @@ pub fn fail_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_path: &
 pub fn create_chanmon_cfgs(node_count: usize) -> Vec<TestChanMonCfg> {
 	let mut chan_mon_cfgs = Vec::new();
 	for i in 0..node_count {
-		let tx_broadcaster = test_utils::TestBroadcaster {
-			txn_broadcasted: Mutex::new(Vec::new()),
-			blocks: Arc::new(Mutex::new(vec![(genesis_block(Network::Testnet), 0)])),
-		};
+		let tx_broadcaster = test_utils::TestBroadcaster::new(Network::Testnet);
 		let fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) };
 		let chain_source = test_utils::TestChainSource::new(Network::Testnet);
 		let logger = test_utils::TestLogger::with_id(format!("node {}", i));