[draft] 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

new_with_checkpoint now returns an error if a point of agreement with the local checkpoint cannot be found. This prevents the scan from proceeding on divergent chains and surfaces reorgs and misconfigs early.

FilterIter can now detect chain reorgs during block filter iteration. When a reorg is detected, it backtracks and re-fetches data for previous heights in order to maintain a consistent chain of headers. A hard cap MAX_REORG_DEPTH is enforced on the number of blocks to backtrack to prevent resource exhaustion in the case of a faulty or malicious node.

FilterIter now stores block headers for all of the scanned blocks. This enables the calling code to store a local chain of headers.

Changelog notice

bdk_bitcoind_rpc

Added method FilterIter::headers for getting the block headers included in a scan.

BREAKING:

Changed FilterIter::new_with_checkpoint to return a Result.
Added ReorgDepthExceeded enum variant to bip158::Error

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?

evanlinjin · 2025-07-26T09:40:39Z

crates/bitcoind_rpc/src/bip158.rs

    /// Get the remote tip.
    ///
-    /// Returns `None` if the remote height is not strictly greater than the height of this
-    /// [`FilterIter`].
+    /// Returns `None` if the remote height is less than the height of this [`FilterIter`].
    pub fn get_tip(&mut self) -> Result<Option<BlockId>, Error> {


If this call is required to "initiate" the iterator, we should mention it somewhere in the docs.

evanlinjin · 2025-07-26T09:49:24Z

crates/bitcoind_rpc/src/bip158.rs

            if cp.hash() == fetched_hash {
-                // ensure this block also exists in self
-                self.blocks.insert(height, cp.hash());
+                // Ensure this block also exists in `self`.
+                self.headers.insert(height, (fetched_hash, header));
                return Ok(cp.block_id());
            }
-            // remember conflicts
-            self.blocks.insert(height, fetched_hash);
-            cp = cp.prev().expect("must break before genesis");
+            // Remember conflicts.
+            self.headers.insert(height, (fetched_hash, header));
+            cp = cp.prev().ok_or(Error::ReorgDepthExceeded)?;


Nit:

Suggested change

if cp.hash() == fetched_hash {

// ensure this block also exists in self

self.blocks.insert(height, cp.hash());

// Ensure this block also exists in `self`.

self.headers.insert(height, (fetched_hash, header));

return Ok(cp.block_id());

}

// remember conflicts

self.blocks.insert(height, fetched_hash);

cp = cp.prev().expect("must break before genesis");

// Remember conflicts.

self.headers.insert(height, (fetched_hash, header));

cp = cp.prev().ok_or(Error::ReorgDepthExceeded)?;

// Ensure visited blocks also exist in `self`.

self.headers.insert(height, (fetched_hash, header));

if cp.hash() == fetched_hash {

return Ok(cp.block_id());

}

cp = cp.prev().ok_or(Error::ReorgDepthExceeded)?;

evanlinjin · 2025-07-26T09:57:46Z

crates/bitcoind_rpc/src/bip158.rs

-        // is also the point of agreement with `self.cp`.
+        // We return blocks up to and including the initial height, all of the matching blocks,
+        // and blocks in the terminal range.
+        let tail_range = self.stop.saturating_sub(9)..=self.stop;


Nit: What do you think about extracting 9 out as a constant?

evanlinjin · 2025-07-26T10:17:13Z

crates/bitcoind_rpc/src/bip158.rs

+            }
+            // Fetch next header.
+            let mut height = self.height;
+            let mut hash = self.client.get_block_hash(height as u64)?;


Nit: You can move this inside the loop to avoid duplicating code. Do break (hash, header) to access the value outside the loop.

evanlinjin · 2025-07-26T10:31:42Z

crates/bitcoind_rpc/src/bip158.rs

+                let prev_height = height.saturating_sub(1);
+                match self.headers.get(&prev_height).copied() {


We do not want to continue looping once we reach genesis.

Suggested change

let prev_height = height.saturating_sub(1);

match self.headers.get(&prev_height).copied() {

let prev_cached_header = height

.checked_sub(1)

.and_then(|h| self.headers.get(&h))

.copied();

match prev_cached_header {

evanlinjin · 2025-07-26T10:53:22Z

crates/bitcoind_rpc/src/bip158.rs

+            // If the filter matches any of our watched SPKs, fetch the full
+            // block and prepare the next event.
+            let next_event = if self.spks.is_empty() {
+                Err(Error::NoScripts)


Are you able to justify this error variant?

Updates the test to assert the matching heights in `.matched`.

..that defines the number of recent block IDs to include in a chain update.

… fallible The logic of `find_base` is moved inside the `new_with_checkpoint` constructor, which will error if a point of agreement (PoA) isn't found. As a result, `get_tip` is now only responsible for setting the stop height, which means it can be called multiple times on a single instance of FilterIter. `cp` field is removed from FilterIter struct, since it is now only used in the constructor to find a base and set the start height. There's no longer a need to remember conflicting blocks on the way to finding the PoA, now that we store all newly fetched headers during `next`.

- use `checked_sub` to get the previous header - remove unneeded type alias - rename `headers` method - increase documentation

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?

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

Open

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 added 7 commits July 28, 2025 09:30

fix(bip158)!: get_tip now errors due to wrong network

0e2a962

feat(bip158): FilterIter detects reorgs

9b2eb6a

feat(bip158): FilterIter stores the headers

59b4651

test(bip158): rewrite test filter_iter_matches_blocks

14242a6

Updates the test to assert the matching heights in `.matched`.

refactor(bip158): add constant CHAIN_SUFFIX_LEN

a0d965f

..that defines the number of recent block IDs to include in a chain update.

refactor(bip158): fixup internals

e368af5

- use `checked_sub` to get the previous header - remove unneeded type alias - rename `headers` method - increase documentation

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[draft] FilterIter API redesign #2000

[draft] FilterIter API redesign #2000

ValuedMammal commented Jul 24, 2025 •

edited

Loading

Uh oh!

evanlinjin Jul 26, 2025 •

edited

Loading

Uh oh!

evanlinjin left a comment •

edited

Loading

Uh oh!

evanlinjin Jul 26, 2025

Uh oh!

evanlinjin Jul 26, 2025

Uh oh!

evanlinjin Jul 26, 2025

Uh oh!

evanlinjin Jul 26, 2025

Uh oh!

evanlinjin Jul 26, 2025

Uh oh!

evanlinjin Jul 26, 2025

Uh oh!

evanlinjin left a comment

Uh oh!

evanlinjin Jul 29, 2025

Uh oh!

Uh oh!

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

		let prev_height = height.saturating_sub(1);
		match self.headers.get(&prev_height).copied() {

-                let prev_height = height.saturating_sub(1);
-                match self.headers.get(&prev_height).copied() {
+                let prev_cached_header = height
+                    .checked_sub(1)
+                    .and_then(|h| self.headers.get(&h))
+                    .copied();
+                match prev_cached_header {

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

[draft] FilterIter API redesign #2000

Are you sure you want to change the base?

[draft] FilterIter API redesign #2000

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!

evanlinjin Jul 26, 2025

Choose a reason for hiding this comment

Uh oh!

evanlinjin Jul 26, 2025

Choose a reason for hiding this comment

Uh oh!

evanlinjin Jul 26, 2025

Choose a reason for hiding this comment

Uh oh!

evanlinjin Jul 26, 2025

Choose a reason for hiding this comment

Uh oh!

evanlinjin Jul 26, 2025

Choose a reason for hiding this comment

Uh oh!

evanlinjin Jul 26, 2025

Choose a reason for hiding this comment

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!

Uh oh!

ValuedMammal commented Jul 24, 2025 •

edited

Loading

evanlinjin Jul 26, 2025 •

edited

Loading

evanlinjin left a comment •

edited

Loading