Merge #1299: electrum: common ancestor is before first changed block

d551aeeeacf5356a0ba25f4929230dd099b01c32 electrum: consider invalidated blocks for reorgs (Michael Mallan)
86965c188e29191b60da51ce7d998157e1d50f77 electrum: common ancestor is before first changed block (Michael Mallan)

Pull request description:

  The common ancestor is the highest block that is strictly lower than the first change.

  When finding the point of change, we can take the first height with any change (including an invalidated block) rather than the first height with a new block, which avoids a loop. The height found in both cases (loop vs no loop) should be almost always the same as we apply the update to our local chain before finding the common ancestor, but perhaps in a test if there's a reorg down to 0 and the chain update only contains invalidated blocks and no new blocks, then checking for the first invalidated block would be better as there would be no new blocks to find and the reorg would be undetected.

ACKs for top commit:
  darosior:
    utACK d551aeeeacf5356a0ba25f4929230dd099b01c32

Tree-SHA512: 96f59d87e6a89f49a371219cac78505d1ae8edcf2b1789f134f526b7f627d4355e036a8af1584e3076cb58a41bc9c2a0c8b81a48f81157986aae2af33f236dbd

src/bitcoin/electrum/mod.rs

@@ -201,23 +201,26 @@ impl Electrum {
         let changeset = self.bdk_wallet.apply_connected_chain_update(chain_update);
 
         let mut changes_iter = changeset.into_iter();
-        let reorg_common_ancestor = loop {
-            match changes_iter.next() {
-                Some((height, Some(_))) => {
-                    // `BlockHash` being `Some(_)` means a checkpoint at this height was added to the chain.
-                    // Since we iterate in ascending height order, we'll see the lowest block height first.
-                    // If the lowest height it adds is higher than our height before syncing, we're good.
-                    // Else if it's adding a block at height before syncing or lower, it's a reorg.
-                    break if height > local_chain_tip.height() {
-                        None
-                    } else {
-                        log::info!("Block chain reorganization detected.");
-                        Some(self.bdk_wallet.find_block_at_or_before_height(height))
-                    };
-                }
-                Some((_, None)) => continue,
-                None => break None,
+        let reorg_common_ancestor = if let Some((height, _)) = changes_iter.next() {
+            // Either a new block has been added at this height or an existing block in our local
+            // chain has been invalidated.
+            // Since we iterate in ascending height order, we'll see the lowest block height first.
+            // If the lowest height is higher than our height before syncing, we're good.
+            // Else if it's adding/invalidating a block at height before syncing or lower,
+            // it's a reorg.
+            if height > local_chain_tip.height() {
+                None
+            } else {
+                log::info!("Block chain reorganization detected.");
+                // We can assume height is positive as genesis block will not have changed.
+                Some(
+                    self.bdk_wallet
+                        .find_block_before_height(height)
+                        .expect("height of first change is greater than 0"),
+                )
             }
+        } else {
+            None
         };
 
         // Unconfirmed transactions have their last seen as 0, so we override to the `sync_count`

src/bitcoin/electrum/wallet.rs

@@ -295,17 +295,20 @@ impl BdkWallet {
         })
     }
 
-    /// Find the first block in the local chain whose height is less than or equal to this.
-    pub fn find_block_at_or_before_height(&self, height: u32) -> BlockChainTip {
+    /// Find the highest block in the local chain whose height is below `height`.
+    ///
+    /// As the local chain will always contain the genesis block, this returns
+    /// `None` only if `height` is 0.
+    pub fn find_block_before_height(&self, height: u32) -> Option<BlockChainTip> {
         for cp in self.local_chain.iter_checkpoints() {
-            if cp.height() <= height {
-                return BlockChainTip {
+            if cp.height() < height {
+                return Some(BlockChainTip {
                     height: height_i32_from_u32(cp.height()),
                     hash: cp.hash(),
-                };
+                });
             }
         }
-        unreachable!("There must be at least the genesis block.")
+        None
     }
 
     /// Apply an update to the local chain.