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.

diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs
index d858c12673daf1e24365bd20d79558f540a9ff86..179d0edb55fd7942eda7b819d16c57f53989fbd8 100644 (file)
--- a/lightning/src/chain/chainmonitor.rs
+++ b/lightning/src/chain/chainmonitor.rs
@@ -80,32 +80,27 @@ impl<ChanSigner: ChannelKeys, C: Deref, T: Deref, F: Deref, L: Deref> 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 4c3db606d9f96b56c773898268aab74279e9390e..366627b6595a815ce518c230fe0e2253ecf502dc 100644 (file)
--- 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 40c6bda8bb788e5700286114e0c922d6dce7ebb7..c71811fa8801b7f0cad95d49b32804fcb4eda034 100644 (file)
--- 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]