feat(core)!: Add prev_blockhash to trait ToBlockHash

[ValuedMammal]

prev_blockhash returns the hash of the previous block data, if it is known, and None otherwise.

Implement prev_blockhash for Header, which returns the header's previous blockhash.

Fixed CheckPoint::push to now check that if the height being pushed directly follows the current CP height, and the data.prev_blockhash contains some value, the previous hash of data is the same as the current CP hash. This is necessary to prevent inconsistent or invalid chains from being constructed.

fix #2021

Description

Notes to the reviewers

Changelog notice

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]

How does this relate to PR #2024? That PR addresses the same issue and handles validation in insert() and merge_chains as well. As written, this only validates push() with contiguous heights - insert() and merge_chains can still create inconsistent chains. Is this intended as a simpler alternative approach, or a foundation for the work in #2024?

I'm also wondering if this is meant to show that we can achieve simpler validation by not introducing optional data fields. However, there is a gap: without tracking "ghost checkpoints" (positions inferred from prev_blockhash but not yet populated), we can't properly validate in insert() or merge_chains().

For example, if I insert a checkpoint at height 100 with prev_blockhash = X, and later insert at height 99 with blockhash = Y where X != Y, we need to detect that conflict. Either:

• We materialize the ghost as a placeholder node (data: Option) so the conflict is caught at insertion (feat(chain)!: make CheckPoint data field optional #2024 ), or
• We use a custom iterator that yields ghost checkpoints during traversal/merge, validating at that boundary (feat!(chain): implement CheckPointEntry WIP #2017 )

The current PR doesn't address either path, so I don't think it fully resolves #2021. Is there a different approach you have in mind for handling these cases?

[ValuedMammal]

Here we don't intend to make the CheckPoint::data field optional, instead displacing a block will just remove it from the chain.

It's missing coverage for when insert properly displaces a conflicting block at the previous height.

The previous BlockId inferred by the current chain could in theory be implemented directly on the CheckPoint like this. I haven't tackled the issues with merge_chains yet.

/// Get the previous [`BlockId`] of this checkpoint.
///
/// I.e. the height and hash of the block immediately preceding this one by consensus,
/// if it can be inferred from the `prev_blockhash` of the inner block data.
pub fn prev_block_id(&self) -> Option<BlockId> {
    let prev_height = self.height().checked_sub(1)?;
    let prev_hash = self.0.data.prev_blockhash()?;
    Some(BlockId {
        height: prev_height,
        hash: prev_hash,
    })
}

[evanlinjin]

Since you're taking over ticket #2021, feel free to checkout my unfinished work for inspiration: https://github.com/bitcoindevkit/bdk/compare/cp_entry.

[evanlinjin]

We should also displace the tail if:

• data's prev_blockhash conflicts with the block directly below. The block directly below should also be displaced.
• data's blockhash conflicts with the the prev_blockhash of the block directly above.

To achieve this, I recommend using CheckPointEntry instead of CheckPoint and add a method such as CheckPointEntry::prev() - this requires the CheckPointEntry::BlockId variant to have a CheckPoint field.

Naming suggestion

   /// An entry yielded by [`CheckPointEntryIter`].
   #[derive(Debug, Clone)]
   pub enum CheckPointEntry<D> {
       /// A placeholder entry: there is no checkpoint stored st this height,
       /// but the checkpoint one height above links back here via it's `prev_blockhash`.
       Placeholder {
           /// The block ID at *this* height.
           block_id: BlockId,
           /// The checkpoint one height *above* that links back to `block_id`.
           checkpoint_above: CheckPoint<D>,
       },
       /// A real checkpoint recorded at this height.
       Occupied(CheckPoint<D>),
   }

[evanlinjin]

The refactored merge_chains does not do anything meaningfully different. It also does not make it easier to read.

The important change to make is to make sure merge_chains takes prev_blockhashes into consideration. The refactoring does not do that.

Suggested changes

• Iterate over CheckPointEntry instead of CheckPoint. Rationale: prev_blockhashes are also block hashes so they can be points of agreements/invalidation.
• Only insert Some(data) into ChangeSet if there is an actual CheckPoint in the update chain at that height.
• Only insert None into ChangeSet if there is an actual CheckPoint in the original chain at that height.

Suggested tests

These tests uses a data type that returns Some for prev_blockhash:

prev_blockhash invalidates

• Original: 1:A, 2:B, 4:D
• Update: 1:A, 3:C' (prev=2:B')
• Result: 1:A, 3:C' (prev=2:B')

Connects due to prev_blockhash

• Original: 2:B, 3:C
• Update: 2:B, 4:D (prev=3:C)
• Result: 2:B, 3:C, 4:D