Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Musab1258
Copy link
Contributor

Description

This PR fixes FilterIter not handling reorgs properly as stated in this issue #1848

Notes to the reviewers z

The code in this fix detects reorgs by checking if the previous_block_hash matches the hash of the previous block in the chain. It then finds the last common block between the old and new chains with the find_fork_point method. And resets the iterator's state to the fork point(i.e. point of reorg) and resume scanning from the new chain.

I also added tests to simulate a reorg and verifythat the iterator correctly switches to the new chain

Changelog notice

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo 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

@Musab1258 Musab1258 marked this pull request as draft March 23, 2025 08:11
@notmandatory notmandatory moved this to In Progress in BDK Chain Mar 27, 2025
@notmandatory notmandatory added the bug Something isn't working label Mar 27, 2025
@Musab1258 Musab1258 closed this Apr 1, 2025
@Musab1258 Musab1258 force-pushed the fix/FilterIter-may-not-handle-reorgs-properly branch from f6fc514 to 71bf53d Compare April 1, 2025 04:10
@github-project-automation github-project-automation bot moved this from In Progress to Done in BDK Chain Apr 1, 2025
@Musab1258 Musab1258 reopened this Apr 1, 2025
@ValuedMammal ValuedMammal moved this from Done to In Progress in BDK Chain Apr 1, 2025
@Musab1258 Musab1258 marked this pull request as ready for review April 10, 2025 15:57
@Musab1258 Musab1258 force-pushed the fix/FilterIter-may-not-handle-reorgs-properly branch from 4ef94df to 6f20e8a Compare April 14, 2025 07:58
}
// Fetch next filter
let mut height = self.height;
let mut hash = self.client.get_block_hash(height as _)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to compare this with the previous header returned? Otherwise how can we detect reorgs between separate calls to next?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @evanlinjin, Thanks for the review. I think the code addresses your concern about detecting reorgs between calls to next.

When next() finishes, it updates self.height on line 177 to the next height it expects to process. It also stores the hash and match status for the block it just processed in self.blocks on line 175 self.insert_block(height, hash);). And it persists both of self.height and self.blocks until the next call to next().

What was done on line 155 and line 156 is that the height and hash variables were initialized with the values of self.height (which is the height we expect to process next) and self.client.get_block_hash(height as _) (which is its hash)

Then, the code from lines 157 to 166 gets the header of the current block let header = self.client.get_block_header(&hash)?;, calculates the height of the assumed parent's block let prev_height = height.saturating_sub(1);, retrieves the hash of the assumed parent through its height, and compares it to the prev_blockhash field from the current block's header. If they match, then the chain is consistent, and if they don't, a reorg has occured.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working module-blockchain
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants