From 0627c0c88a6672b6813b7b277dc925b8bf7cab49 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 12 Apr 2022 17:28:15 +0000 Subject: [PATCH] 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. --- lightning/src/ln/functional_test_utils.rs | 14 ++++---- lightning/src/ln/functional_tests.rs | 44 ++++++++++++----------- lightning/src/ln/peer_handler.rs | 19 +++++++--- lightning/src/ln/reorg_tests.rs | 2 +- 4 files changed, 45 insertions(+), 34 deletions(-) diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index fe78395e2..dcc41374f 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/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(..) { diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index e58b54172..2552c995b 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/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); diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index 2a026821f..f8339f0ea 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/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] diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index 29349392f..561fa8104 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/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); -- 2.39.5