kernel: document that blocks cache their check results #36353

1. 
   FlashWayne commented at 10:36 PM on September 26, 2026: contributor

   CheckBlock returns true right away when block.fChecked is set. In the kernel API a btck_Block wraps a shared CBlock, and btck_block_copy only adds a reference, so once a block passed btck_block_check or btck_chainstate_manager_process_block, later calls on it (or on a copy) skip the checks, whatever consensus params or flags are passed. For example a regtest block that has passed once with regtest params also returns 1 from btck_block_check with mainnet params.

   bitcoind isn't affected, since it only has one chainstate manager and one set of params. btck_block_check and custom signet params are new in 32.0, and the header docs don't mention this.

   Following sedited's review, this no longer copies the block on every call. It only documents on btck_block_check and btck_chainstate_manager_process_block that a block caches its check results and should only be used with one set of consensus parameters.

2. DrahtBot added the label Validation on Sep 26, 2026
3. 
   DrahtBot commented at 10:36 PM on September 26, 2026: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   External sites

   • Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36353
   • Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36353.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36353#author-ralph

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline and AI policy for information on the review process.

   Type	Reviewers
   Concept ACK	l0rinc
   Approach NACK	sedited
   Stale ACK	yuvicc, jcastros

   If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   No conflicts as of last run.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

4. DrahtBot added the label CI failed on Sep 28, 2026
5. DrahtBot removed the label CI failed on Sep 29, 2026
6. 
   yuvicc commented at 5:54 AM on September 30, 2026: contributor

   Concept ACK! Good observation.

7. in src/test/kernel/test_kernel.cpp:1117 in 9d735dae13 outdated

   1112 | +    BOOST_CHECK_EQUAL(process("51", "signet_op_true_test_bitcoin_kernel"), 1);
   1113 | +    // The same block object must be rejected by a signet with another challenge.
   1114 | +    BOOST_CHECK_EQUAL(process("6a", "signet_op_return_test_bitcoin_kernel"), 0);
   1115 | +    BlockValidationState state;
   1116 | +    ChainParams op_return_params{hex_string_to_byte_vec("6a")};
   1117 | +    BOOST_CHECK(!block.Check(op_return_params.GetConsensusParams(), BlockCheckFlags::ALL, state));
   

   ---

   ---

   

   yuvicc commented at 6:38 AM on September 30, 2026:

   could also assert the reason for rejection:

   BOOST_CHECK(state.GetBlockValidationResult() == BlockValidationResult::CONSENSUS);
   

8. in src/kernel/bitcoinkernel.cpp:1247 in 9d735dae13

   ---

   ---

   

   yuvicc commented at 7:06 AM on September 30, 2026:

   documenting would be useful src/kernel/bitcoinkernel.h, in the docs of both btck_block_check and btck_chainstate_manager_process_block:

    * The block is not modified, and earlier checks of it (e.g. under other
    * consensus params) do not affect the result.
   

9. 
   yuvicc commented at 7:08 AM on September 30, 2026: contributor

   cr-ACK 9d735dae130dff17153c0aa5678b5cfb1999f00d

   The only overhead is that a block passed in a second time re-runs the context-free checks instead of returning early; for a duplicate process_block call, AcceptBlock still returns early on fAlreadyHave, so nothing is re-stored or re-connected.

   Left some nits.

10. FlashWayne force-pushed on Sep 30, 2026
11. 
    FlashWayne commented at 7:39 AM on September 30, 2026: contributor

    Thanks for the review! Took both suggestions in ac8a65d939: the signet test now also asserts BlockValidationResult::CONSENSUS, and the note is added to the docs of btck_block_check and btck_chainstate_manager_process_block in bitcoinkernel.h.

12. 
    l0rinc commented at 7:02 PM on October 1, 2026: contributor

    Concept ACK

13. 
    jcastros commented at 5:46 PM on October 3, 2026: contributor

    I tested ac8a65d939 with -DBUILD_KERNEL_LIB=ON. I ran the new tests with bitcoinkernel.cpp from the parent, ed7dd7cf4e, and test_kernel failed. When I put this commit's file back, it passed.

    The description says 4 failures, and I saw 5. The extra check is the CONSENSUS result at test_kernel.cpp:1118, which wasn't in 9d735dae.

    Should this be backported to 32.x? btck_block_check is new in that release.

    ACK ac8a65d939

14. DrahtBot requested review from yuvicc on Oct 3, 2026
15. DrahtBot requested review from l0rinc on Oct 3, 2026
16. 
    FlashWayne commented at 6:23 PM on October 3, 2026: contributor

    Thanks for testing! You're right, the fifth one is the CONSENSUS assertion I added after yuvicc's review; I updated the description.

    On backporting: btck_block_check, btck_chainstate_manager_process_block and btck_chain_parameters_create_signet are all in 32.x, so the issue is present in the 32.0 release and I think a backport makes sense. Happy to open one if maintainers want it.

17. DrahtBot added the label Needs rebase on Oct 8, 2026
18. 
    sedited commented at 4:34 PM on October 8, 2026: contributor

    Approach NACK

    I don't think this is a worthwhile fix. The same problem exists in the normal unit tests as well, and shouldn't be fixed there with this approach either in my opinion. Paying a copy for every check call to protect against a scenario that should never happen, does not seem worthwhile to me.

19. kernel: document that blocks cache their check results

    A btck_Block caches the result of its context-free checks, and copies
    made with btck_block_copy share that cache. Once a block passed
    btck_block_check or btck_chainstate_manager_process_block, later calls
    with other consensus parameters or flags are not checked again.
    
    Document that a block should only be used with a single set of
    consensus parameters, instead of paying for a copy on every call.

    54b363a291
20. FlashWayne force-pushed on Oct 8, 2026
21. FlashWayne renamed this:
    kernel: don't reuse cached CheckBlock results across params
    kernel: document that blocks cache their check results
    on Oct 8, 2026
22. 
    FlashWayne commented at 5:26 PM on October 8, 2026: contributor

    Thanks for the review, sedited. You're right: a copy on every call isn't worth it for a case that shouldn't happen, and the unit tests have the same property.

    I dropped the copy and the tests. The PR is now a single docs commit (54b363a291, rebased on current master, which also clears the conflict). It states on btck_block_check and btck_chainstate_manager_process_block that a btck_Block caches its check results, that copies from btck_block_copy share the cache, and that a block should only be used with one set of consensus parameters.

    I changed the title and description to match. If you'd rather not have the note either, I'm happy to close this.

23. DrahtBot removed the label Needs rebase on Oct 8, 2026
24. sedited removed review request from yuvicc on Oct 9, 2026
25. sedited requested review from alexanderwiederin on Oct 9, 2026
26. kernel: limit cache docs to blocks checked for POW and merkle root

    The cache is only set if both the proof of work and the merkle root
    were checked.

    47cb9a682b
27. in src/kernel/bitcoinkernel.h:1465 in 54b363a291 outdated

    1457 | @@ -1453,6 +1458,11 @@ typedef uint32_t btck_BlockCheckFlags;
    1458 |   * transaction script, timestamps, order, or other checks that may require more
    1459 |   * context.
    1460 |   *
    1461 | + * Note that a btck_Block caches the result of its context-free checks, and
    1462 | + * copies made with btck_block_copy share that cache. A block that was checked
    1463 | + * or processed once is not checked again, so a block should only be used with
    1464 | + * a single set of consensus parameters.
    1465 | + *
    

    ---

    ---

    

    alexanderwiederin commented at 12:03 PM on October 9, 2026:

    I think the description is too broad. The cache only takes effect when POW and the Merkle root are checked.

    https://github.com/bitcoin/bitcoin/blob/4bacf21a13c2ed25ef9362ca38f26bf0a67d22c9/src/validation.cpp#L3987-L3988

28. 
    FlashWayne commented at 12:37 PM on October 9, 2026: contributor

    Thanks @alexanderwiederin, you're right. The cache is only set when both the proof of work and the merkle root were checked. I narrowed both doc comments accordingly in 47cb9a682b. Happy to squash if preferred.

29. kernel: drop POW/merkle clause from process_block docs

    btck_chainstate_manager_process_block always checks the proof of work
    and the merkle root, so the clause is only needed for btck_block_check.

    3add4efcd2
30. in src/kernel/bitcoinkernel.h:1355 in 47cb9a682b

    1349 | @@ -1350,6 +1350,12 @@ BITCOINKERNEL_API int BITCOINKERNEL_WARN_UNUSED_RESULT btck_chainstate_manager_i
    1350 |   * be retrieved by registering the `block_checked` callback in the validation
    1351 |   * interface.
    1352 |   *
    1353 | + * Note that a btck_Block caches the result of its context-free checks, and
    1354 | + * copies made with btck_block_copy share that cache. The cache is only set if
    1355 | + * both the proof of work and the merkle root were checked. Such a block is not
    

    ---

    ---

    

    alexanderwiederin commented at 3:33 PM on October 9, 2026:

    You don't have to add the clause here. ProcessNewBlock calls CheckBlock with fCheckPOW and fCheckMerkleRoot set to their default true.

    ---

    

    FlashWayne commented at 3:51 PM on October 9, 2026:

    Thanks, dropped the clause from the process_block docs in 3add4efcd2af (kept it only for btck_block_check, and fixed the @param alignment).

Contributors

FlashWayneDrahtBotyuviccl0rincjcastrosseditedalexanderwiederin

Review Requested
l0rinc