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

LagginTimes · 2025-07-01T21:16:58Z

Replaces #1909.
Fixes #1848.

Description

This patch enhances FilterIter to robustly detect and handle chain reorgs. Reorgs are detected by checking if the block hash at the current height has changed, or if the block's prev_blockhash no longer matches the hash of the block at the previous height. This allows detection of reorgs that occur either at the current height or one block prior.

When a reorg is detected, the iterator rewinds up to MAX_REORG_DEPTH blocks to locate the most recent common ancestor. It then resets its internal state and resumes scanning from the fork point on the new chain.

This update also includes tests that simulate reorgs — including back-to-back reorgs at the same height — and verifies that FilterIter correctly switches to the new chain and yields accurate events.

Changelog notice

Improve FilterIter to detect and handle reorgs at the current height or its parent.

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo +nightly fmt and cargo clippy before committing

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

ValuedMammal

Thanks for picking this up @LagginTimes

crates/bitcoind_rpc/src/bip158.rs

crates/bitcoind_rpc/tests/test_filter_iter.rs

ValuedMammal

It's missing a test for max reorg depth.

Co-authored-by: valued mammal <valuedmammal@protonmail.com>

LagginTimes · 2025-07-11T08:27:16Z

It's missing a test for max reorg depth.

The test for this is still WIP but I pushed out the changes that resolved the other open comments.

crates/bitcoind_rpc/src/bip158.rs

Co-authored-by: valued mammal <valuedmammal@protonmail.com>

ValuedMammal

ACK c2ae4da

evanlinjin · 2025-07-17T09:10:39Z

crates/bitcoind_rpc/src/bip158.rs

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


Maybe a little out of scope for this PR, but the documentation here is a little lacking.

get_tip, as I understand, serves the following purposes:

Initialize the FilerIter. Without calling it, FilterIter returns nothing.

If it returns None, it means we are up to date with remote.

Is this the case?

If so, purpose 2. is not satisfied because reorgs are not detected by get_tip.

@ValuedMammal are you able to provide some clarity, thanks!

get_tip is used to set the start and stop height of a scan and is only necessary to call once per instance of FilterIter. During review I was going to mention that I think get_tip should actually clear self.blocks, because I didn't anticipate the user interleaving calls to get_tip and next.

@ValuedMammal Thank you for the update.

The internal implementation of FilterIter implies that it is a single-use structure (construct once, call .next till end). However, the API design of it implies something different.

What do you think about changing the API to properly represent a "single-use" workflow? I.e. I would change the following:

Remove or unexpose add_spk{s}.

Change constructor methods to also take in spks. Return None if there are no spks provided.

Make sure get_tip logic is run on the first call to next.

Rename get_tip to init which would return the target height.

(Separate PR of course)

Let me know what you think.

I like this idea, and can make a follow up PR to implement this.

evanlinjin

Thanks for taking this forward.

I addressed my own review comments in commit 30e25d1 of evanlinjin:bdk @ filter_iter. Let me know what you think. Check commit message for rationale.

crates/bitcoind_rpc/src/bip158.rs

* Remove `MAX_REORG_DEPTH` check and error variant. For the scenario where the `FilterIter` is of a different network than the receiver, the genesis block will be emitted, thus making the receiver error out. * Fix `Iterator::next` impl so that we only mutate internal state after we are certain that no more errors will be returned. * `Error::NoScripts` is no longer returned since I could not justify a rationale for it. * Refactor `Iterator::next` impl be cleaner.

stale

ValuedMammal

I'm not convinced that this represents an objective improvement over Musab's PR.

evanlinjin · 2025-07-23T04:30:53Z

I'm not convinced that this represents an objective improvement over Musab's PR.

@ValuedMammal are you able to provide more context? I'm assuming you are referring to changes in 071fe40?

Here is my justification why these changes are superior:

Separation of mutating/non-mutating code - No state mutations made if anything errors - without this, we may miss blocks if network drops. Better readability.
No duplicated logic - We only need to check whether prev_blockhash matches in one place (rather than two places).
Remove MAX_REORG_DEPTH - As MAX_REORG_DEPTH does not provide any additional security and 100 block reorgs are possible. We are not checking PoW anyway; a malicious node can freely waste our time/return gibberish.

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

LagginTimes mentioned this pull request Jul 1, 2025

fix(bitcoind_rpc): fix filter iter may not handle reorgs properly #1909

Closed

8 tasks

LagginTimes force-pushed the filter_iter branch from 4dfca3a to c798c5b Compare July 1, 2025 21:22

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

LagginTimes moved this to In Progress in BDK Chain Jul 1, 2025

LagginTimes self-assigned this Jul 1, 2025

notmandatory added the bug Something isn't working label Jul 2, 2025

LagginTimes force-pushed the filter_iter branch from c798c5b to 8d076fc Compare July 7, 2025 21:11

LagginTimes moved this from In Progress to Needs Review in BDK Chain Jul 7, 2025

ValuedMammal reviewed Jul 9, 2025

View reviewed changes

ValuedMammal requested changes Jul 9, 2025

View reviewed changes

test(bitcoind_rpc): update FilterIter tests

10fa62a

Co-authored-by: valued mammal <valuedmammal@protonmail.com>

LagginTimes force-pushed the filter_iter branch from 8d076fc to 1a77944 Compare July 11, 2025 08:26

ValuedMammal reviewed Jul 11, 2025

View reviewed changes

crates/bitcoind_rpc/src/bip158.rs Outdated Show resolved Hide resolved

ValuedMammal reviewed Jul 11, 2025

View reviewed changes

crates/bitcoind_rpc/src/bip158.rs Outdated Show resolved Hide resolved

crates/bitcoind_rpc/src/bip158.rs Outdated Show resolved Hide resolved

LagginTimes and others added 2 commits July 11, 2025 17:43

fix(bitcoind_rpc): FilterIter detects reorgs

7e1cc26

Co-authored-by: valued mammal <valuedmammal@protonmail.com>

test(bitcoind_rpc): tests for reorg between next() and MAX_REORG_DEPTH

c2ae4da

LagginTimes force-pushed the filter_iter branch from 1a77944 to c2ae4da Compare July 11, 2025 17:44

ValuedMammal previously approved these changes Jul 11, 2025

View reviewed changes

evanlinjin reviewed Jul 17, 2025

View reviewed changes

evanlinjin requested changes Jul 18, 2025

View reviewed changes

crates/bitcoind_rpc/src/bip158.rs Outdated Show resolved Hide resolved

crates/bitcoind_rpc/src/bip158.rs Outdated Show resolved Hide resolved

crates/bitcoind_rpc/src/bip158.rs Outdated Show resolved Hide resolved

LagginTimes requested review from evanlinjin and ValuedMammal July 21, 2025 07:00

ValuedMammal reviewed Jul 22, 2025

View reviewed changes

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

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

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

Uh oh!

Description

Changelog notice

Checklists

All Submissions:

New Features:

Bugfixes:

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

LagginTimes commented Jul 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

evanlinjin Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

ValuedMammal Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

evanlinjin Jul 18, 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!

LagginTimes Jul 21, 2025

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!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

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

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

LagginTimes commented Jul 1, 2025 •

edited

Loading

evanlinjin Jul 18, 2025 •

edited

Loading

evanlinjin left a comment •

edited

Loading

evanlinjin commented Jul 23, 2025 •

edited

Loading