#35570 refactor: Change some validation.cpp methods to return BlockValidationState
https://github.com/bitcoin/bitcoin/pull/35570 · · +263/-226 in 16 files, 14 commits · labels: Refactoring
Goal
- Prevent subtle errors where validation outcome flags contradict internal validation state
- Eliminate shared mutable error state across sequential block validation steps
Refactors several core validation functions in validation.cpp (such as CheckBlock, AcceptBlock, ContextualCheckBlockHeader, and ProcessNewBlockHeaders) to return BlockValidationState by value rather than accepting it as an output reference parameter alongside a boolean return flag. Introduces MakeInvalid and MakeError static constructor helpers to BlockValidationState to streamline error path instantiation.
Problem: Previously, methods returned both a boolean and mutated a BlockValidationState output reference, creating the possibility that the boolean return value and state.IsValid() could disagree, or that chained calls could unintentionally interact through shared mutable state.
Category: Validation (#38 of 48)
P3 · cleanup
- P3 because it is an internal cleanup that does not change consensus rules or validation speed
- Benefit is modest, slightly reducing maintenance hazards when updating validation logic
P3 because this is an internal refactoring that improves code safety and readability without altering consensus rules or validation performance. Replacing out-parameters and dual return states with a single returned BlockValidationState eliminates subtle bugs where a boolean return and a validation state could diverge.
Membership: Directly modifies signatures and implementation of key validation functions in src/validation.cpp and consensus/validation.h.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1
Reviewability: Ready
- Ready for review as the branch merges cleanly and passes all automated tests
The branch merges cleanly against master, CI is passing, and the author actively addresses review comments.
Author status: Active, promptly responding to reviews and adopting reviewer suggestions.
Open concerns:
- hodlinator prefers enforcing valid/invalid distinctions via narrower return types or merging #35646 (which introduces util::Expected) prior to landing this refactor, rather than relying on runtime asserts/comments.
Resolved concerns:
- A potential behavior divergence where FlushStateToDisk return values were ignored in AcceptBlock was isolated and merged independently in #35621.
- arejula27 and w0xlt noted missing nodiscard attributes, const qualifiers, and a fuzz test bug introduced by setting state to Error before validation assertions; all addressed in updates.
Agreement: Positive
- Broad support across multiple contributors for simplifying validation return values
- Full code approval following review of edge cases and test fixes (arejula27)
- Nonblocking preference to land return-type improvements in #35646 first (hodlinator)
- Concept approval without stated reasons (w0xlt, purpleKarrot, stringintech, yuvicc, nervana21)
Broad support and one full code ACK; hodlinator suggests coordinating with #35646 but does not block.
The refactor has wide concept approval from multiple contributors. Detailed code review has been provided by arejula27 (ACK) and hodlinator, who raised architectural thoughts regarding #35646 without blocking progress.
- Concept ACKs from w0xlt, purpleKarrot, stringintech, yuvicc, nervana21.
- arejula27 provided repeated detailed reviews and ACKed the refactoring.
- hodlinator offered constructive feedback and a suggestions branch, noting a preference for #35646 landing first.
Review verdicts (DrahtBot): 0 (+1)
- Stale ACK: arejula27
- Concept ACK: w0xlt, purpleKarrot, stringintech, yuvicc, nervana21
Files
75 lines under test/bench/ci.
- src/validation.cpp +166/-149
- src/consensus/validation.h +24/-2
- src/net_processing.cpp +13/-12
- src/validation.h +12/-13
- src/test/fuzz/block.cpp +10/-13
- src/kernel/bitcoinkernel.cpp +4/-6
- src/node/blockstorage.cpp +7/-3
- src/test/chainstate_write_tests.cpp +4/-5
- src/test/validation_block_tests.cpp +7/-2
- src/test/validation_chainstate_tests.cpp +4/-5
- src/test/baseindex_tests.cpp +4/-3
- src/test/fuzz/utxo_snapshot.cpp +2/-4
- src/bench/checkblock.cpp +2/-3
- src/bench/duplicate_inputs.cpp +2/-2
- src/rpc/mining.cpp +1/-2
- src/test/util/mining.cpp +1/-2
Card
PR 35570 refactors several validation functions (including CheckBlock, AcceptBlock, and ProcessNewBlockHeaders) to return BlockValidationState by value rather than through an output parameter. This eliminates potential inconsistency between boolean return flags and validation states, making validation error propagation simpler and less prone to subtle bugs. The PR is an internal cleanup with no consensus or behavioral changes. Review is progressing well with wide Concept ACKs and one full ACK, alongside a non-blocking discussion on how this integrates with broader util::Expected efforts (#35646).