From 2bd571d2f9b1e4868708eb4b46832d290d497e04 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 30 Sep 2020 15:18:29 -0400 Subject: [PATCH] Drop last bits of rescan as its too complicated to be worth having Previously, we had a concept of "rescaning" blocks when we detected a need to monitor for a new set of outputs in future blocks while connecting a block. In such cases, we'd need to possibly learn about these new spends later in the *same block*, requiring clients who filter blocks to get a newly-filtered copy of the same block. While redoing the chain access API, it became increasingly clear this was an overly complicated API feature, and it seems likely most clients will not use it anyway. Further, any client who *does* filter blocks can simply update their filtering algorithm to include any descendants of matched transactions in the filter results, avoiding the need for rescan support entirely. Thus, it was decided that we'd move forward without rescan support in #649, however to avoid significant further changes in the already-large 649, we decided to fully remove support in a follow-up. Here, we remove the API features that existed for rescan and fix the few tests to not rely on it. After this commit, we now only ever have one possible version of block connection transactions, making it possible to be significantly more confident in our test coverage actually capturing all realistic scenarios. --- lightning/src/chain/chainmonitor.rs | 31 ++++++++++------------- lightning/src/ln/functional_test_utils.rs | 2 +- lightning/src/ln/functional_tests.rs | 29 +++++++++------------ 3 files changed, 26 insertions(+), 36 deletions(-) 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 40c6bda8..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] @@ -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] -- 2.30.2