f - test fixups

* Explicitly assert no hanging messages after off-chain actions
* Get rid of manual user force-close for timeout
* Connect blocks on nodes[0] to make sure they don't go on-chain
* Add comment on syncing block height on nodes
* Simplify and wrap especially long lines
* Add more confirmations to onchain test cases
* Increase feerate to hit fail back code path now that we have
a threshold for HTLC value
* Exepct new fail events now that we don't prevent duplicate fail
events
* Replace timeout buffer with grace period

Author: alecchendev

lightning/src/ln/channel.rs

@@ -2277,10 +2277,6 @@ impl<SP: Deref> Channel<SP> where
 			}
 		}
 		if pending_idx == core::usize::MAX {
-			#[cfg(any(test, fuzzing))]
-			// If we failed to find an HTLC to fulfill, make sure it was previously fulfilled and
-			// this is simply a duplicate claim, not previously failed and we lost funds.
-			debug_assert!(self.context.historical_inbound_htlc_fulfills.contains(&htlc_id_arg));
 			return UpdateFulfillFetch::DuplicateClaim {};
 		}
 
@@ -2449,10 +2445,6 @@ impl<SP: Deref> Channel<SP> where
 			}
 		}
 		if pending_idx == core::usize::MAX {
-			#[cfg(any(test, fuzzing))]
-			// If we failed to find an HTLC to fail, make sure it was previously fulfilled and this
-			// is simply a duplicate fail, not previously failed and we failed-back too early.
-			debug_assert!(self.context.historical_inbound_htlc_fulfills.contains(&htlc_id_arg));
 			return Ok(None);
 		}
 

lightning/src/ln/functional_tests.rs

@@ -14,7 +14,7 @@
 use crate::chain;
 use crate::chain::{ChannelMonitorUpdateStatus, Confirm, Listen, Watch};
 use crate::chain::chaininterface::LowerBoundedFeeEstimator;
-use crate::chain::channelmonitor::{self, TIMEOUT_FAIL_BACK_BUFFER};
+use crate::chain::channelmonitor;
 use crate::chain::channelmonitor::{CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY};
 use crate::chain::transaction::OutPoint;
 use crate::sign::{ChannelSigner, EcdsaChannelSigner, EntropySource, SignerProvider};
@@ -2236,56 +2236,69 @@ fn do_test_fail_back_before_backwards_timeout(post_fail_back_action: PostFailBac
 	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
 	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
 	let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+	for node in nodes.iter() {
+		*node.fee_estimator.sat_per_kw.lock().unwrap() = 2000;
+	}
 
 	create_announced_chan_between_nodes(&nodes, 0, 1);
 	let chan_2 = create_announced_chan_between_nodes(&nodes, 1, 2);
 
+	// Start every node on the same block height to make reasoning about timeouts easier
 	connect_blocks(&nodes[0], 2*CHAN_CONFIRM_DEPTH + 1 - nodes[0].best_block_info().1);
 	connect_blocks(&nodes[1], 2*CHAN_CONFIRM_DEPTH + 1 - nodes[1].best_block_info().1);
 	connect_blocks(&nodes[2], 2*CHAN_CONFIRM_DEPTH + 1 - nodes[2].best_block_info().1);
 
 	let (payment_preimage, payment_hash, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 3_000_000);
 
 	// Force close downstream with timeout
-	nodes[1].node.force_close_broadcasting_latest_txn(&chan_2.2, &nodes[2].node.get_our_node_id()).unwrap();
-	check_added_monitors!(nodes[1], 1);
-	check_closed_broadcast!(nodes[1], true);
-
-	connect_blocks(&nodes[1], TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS + 1);
+	let timeout_blocks = TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS + 1;
+	connect_blocks(&nodes[1], timeout_blocks);
 	let node_1_txn = test_txn_broadcast(&nodes[1], &chan_2, None, HTLCType::TIMEOUT);
-	check_closed_event(&nodes[1], 1, ClosureReason::HolderForceClosed, false,
+	check_closed_event(&nodes[1], 1, ClosureReason::CommitmentTxConfirmed, false,
 		&[nodes[2].node.get_our_node_id(); 1], 100_000);
+	check_closed_broadcast!(nodes[1], true);
+	check_added_monitors!(nodes[1], 1);
 
 	// Nothing is confirmed for a while
-	connect_blocks(&nodes[1], MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - TIMEOUT_FAIL_BACK_BUFFER);
+	// We subtract `LATENCY_GRACE_PERIOD_BLOCKS` once because we already confirmed these blocks
+	// to force-close downstream, and once more because it's also used as the buffer when failing
+	// upstream.
+	let upstream_timeout_blocks =
+		MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - LATENCY_GRACE_PERIOD_BLOCKS;
+	connect_blocks(&nodes[1], upstream_timeout_blocks);
+
+	// Connect blocks for nodes[0] to make sure they don't go on-chain
+	connect_blocks(&nodes[0], timeout_blocks + upstream_timeout_blocks);
 
 	// Check that nodes[1] fails the HTLC upstream
-	expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], vec![HTLCDestination::NextHopChannel { node_id: Some(nodes[2].node.get_our_node_id()), channel_id: chan_2.2 }]);
+	expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1],
+		vec![HTLCDestination::NextHopChannel {
+			node_id: Some(nodes[2].node.get_our_node_id()),
+			channel_id: chan_2.2
+		}]);
 	check_added_monitors!(nodes[1], 1);
-	let events = nodes[1].node.get_and_clear_pending_msg_events();
-	assert_eq!(events.len(), 1);
-	let (update_fail, commitment_signed) = match events[0] {
-		MessageSendEvent::UpdateHTLCs { node_id: _, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, ref update_fee, ref commitment_signed } } => {
-			assert!(update_add_htlcs.is_empty());
-			assert!(update_fulfill_htlcs.is_empty());
-			assert_eq!(update_fail_htlcs.len(), 1);
-			assert!(update_fail_malformed_htlcs.is_empty());
-			assert!(update_fee.is_none());
-			(update_fail_htlcs[0].clone(), commitment_signed.clone())
-		},
-		_ => panic!("Unexpected event"),
-	};
+	let htlc_updates = get_htlc_update_msgs(&nodes[1], &nodes[0].node.get_our_node_id());
+	let msgs::CommitmentUpdate { update_fail_htlcs, commitment_signed, .. } = htlc_updates;
 
-	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &update_fail);
+	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &update_fail_htlcs[0]);
 	commitment_signed_dance!(nodes[0], nodes[1], commitment_signed, false);
-	expect_payment_failed_conditions(&nodes[0], payment_hash, false, PaymentFailedConditions::new().blamed_chan_closed(true));
+	expect_payment_failed_conditions(&nodes[0], payment_hash, false,
+		PaymentFailedConditions::new().blamed_chan_closed(true));
 
 	// Make sure we handle possible duplicate fails or extra messages after failing back
 	match post_fail_back_action {
 		PostFailBackAction::TimeoutOnChain => {
 			// Confirm nodes[1]'s claim with timeout, make sure we don't fail upstream again
 			mine_transaction(&nodes[1], &node_1_txn[0]); // Commitment
 			mine_transaction(&nodes[1], &node_1_txn[1]); // HTLC timeout
+			connect_blocks(&nodes[1], ANTI_REORG_DELAY);
+			// Expect handling another fail back event, but the HTLC is already gone
+			expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1],
+				vec![HTLCDestination::NextHopChannel {
+					node_id: Some(nodes[2].node.get_our_node_id()),
+					channel_id: chan_2.2
+				}]);
+			assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 		},
 		PostFailBackAction::ClaimOnChain => {
 			nodes[2].node.claim_funds(payment_preimage);
@@ -2302,17 +2315,21 @@ fn do_test_fail_back_before_backwards_timeout(post_fail_back_action: PostFailBac
 
 			mine_transaction(&nodes[1], &node_2_txn[0]); // Commitment
 			mine_transaction(&nodes[1], &node_2_txn[1]); // HTLC success
+			connect_blocks(&nodes[1], ANTI_REORG_DELAY);
+			assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 		},
 		PostFailBackAction::FailOffChain => {
 			nodes[2].node.fail_htlc_backwards(&payment_hash);
-			expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[2], vec![HTLCDestination::FailedPayment { payment_hash: payment_hash.clone() }]);
+			expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[2],
+				vec![HTLCDestination::FailedPayment { payment_hash: payment_hash.clone() }]);
 			check_added_monitors!(nodes[2], 1);
 			let commitment_update = get_htlc_update_msgs(&nodes[2], &nodes[1].node.get_our_node_id());
 			let update_fail = commitment_update.update_fail_htlcs[0].clone();
 
 			nodes[1].node.handle_update_fail_htlc(&nodes[2].node.get_our_node_id(), &update_fail);
 			let err_msg = get_err_msg(&nodes[1], &nodes[2].node.get_our_node_id());
 			assert_eq!(err_msg.channel_id, chan_2.2);
+			assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 		},
 		PostFailBackAction::ClaimOffChain => {
 			nodes[2].node.claim_funds(payment_preimage);
@@ -2324,6 +2341,7 @@ fn do_test_fail_back_before_backwards_timeout(post_fail_back_action: PostFailBac
 			nodes[1].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &update_fulfill);
 			let err_msg = get_err_msg(&nodes[1], &nodes[2].node.get_our_node_id());
 			assert_eq!(err_msg.channel_id, chan_2.2);
+			assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 		},
 	};
 }