Skip to content

Conversation

LagginTimes
Copy link
Contributor

@LagginTimes LagginTimes commented Sep 18, 2025

Resolves #2021.

Description

Introduce CheckPointEntry to represent either a real CheckPoint or a prev_blockhash placeholder.
merge_chains() now handles updates containing prev_blockhash data while preserving previous behavior when only CheckPoints are present.
is_block_in_chain() can infer membership using prev_blockhash.

Notes to the reviewers

TODO: Add tests.

Changelog notice

  • TODO

Checklists

All Submissions:

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

@LagginTimes LagginTimes self-assigned this Sep 18, 2025
@LagginTimes LagginTimes marked this pull request as draft September 18, 2025 14:17
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

The original implementation of merge_chains was much more efficient. You don't need to change it so drastically to take into account prevhashes. Have an enum CheckPointEntry where both variants have a CheckPoint and you can call next to get to the closest lower height.

@oleonardolima oleonardolima moved this to In Progress in BDK Chain Sep 25, 2025
@oleonardolima oleonardolima added this to the Chain 0.24.0 milestone Sep 25, 2025
@oleonardolima oleonardolima added the api A breaking API change label Sep 25, 2025
Comment on lines +771 to +776
} else if o.height() > u.height() {
// Original > Update: mark original as potentially invalidated.
potentially_invalidated_heights.push(o.height());
prev_orig_was_invalidated = false;
prev_orig = curr_orig.take();
is_update_height_superset_of_original = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

question: isn't this already being covered by the lines L652-656 ?

@evanlinjin
Copy link
Member

Thanks for working on this.

CheckPointEntry being public is what I attempted at the very start (not sure where that implementation is now). I thought you would have made it private and only used for merge_chains.

Even looking at the unfinished state of this, I think I would much prefer #2024 (or the initial public CheckPointEntry).

With this implementation, we need alternative methods to figure out if a height "exists": a height with no checkpoint where the next adjacent checkpoint has a prev-blockhash.

@LagginTimes
Copy link
Contributor Author

Closing this and keeping #2024 per feedback.

@github-project-automation github-project-automation bot moved this from In Progress to Done in BDK Chain Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Ensure CheckPoint chain methods validate and link via previous blockhash
3 participants