#36353 kernel: document that blocks cache their check results
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36353 kernel: document that blocks cache their check results | Ready | 0 (+2) -1 | Mild | 11 | |||
|
|
|
| ||||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Document in the libbitcoinkernel C API headers that `btck_Block` instances cache context-free validation results. The header comments for `btck_block_check` and `btck_chainstate_manager_process_block` now warn callers that cached checks persist across shallow copies and that a block should only be used with a single set of consensus parameters.
Problem: In `libbitcoinkernel`, `btck_Block` wraps a shared `CBlock`. Once context-free checks pass under one set of consensus parameters, `CheckBlock` sets `fChecked`, causing subsequent checks or processing calls on that block (or its copies) under different parameters to skip validation and return success without warning the API consumer.
Evidence of importance:
- PR description: '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.'
Risk of a mistake: low (docs). This is a documentation-only change to the libbitcoinkernel public header.
Changed since the previous assessment: Switched from a code fix that created shallow unflagged copies of the block on every call to purely documentation notes in `src/kernel/bitcoinkernel.h` advising users to not reuse a block across multiple sets of consensus parameters.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: alexanderwiederin and others to review updated documentation in 3add4efcd2af 2026-10-09
Agreement
Mild
NACK from sedited not withdrawn (Paying a copy for every check call to protect against a scenario that should never happen is not worthwhile)
Thread notes: alexanderwiederin pointed out that CheckBlock caching requires POW and Merkle root checks, and that process_block always performs both checks; the author addressed both suggestions in updates 47cb9a682b and 3add4efcd2af.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| yuvicc (contributor) | suggestion, correctness | assert the reason for rejection in test_kernel.cpp | fixed · fix pushed · implemented in ac8a65d939 | answered | yes | 2026-09-30: 'could also assert the reason for rejection' Status from: 2026-09-30: FlashWayne: 'the signet test now also asserts BlockValidationResult::CONSENSUS' |
| yuvicc (contributor) | suggestion, interface | document in bitcoinkernel.h that the block is not modified and earlier checks do not affect results | fixed · fix pushed · documentation added in ac8a65d939 and maintained in docs-only rework 54b363a291 | answered | yes | 2026-09-30: 'documenting would be useful `src/kernel/bitcoinkernel.h`, in the docs of both `btck_block_check` and `btck_chainstate_manager_process_block`' Status from: 2026-09-30: FlashWayne: 'the note is added to the docs of btck_block_check and btck_chainstate_manager_process_block in bitcoinkernel.h' |
| sedited (member) | objection, approach | Paying a copy for every check call to protect against a scenario that should never happen is not worthwhile | fixed · fix pushed · author dropped the copy and switched PR to docs only in 54b363a291 | changed | yes | 2026-10-08: 'Paying a copy for every check call to protect against a scenario that should never happen, does not seem worthwhile to me.' Status from: 2026-10-08: FlashWayne: 'I dropped the copy and the tests. The PR is now a single docs commit (54b363a291, rebased on current master' |
| alexanderwiederin (member) | suggestion, correctness | Narrow the description to specify that the cache only takes effect when POW and Merkle root are checked | fixed · fix pushed · author narrowed both doc comments in 47cb9a682b | answered | yes | 2026-10-09: 'I think the description is too broad. The cache only takes effect when POW and the Merkle root are checked.' Status from: 2026-10-09: FlashWayne: 'I narrowed both doc comments accordingly in 47cb9a682b.' |
| alexanderwiederin (member) | suggestion, correctness | Drop the clause from btck_chainstate_manager_process_block since ProcessNewBlock sets fCheckPOW and fCheckMerkleRoot to true | fixed · fix pushed · author dropped the clause from process_block docs in 3add4efcd2af | answered | yes | 2026-10-09: 'You don't have to add the clause here. `ProcessNewBlock` calls `CheckBlock` with `fCheckPOW` and `fCheckMerkleRoot` set to their default `true`.' Status from: 2026-10-09: FlashWayne: 'dropped the clause from the `process_block` docs in 3add4efcd2af' |
Support:
- yuvicc (contributor) (Concept ACK, 2026-09-30): Good observation. [not substantive]
- yuvicc (contributor) (ACK, 2026-09-30) [read: src/kernel/bitcoinkernel.cpp]: 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
- l0rinc (member) (Concept ACK, 2026-10-01): (no reason given) [not substantive]
- jcastros (contributor) (ACK, 2026-10-03) [tested]: tested ac8a65d939 with -DBUILD_KERNEL_LIB=ON; tests failed with parent bitcoinkernel.cpp and passed with the PR commit
Participants:
- yuvicc (contributor, support, 4 statements 2026-09-30 to 2026-09-30): Concept ACK and ACK; suggested asserting rejection reason and documenting kernel behavior
- l0rinc (member, support, 1 statements 2026-10-01 to 2026-10-01): Concept ACK
- jcastros (contributor, support, 1 statements 2026-10-03 to 2026-10-03): ACK after testing against parent commit
- sedited (member, objection, 1 statements 2026-10-08 to 2026-10-08): Approach NACK on copying the block on every call; author switched to docs-only
- alexanderwiederin (member, neutral, 2 statements 2026-10-09 to 2026-10-09): pointed out cache conditions and suggested refining doc phrasing
Files
0 lines under test/bench/ci.
- src/kernel/bitcoinkernel.h +11/-0
Card
The short text the ranking pass compares across PRs.
Documents that `btck_Block` instances cache context-free validation results in the libbitcoinkernel C API. Because `btck_Block` wraps a shared `CBlock` and sets `fChecked`, verifying a block under one set of consensus parameters causes subsequent validation under different parameters to skip checks and pass erroneously. This PR warns API consumers in header documentation to only evaluate a block against a single set of consensus parameters rather than altering validation behavior in code.
Data
thread read through 2026-10-09 (14 statements), head 3add4efcd2
labels: Validation
Categories
Kernel (libbitcoinkernel)
P3 · cleanup (ranking pass: Moved up two bands' worth of positions relative to its score because it warns of a silent validation bypass (cached fChecked across parameter sets), a correctness hazard for consumers rather than ordinary doc polish.)
Clarifies API boundary invariants and caveats for library consumers. The comments warn callers that `btck_Block` instances cache context-free checks via internal `fChecked` flags that persist across shallow copies, preventing unexpected validation bypasses when reusing block objects across differing consensus parameters.
Membership: Changes `src/kernel/bitcoinkernel.h` to document C API block validation caching behavior.
Factors: security/stability 0, bug 0, performance 0, user value 1, leverage 0