Fix some test theoretical lock inversions

In the next commit we add lockorder testing based on the line each
mutex was created on rather than the particular mutex instance.
This causes some additional test failure because of lockorder
inversions for the same mutex across different tests, which is
fixed here.

Author: TheBlueMatt

lightning/src/ln/functional_test_utils.rs

@@ -352,6 +352,11 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
 				}
 			}
 
+			let broadcaster = test_utils::TestBroadcaster {
+				txn_broadcasted: Mutex::new(self.tx_broadcaster.txn_broadcasted.lock().unwrap().clone()),
+				blocks: Arc::new(Mutex::new(self.tx_broadcaster.blocks.lock().unwrap().clone())),
+			};
+
 			// Before using all the new monitors to check the watch outpoints, use the full set of
 			// them to ensure we can write and reload our ChannelManager.
 			{
@@ -367,20 +372,13 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
 					keys_manager: self.keys_manager,
 					fee_estimator: &test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) },
 					chain_monitor: self.chain_monitor,
-					tx_broadcaster: &test_utils::TestBroadcaster {
-						txn_broadcasted: Mutex::new(self.tx_broadcaster.txn_broadcasted.lock().unwrap().clone()),
-						blocks: Arc::new(Mutex::new(self.tx_broadcaster.blocks.lock().unwrap().clone())),
-					},
+					tx_broadcaster: &broadcaster,
 					logger: &self.logger,
 					channel_monitors,
 				}).unwrap();
 			}
 
 			let persister = test_utils::TestPersister::new();
-			let broadcaster = test_utils::TestBroadcaster {
-				txn_broadcasted: Mutex::new(self.tx_broadcaster.txn_broadcasted.lock().unwrap().clone()),
-				blocks: Arc::new(Mutex::new(self.tx_broadcaster.blocks.lock().unwrap().clone())),
-			};
 			let chain_source = test_utils::TestChainSource::new(Network::Testnet);
 			let chain_monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &broadcaster, &self.logger, &feeest, &persister, &self.keys_manager);
 			for deserialized_monitor in deserialized_monitors.drain(..) {

lightning/src/ln/functional_tests.rs

@@ -3410,7 +3410,7 @@ fn test_htlc_ignore_latest_remote_commitment() {
 	check_added_monitors!(nodes[0], 1);
 	check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed);
 
-	let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
+	let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
 	assert_eq!(node_txn.len(), 3);
 	assert_eq!(node_txn[0], node_txn[1]);
 
@@ -4828,7 +4828,7 @@ fn test_claim_on_remote_sizeable_push_msat() {
 	check_added_monitors!(nodes[0], 1);
 	check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed);
 
-	let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
+	let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
 	assert_eq!(node_txn.len(), 1);
 	check_spends!(node_txn[0], chan.3);
 	assert_eq!(node_txn[0].output.len(), 2); // We can't force trimming of to_remote output as channel_reserve_satoshis block us to do so at channel opening
@@ -5034,7 +5034,7 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() {
 	check_closed_event!(nodes[0], 1, ClosureReason::CommitmentTxConfirmed);
 	connect_blocks(&nodes[0], TEST_FINAL_CLTV - 1); // Confirm blocks until the HTLC expires
 
-	let revoked_htlc_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
+	let revoked_htlc_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
 	assert_eq!(revoked_htlc_txn.len(), 2);
 	check_spends!(revoked_htlc_txn[0], chan_1.3);
 	assert_eq!(revoked_htlc_txn[1].input.len(), 1);
@@ -7839,7 +7839,7 @@ fn test_bump_penalty_txn_on_revoked_htlcs() {
 	check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed);
 	connect_blocks(&nodes[1], 49); // Confirm blocks until the HTLC expires (note CLTV was explicitly 50 above)
 
-	let revoked_htlc_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
+	let revoked_htlc_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
 	assert_eq!(revoked_htlc_txn.len(), 3);
 	check_spends!(revoked_htlc_txn[1], chan.3);
 
@@ -8100,22 +8100,26 @@ fn test_counterparty_raa_skip_no_crash() {
 	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 	let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).2;
 
-	let mut guard = nodes[0].node.channel_state.lock().unwrap();
-	let keys = guard.by_id.get_mut(&channel_id).unwrap().get_signer();
+	let per_commitment_secret;
+	let next_per_commitment_point;
+	{
+		let mut guard = nodes[0].node.channel_state.lock().unwrap();
+		let keys = guard.by_id.get_mut(&channel_id).unwrap().get_signer();
 
-	const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1;
+		const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1;
 
-	// Make signer believe we got a counterparty signature, so that it allows the revocation
-	keys.get_enforcement_state().last_holder_commitment -= 1;
-	let per_commitment_secret = keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER);
+		// Make signer believe we got a counterparty signature, so that it allows the revocation
+		keys.get_enforcement_state().last_holder_commitment -= 1;
+		per_commitment_secret = keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER);
 
-	// Must revoke without gaps
-	keys.get_enforcement_state().last_holder_commitment -= 1;
-	keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 1);
+		// Must revoke without gaps
+		keys.get_enforcement_state().last_holder_commitment -= 1;
+		keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 1);
 
-	keys.get_enforcement_state().last_holder_commitment -= 1;
-	let next_per_commitment_point = PublicKey::from_secret_key(&Secp256k1::new(),
-		&SecretKey::from_slice(&keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)).unwrap());
+		keys.get_enforcement_state().last_holder_commitment -= 1;
+		next_per_commitment_point = PublicKey::from_secret_key(&Secp256k1::new(),
+			&SecretKey::from_slice(&keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)).unwrap());
+	}
 
 	nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(),
 		&msgs::RevokeAndACK { channel_id, per_commitment_secret, next_per_commitment_point });
@@ -8457,12 +8461,12 @@ fn test_reject_funding_before_inbound_channel_accepted() {
 	// `MessageSendEvent::SendAcceptChannel` event. The message is passed to `nodes[0]`
 	// `handle_accept_channel`, which is required in order for `create_funding_transaction` to
 	// succeed when `nodes[0]` is passed to it.
-	{
+	let accept_chan_msg = {
 		let mut lock;
 		let channel = get_channel_ref!(&nodes[1], lock, temp_channel_id);
-		let accept_chan_msg = channel.get_accept_channel_message();
-		nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), InitFeatures::known(), &accept_chan_msg);
-	}
+		channel.get_accept_channel_message()
+	};
+	nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), InitFeatures::known(), &accept_chan_msg);
 
 	let (temporary_channel_id, tx, _) = create_funding_transaction(&nodes[0], &nodes[1].node.get_our_node_id(), 100000, 42);
 

lightning/src/ln/peer_handler.rs

@@ -1926,11 +1926,18 @@ mod tests {
 		peer_a.new_inbound_connection(fd_a.clone(), None).unwrap();
 		assert_eq!(peer_a.read_event(&mut fd_a, &initial_data).unwrap(), false);
 		peer_a.process_events();
-		assert_eq!(peer_b.read_event(&mut fd_b, &fd_a.outbound_data.lock().unwrap().split_off(0)).unwrap(), false);
+
+		let a_data = fd_a.outbound_data.lock().unwrap().split_off(0);
+		assert_eq!(peer_b.read_event(&mut fd_b, &a_data).unwrap(), false);
+
 		peer_b.process_events();
-		assert_eq!(peer_a.read_event(&mut fd_a, &fd_b.outbound_data.lock().unwrap().split_off(0)).unwrap(), false);
+		let b_data = fd_b.outbound_data.lock().unwrap().split_off(0);
+		assert_eq!(peer_a.read_event(&mut fd_a, &b_data).unwrap(), false);
+
 		peer_a.process_events();
-		assert_eq!(peer_b.read_event(&mut fd_b, &fd_a.outbound_data.lock().unwrap().split_off(0)).unwrap(), false);
+		let a_data = fd_a.outbound_data.lock().unwrap().split_off(0);
+		assert_eq!(peer_b.read_event(&mut fd_b, &a_data).unwrap(), false);
+
 		(fd_a.clone(), fd_b.clone())
 	}
 
@@ -2084,14 +2091,16 @@ mod tests {
 
 		assert_eq!(peers[0].read_event(&mut fd_a, &initial_data).unwrap(), false);
 		peers[0].process_events();
-		assert_eq!(peers[1].read_event(&mut fd_b, &fd_a.outbound_data.lock().unwrap().split_off(0)).unwrap(), false);
+		let a_data = fd_a.outbound_data.lock().unwrap().split_off(0);
+		assert_eq!(peers[1].read_event(&mut fd_b, &a_data).unwrap(), false);
 		peers[1].process_events();
 
 		// ...but if we get a second timer tick, we should disconnect the peer
 		peers[0].timer_tick_occurred();
 		assert_eq!(peers[0].peers.read().unwrap().len(), 0);
 
-		assert!(peers[0].read_event(&mut fd_a, &fd_b.outbound_data.lock().unwrap().split_off(0)).is_err());
+		let b_data = fd_b.outbound_data.lock().unwrap().split_off(0);
+		assert!(peers[0].read_event(&mut fd_a, &b_data).is_err());
 	}
 
 	#[test]

lightning/src/ln/reorg_tests.rs

@@ -82,7 +82,7 @@ fn do_test_onchain_htlc_reorg(local_commitment: bool, claim: bool) {
 		check_added_monitors!(nodes[2], 1);
 		check_closed_broadcast!(nodes[2], true); // We should get a BroadcastChannelUpdate (and *only* a BroadcstChannelUpdate)
 		check_closed_event!(nodes[2], 1, ClosureReason::CommitmentTxConfirmed);
-		let node_2_commitment_txn = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap();
+		let node_2_commitment_txn = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
 		assert_eq!(node_2_commitment_txn.len(), 3); // ChannelMonitor: 1 offered HTLC-Claim, ChannelManger: 1 local commitment tx, 1 Received HTLC-Claim
 		assert_eq!(node_2_commitment_txn[1].output.len(), 2); // to-remote and Received HTLC (to-self is dust)
 		check_spends!(node_2_commitment_txn[1], chan_2.3);