Make tests more robust against different connection styles

In the next commit we'll randomize the `ConnectStyle` used in each
test. However, some tests are slightly too prescriptive, which we
address here in a few places.

Author: TheBlueMatt

lightning/src/ln/functional_tests.rs

@@ -1280,23 +1280,41 @@ fn test_duplicate_htlc_different_direction_onchain() {
 	check_closed_event!(nodes[0], 1, ClosureReason::CommitmentTxConfirmed);
 	connect_blocks(&nodes[0], TEST_FINAL_CLTV - 1); // Confirm blocks until the HTLC expires
 
-	// Check we only broadcast 1 timeout tx
 	let claim_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clone();
 	assert_eq!(claim_txn.len(), 8);
-	assert_eq!(claim_txn[1], claim_txn[4]);
-	assert_eq!(claim_txn[2], claim_txn[5]);
-	check_spends!(claim_txn[1], chan_1.3);
-	check_spends!(claim_txn[2], claim_txn[1]);
-	check_spends!(claim_txn[7], claim_txn[1]);
+
+	check_spends!(claim_txn[0], remote_txn[0]); // Immediate HTLC claim with preimage
+
+	check_spends!(claim_txn[1], chan_1.3); // Alternative commitment tx
+	check_spends!(claim_txn[2], claim_txn[1]); // HTLC spend in alternative commitment tx
+
+	let bump_tx = if claim_txn[1] == claim_txn[4] {
+		assert_eq!(claim_txn[1], claim_txn[4]);
+		assert_eq!(claim_txn[2], claim_txn[5]);
+
+		check_spends!(claim_txn[7], claim_txn[1]); // HTLC timeout on alternative commitment tx
+
+		check_spends!(claim_txn[3], remote_txn[0]); // HTLC timeout on broadcasted commitment tx
+		&claim_txn[3]
+	} else {
+		assert_eq!(claim_txn[1], claim_txn[3]);
+		assert_eq!(claim_txn[2], claim_txn[4]);
+
+		check_spends!(claim_txn[5], claim_txn[1]); // HTLC timeout on alternative commitment tx
+
+		check_spends!(claim_txn[7], remote_txn[0]); // HTLC timeout on broadcasted commitment tx
+
+		&claim_txn[7]
+	};
 
 	assert_eq!(claim_txn[0].input.len(), 1);
-	assert_eq!(claim_txn[3].input.len(), 1);
-	assert_eq!(claim_txn[0].input[0].previous_output, claim_txn[3].input[0].previous_output);
+	assert_eq!(bump_tx.input.len(), 1);
+	assert_eq!(claim_txn[0].input[0].previous_output, bump_tx.input[0].previous_output);
 
 	assert_eq!(claim_txn[0].input.len(), 1);
 	assert_eq!(claim_txn[0].input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); // HTLC 1 <--> 0, preimage tx
-	check_spends!(claim_txn[0], remote_txn[0]);
 	assert_eq!(remote_txn[0].output[claim_txn[0].input[0].previous_output.vout as usize].value, 800);
+
 	assert_eq!(claim_txn[6].input.len(), 1);
 	assert_eq!(claim_txn[6].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); // HTLC 0 <--> 1, timeout tx
 	check_spends!(claim_txn[6], remote_txn[0]);
@@ -2351,7 +2369,8 @@ fn test_justice_tx() {
 	chanmon_cfgs[1].keys_manager.disable_revocation_policy_check = true;
 	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &user_cfgs);
-	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+	*nodes[0].connect_style.borrow_mut() = ConnectStyle::FullBlockViaListen;
 	// Create some new channels:
 	let chan_5 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
 
@@ -2583,11 +2602,7 @@ fn claim_htlc_outputs_single_tx() {
 		expect_payment_failed!(nodes[1], payment_hash_2, true);
 
 		let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
-		assert_eq!(node_txn.len(), 9);
-		// ChannelMonitor: justice tx revoked offered htlc, justice tx revoked received htlc, justice tx revoked to_local (3)
-		// ChannelManager: local commmitment + local HTLC-timeout (2)
-		// ChannelMonitor: bumped justice tx, after one increase, bumps on HTLC aren't generated not being substantial anymore, bump on revoked to_local isn't generated due to more room for expiration (2)
-		// ChannelMonitor: local commitment + local HTLC-timeout (2)
+		assert!(node_txn.len() == 9 || node_txn.len() == 10);
 
 		// Check the pair local commitment and HTLC-timeout broadcast due to HTLC expiration
 		assert_eq!(node_txn[0].input.len(), 1);
@@ -5283,21 +5298,30 @@ fn test_duplicate_payment_hash_one_failure_one_success() {
 	let htlc_timeout_tx;
 	{ // Extract one of the two HTLC-Timeout transaction
 		let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
-		// ChannelMonitor: timeout tx * 3, ChannelManager: local commitment tx
-		assert_eq!(node_txn.len(), 4);
+		// ChannelMonitor: timeout tx * 2-or-3, ChannelManager: local commitment tx
+		assert!(node_txn.len() == 4 || node_txn.len() == 3);
 		check_spends!(node_txn[0], chan_2.3);
 
 		check_spends!(node_txn[1], commitment_txn[0]);
 		assert_eq!(node_txn[1].input.len(), 1);
-		check_spends!(node_txn[2], commitment_txn[0]);
-		assert_eq!(node_txn[2].input.len(), 1);
-		assert_eq!(node_txn[1].input[0].previous_output, node_txn[2].input[0].previous_output);
-		check_spends!(node_txn[3], commitment_txn[0]);
-		assert_ne!(node_txn[1].input[0].previous_output, node_txn[3].input[0].previous_output);
+
+		if node_txn.len() > 3 {
+			check_spends!(node_txn[2], commitment_txn[0]);
+			assert_eq!(node_txn[2].input.len(), 1);
+			assert_eq!(node_txn[1].input[0].previous_output, node_txn[2].input[0].previous_output);
+
+			check_spends!(node_txn[3], commitment_txn[0]);
+			assert_ne!(node_txn[1].input[0].previous_output, node_txn[3].input[0].previous_output);
+		} else {
+			check_spends!(node_txn[2], commitment_txn[0]);
+			assert_ne!(node_txn[1].input[0].previous_output, node_txn[2].input[0].previous_output);
+		}
 
 		assert_eq!(node_txn[1].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
 		assert_eq!(node_txn[2].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
-		assert_eq!(node_txn[3].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
+		if node_txn.len() > 3 {
+			assert_eq!(node_txn[3].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
+		}
 		htlc_timeout_tx = node_txn[1].clone();
 	}
 
@@ -7957,13 +7981,24 @@ fn test_bump_penalty_txn_on_remote_commitment() {
 		assert_eq!(node_txn[6].input.len(), 1);
 		check_spends!(node_txn[0], remote_txn[0]);
 		check_spends!(node_txn[6], remote_txn[0]);
-		assert_eq!(node_txn[0].input[0].previous_output, node_txn[3].input[0].previous_output);
-		preimage_bump = node_txn[3].clone();
 
 		check_spends!(node_txn[1], chan.3);
 		check_spends!(node_txn[2], node_txn[1]);
-		assert_eq!(node_txn[1], node_txn[4]);
-		assert_eq!(node_txn[2], node_txn[5]);
+
+		if node_txn[0].input[0].previous_output == node_txn[3].input[0].previous_output {
+			preimage_bump = node_txn[3].clone();
+			check_spends!(node_txn[3], remote_txn[0]);
+
+			assert_eq!(node_txn[1], node_txn[4]);
+			assert_eq!(node_txn[2], node_txn[5]);
+		} else {
+			preimage_bump = node_txn[7].clone();
+			check_spends!(node_txn[7], remote_txn[0]);
+			assert_eq!(node_txn[0].input[0].previous_output, node_txn[7].input[0].previous_output);
+
+			assert_eq!(node_txn[1], node_txn[3]);
+			assert_eq!(node_txn[2], node_txn[4]);
+		}
 
 		timeout = node_txn[6].txid();
 		let index = node_txn[6].input[0].previous_output.vout;

lightning/src/ln/payment_tests.rs

@@ -367,7 +367,7 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) {
 	let nodes_0_deserialized: ChannelManager<EnforcingSigner, &test_utils::TestChainMonitor, &test_utils::TestBroadcaster, &test_utils::TestKeysInterface, &test_utils::TestFeeEstimator, &test_utils::TestLogger>;
 	let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
 
-	let (_, _, chan_id, funding_tx) = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
+	let chan_id = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).2;
 	let (_, _, chan_id_2, _) = create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known());
 
 	// Serialize the ChannelManager prior to sending payments
@@ -504,14 +504,19 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) {
 	expect_payment_sent!(nodes[0], payment_preimage_1);
 	connect_blocks(&nodes[0], TEST_FINAL_CLTV*4 + 20);
 	let as_htlc_timeout_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
-	check_spends!(as_htlc_timeout_txn[2], funding_tx);
-	check_spends!(as_htlc_timeout_txn[0], as_commitment_tx);
-	check_spends!(as_htlc_timeout_txn[1], as_commitment_tx);
 	assert_eq!(as_htlc_timeout_txn.len(), 3);
-	if as_htlc_timeout_txn[0].input[0].previous_output == bs_htlc_claim_txn[0].input[0].previous_output {
-		confirm_transaction(&nodes[0], &as_htlc_timeout_txn[1]);
+	let (first_htlc_timeout_tx, second_htlc_timeout_tx) = if as_htlc_timeout_txn[0] == as_commitment_tx {
+		(&as_htlc_timeout_txn[1], &as_htlc_timeout_txn[2])
 	} else {
-		confirm_transaction(&nodes[0], &as_htlc_timeout_txn[0]);
+		assert_eq!(as_htlc_timeout_txn[2], as_commitment_tx);
+		(&as_htlc_timeout_txn[0], &as_htlc_timeout_txn[1])
+	};
+	check_spends!(first_htlc_timeout_tx, as_commitment_tx);
+	check_spends!(second_htlc_timeout_tx, as_commitment_tx);
+	if first_htlc_timeout_tx.input[0].previous_output == bs_htlc_claim_txn[0].input[0].previous_output {
+		confirm_transaction(&nodes[0], &second_htlc_timeout_tx);
+	} else {
+		confirm_transaction(&nodes[0], &first_htlc_timeout_tx);
 	}
 	nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clear();
 	expect_payment_failed_conditions!(nodes[0], payment_hash, false, PaymentFailedConditions::new().mpp_parts_remain());
@@ -627,7 +632,8 @@ fn do_test_dup_htlc_onchain_fails_on_reload(persist_manager_post_event: bool, co
 	let funding_txo = OutPoint { txid: funding_tx.txid(), index: 0 };
 	let mon_updates: Vec<_> = chanmon_cfgs[0].persister.chain_sync_monitor_persistences.lock().unwrap()
 		.get_mut(&funding_txo).unwrap().drain().collect();
-	assert_eq!(mon_updates.len(), 1);
+	// If we are using chain::Confirm instead of chain::Listen, we will get the same update twice
+	assert!(mon_updates.len() == 1 || mon_updates.len() == 2);
 	assert!(nodes[0].chain_monitor.release_pending_monitor_events().is_empty());
 	assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
 
@@ -643,7 +649,9 @@ fn do_test_dup_htlc_onchain_fails_on_reload(persist_manager_post_event: bool, co
 	chanmon_cfgs[0].persister.set_update_ret(Ok(()));
 	let mut chan_0_monitor_serialized = test_utils::TestVecWriter(Vec::new());
 	get_monitor!(nodes[0], chan_id).write(&mut chan_0_monitor_serialized).unwrap();
-	nodes[0].chain_monitor.chain_monitor.channel_monitor_updated(funding_txo, mon_updates[0]).unwrap();
+	for update in mon_updates {
+		nodes[0].chain_monitor.chain_monitor.channel_monitor_updated(funding_txo, update).unwrap();
+	}
 	if payment_timeout {
 		expect_payment_failed!(nodes[0], payment_hash, true);
 	} else {