`FilterIter` API redesign #2000

ValuedMammal · 2025-07-24T19:26:49Z

Previously FilterIter did not detect or handle reorgs between next calls, meaning that if a reorg occurred, we might process blocks from a stale fork potentially resulting in an invalid wallet state. This PR aims to fix that by adding logic to explicitly check for and respond to a reorg on every call to next.

Notes to the reviewers

The old implementation required storing block IDs of scanned blocks before creating a checkpoint update, but because the interface was split across different methods, it introduced a timing risk between method calls which, when we consider the possibility of reorgs, made the implementation somewhat brittle.

To address this, we make sure that 1) Finding the start block and 2) Updating the internal checkpoint are directly tied to the logic of next. Since the checkpoint in practice is derived from a clone of the local chain, this ensures that the checkpoint returned by next can always find a connection point with the receiver. Additionally we now emit a checkpoint at every height to ensure that any "must-include" heights are not missing.

For example usage see examples/filter_iter.rs

fixes #1848

Changelog notice

Fixed
- `FilterIter` now handles reorgs to ensure consistency of the header chain.

Changed
- `Event` is now a struct instead of enum

Added
- `FilterIter::new` constructor that takes as input a reference to the RPC client, checkpoint, and a list of SPKs.
- `Error::TryFromInt` variant

Removed
- `FilterIter::new_with_height`
- `FilterIter::new_with_checkpoint`
- `EventInner` type
- `FilterIter::get_tip`
- `FilterIter::chain_update`
- `Error::NoScripts` variant

Checklists

All Submissions:

I followed the contribution guidelines

New Features:

I've added tests for the new feature
I've added docs for the new feature

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

evanlinjin · 2025-07-26T09:37:58Z

crates/bitcoind_rpc/src/bip158.rs

+    /// Hard cap on how far to walk back when a reorg is detected.
+    const MAX_REORG_DEPTH: u32 = 100;


I understand that the main motivation for this is to protect against attacks from a malicious/faulty node?

This is not going to work as BDK does not check PoW (not currently). The node can just cheaply construct multiple <100 block reorgs and exhaust resources that way.

The only way to protect against attacks is to check PoW.

Note that I've already mentioned this here: #1985 (comment)

evanlinjin

I still need to understand the rationale of this PR (since we already had #1985 going). If you can expand on this, it would be appreciated.

I can see how the reorg-logic is simpler here (since it uses the pre-cached headers instead of having the check both the checkpoints/headers). Is this the main reasoning?

I'm assuming the main rationale for header-caching is so that it will be easier/faster to emit CheckPoints with Headers once that becomes available. Is this correct?

crates/bitcoind_rpc/src/bip158.rs

evanlinjin

I've demonstrated that the chain-update construction logic is unsound with the following test: 21fa815.

My intuition tells me that making the FilterIter checkpoint-centric would make it easier to ensure correctness here. To make this current approach work would mean copying everying in the CheckPoint into FilterIter::headers which would be counter-intuitive.

evanlinjin · 2025-07-29T11:32:48Z

crates/bitcoind_rpc/src/bip158.rs

+    /// Number of recent blocks from the tip to be returned in a chain update.
+    const CHAIN_SUFFIX_LEN: u32 = 10;


Can we include a rationale for this?

From my understanding, this will make it faster to find the "point of agreement" if there is a reorg of depth < 10.

On second though, I'm not sure how impactful this is - reorgs are relatively rare and rescanning blocks should be pretty fast with filters?

evanlinjin · 2025-08-01T01:37:08Z

This is how I propose we move forward with #2000/#1985:

Make the internals CheckPoint-centric. This will solve the chain-update-may-not-connect problem at it's core.
Use getblockheader (verbose = true) to fetch blocks (instead of calling getblockhash+getblockheader). This reduces round trips and the getblockheader (verbose = true) result contains confirmations+nextblockhash+previousblockhash information which can be used reorg-detection (Note that confirmations == -1 means the block is not in the best chain).
Emit CheckPoint as part of the Event (for matched blocks). Only add on checkpoints for matched blocks and tip.
Remove stop-height from internals. Stop when there is no nextblockhash.

Reorg logic

Backtrack using previousblockhash until confirmations >= 0. Purge all checkpoints with a height >= current.

No need to cache multiple headers/blockhashes. Just keep track of the last emitted header/blockhash.

No need to have Self::matched. Matched checkpoints are emitted straight away.

evanlinjin · 2025-08-01T08:51:04Z

To address the claim that there is a bug in LocalChain:

I updated the test to print the update chain and the original chain (and also added block of height 50 to the original chain to show that it's not some sort of "genesis block problem"): 4f55f2f

Heights of the original chain: 0, 50, 101.
Heights of the update chain: 101, 102.

Note that the block hash of the block at height 101 are not the same between original and update. With this limited information, the checkpoint/localchain logic cannot connect the update to the original chain. In other words, how can we be certain that 50 and 102 exist on the same chain?

To have the update connect, the chain source should include 50.

evanlinjin · 2025-08-01T08:51:50Z

@ValuedMammal Sorry I pushed 4f55f2f on the wrong remote! Feel free to get rid of it.

ValuedMammal · 2025-08-01T15:43:50Z

You're right I take back the claim. I was thinking along the lines of a "transitive invalidation with no point of agreement", but since we traverse over height 50 of the original (which is still valid) the update is ambiguous.

Emit CheckPoint as part of the Event (for matched blocks). Only add on checkpoints for matched blocks and tip.

Good idea - only thing is that the caller might eventually wish to collect the headers?

evanlinjin · 2025-08-02T08:53:19Z

Good idea - only thing is that the caller might eventually wish to collect the headers?

@ValuedMammal What would be the usecase of these headers? Would the caller want headers from every single block or just the relevant blocks?

ValuedMammal · 2025-08-02T16:50:20Z

@ValuedMammal What would be the usecase of these headers? Would the caller want headers from every single block or just the relevant blocks?

Still just the relevant ones. Maybe a better way to state the question is if CheckPoint becomes generic, the implementation needs to decide whether to emit checkpoints containing headers or just the classic checkpoint. Can probably be considered for future work.

evanlinjin · 2025-08-08T03:47:06Z

@ValuedMammal yes I think it should be considered later down the line. Since the blockhash is "stable" per height, we can take in a closure for constructing any checkpoint type.

evanlinjin · 2025-08-09T02:24:45Z

Concept ACK.

This is looking really good.

evanlinjin

Addresses some pain points in making LocalChain updates apply reliably.

P.S. Hopefully this highlights the value of the BlockGraph work you’ve been leading.

evanlinjin · 2025-08-10T11:08:06Z

crates/bitcoind_rpc/src/bip158.rs

+    fn find_base(&self) -> Result<GetBlockHeaderResult, Error> {
+        for cp in self.cp.iter() {
+            let height = cp.height();

-        // if we have a checkpoint we use a lookback of ten blocks
-        // to ensure consistency of the local chain
-        if let Some(cp) = self.cp.as_ref() {
-            // adjust start height to point of agreement + 1
-            let base = self.find_base_with(cp.clone())?;
-            self.height = base.height + 1;
+            let fetched_hash = self.client.get_block_hash(height as u64)?;

-            for _ in 0..9 {
-                let hash = match header.previous_block_hash {
-                    Some(hash) => hash,
-                    None => break,
-                };
-                header = self.client.get_block_header_info(&hash)?;
-                let height = header.height as u32;
-                if height < self.height {
-                    break;
-                }
-                self.blocks.insert(height, hash);
+            if fetched_hash == cp.hash() {
+                return Ok(self.client.get_block_header_info(&fetched_hash)?);
            }
        }

-        self.stop = tip_height;
-
-        Ok(Some(BlockId {
-            height: tip_height,
-            hash: tip_hash,
-        }))
+        Err(Error::ReorgDepthExceeded)


There is an unfortunate restriction with LocalChain (as it exists currently). The only safe way to ensure two CheckPoint chains connect is if the update chain contains all heights of the original chain from the point of agreement. We probably need to keep track of these must-be-included-heights internally in the FilterIter.

We probably also need to change how we structure an Event since non-matching blocks which are not tips may need to return a CheckPoint update. I propose the following:

type SyncPoint { CheckPoint(CheckPoint), BlockId(BlockId), } type Event { at: SyncPoint, match_block: Option<Block>, } // Suggestions for helper methods. impl Event { pub fn checkpoint(&self) -> Option<CheckPoint> { /*TODO*/ } pub fn block_id(&self) -> BlockId { /*TODO*/ } pub fn height(&self) -> u32 { /*TODO*/ } pub fn is_match(&self) -> bool { /*TODO*/ } }

Let me know what you think.

Additionally, what do you think about changing the find_base logic to call get_block_header_info against cp.hash (so no need for a separate get_block_hash call)? However, we may need to look at the RPC error code for if a block does not exist (instead of directly returning the error).

evanlinjin · 2025-08-10T11:09:50Z

crates/bitcoind_rpc/src/bip158.rs

+                cp = cp
+                    .range(..=next_height)
+                    .next()
+                    .ok_or(Error::ReorgDepthExceeded)?;


We'll need to keep track of the heights we have removed to ensure the checkpoint update connects.

ValuedMammal · 2025-08-11T12:53:44Z

Will it be easier to just include the checkpoint with every event?

/// Event returned by [`FilterIter`].
#[derive(Debug, Clone)]
pub struct Event {
    /// Checkpoint
    pub cp: CheckPoint,
    /// Block, will be `Some(..)` for matching blocks.
    pub block: Option<Block>,
}

The API now consists of the methods `new` and `next`. The local checkpoint and SPK inventory are provided to the constructor. `next` is now responsible for locating the point of agreement. The next filter to fetch is determined by the `next_block_hash` field of `GetBlockHeaderResult`. If the next header has negative confirmations due to a reorg, we rewind the internal state until we find a header still in the best chain. Matched block events contain the most up to date checkpoint, so it can be applied directly to the local chain as events are processed. Added `Tip` variant to `Event` enum for emitting the tip checkpoint when all blocks have been scanned. Removed `EventInner`.

Change `Event` to a simple struct containing `cp` and optional `block`. The checkpoint is updated on each iteration whether or not it corresponds to a matching block. We use `CheckPoint::insert` which will also purge evicted blocks if needed. Change implementation of `find_base` to use `get_block_header_info` which helps to reduce the number of RPC calls. Add test `event_checkpoint_connects_to_local_chain` to check the expected events after a reorg, and check that intermediate updates can be applied to the local chain.

ValuedMammal · 2025-08-11T17:13:13Z

I added a commit ff92f30 and rebased.

evanlinjin · 2025-08-12T15:24:07Z

Will it be easier to just include the checkpoint with every event?

/// Event returned by [`FilterIter`].
#[derive(Debug, Clone)]
pub struct Event {
    /// Checkpoint
    pub cp: CheckPoint,
    /// Block, will be `Some(..)` for matching blocks.
    pub block: Option<Block>,
}

@ValuedMammal yes, but the caller can't easily determine the height of non-matching events.

In which case, maybe we should only emit matched blocks + tip? This ensures that the cp and block of an event refers to the same block.

github-project-automation bot added this to BDK Chain Jul 24, 2025

ValuedMammal marked this pull request as ready for review July 25, 2025 15:27

LagginTimes mentioned this pull request Jul 25, 2025

fix(bitcoind_rpc): properly handle reorgs in FilterIter #1985

Draft

8 tasks

evanlinjin reviewed Jul 26, 2025

View reviewed changes

evanlinjin requested changes Jul 26, 2025

View reviewed changes

LagginTimes moved this to Needs Review in BDK Chain Jul 28, 2025

LagginTimes added this to the Wallet 2.1.0 milestone Jul 28, 2025

LagginTimes assigned ValuedMammal Jul 28, 2025

LagginTimes added the bug Something isn't working label Jul 28, 2025

ValuedMammal force-pushed the feat/filter_iter_detects_reorgs branch from 89b1584 to e368af5 Compare July 28, 2025 14:00

LagginTimes modified the milestones: Wallet 2.1.0, Wallet 3.0.0 Jul 29, 2025

evanlinjin requested changes Jul 30, 2025

View reviewed changes

ValuedMammal changed the title ~~feat!: FilterIter detects reorgs~~ [draft] FilterIter API redesign Jul 30, 2025

ValuedMammal marked this pull request as draft July 30, 2025 00:07

ValuedMammal force-pushed the feat/filter_iter_detects_reorgs branch from 4f55f2f to d8e916f Compare August 5, 2025 17:52

ValuedMammal force-pushed the feat/filter_iter_detects_reorgs branch from d8e916f to 88e8d8e Compare August 9, 2025 14:50

ValuedMammal changed the title ~~[draft] FilterIter API redesign~~ FilterIter API redesign Aug 9, 2025

evanlinjin requested changes Aug 10, 2025

View reviewed changes

ValuedMammal added 2 commits August 11, 2025 12:33

ValuedMammal force-pushed the feat/filter_iter_detects_reorgs branch from 88e8d8e to ff92f30 Compare August 11, 2025 16:58

ValuedMammal marked this pull request as ready for review August 11, 2025 17:10

		/// Hard cap on how far to walk back when a reorg is detected.
		const MAX_REORG_DEPTH: u32 = 100;

		/// Number of recent blocks from the tip to be returned in a chain update.
		const CHAIN_SUFFIX_LEN: u32 = 10;

FilterIter API redesign #2000

Are you sure you want to change the base?

FilterIter API redesign #2000

Uh oh!

Conversation

ValuedMammal commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

Bugfixes:

Uh oh!

evanlinjin Jul 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanlinjin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

evanlinjin Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

evanlinjin commented Aug 1, 2025

Reorg logic

Uh oh!

evanlinjin commented Aug 1, 2025

Uh oh!

evanlinjin commented Aug 1, 2025

Uh oh!

ValuedMammal commented Aug 1, 2025

Uh oh!

evanlinjin commented Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ValuedMammal commented Aug 2, 2025

Uh oh!

evanlinjin commented Aug 8, 2025

Uh oh!

evanlinjin commented Aug 9, 2025

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

evanlinjin Aug 10, 2025

Choose a reason for hiding this comment

Uh oh!

evanlinjin Aug 10, 2025

Choose a reason for hiding this comment

Uh oh!

ValuedMammal commented Aug 11, 2025

Uh oh!

ValuedMammal commented Aug 11, 2025

Uh oh!

evanlinjin commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

`FilterIter` API redesign #2000

`FilterIter` API redesign #2000

ValuedMammal commented Jul 24, 2025 •

edited

Loading

evanlinjin Jul 26, 2025 •

edited

Loading

evanlinjin left a comment •

edited

Loading

evanlinjin commented Aug 2, 2025 •

edited

Loading

evanlinjin commented Aug 12, 2025 •

edited

Loading