fix: bound mnListsCache admission to stop getmnlistd memory DoS - #62
Draft
PastaPastaPasta wants to merge 10 commits into
Draft
fix: bound mnListsCache admission to stop getmnlistd memory DoS#62PastaPastaPasta wants to merge 10 commits into
PastaPastaPasta wants to merge 10 commits into
Conversation
Replace the assert in ReadKeyValue that compared the DB record's key type against CHDChain::IsCrypted() with a wallet load error. A wallet whose record type and payload disagree is corrupt, not a programmer bug, and should not abort the process. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dashpay#7424 bounded ChainlockHandler::seenChainLocks by switching it to an unordered_limitedmap constructed as `seenChainLocks{MAX_SEEN_CHAINLOCKS}`. unordered_limitedmap defaults nPruneAfterSize to nMaxSize, so prune() runs on every unique insertion past 1024 entries: it allocates a vector of all 1025 iterators, std::sort()s it in full, and erases a single element, all while ChainlockHandler::cs is held. ProcessNewChainLock records the CLSIG hash before both the stale-height early return and VerifyChainLock, and stale-height CLSIGs carry no misbehavior penalty, so a peer could turn a stream of unique, unverified CLSIG hashes into a stream of O(n log n) sorts under cs -- CPU amplification on the ChainLocks relay path. Construct the cache with an explicit prune-after size of twice the retained size instead. The map now grows to 2048 entries and is pruned back to 1024 in one batch, amortising each sort over 1024 evictions rather than one, at the cost of a larger transient cache. The 2x ratio matches the default already used by unordered_lru_cache. Note that pruning less often also reduces the chance that an entry is evicted by the same prune that inserted it, since entries inserted within one second share a timestamp and tie under prune()'s comparator. Stale CLSIG duplicate suppression is unchanged: hashes are still recorded before the stale-height return, and the 24h time-based Cleanup() is unaffected. Naming now distinguishes the retained size from the temporary prune threshold (MAX_SEEN_CHAINLOCKS -> SEEN_CHAINLOCKS_RETAINED_SIZE plus a new SEEN_CHAINLOCKS_PRUNE_AFTER_SIZE), and the constructor's defaulted nPruneAfterSize is documented as the footgun it is. Tests: two new limitedmap cases prove the generic container semantics -- no prune at retained max+1, growth bounded by the trigger, and a batch prune back to the retained size on crossing it -- plus the default-threshold behavior. The ChainLock handler test no longer asserts a strict instantaneous 1024 cap; it now verifies how the handler wires the cache up and that growth past the retained size is retained until the trigger is crossed. Verified the handler test fails when the fix is reverted.
|
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 |
Flag GitHub @username mentions in PR bodies so they are not copied into merge commits and re-notify people on merge, rebase, or backport. Use a dedicated Python helper with unit tests, preserve the existing required-check name, and allow complete email addresses.
64cf27d fix: prune ChainLock seen cache with hysteresis (PastaClaw) Pull request description: ## Issue being fixed or feature implemented Dash dashpay#7424 bounded `ChainlockHandler::seenChainLocks` with `unordered_limitedmap`, but the single-argument constructor makes the prune trigger equal to the 1,024-entry retained size. After the cache fills, every unique insertion creates and sorts a 1,025-element iterator vector and removes one entry while holding `ChainlockHandler::cs`. `ProcessNewChainLock()` records the hash before its stale-height return and before signature verification. A peer can therefore submit unlimited unique stale-height CLSIGs, receive no misbehavior penalty, and trigger the full sort on every message. ## What was done? - Constructed `seenChainLocks` with a 1,024-entry retained size and a 2,048-entry prune trigger, so one sort removes a batch of 1,025 entries instead of one. - Renamed the constants and test accessors to distinguish the retained size from the temporary prune threshold. - Documented `unordered_limitedmap`'s default prune behavior and exposed its configured threshold for focused tests. - Updated the generic limited-map and ChainLock tests to verify growth through the hysteresis window, batch pruning back to the retained size, stale CLSIG duplicate suppression, and best-ChainLock lookup after eviction. Stale CLSIG hashes are still remembered before the early return, and the existing 24-hour cleanup behavior is unchanged. ## How Has This Been Tested? Tested locally on macOS against current `develop`: - `./autogen.sh` and `./configure` with the existing `aarch64-apple-darwin` depends prefix - `make -C src test/test_dash` - `./src/test/test_dash --run_test=limitedmap_tests,llmq_chainlock_tests` - `./src/test/test_dash --run_test=limitedmap_tests,llmq_chainlock_tests,llmq_signing_tests,llmq_dkg_tests,evo_islock_tests` - `git diff --check upstream/develop..HEAD` - `COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py` - `test/lint/lint-circular-dependencies.py` - `test/lint/lint-include-guards.py` - Scoped clang-format verification for the changed ChainLock files - Negative control: restoring the single-argument constructor makes the new handler regression test fail; restoring the fix passes it - Exact-head pre-PR code-review gate: `ship`, zero findings ## Breaking Changes None. ## 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 _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 64cf27d Tree-SHA512: 90f014827163c1e898d39b90d7c88bd238d30d4cf9dfac15717dc4f260c5e1a1049f626895bdcf55ba0c74625535001fa602ba9403e11af79eeec375159328b1
Reviewers asked to keep this check simple; the helper itself is enough without a dedicated CI unit-test file.
8fbae2b fix: gracefully handle HD chain type mismatch on wallet load (PastaClaw) Pull request description: ## Issue being fixed or feature implemented Malformed wallet database records can currently trip a process assertion while loading HD chain state. A wallet database whose HD chain key type disagrees with the serialized chain encryption flag is corrupt input and should be reported as a wallet load error instead of aborting the process. ## What was done? Changed `ReadKeyValue` to validate `HDCHAIN` / `CRYPTED_HDCHAIN` records with a normal conditional check. When the key type and serialized `CHDChain` state do not match, wallet loading now returns a descriptive database error. Added focused unit coverage for both mismatch directions. ## How Has This Been Tested? Ran: ```bash git diff --check upstream/develop...HEAD src/test/test_dash --run_test=walletdb_hdchain_tests code-review dashpay/dash upstream/develop e85b88f74882b2ecf437cbce56441177e1955e11 "Reject mismatched HDCHAIN/CRYPTED_HDCHAIN wallet records gracefully instead of asserting during wallet load" ``` The pre-PR review gate returned `ship`. ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [ ] 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 _(for repository code-owners and collaborators only)_ ACKs for top commit: thepastaclaw: @PastaPastaPasta Ready for merge consideration on head `8fbae2b3`. - @UdjinM6 utACK'd this exact head on 2026-07-21. - Branch is mergeable against `develop`; PastaClaw gate and CodeRabbit are clean on this SHA. - The only remaining red jobs are the known unrelated functional-test flakes (`linux64-test` / `linux64_tsan-test`), already scoped as out-of-diff in earlier comments — no branch change needed for those. Happy to handle any follow-up if you want a different landing path. UdjinM6: Makes sense, utACK 8fbae2b Tree-SHA512: fdccad171b07c0878e5185a9acd09d8549d982bc637a4c025daa0cda86e37575f4996359b7402b135253a3af26bbd33c7a764febd528441203a13151ba13231e
9cde0fa ci: drop unit tests for PR description mention check (PastaClaw) 9bc7ed5 ci: reject @mentions in pull request descriptions (PastaClaw) Pull request description: ## Issue being fixed or feature implemented PR descriptions that include GitHub username mentions get copied into merge commits. After that, those people get notified again whenever the PR is merged, rebased, or backported. See dashpay#7493 for the motivating case ("Thanks knst" / similar mentions in a release PR description). This change makes CI fail when a PR description contains those mentions. ## What was done? Extended the existing semantic PR title workflow (`.github/workflows/semantic-pull-request.yml`) so it also validates the PR description: - Renamed the workflow/job to cover title and description - Kept the existing conventional-commit title check (`amannn/action-semantic-pull-request@v6`) unchanged - Added a step that reads `github.event.pull_request.body` via env on `pull_request_target` (`opened` / `edited` / `synchronize`), writes it to a file, and fails on GitHub-style `@username` mentions - Email addresses and empty descriptions are allowed - Failure message explains the merge/rebase/backport notification problem ## How Has This Been Tested? - YAML parses cleanly - `git diff --check upstream/develop...HEAD` clean - Local regex checks (same pattern as the workflow step): - `Thanks @knst.` → fail - `@PastaPastaPasta please look` → fail - `contact dev@example.com` → pass - `no mentions here` / empty body / bare `@` / `@@` diff markers → pass ## Breaking Changes None. Existing PRs with `@username` mentions in their description will fail this check on the next `edited` / `synchronize` event until the mentions are removed. ## 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: 0fa3cb00d87f59e162add5ad62d0fd7aba2373dd4e4c01e8f4745b8e4e951d4830878ca9b7760f133fe6a5915b49ebef9967e46cc05620417a78e3d49ca26429
…lock Add mn_lists_cache_bounded regression: after N distinct historical list loads without CleanupCache, mnListsCache size must stay <= MAX_CACHE_LISTS. Expose size getters and hard-cap constants for the assertion. Pre-fix fails with size 320 > 256.
Unauthenticated GETMNLISTDIFF / GetListForBlock could force arbitrary historical CDeterministicMNList snapshots into an uncapped mnListsCache until the next block advanced CleanupCache. Gate inserts with the existing LIST_DIFFS_CACHE_SIZE recency window and hard-cap list/diff caches with oldest-height eviction.
The rebuild walk in GetListForBlockInternal() enforced the diffs cap on every insertion, so a long walk could evict a diff that the very same walk still needed. The previous commit papered over this with a disk re-read plus a bare assert(false) in the apply loop, adding a failure mode that did not exist before the fix. Enforce the diffs cap once, after the apply loop, instead. The walk then always resolves from cache, so the .at() lookup is restored unchanged from upstream and the re-read/assert path is deleted. The cap still bounds the map, just at a walk boundary rather than mid-walk. Keep DISK_SNAPSHOT_PERIOD, DISK_SNAPSHOTS and LIST_DIFFS_CACHE_SIZE private; only the two new caps needed to become public for the test. Document why post-hoc CleanupCache() is insufficient. Extend the regression test to pin the property that actually matters: the caches are pure memoisation, so bounding them must not change results. It now records lists for a spread of historical heights, forces eviction by sweeping every height, and re-reads them to confirm the recomputed values are identical. Observed cache size after the sweep is exactly MAX_CACHE_LISTS (256) against 320 swept heights, so the cap is the binding constraint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PastaPastaPasta
force-pushed
the
sec/v044
branch
from
August 1, 2026 18:25
bf91603 to
f2f1e9f
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
2 participants
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