From: Matt Corallo <649246+TheBlueMatt@users.noreply.github.com> Date: Fri, 2 Oct 2020 18:53:48 +0000 (-0700) Subject: Merge pull request #731 from TheBlueMatt/2020-10-fix-big-lock X-Git-Tag: v0.0.12~18 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=937d1d8baed16167e642bffda169798268ea8c34;hp=ddebf36eaeef90684eb48ddab8db924b46ee74ac;p=rust-lightning Merge pull request #731 from TheBlueMatt/2020-10-fix-big-lock Actually hold the total_consistency_lock instead of take-and-drop --- diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index d858c126..179d0edb 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -80,32 +80,27 @@ impl ChainMonit /// [`ChannelMonitor::block_connected`] for details. Any HTLCs that were resolved on chain will /// be returned by [`chain::Watch::release_pending_monitor_events`]. /// - /// Calls back to [`chain::Filter`] if any monitor indicated new outputs to watch, returning - /// `true` if so. Subsequent calls must not exclude any transactions matching the new outputs - /// nor any in-block descendants of such transactions. It is not necessary to re-fetch the block - /// to obtain updated `txdata`. + /// Calls back to [`chain::Filter`] if any monitor indicated new outputs to watch. Subsequent + /// calls must not exclude any transactions matching the new outputs nor any in-block + /// descendants of such transactions. It is not necessary to re-fetch the block to obtain + /// updated `txdata`. /// /// [`ChannelMonitor::block_connected`]: ../channelmonitor/struct.ChannelMonitor.html#method.block_connected /// [`chain::Watch::release_pending_monitor_events`]: ../trait.Watch.html#tymethod.release_pending_monitor_events /// [`chain::Filter`]: ../trait.Filter.html - pub fn block_connected(&self, header: &BlockHeader, txdata: &TransactionData, height: u32) -> bool { - let mut has_new_outputs_to_watch = false; - { - let mut monitors = self.monitors.lock().unwrap(); - for monitor in monitors.values_mut() { - let mut txn_outputs = monitor.block_connected(header, txdata, height, &*self.broadcaster, &*self.fee_estimator, &*self.logger); - has_new_outputs_to_watch |= !txn_outputs.is_empty(); - - if let Some(ref chain_source) = self.chain_source { - for (txid, outputs) in txn_outputs.drain(..) { - for (idx, output) in outputs.iter().enumerate() { - chain_source.register_output(&OutPoint { txid, index: idx as u16 }, &output.script_pubkey); - } + pub fn block_connected(&self, header: &BlockHeader, txdata: &TransactionData, height: u32) { + let mut monitors = self.monitors.lock().unwrap(); + for monitor in monitors.values_mut() { + let mut txn_outputs = monitor.block_connected(header, txdata, height, &*self.broadcaster, &*self.fee_estimator, &*self.logger); + + if let Some(ref chain_source) = self.chain_source { + for (txid, outputs) in txn_outputs.drain(..) { + for (idx, output) in outputs.iter().enumerate() { + chain_source.register_output(&OutPoint { txid, index: idx as u16 }, &output.script_pubkey); } } } } - has_new_outputs_to_watch } /// Dispatches to per-channel monitors, which are responsible for updating their on-chain view diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 4c3db606..366627b6 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -81,7 +81,7 @@ pub fn connect_blocks<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, depth: u32, he pub fn connect_block<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, block: &Block, height: u32) { let txdata: Vec<_> = block.txdata.iter().enumerate().collect(); - while node.chain_monitor.chain_monitor.block_connected(&block.header, &txdata, height) {} + node.chain_monitor.chain_monitor.block_connected(&block.header, &txdata, height); node.node.block_connected(&block.header, &txdata, height); } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 59b0d071..c71811fa 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -4852,8 +4852,7 @@ fn test_claim_on_remote_sizeable_push_msat() { connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1, 1, true, header.block_hash()); let spend_txn = check_spendable_outputs!(nodes[1], 1, node_cfgs[1].keys_manager, 100000); - assert_eq!(spend_txn.len(), 2); - assert_eq!(spend_txn[0], spend_txn[1]); + assert_eq!(spend_txn.len(), 1); check_spends!(spend_txn[0], node_txn[0]); } @@ -4885,10 +4884,9 @@ fn test_claim_on_remote_revoked_sizeable_push_msat() { connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1, 1, true, header.block_hash()); let spend_txn = check_spendable_outputs!(nodes[1], 1, node_cfgs[1].keys_manager, 100000); - assert_eq!(spend_txn.len(), 3); - assert_eq!(spend_txn[0], spend_txn[1]); // to_remote output on revoked remote commitment_tx - check_spends!(spend_txn[0], revoked_local_txn[0]); - check_spends!(spend_txn[2], node_txn[0]); + assert_eq!(spend_txn.len(), 2); + check_spends!(spend_txn[0], revoked_local_txn[0]); // to_remote output on revoked remote commitment_tx + check_spends!(spend_txn[1], node_txn[0]); } #[test] @@ -4983,8 +4981,8 @@ fn test_static_spendable_outputs_timeout_tx() { expect_payment_failed!(nodes[1], our_payment_hash, true); let spend_txn = check_spendable_outputs!(nodes[1], 1, node_cfgs[1].keys_manager, 100000); - assert_eq!(spend_txn.len(), 3); // SpendableOutput: remote_commitment_tx.to_remote (*2), timeout_tx.output (*1) - check_spends!(spend_txn[2], node_txn[0].clone()); + assert_eq!(spend_txn.len(), 2); // SpendableOutput: remote_commitment_tx.to_remote, timeout_tx.output + check_spends!(spend_txn[1], node_txn[0]); } #[test] @@ -5159,12 +5157,11 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_success_tx() { // Check A's ChannelMonitor was able to generate the right spendable output descriptor let spend_txn = check_spendable_outputs!(nodes[0], 1, node_cfgs[0].keys_manager, 100000); - assert_eq!(spend_txn.len(), 3); // Duplicated SpendableOutput due to block rescan after revoked htlc output tracking - assert_eq!(spend_txn[0], spend_txn[1]); + assert_eq!(spend_txn.len(), 2); assert_eq!(spend_txn[0].input.len(), 1); check_spends!(spend_txn[0], revoked_local_txn[0]); // spending to_remote output from revoked local tx assert_ne!(spend_txn[0].input[0].previous_output, revoked_htlc_txn[0].input[0].previous_output); - check_spends!(spend_txn[2], node_txn[1]); // spending justice tx output on the htlc success tx + check_spends!(spend_txn[1], node_txn[1]); // spending justice tx output on the htlc success tx } #[test] @@ -5228,7 +5225,7 @@ fn test_onchain_to_onchain_claim() { assert_eq!(b_txn[2].input[0].witness.clone().last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); assert!(b_txn[2].output[0].script_pubkey.is_v0_p2wsh()); // revokeable output assert_ne!(b_txn[2].lock_time, 0); // Timeout tx - check_spends!(b_txn[0], c_txn[1]); // timeout tx on C remote commitment tx, issued by ChannelMonitor, * 2 due to block rescan + check_spends!(b_txn[0], c_txn[1]); // timeout tx on C remote commitment tx, issued by ChannelMonitor assert_eq!(b_txn[0].input[0].witness.clone().last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); assert!(b_txn[0].output[0].script_pubkey.is_v0_p2wpkh()); // direct payment assert_ne!(b_txn[2].lock_time, 0); // Timeout tx @@ -5726,10 +5723,9 @@ fn test_dynamic_spendable_outputs_local_htlc_timeout_tx() { // Verify that A is able to spend its own HTLC-Timeout tx thanks to spendable output event given back by its ChannelMonitor let spend_txn = check_spendable_outputs!(nodes[0], 1, node_cfgs[0].keys_manager, 100000); - assert_eq!(spend_txn.len(), 3); - assert_eq!(spend_txn[0], spend_txn[1]); + assert_eq!(spend_txn.len(), 2); check_spends!(spend_txn[0], local_txn[0]); - check_spends!(spend_txn[2], htlc_timeout); + check_spends!(spend_txn[1], htlc_timeout); } #[test] @@ -5797,10 +5793,9 @@ fn test_key_derivation_params() { // Verify that A is able to spend its own HTLC-Timeout tx thanks to spendable output event given back by its ChannelMonitor let new_keys_manager = test_utils::TestKeysInterface::new(&seed, Network::Testnet); let spend_txn = check_spendable_outputs!(nodes[0], 1, new_keys_manager, 100000); - assert_eq!(spend_txn.len(), 3); - assert_eq!(spend_txn[0], spend_txn[1]); + assert_eq!(spend_txn.len(), 2); check_spends!(spend_txn[0], local_txn_1[0]); - check_spends!(spend_txn[2], htlc_timeout); + check_spends!(spend_txn[1], htlc_timeout); } #[test]