Update `ChannelMonitor::best_block` before calling block_confirmed

No matter the context, if we're told about a block which is
guaranteed by our API semantics to be on the best chain, and it has
a higher height than our current understanding of the best chain,
we should update our understanding. This avoids complexity
in `block_confirmed` by never having a height set which is *higher*
than our current best chain, potentially avoiding some bugs in the
rather-complicated code.

It also requires a minor test tweak as we in some cases now no
longer broadcast a conflicting transaction after the original has
reached the ANTI_REORG_DELAY.

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index d1e6e8114e720816416da16e36064d153c569c7b..7d234121036def50fce1ef6b72ef2e2eed87d1d0 100644 (file)
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -2004,6 +2004,10 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                        self.is_paying_spendable_output(&tx, height, &logger);
                }
 
+               if height > self.best_block.height() {
+                       self.best_block = BestBlock::new(block_hash, height);
+               }
+
                self.block_confirmed(height, txn_matched, watch_outputs, claimable_outpoints, broadcaster, fee_estimator, logger)
        }
 

diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index 060097c5f4a30abe18139ebb9ab14800a38d50f3..15f7a715263690cd5a2637175529125bc7c60349 100644 (file)
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -3001,10 +3001,16 @@ fn do_test_htlc_on_chain_timeout(connect_style: ConnectStyle) {
 
        connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
        {
-               // B will rebroadcast its own holder commitment transaction here...just because
+               // B may rebroadcast its own holder commitment transaction here, as a safeguard against
+               // some incredibly unlikely partial-eclipse-attack scenarios. That said, because the
+               // original commitment_tx[0] (also spending chan_2.3) has reached ANTI_REORG_DELAY B really
+               // shouldn't broadcast anything here, and in some connect style scenarios we do not.
                let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
-               assert_eq!(node_txn.len(), 1);
-               check_spends!(node_txn[0], chan_2.3);
+               if node_txn.len() == 1 {
+                       check_spends!(node_txn[0], chan_2.3);
+               } else {
+                       assert_eq!(node_txn.len(), 0);
+               }
        }
 
        expect_pending_htlcs_forwardable!(nodes[1]);