kernel: add btck_block_tree_entry

stickies-v commented at 3:22 pm on November 11, 2025: contributor

BlockTreeEntry objects are often compared. This happens frequently in our own codebase and seems likely to be the case for clients, too. Users can already work around this by comparing based on block hash (and optionally height as belt-and-suspenders), but I think this should be part of the interface for performance and consistency reasons.

Note: perhaps this is too ad-hoc, and we should extend this PR to add the operator for more types? BlockTreeEntry is the main one I’ve needed this for in developing py-bitcoinkernel, though.

DrahtBot added the label Validation on Nov 11, 2025

DrahtBot commented at 3:22 pm on November 11, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33855.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, maflcko, yuvicc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

in src/test/kernel/test_kernel.cpp:805 in c6b17f87ef

796@@ -797,6 +797,44 @@ BOOST_AUTO_TEST_CASE(btck_block_hash_tests)
797     CheckHandle(block_hash, block_hash_2);
798 }
799 
800+BOOST_AUTO_TEST_CASE(btck_block_tree_entry_tests)
801+{
802+    auto test_directory{TestDirectory{"block_tree_entry_test_bitcoin_kernel"}};
803+    auto notifications{std::make_shared<TestKernelNotifications>()};
804+    auto context{create_context(notifications, ChainType::REGTEST)};
805+    auto chainman{create_chainman(test_directory, false, false, true, true, context)};

maflcko commented at 4:06 pm on November 11, 2025:

would be nice to use clang-tidy named args for new code, instead of unnamed literal values

stickies-v force-pushed on Nov 11, 2025

stickies-v commented at 5:05 pm on November 11, 2025: contributor

Force-pushed to add clang-tidy argument names, addressing feedback.

DrahtBot added the label CI failed on Nov 11, 2025

DrahtBot removed the label CI failed on Nov 11, 2025

kernel: add btck_block_tree_entry_equals

BlockTreeEntry objects are often compared. By exposing an equality
function, clients don't have to implement more expensive
comparisons based on height and block hash.

096924d39d

in src/kernel/bitcoinkernel.cpp:1110 in 30916ecf1d outdated

1103@@ -1104,6 +1104,11 @@ const btck_BlockHash* btck_block_tree_entry_get_block_hash(const btck_BlockTreeE
1104     return btck_BlockHash::ref(btck_BlockTreeEntry::get(entry).phashBlock);
1105 }
1106 
1107+int btck_block_tree_entry_equals(const btck_BlockTreeEntry* entry1, const btck_BlockTreeEntry* entry2)
1108+{
1109+    return &btck_BlockTreeEntry::get(entry1) == &btck_BlockTreeEntry::get(entry2);
1110+}

sedited commented at 9:08 pm on November 11, 2025:

This just checks pointer equality. Do we really need a separate function for that? Maybe we can just handle this on the C++ wrapper layer?

stickies-v commented at 11:40 am on November 12, 2025:

The fact that CBlockIndex equality is currently defined as pointer equality seems like an implementation detail, tightly coupled with our block tree implementation. Exposing that as part of our public interface seems brittle and unergonomic? Wrappers should not contain business logic.

Logically, I think two BlockTreeEntry objects are equal if they point to the same Block. I’ve force-pushed a slight doc update to reflect that.

sedited commented at 11:50 am on November 12, 2025:

Logically, I think two BlockTreeEntry objects are equal if they point to the same Block. I’ve force-pushed a slight doc update to reflect that.

I would contest that - a block tree entry has a bunch of additional properties besides the block it points to that could make an equality operator by block confusing. Lets say we eventually introduce a way for the user to construct their own block tree entries. Would two such entries be equal if they have a bunch of different values in their fields, but point to the same block? Along the same lines I’m also not sure if this is a detail. If the operation within this function changes to i.e. block hash comparison, can the same contract still be enforced?

stickies-v commented at 12:33 pm on November 12, 2025:

Lets say we eventually introduce a way for the user to construct their own block tree entries. Would two such entries be equal if they have a bunch of different values in their fields, but point to the same block?

I’m not sure what that would mean conceptually. If we’re using the block tree to store validation status as we do now, that would mean we’d let the user define conflicting validation states about a block in the same index?

My conceptual understanding of a BlockTreeEntry is that it’s just an iterator into a block tree to efficiently let us look up information (e.g. serialized bytes, validation information, …) about that block, that wouldn’t be possible with e.g. just a block hash. The fact that CBlockIndex currently embeds, rather than lets us look up, some of that information (e.g. nStatus) seems like an implementation detail that we should not expose in the kernel API? If we then want to allow multiple validation states for the same block, the user could create multiple “validation trees” and use the same BlockTreeEntry to query or create validation information across those validation trees?

So yes, it seems to me like BlockTreeEntry should be equal when they refer to the same block.

stickies-v force-pushed on Nov 12, 2025

sedited approved

sedited commented at 1:33 pm on November 12, 2025: contributor

ACK 096924d39d644acc826cbffd39bb34038ecee6cd

fanquake requested review from laanwj on Nov 12, 2025

alexanderwiederin commented at 8:04 am on November 13, 2025: none

@stickies-v, what is the real benefit over a hash-based equality check?

I wonder if we can avoid committing to a pointer-based check here given it’s a non-trivial decision that is hard to reverse. The performance gain is negligible from what I understand.

What do you think?

in src/kernel/bitcoinkernel.cpp:1109 in 096924d39d

1103@@ -1104,6 +1104,11 @@ const btck_BlockHash* btck_block_tree_entry_get_block_hash(const btck_BlockTreeE
1104     return btck_BlockHash::ref(btck_BlockTreeEntry::get(entry).phashBlock);
1105 }
1106 
1107+int btck_block_tree_entry_equals(const btck_BlockTreeEntry* entry1, const btck_BlockTreeEntry* entry2)
1108+{
1109+    return &btck_BlockTreeEntry::get(entry1) == &btck_BlockTreeEntry::get(entry2);

stickies-v commented at 10:21 am on November 13, 2025:

response to #33855 (comment) @alexanderwiederin opening this thread for easier focus. Are you suggesting we 1) implement btck_block_tree_entry_equals with a hash equality check, or 2) to not expose btck_block_tree_entry_equals at all and let the user define it however they want (such as using hash equality), or 3) something else?

If 1), I don’t see the benefit? Pointer comparisons are faster, and are conceptually correct, and what we use internally. Because the implementation is opaque, we can refactor this however we like to, as long as we ensure that btck_block_tree_entry_equals returns true whenever the btck_BlockTreeEntry objects refer to the same block, as committed to in the (new) contract specified in the docstring.

If 2), I don’t think ambiguity is good. E.g. we could specify in the btck_BlockTreeEntry docstring that two entries are equal when their hash are equal, but relying on an actual interface is more ergonomic and safer, and the function is trivial to implement?

If 3) can you please elaborate on your concerns?

alexanderwiederin commented at 3:09 pm on November 13, 2025:

I’d prefer 1. The benefits are:

a) Self-enforcing contract - the implementation maintains the ‘same block’ semantics regardless of internal changes. b) Simpler reasoning - equality semantics are clear without needing much further context.

Regarding the pointer based approach: I don’t think the performance improvement justifies coupling the API to Core’s current internal architecture.

I can work with either approach - thought it was worth discussing the trade-offs.

stickies-v commented at 3:28 pm on November 13, 2025:

coupling the API to Core’s current internal architecture.

CBlockIndex is very much a kernel/validation class, not a node class (which I assume is what you mean with Core?). We’re also not coupling the API to any internals, the API has no knowledge of the pointer comparison.

b) Simpler reasoning - equality semantics are clear without needing much further context.

I feel like the current implementation is simple enough and very consistent with how validation works internally, but yeah that’s personal. CBlockIndex’s being singletons is not immediately obvious when not familiar with the codebase. If other people feel it’s worth trading off performance for their better understanding, I’m happy to consider it, but I don’t personally really see the benefit at this point.

maflcko commented at 3:43 pm on November 13, 2025: member

review ACK 096924d39d644acc826cbffd39bb34038ecee6cd 📓

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 096924d39d644acc826cbffd39bb34038ecee6cd 📓
3HYr1phQUu/wLKfZxvLvkGc495obLBK3h03iTKQSIUDTCuxWfUZdg/s84UIqC1MVVUJZjPPWMcvbonnHu2eGnCA==

?

added_to_project_v2 sedited

?

project_v2_item_status_changed sedited

yuvicc commented at 12:56 pm on November 17, 2025: contributor

Code Review ACK 096924d39d644acc826cbffd39bb34038ecee6cd

fanquake merged this on Nov 25, 2025

fanquake closed this on Nov 25, 2025

?

project_v2_item_status_changed github-project-automation[bot]

stickies-v deleted the branch on Nov 25, 2025

alexanderwiederin referenced this in commit 7334556a3f on Dec 1, 2025

stringintech referenced this in commit b7c3ec1af3 on Dec 12, 2025

kernel: add btck_block_tree_entry_equals #33855

Code Coverage & Benchmarks

Reviews