kernel: add btck_block_tree_entry_equals #33855

1. 
   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.

2. DrahtBot added the label Validation on Nov 11, 2025
3. 
   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

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

4. 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
5. stickies-v force-pushed on Nov 11, 2025
6. 
   stickies-v commented at 5:05 pm on November 11, 2025: contributor

   Force-pushed to add clang-tidy argument names, addressing feedback.
7. DrahtBot added the label CI failed on Nov 11, 2025
8. DrahtBot removed the label CI failed on Nov 11, 2025
9. 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
10. 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+}
    

    ---

    ---

    

    TheCharlatan 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.

    ---

    

    TheCharlatan 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.

11. stickies-v force-pushed on Nov 12, 2025
12. TheCharlatan approved
13. 
    TheCharlatan commented at 1:33 pm on November 12, 2025: contributor

    ACK 096924d39d644acc826cbffd39bb34038ecee6cd
14. fanquake requested review from laanwj on Nov 12, 2025
15. 
    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?

Contributors
stickies-v DrahtBot maflcko TheCharlatan alexanderwiederin

Review Requested
laanwj

Labels
Validation