From: Matt Corallo <git@bluematt.me>
Date: Mon, 28 Jun 2021 16:23:19 +0000 (+0000)
Subject: Update `ChannelMonitor::best_block` before calling block_confirmed
X-Git-Tag: v0.0.99~7^2~3
X-Git-Url: http://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=commitdiff_plain;h=599c74cd427592c318f90bf0cc91ad88dc14bc06

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 d1e6e811..7d234121 100644
--- 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 060097c5..15f7a715 100644
--- 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]);