Skip to content

feat(chain)!: make CheckPoint data field optional #2024

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

feat(chain)!: make CheckPoint data field optional #2024

wants to merge 6 commits into from

Conversation

LagginTimes
Copy link
Contributor

@LagginTimes LagginTimes commented Sep 10, 2025 •
edited
Loading

Resolves #2021.

Description

This PR makes the CheckPoint::data field optional. Gaps inferred from prev_blockhash are represented as placeholder checkpoints where data is None. Subsequent insert()s will provide data for the placeholders when a matching block arrives.

See: #2017 (comment)

Notes to the reviewers

WIP

Changelog notice

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 10, 2025
@LagginTimes LagginTimes marked this pull request as draft September 10, 2025 06:19
@LagginTimes LagginTimes reopened this Sep 10, 2025
@LagginTimes LagginTimes moved this from Done to In Progress in BDK Chain Sep 10, 2025
@LagginTimes LagginTimes added this to the Wallet 3.0.0 milestone Sep 10, 2025
evanlinjin
evanlinjin requested changes Sep 10, 2025
View reviewed changes
evanlinjin
evanlinjin reviewed Sep 10, 2025
View reviewed changes
@LagginTimes LagginTimes mentioned this pull request Sep 10, 2025
Closed
6 tasks
@LagginTimes LagginTimes force-pushed the optional_data branch 2 times, most recently from cf98f58 to eddadd4 Compare September 10, 2025 17:26
evanlinjin
evanlinjin reviewed Sep 11, 2025
View reviewed changes
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.

.

@evanlinjin
Copy link
Member

Some recommended test scenarios:

  • Update chain that adds data to a placeholder checkpoint in the original chain.
  • Update chain which has a PoA with the original chain at a placeholder checkpoint (and vise-versa).

With the above tests:

  • Ensure the resultant chain has no "floating" checkpoints.

@LagginTimes LagginTimes mentioned this pull request Sep 12, 2025
Open
3 tasks
@LagginTimes LagginTimes force-pushed the optional_data branch 2 times, most recently from 942d438 to e1166b6 Compare September 13, 2025 11:43
LagginTimes and others added 3 commits September 16, 2025 04:30
@LagginTimes
feat(chain)!: make CheckPoint data field optional
b73dcd6
@evanlinjin @LagginTimes
fix(chain): Fix merge_chains logic
06b6d7e 
* Base tip must always have data.
* Update should be able to introduce data to a placeholder checkpoint in
  the original chain.
* Remove unnecessary logic.
@LagginTimes
test(chain): add test for merge_chains
46bd424
@LagginTimes LagginTimes marked this pull request as ready for review September 16, 2025 06:22
oleonardolima
oleonardolima reviewed Sep 16, 2025
View reviewed changes
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

cACK c24dd6a

It looking good code-wise, though we probably need further discussion on what's the best approach, as discussed by Evan and VM in discord.

@evanlinjin evanlinjin mentioned this pull request Sep 25, 2025
Closed
6 tasks
evanlinjin
evanlinjin requested changes Sep 25, 2025
View reviewed changes
@evanlinjin @claude @LagginTimes
test(core): add tests for CheckPoint::push and insert methods
a0cb974 
Add comprehensive tests for CheckPoint::push error cases:
- Push fails when height is not greater than current
- Push fails when prev_blockhash conflicts with self
- Push succeeds when prev_blockhash matches
- Push creates placeholder for non-contiguous heights

Include tests for CheckPoint::insert conflict handling:
- Insert with conflicting prev_blockhash
- Insert purges conflicting tail
- Insert between conflicting checkpoints

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
evanlinjin
evanlinjin reviewed Sep 29, 2025
View reviewed changes
crates/core/src/checkpoint.rs
///
/// This panics if called with a genesis block that differs from that of `self`.
#[must_use]
pub fn insert(self, height: u32, data: D) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Insert now looks really complicated. Do you have any ideas to simplify it?

oleonardolima
oleonardolima reviewed Oct 2, 2025
View reviewed changes
evanlinjin
evanlinjin reviewed Oct 11, 2025
View reviewed changes
crates/core/src/checkpoint.rs
if height <= self.height() {
return Err(self);
}

Copy link
Member

Choose a reason for hiding this comment

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

Let's prune any floating placeholder checkpoints from self here.

Rationale: It is public API to find a placeholder checkpoint and call push on it. If we push without pruning, we may result in a chain with 2 consecutive placeholder checkpoints.

Additional commentary: It is possible for a chain source to submit an update with a placeholder tip - and therefore not be persisted. This is the consequence of this optional-data checkpoint API. The benefits of this optional-data API is "less concepts". The alternative is the CheckPointEntry API (which introduces an extra concept), but avoids this oddity.

@evanlinjin
Copy link
Member

@LagginTimes please review the modifications I made to push and insert here: evanlinjin@1939dd2

This implementation builds upon and improves the original PR's approach to optional checkpoint data in several key ways:

  1. Cleaner insert logic
    The original PR had complex nested conditions for handling insertions. This version:

    • Splits the chain into base and tail components upfront
    • Uses a systematic rebuild approach that leverages push for validation
    • Handles all edge cases (displacement, purging, placeholder filling) in a unified flow

  2. Proper displacement handling

    • Clear distinction: When a prev_blockhash conflicts, the checkpoint is displaced (converted to placeholder) rather than purged, preserving the chain structure
    • Orphan management: Checkpoints that become orphaned after displacement are correctly purged
    • The original PR struggled with maintaining consistency when conflicts occurred at different points in the chain

  3. Placeholder cleanup in push

    • Added logic to remove trailing placeholder checkpoints before pushing new data
    • Prevents accumulation of unnecessary placeholders at the chain tip
    • Maintains cleaner chain structure over time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@evanlinjin evanlinjin evanlinjin requested changes

@oleonardolima oleonardolima oleonardolima left review comments

Requested changes must be addressed to merge this pull request.

Assignees

@LagginTimes LagginTimes

Labels

None yet

Projects

BDK Chain
Status: In Progress

Milestone

Wallet 3.0.0

Development

Successfully merging this pull request may close these issues.

Ensure CheckPoint chain methods validate and link via previous blockhash

3 participants

@LagginTimes @evanlinjin @oleonardolima