refactor: untangle assumevalid decision branches
What the developer wrote
Authored by Lőrinc
refactor: untangle assumevalid decision branches
Flatten nested conditionals into a linear gating sequence for readability and precise logging. No functional change, TODOs are addressed in next commit
What changed, and why it matters
This commit is a pure code cleanup in Bitcoin Core's block validation logic. It rewrites a set of nested 'if' checks into a flat sequence of 'if/else if' checks and adds named constants and TODO comments. The commit message explicitly says 'No functional change', and the logic appears identical to the original.
No security action needed. Treat as a normal readability refactor. Review the follow-up commit that addresses the TODOs for any functional or logging changes.
Security signals we found
No strong security signals were identified.
Evidence from the diff
In Chainstate::ConnectBlock(), the decision for whether to run script checks (fScriptChecks) under the assumevalid optimization was previously computed through nested conditionals. The patch flattens those into a linear guard sequence: null assumevalid, missing block index, ancestor mismatch on the assumed-valid chain, ancestor mismatch on the best header, insufficient chain work, and equivalent-time check. The final else branch sets fScriptChecks = false, matching the original behavior where all conditions had to pass to skip script checks. A constant TWO_WEEKS_IN_SECONDS replaces the literal 60602472. The TODO placeholders indicate logging will be added in a follow-up commit.
Changed components
src/validation.cppInspect captured patch +30 / −21
diff --git a/src/validation.cpp b/src/validation.cpp
index e3714192..010e0050 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2424,33 +2424,42 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
}
bool fScriptChecks = true;
- if (!m_chainman.AssumedValidBlock().IsNull()) {
+ if (m_chainman.AssumedValidBlock().IsNull()) {
+ // TODO
+ } else {
+ constexpr int64_t TWO_WEEKS_IN_SECONDS{60 * 60 * 24 * 7 * 2};
// We've been configured with the hash of a block which has been externally verified to have a valid history.
// A suitable default value is included with the software and updated from time to time. Because validity
// relative to a piece of software is an objective fact these defaults can be easily reviewed.
// This setting doesn't force the selection of any particular chain but makes validating some faster by
// effectively caching the result of part of the verification.
BlockMap::const_iterator it{m_blockman.m_block_index.find(m_chainman.AssumedValidBlock())};
- if (it != m_blockman.m_block_index.end()) {
- if (it->second.GetAncestor(pindex->nHeight) == pindex &&
- m_chainman.m_best_header->GetAncestor(pindex->nHeight) == pindex &&
- m_chainman.m_best_header->nChainWork >= m_chainman.MinimumChainWork()) {
- // This block is a member of the assumed verified chain and an ancestor of the best header.
- // Script verification is skipped when connecting blocks under the
- // assumevalid block. Assuming the assumevalid block is valid this
- // is safe because block merkle hashes are still computed and checked,
- // Of course, if an assumed valid block is invalid due to false scriptSigs
- // this optimization would allow an invalid chain to be accepted.
- // The equivalent time check discourages hash power from extorting the network via DOS attack
- // into accepting an invalid block through telling users they must manually set assumevalid.
- // Requiring a software change or burying the invalid block, regardless of the setting, makes
- // it hard to hide the implication of the demand. This also avoids having release candidates
- // that are hardly doing any signature verification at all in testing without having to
- // artificially set the default assumed verified block further back.
- // The test against the minimum chain work prevents the skipping when denied access to any chain at
- // least as good as the expected chain.
- fScriptChecks = (GetBlockProofEquivalentTime(*m_chainman.m_best_header, *pindex, *m_chainman.m_best_header, params.GetConsensus()) <= 60 * 60 * 24 * 7 * 2);
- }
+ if (it == m_blockman.m_block_index.end()) {
+ // TODO
+ } else if (it->second.GetAncestor(pindex->nHeight) != pindex) {
+ // TODO
+ } else if (m_chainman.m_best_header->GetAncestor(pindex->nHeight) != pindex) {
+ // TODO
+ } else if (m_chainman.m_best_header->nChainWork < m_chainman.MinimumChainWork()) {
+ // TODO
+ } else if (GetBlockProofEquivalentTime(*m_chainman.m_best_header, *pindex, *m_chainman.m_best_header, params.GetConsensus()) <= TWO_WEEKS_IN_SECONDS) {
+ // TODO
+ } else {
+ // This block is a member of the assumed verified chain and an ancestor of the best header.
+ // Script verification is skipped when connecting blocks under the
+ // assumevalid block. Assuming the assumevalid block is valid this
+ // is safe because block merkle hashes are still computed and checked,
+ // Of course, if an assumed valid block is invalid due to false scriptSigs
+ // this optimization would allow an invalid chain to be accepted.
+ // The equivalent time check discourages hash power from extorting the network via DOS attack
+ // into accepting an invalid block through telling users they must manually set assumevalid.
+ // Requiring a software change or burying the invalid block, regardless of the setting, makes
+ // it hard to hide the implication of the demand. This also avoids having release candidates
+ // that are hardly doing any signature verification at all in testing without having to
+ // artificially set the default assumed verified block further back.
+ // The test against the minimum chain work prevents the skipping when denied access to any chain at
+ // least as good as the expected chain.
+ fScriptChecks = false;
}
}
Why this scored 15/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.