Kernel: Remove BlockTreeEntry from the API#43
Open
janb84 wants to merge 2 commits into
Open
Conversation
| typedef void (*btck_ValidationInterfaceBlockDisconnected)(void* user_data, btck_Block* block, const btck_BlockTreeEntry* entry); | ||
| typedef void (*btck_ValidationInterfacePoWValidBlock)(void* user_data, btck_Block* block, const btck_Chain* chain); | ||
| typedef void (*btck_ValidationInterfaceBlockConnected)(void* user_data, btck_Block* block, const btck_Chain* chain); | ||
| typedef void (*btck_ValidationInterfaceBlockDisconnected)(void* user_data, btck_Block* block, const btck_Chain* chain); |
There was a problem hiding this comment.
Since the block can be retrieved from the chain tip, should block even be passed in the callback?
Also, I wonder why the block is mutable here. Is this callback supposed to be able to modify the block?
| }; | ||
|
|
||
| class BlockTreeEntry : public View<btck_BlockTreeEntry> | ||
| class ChainView : public View<btck_Chain> |
There was a problem hiding this comment.
ChainView should inherit from std::ranges::view_interface<ChainView>.
|
|
||
| BlockTreeEntry GetAncestor(int32_t height) const | ||
| //! The longest common prefix of this chain and another, beginning at genesis. | ||
| std::optional<ChainView> Mismatch(const ChainView& other) const |
There was a problem hiding this comment.
mismatch should return std::ranges::mismatch_result.
Author
There was a problem hiding this comment.
Yes ok but performance, so what do you think about the compromise below. std::ranges::mismatch is O(n) this solution is O(log n) the 'compare' is walking from genesis to tip to find a mismatch (also side rant why from genesis ? such a deep reorg is unfeasible)
std::optional<ChainView> FindFork(const ChainView& other) const
{
auto fork{btck_chain_find_fork(get(), other.get())};
if (!fork) return std::nullopt;
return ChainView{fork};
}
std::ranges::mismatch_result<std::optional<ChainView>, std::optional<ChainView>>
Mismatch(const ChainView& other) const
{
auto fork{FindFork(other)};
int32_t fork_height{fork ? fork->Height() : -1};
std::optional<ChainView> in1{fork_height < Height() ? std::optional{GetAncestor(fork_height + 1)} : std::nullopt};
std::optional<ChainView> in2{fork_height < other.Height() ? std::optional{other.GetAncestor(fork_height + 1)} : std::nullopt};
return {in1, in2};
}(in this case btck_chain_mismatch() is renamed to btck_chain_find_fork() which also matches the result better)
Introduce a chain-oriented view of the block index that consumers can use instead of btck_BlockTreeEntry. A btck_Chain represents the linear sequence of blocks from tip back to genesis; because a block uniquely determines its path to genesis, a chain is fully determined by its tip and shares the same underlying CBlockIndex as the corresponding block tree entry. New chain functions: - btck_chainstate_manager_get_best_chain / _find return chains - btck_chain_get_block_header / _get_block_hash / _get_ancestor - btck_chain_get_parent (O(1) step towards genesis) - btck_chain_starts_with / _mismatch / _equals A new equivalence test to test the chain-based and entry-based queries return identical results (height, hash, header, lookup-by-hash, membership, block and spent-output reads, and previous-block stepping).
The previous commit introduced btck_Chain alongside btck_BlockTreeEntry
and proved the two are equivalent.
This removes the TreeEntry type
entirely so the API exposes a single block abstraction.
Removed:
- the btck_BlockTreeEntry type and its accessors (get_previous,
get_block_header, get_height, get_block_hash, equals, get_ancestor)
- btck_chainstate_manager_get_best_entry and
btck_chainstate_manager_get_block_tree_entry_by_hash
- btck_chain_get_by_height and btck_chain_contains
- the btck_chain_get_tip / btck_block_tree_entry_get_chain bridges
that allowed the two APIs to interoperate during migration
This makes the final state as described in
purplekarrot.net/blog/drop-block-tree-entry-from-bitcoin-kernel.html.
janb84
force-pushed
the
2140/blocktreeentry-chain-equivalence
branch
from
b66cc0e to
08ea314
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR replaces the
btck_BlockTreeEntrytype in the kernel C API with a single chain-oriented abstraction,btck_Chain, matching the end state described in https://purplekarrot.net/blog/drop-block-tree-entry-from-bitcoin-kernel.html.Since a block uniquely determines its path to genesis, a chain is fully determined by its tip and shares the same underlying CBlockIndex. Consolidating on
btck_Chainalso better hides the kernel's internals, exposing an opaque chain handle rather than leaking the block-index structure into the API (leaving the kernel free to change that without breaking the C API).New chain functions:
This design follows @purpleKarrot's writeup and parts of the approach are based on his blogpost ofc. happy to add a Co-authored-by: if he's comfortable with it (quality wise) .