fix: conflict ProReg collateral reuse with in-mempool masternode updates - #63
Draft
PastaPastaPasta wants to merge 21 commits into
Draft
fix: conflict ProReg collateral reuse with in-mempool masternode updates#63PastaPastaPasta wants to merge 21 commits into
PastaPastaPasta wants to merge 21 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PastaPastaPasta
force-pushed
the
sec/v054
branch
2 times, most recently
from
August 1, 2026 18:26
e31b6f8 to
1ad32c4
Compare
The cppcheck linter has been silently analyzing nothing (see next commit), letting several warnings in non-backported files accumulate. Fix the ones that the linter's ALWAYS_ENABLED_WARNINGS patterns force-report: remove unused/dead locals, make single-argument constructors explicit (with an inline suppression for CBLSIdImplicit, whose implicit conversion is intentional), narrow benchmark counters to the scope they are used in, pass CSigBase and BlsCheck constructor arguments by reference/move, and inline-suppress a danglingTempReference false positive on a lifetime-extended range-for temporary.
The linter has been vacuous in two ways. First, without __GNUC__ defined, src/attributes.h hits '#error No known always_inline attribute', which aborts cppcheck's analysis of nearly every translation unit; the resulting preprocessorErrorDirective lines were then dropped by the output filter because they don't point at files from non-backported.txt. Second, even with preprocessing fixed, cppcheck 2.17.1 crashes with an assertion in TokenList::setLang under --check-level=exhaustive, and that crash was explicitly suppressed. Define __GNUC__ so preprocessing succeeds, bump cppcheck to 2.21.0 (which no longer crashes with exhaustive checking) and drop the crash suppression, and treat analysis failures (preprocessorErrorDirective, syntaxError, internal errors) as lint failures regardless of which file they point at so the linter can never silently go vacuous again. Making syntaxError fatal immediately surfaced a real case: QT_VERSION_CHECK is a function-like macro cppcheck cannot evaluate, which aborted analysis of the Qt translation units, so define it on the command line too. Fail on any nonzero cppcheck exit status. Without --error-exitcode, diagnostics never make cppcheck return nonzero, so a nonzero status always means the analysis itself failed (bad arguments, unloadable config, OOM kill, crash) and must not pass. Filter out 'note:'/source-context lines, which don't carry the check id that suppressions match on and would leak through when their parent warning is suppressed, while matching all real diagnostic severities (the gcc template currently renders them all as 'warning:', but match the raw severities too in case that changes). Finally, suppress the check ids with pre-existing violations in the tree so the linter can be enforced; these should be burned down and re-enabled over time.
Avoid dangling reference in C++20 by binding the temporary RPCResult returned by CGovernanceObject::GetVotesJsonHelp to a named local variable before iterating over m_inner.
Define QT_CONFIG(x)=0 so cppcheck doesn't hit a fatal syntaxError when analyzing Qt translation units that include uic-generated headers (e.g. ui_masternodelist.h) in built working trees.
…port warnings dc99412 fix(lint): define QT_CONFIG macro in lint-cppcheck-dash (pasta) 9c39224 fix(rpc): bind temporary RPCResult to local variable in ListObjectsHelp (pasta) 9303de4 lint: re-enable uninitMemberVarNoCtor cppcheck (pasta) ab62dfe lint: re-enable shadowFunction cppcheck (pasta) 0bce8c3 lint: re-enable functionStatic cppcheck (pasta) 7bbba5d lint: re-enable knownConditionTrueFalse cppcheck (pasta) 99162fa lint: re-enable missingOverride cppcheck (pasta) 8155ea7 lint: re-enable constParameterReference cppcheck (pasta) 64ce038 lint: re-enable shadowMember cppcheck (pasta) 12c0711 lint: re-enable shadowVariable cppcheck (pasta) 964de2d lint: re-enable useInitializationList cppcheck (pasta) 47022ca lint: re-enable returnByReference cppcheck (pasta) c85b5f9 lint: re-enable constVariableReference cppcheck (pasta) 9477b4d lint: re-enable constVariablePointer cppcheck (pasta) 81b33d3 lint: re-enable assertWithSideEffect cppcheck (pasta) ddbfd7b fix(lint): make lint-cppcheck-dash actually report warnings (pasta) 1b2e756 fix: address cppcheck warnings hidden by vacuous lint-cppcheck-dash (pasta) Pull request description: ## Issue being fixed or feature implemented `test/lint/lint-cppcheck-dash.py` has been effectively vacuous — it ran cppcheck, silently analyzed almost nothing, and always passed. Two failure modes: 1. With the script's `-D` list, `__GNUC__` is not defined, so `src/attributes.h` hits `#error No known always_inline attribute` (`preprocessorErrorDirective`), aborting cppcheck's analysis of nearly every translation unit. The error lines were then dropped by the output filter, which only keeps lines pointing at files from `test/util/data/non-backported.txt`. 2. Even with preprocessing fixed, cppcheck 2.17.1 with `--check-level=exhaustive` crashes with an assertion in `TokenList::setLang` (child dies with signal 6) — and that crash message was explicitly listed in `SUPPRESSED_WARNINGS`. Net effect: an injected canary warning in a non-backported file was not flagged, locally or in CI. ## What was done? **Linter (`test/lint/lint-cppcheck-dash.py`):** - Define `__GNUC__` so preprocessing succeeds. - Add a `FATAL_ERRORS` list (`preprocessorErrorDirective`, `syntaxError`, internal errors/crashes) that fails the lint regardless of which file the line points at, so analysis failures can never again be silently filtered away. Making `syntaxError` fatal immediately surfaced real cases: `QT_VERSION_CHECK` and `QT_CONFIG` are function-like macros cppcheck cannot evaluate on its own, which aborted analysis of Qt translation units (especially in working trees with `uic`-generated headers like `ui_masternodelist.h`) — they are now defined on the cppcheck command line (`-DQT_VERSION_CHECK=...`, `-DQT_CONFIG(x)=0`). - Fail on any nonzero cppcheck exit status. Without `--error-exitcode`, diagnostics never make cppcheck return nonzero, so a nonzero status always means the analysis itself failed (bad arguments, unloadable config, OOM kill, crash) and cannot be allowed to pass. - Drop the signal-6 crash suppression (the TODO said to remove it with a newer cppcheck). - Skip `note:`/source-context lines: they don't carry the check id that suppressions match on, so orphaned notes of suppressed warnings leaked through the filter. - Suppress pre-existing violations so the linter can be enforced now, documented as a burn-down TODO. Most suppressions are deliberately narrow — targeted message/class-scoped regexes for `knownConditionTrueFalse` (always-false `state.Invalid(...)`/`state.Error(...)` returns), `shadowFunction` (the `_` translation function), and `uninitMemberVarNoCtor` (`ActiveDKG`/`UtilParameters` members) — so new violations of these checks elsewhere in the tree are still reported. `duplInheritedMember` and `useStlAlgorithm` are suppressed wholesale by check id: the former as a plain burn-down entry, the latter deliberately — earlier revisions of this branch rewrote the flagged raw loops into `std::ranges` algorithms, but lambda-based algorithms lose Clang thread-safety-analysis lock context (`cs_wallet`, `cs_coinjoin`, `cs_store`) and obscure otherwise-clear control flow, so the loops stay and the check is disabled instead. Messages matching `ALWAYS_ENABLED_WARNINGS` still override these suppressions. **Container (`contrib/containers/ci/ci-slim.Dockerfile`):** bump cppcheck 2.17.1 → 2.21.0, which no longer crashes under `--check-level=exhaustive`. **Code fixes** for the ~11 warnings that `ALWAYS_ENABLED_WARNINGS` patterns force-report (these cannot be suppressed by check id): removed dead locals (`src/active/dkgsession.cpp`, `src/rpc/evo.cpp`), made single-argument constructors `explicit` (`chainlock::Chainlocks`, `CDSTXManager`; no implicit-conversion call sites exist), inline-suppressed `CBLSIdImplicit`'s intentionally implicit constructor, narrowed three static benchmark counters to their usage scope in `src/evo/specialtxman.cpp` (static storage duration unchanged), passed `CSigBase` by const reference in `InitSession` (callers already sliced identically by value; accessors are non-virtual), moved `BlsCheck`'s by-value constructor parameters into members, and bound the temporary `RPCResult` returned by `GetVotesJsonHelp` to a local variable in `src/rpc/governance.cpp` (avoiding a C++20 dangling reference). ## How Has This Been Tested? With cppcheck 2.21.0 locally (matching the bumped container version): - Injected a canary (`unreadVariable`) into `src/spork.cpp`: the linter reports exactly that warning and exits 1. - Canary removed: full 273-file run is clean and exits 0 (tested both clean and built working trees). - Removed `-D__GNUC__` to simulate failure mode 1: the `attributes.h` `#error` line surfaces via `FATAL_ERRORS` and the linter exits 1. - All touched translation units pass `clang -fsyntax-only` with the project's compile flags; `test/lint/lint-python.py` passes. Note: the previously-suppressed `count_if`/`find_if` `useStlAlgorithm` variants are now covered by the wholesale id suppression along with the rest of the burn-down list. ## Breaking Changes None. CI lint may take somewhat longer since cppcheck now actually analyzes all 273 non-backported files with `--check-level=exhaustive`. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ Top commit has no ACKs. Tree-SHA512: 3da03772c968eebd5e2945cfc000c4249fc5cf630db1738f38dbb54bdc70e16fce8b851e984125cac1a1676acd5f0687610eecb3d1c7f24c21bb309ca2e91902
A ProRegTx that reuses a confirmed external collateral deletes the live MN mid-block. The same-block update for that proTxHash then fails BuildNewListFromBlock with bad-protx-hash and aborts CreateNewBlock. Assert the in-block hazard and that existsProviderTxConflict must reject either ordering in the mempool.
existsProviderTxConflict now links a replacement ProRegTx that reuses a live external collateral to any in-mempool ProUpServ/ProUpReg/ProUpRev for the MN being replaced, so both cannot coexist and CreateNewBlock cannot package the bad-protx-hash ordering. removeProTxConflicts also drops those updates when such a replacement is mined. Consensus block acceptance is unchanged; only mempool packaging/eviction is tightened.
The earlier fix inlined a copy of removeProTxSpentCollateralConflicts' inner loop into removeProTxConflicts, minus the diagnostic log on the should-never-happen branch. Hoist that loop into a named CTxMemPool::removeProTxReferences helper and call it from both sites, so the two paths that drop TXs naming a vanished MN cannot drift apart. Also add a removeForBlock assertion to the new test. existsProviderTxConflict only gates our own acceptance and cannot stop an attacker from mining the replacement ProRegTx themselves; the eviction hunk in removeProTxConflicts is what keeps the orphaned update from stalling our block assembly afterwards, and it previously had no coverage. Verified as a negative control: the new assertion fails (1 != 0) with that hunk disabled.
PastaPastaPasta
force-pushed
the
sec/v054
branch
from
August 8, 2026 18:03
1ad32c4 to
3256c90
Compare
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
1 participant
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.
Audit finding V054 (high, CONFIRMED — upgraded from medium).
Issue
A ProRegTx that reuses an external collateral currently backing a live masternode replaces that MN, so the MN ceases to exist. Mempool conflict detection did not account for this:
removeProTxSpentCollateralConflictsonly covers collateral spends, not reuse.The result is an in-block ordering hazard. If a replacement ProRegTx and an update targeting the replaced MN's
proTxHashare mined in the same block,BuildNewListFromBlockfails withbad-protx-hash— aborting block assembly.Fix
src/txmempool.cpp: inremoveProTxConflicts, when a ProRegTx reuses an external collateral, look up the MN that collateral currently backs and drop any mempool transaction still referencing itsproTxHash— such an update can never be mined afterwards.existsProviderTxConflictgains the matching acceptance-time check, so the conflicting update is rejected at entry rather than only evicted later.removeSpentCollateralConflictlambda is extracted to aCTxMemPool::removeProTxReferencesmember, since both call sites now need it. The body is unchanged.Tests
test: prove ProReg replacement conflicts with same-MN updatesprecedes the fix and fails without it. The review follow-up adds coverage for the replacement-eviction path specifically.Review notes
LogPrinton the "should not happen" branch. Reviewing it separately from the behavior change should be quick.dashpay/dashdevelop @6d04c60ef36. Not rebase-tested against a newer tip; full functional suite not run.🤖 Generated with Claude Code