From 0627c0c88a6672b6813b7b277dc925b8bf7cab49 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 12 Apr 2022 17:28:15 +0000 Subject: [PATCH 1/4] 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 fe78395e2f..dcc41374f4 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 e58b541723..2552c995bc 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 2a026821f8..f8339f0eae 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 29349392f7..561fa81046 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); From 625cda108c9f5be5443e8c42007bb987261c5270 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 15 Jul 2022 16:18:42 +0000 Subject: [PATCH 2/4] Construct all ChannelMonitor mutexes in the same function When we add lockorder detection based on mutex construction site rather than mutex instance in the next commit, ChannelMonitor's PartialEq implementation causes spurious failures. This is caused by the lockorder detection logic considering the ChannelMonitor inner mutex to be two distinct mutexes - one when monitors are deserialized and one when monitors are created fresh. Instead, we attempt to tell the lockorder detection logic that they are the same by ensuring they're constructed in the same place - in this case a util method. --- lightning/src/chain/channelmonitor.rs | 171 +++++++++++++------------- 1 file changed, 87 insertions(+), 84 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 80cd9cb9d4..8dd3d4b43b 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -965,6 +965,13 @@ impl Writeable for ChannelMonitorImpl { } impl ChannelMonitor { + /// For lockorder enforcement purposes, we need to have a single site which constructs the + /// `inner` mutex, otherwise cases where we lock two monitors at the same time (eg in our + /// PartialEq implementation) we may decide a lockorder violation has occurred. + fn from_impl(imp: ChannelMonitorImpl) -> Self { + ChannelMonitor { inner: Mutex::new(imp) } + } + pub(crate) fn new(secp_ctx: Secp256k1, keys: Signer, shutdown_script: Option