fix: bound mnListsCache admission to stop getmnlistd memory DoS - #62
Draft
PastaPastaPasta wants to merge 24 commits into
Draft
fix: bound mnListsCache admission to stop getmnlistd memory DoS#62PastaPastaPasta wants to merge 24 commits into
PastaPastaPasta wants to merge 24 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/v044
branch
from
August 1, 2026 18:25
bf91603 to
f2f1e9f
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.
FundTransaction always paid the change back to the payout script, which only works when that script is spendable. A test funding a governance proposal fee has to burn the amount to an OP_RETURN, so the change needs its own destination.
…xture The existing governance unit tests run on a fixture with no chain, so the tip masternode list is empty and every CGovernanceVote::IsValid() call short-circuits at GetMNByCollateral before any signature is verified. Nothing exercised CheckSignature, and nothing proved that a legitimately signed vote is accepted at all. Add a fixture that mines a regtest chain, registers a masternode via a real ProRegTx and keeps its voting (ECDSA) and operator (BLS) keys, so votes can be signed for real. On top of it: an orphan vote (parent object unknown) from a registered masternode is cached, requested and replayed onto the object once its fee collateral confirms and the proposal arrives; forged voting-key (ECDSA) and operator-key (BLS) signatures, unknown-masternode and future-dated votes are rejected with a peer penalty; and funding votes on a proposal are accepted only from the voting key while other signals accept the operator key. The orphan replay leaves the vote unindexed in cmapVoteToObject (so the inv relayed during replay cannot be served); the test pins that known gap so a future fix has to update it. Verified by mutation: inverting a masternode/signature gate in front of the orphan cache fails the orphan test, and making CheckSignature always succeed fails the rejection tests.
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
…th a chain-backed fixture 6a31b03 test: cover the governance vote signature path with a chain-backed fixture (pasta) 5c001ef refactor(utils): let FundTransaction return change to a separate script (pasta) Pull request description: ## Issue being fixed or feature implemented No governance unit test ever reaches `CGovernanceVote::CheckSignature`. The existing governance fixtures (e.g. `governance_inv_tests.cpp`) are built on `TestingSetup` with no chain, so `m_dmnman.GetListAtChainTip()` returns an empty masternode list and every `CGovernanceVote::IsValid()` call short-circuits at `GetMNByCollateral()` before any signature is verified. Consequently: * nothing proves a correctly signed vote from a registered masternode is accepted at all; * nothing proves a forged signature is rejected; * nothing proves the orphan-vote path (a vote that arrives before its parent object) actually recovers once the object shows up. That last one is the dangerous gap: a change that made vote acceptance stricter in the wrong place would silently break governance orphan-vote recovery entirely, and the unit tests would stay green. `feature_governance.py` does not deterministically produce a vote that arrives before its parent object, so it is not reliable coverage for that path either. ## What was done? Added `src/test/governance_vote_processing_tests.cpp`: a chain-backed fixture plus three test cases. No production code is touched. The fixture mines a regtest chain, activates DIP3, registers one masternode with a real `ProRegTx` and keeps its voting (ECDSA) and operator (BLS) keys, so votes can be signed for real and the signature checks are genuinely exercised. It also enables the tx index, which `CGovernanceObject::IsCollateralValid()` reads the proposal fee transaction from — without it no proposal can ever be accepted. Test cases: * `orphan_vote_is_cached_and_applied_when_parent_arrives` — a properly signed vote whose parent object is unknown is cached as an orphan, reported back as the object hash to request from the peer, and raises no peer penalty. The proposal fee is then burned to an `OP_RETURN` committing to the object hash and buried under the required confirmations, the proposal is accepted for real, and the orphan vote is replayed onto it (yes-count 1, orphan list empty). A peer re-sending the vote afterwards is not punished. * `unsigned_and_unknown_masternode_votes_are_rejected` — a forged signature, a vote from an outpoint belonging to no masternode, and a vote dated too far in the future are each rejected with a permanent error and a penalty of 20; a repeat of a vote already known to be invalid is still penalised; the object's vote count stays at zero. * `proposal_funding_votes_require_the_voting_key` — an operator-key-signed funding vote on a proposal is rejected (valid BLS signature, wrong key for that signal), a voting-key-signed funding vote is accepted and counted, an operator-key-signed `VALID` vote is accepted and counted, and a duplicate of an accepted vote is dropped without a penalty. The chain/ProRegTx plumbing comes from `src/test/util/masternode.h`, the shared module added in dashpay#7536, so nothing is duplicated here. One preparatory commit is needed for that: `FundTransaction()` always paid the change back to the payout script, which does not work for the proposal fee transaction — its payout is an `OP_RETURN` burn, and `IsCollateralValid()` rejects the transaction unless the change lands on a P2PKH output. An overload taking a separate change script covers that; the existing five-argument form keeps its behaviour and no existing call site changes. ## How Has This Been Tested? * `./src/test/test_dash --run_test=governance_vote_processing_tests` — passes, roughly 0.6s for all three cases. * `./src/test/test_dash --run_test=block_reward_reallocation_tests` and `--run_test=evo_dip3_activation_tests` — pass, covering the other users of the shared `FundTransaction()` helper. * Full `./src/test/test_dash` run — 793 cases, no errors. The pre-existing governance suites (`governance_inv_tests`, `governance_validators_tests`, `governance_superblock_tests`, `governance_vote_wire_tests`) and `evo_dip3_activation_tests` are unaffected. * `test/lint/all-lint.py` — clean apart from cppcheck warnings that already exist on develop in unrelated files; `clang-format` reports no differences on the new file. The tests were checked to actually bite, by mutation: * Adding a masternode/signature gate in front of the orphan cache in `CGovernanceManager::ProcessVote` with its condition inverted (so that legitimate votes are rejected) fails `orphan_vote_is_cached_and_applied_when_parent_arrives` on 7 assertions. * Making both `CGovernanceVote::CheckSignature` overloads return `true` unconditionally fails the other two cases on 15 assertions. * Forcing a fixture-constructor invariant to fail leaves the tx index torn down correctly and the failure contained to this suite: the three cases fail and `evo_dip3_activation_tests` / `txindex_tests` still pass in the same binary run. Environment: macOS (arm64), depends build, `--enable-debug`. ## Breaking Changes None. Test-only change. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] 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 Top commit has no ACKs. Tree-SHA512: a71ff1ff8179c476df38eaec1eb32868f5ec7f30167a59ef9b924e50e846eb346f7a43178fab3240af52a796e4d6f15d8a03d2e9decb5636a7740e7011bfb674
CDeterministicMNManager's in-memory caches (mnListsCache, mnListDiffsCache) are only trimmed by CleanupCache(), which runs when a new block arrives. Between blocks there is no bound: GETMNLISTDIFF accepts an arbitrary historical baseBlockHash and GetListForBlock appends a cache entry per requested block, so an unauthenticated peer requesting many distinct historical blocks drives cache growth without ceiling (a full mainnet MN list is several MB). Bound admission through new CacheMNList()/CacheMNListDiff() helpers: entries older than the recency window CleanupCache would drop anyway (height + LIST_DIFFS_CACHE_SIZE < tip) are not retained at all, and hard caps evict the lowest-height entries in a single pass (std::nth_element), never the tip snapshot. MAX_CACHE_LISTS = DISK_SNAPSHOT_PERIOD * 2: lists are rebuilt by applying up to DISK_SNAPSHOT_PERIOD - 1 diffs from the previous on-disk snapshot, so validation/invalidation spanning a snapshot boundary can keep two snapshot periods of lists resident without eviction thrash. MAX_CACHE_DIFFS = LIST_DIFFS_CACHE_SIZE + 64. The rebuild walk in GetListForBlockInternal() admits every diff it reads unconditionally so the apply loop can resolve every walked hash; the diff cap is enforced once after the walk completes, so eviction can never drop a diff the walk still needs. Add mn_lists_cache_bounded regression test: drive GetListForBlock over more distinct historical heights than the cap without running cleanup, assert both caches stay bounded, and prove eviction never changes a returned list by re-querying entries guaranteed to have been evicted.
PastaPastaPasta
force-pushed
the
sec/v044
branch
2 times, most recently
from
August 9, 2026 01:23
b3ffe31 to
aa93a4c
Compare
Keep a hard cap on recent masternode lists and diffs so unauthenticated historical GETMNLISTDIFF traffic cannot grow memory without bound between blocks. Add a small LRU tier for stale mini-snapshots so repeated requests for heights older than the recency window stay cheap after first warm-up, instead of re-walking up to ~575 diffs under cs_main on every call. Extend the unit test past the recency and diff-cap windows, and document the change in release notes.
PastaPastaPasta
force-pushed
the
sec/v044
branch
from
August 9, 2026 01:24
aa93a4c to
0340fcd
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 V044. An unauthenticated peer can grow
CDeterministicMNManager's in-memory caches without bound viagetmnlistd.Issue
mnListsCacheandmnListDiffsCachewere only trimmed byCleanupCache(), which runs when a new block arrives. Between blocks there is no bound at all: a peer spamminggetmnlistdfor historical blocks drivesGetListForBlockto append an entry per requested block, and a full mainnet MN list is several MB. No proof of work, no authentication, no rate limit on the request side.Fix
Bound admission two ways, in
src/evo/deterministicmns.{cpp,h}:ShouldRetainCacheHeight()— do not retain at all any height older than the recency windowCleanupCachewould have dropped anyway (height + LIST_DIFFS_CACHE_SIZE >= tipIndex->nHeight). Retains freely before the tip is known, for early startup.EnforceListsCacheLimit()/EnforceDiffsCacheLimit()— hard caps (MAX_CACHE_LISTS = 256,MAX_CACHE_DIFFS = LIST_DIFFS_CACHE_SIZE + 64) that evict the oldest-height entry, never the tip snapshot.Admission is funneled through new
CacheMNList()/CacheMNListDiff()helpers so no call site can bypass the bound.MAX_CACHE_LISTSis sized well above honest steady-state usage (tip + live quorum bases + mini-snapshots within the recency window).Tests
test: prove mnListsCache grows unboundedly via historical GetListForBlockprecedes the fix and fails without it.GetListCacheSize()/GetListDiffsCacheSize()accessors expose real production state rather than adding test-only mutation hooks.Review notes
The third commit is a review follow-up worth reading on its own: the initial fix evicted diffs during the rebuild walk, so a diff the apply loop still needed could be evicted mid-walk, landing on a bare
assert(false)— a remotely-triggerable crash, i.e. worse than the DoS being fixed. The follow-up removes the mid-walk eviction, admits every diff the walk reads unconditionally, and enforces the cap once the walk is done. It also deletes theassert(false)path.Two locking details, both deliberate:
CacheMNListprefersemplaceover assignment becauseCDeterministicMNList::operator=locksm_cached_sml_mutex, which must not run whilecsis held (lock-order checker).EXCLUSIVE_LOCKS_REQUIRED(cs)withAssertLockHeld.Based on
dashpay/dashdevelop @6d04c60ef36. Not rebase-tested against any newer tip; full functional suite not run.🤖 Generated with Claude Code