nuke validation#27
Open
ismaelsadeeq wants to merge 6 commits into
Open
Conversation
ImportBlocks, which loads block files from disk during startup (-reindex
and -loadblock), does not belong in blockstorage.cpp alongside the
BlockManager class it merely uses. Move it to node/blockimport.{cpp,h}
so that each file has a single clear responsibility.
This is a pure move: no logic is changed. blockimport.cpp is given only
the includes it directly requires; util/result.h is removed from
blockstorage.cpp where it became unused once ImportBlocks left.
CheckFinalTxAtTip is a pure transaction finality check with no
dependency on chainstate. Move it to consensus/tx_verify.{cpp,h}
where it sits alongside the other transaction verification helpers.
DisconnectResult, ApplyTxInUndo, and UpdateCoins are UTXO mutation
primitives called during block (dis)connection. They depend only on
CCoinsViewCache and CTransaction, with no chainstate coupling. Move
them to coins.{cpp,h} alongside the existing AddCoins/SpendCoin
helpers they naturally extend.
These moves eliminate the need for validation.cpp callers to reach
into validation.h solely for these low-level helpers.
Moving ApplyTxInUndo and UpdateCoins into coins.h introduces a new
expected circular dependency: coins -> undo -> coins. undo.h includes
coins.h for Coin, and coins.h now includes undo.h for BlockUndo.
Extract mempool-related validation functions from validation.cpp into
new files mempool_validation.{h,cpp}:
- MempoolAcceptResult and PackageMempoolAcceptResult result types
- ProcessTransaction, AcceptToMemoryPool, ProcessNewPackage
- LimitMempoolSize, CalculateLockPointsAtTip, CheckSequenceLocksAtTip
- MaybeUpdateMempoolForReorg
Behavioral change: MaybeUpdateMempoolForReorg now takes an explicit
CTxMemPool& mempool parameter instead of obtaining it via
chainstate.GetMempool(). This is required because the Clang Thread
Safety Analyzer cannot alias *chainstate.MempoolMutex() to mempool.cs
when they are obtained via different expressions. The four call sites in
validation.cpp are guarded with `if (m_mempool)` null checks.
Introduces circular dependency:
mempool_validation -> validation -> mempool_validation
(validation.cpp still includes mempool_validation.h for the transition
period; to be resolved in a follow-up)
Extract block-related validation functions from validation.cpp into
new files block_validation.{h,cpp}:
- CheckBlockHeader, CheckBlock, BlockManuallyRequested
- ConnectBlock, DisconnectBlock, UpdateTip
- ActivateBestChainStep, ActivateBestChain
- InvalidateBlock, ReconsiderBlock
- GetSpendHeight, GetUTXOStats, VerifyScript helpers
- TestBlockValidity, BuildChainState
Also extract script checking infrastructure into script/script_check.{h,cpp}.
Introduces circular dependencies:
block_validation -> validation -> block_validation
block_validation -> validation -> mempool_validation -> block_validation
(validation.cpp still includes block_validation.h for the transition
period; to be resolved in a follow-up)
Rename node/chainstate.{cpp,h} to node/chainstate_load.{cpp,h} to avoid
a naming conflict with the upcoming rename of validation.{cpp,h} to
chainstate.{cpp,h}. The node/chainstate module hosts LoadChainstate()
and VerifyLoadedChainstate(), so chainstate_load better describes its
role.
After extracting mempool validation and block validation into their own
files, the code remaining in validation.{cpp,h} is exclusively
chainstate management: Chainstate, ChainstateManager, and their
supporting infrastructure. Rename the files to match what they
actually contain and update all include directives accordingly.
The rename also updates the names of existing circular dependencies in
the lint allowlist: node/blockstorage -> validation -> node/blockstorage
becomes chainstate -> node/blockstorage -> chainstate, and likewise for
kernel/coinstats. The previously introduced
block_validation -> validation -> block_validation and
mempool_validation -> validation -> mempool_validation are updated to
reflect the new filename.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
validation.cppisn't a misnomer; it's a validation file that includes block validation, mempool validation, chainstate management, consensus helpers, and friends well over 5,000 lines across multiple concerns.This makes it hard to navigate, and hard to reason about.
This PR breaks it apart by moving code to where it actually belongs.
mempool_validation.{h,cpp}block_validation.{h,cpp}CScriptChecktoscript/script_check.{h,cpp}consensus/tx_verify.{h, cpp}coins.{h, cpp}.node/blockimport.{h, cpp}Once the extraction is complete, what remains in
validation.{cpp,h}is exclusivelyChainstateandChainstateManager, so the files are renamed tochainstate.{h,cpp}to reflect what they actually contain. node/chainstate.{cpp,h} is renamed to node/chainstate_load.{cpp,h} beforehand, also for clarity.The circular dependencies here actually makes the underlying tight coupling more obvious.
chainstate.h, while chainstate.h needs types declared in block_validation.h.
The circular dependencies are added to the lint allowlist as temporary known exceptions.
The path to resolving these is to reduce what the extracted functions need to take as parameters. Rather than passing a full ChainstateManager&, functions in block_validation and mempool_validation can be refactored to take narrower dependencies, for example, passing a BlockManager& directly, or introducing a small context struct which will be defined in a separate file, so they no longer need to include chainstate.h at all.
That work is left as a follow-up to keep this PR a pure move.
Pros of the end goal of this are:
On this branch, there are cons because this is an intermediate state.
separation is done but the logical separation is not the full benefit requires significant follow-up work