fix(net): amortize ChainLock seen-cache pruning - #7482
Merged
PastaPastaPasta merged 1 commit intoAug 1, 2026
Merged
Conversation
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.
Author
|
✅ Final review complete — no blockers (commit 64cf27d) |
thepastaclaw
commented
Jul 25, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Author
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The change correctly configures the ChainLock seen cache to retain 1,024 entries while allowing growth to 2,048 before batch pruning, amortizing the peer-triggerable sort cost without changing stale CLSIG duplicate suppression or cleanup behavior. The generic container semantics, handler integration, focused tests, commit history, and exact-head diff are internally consistent, with no correctness or Dash-specific integration issues identified.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— general (completed)
PastaPastaPasta
marked this pull request as ready for review
July 29, 2026 00:39
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Walkthrough
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Aug 2, 2026
…ning bounded caches b498b67 docs: stop describing bounded-cache pruning as sorting (pasta) e991ef9 test: add unit tests for unordered_lru_cache (pasta) b0b1dba perf: use std::nth_element instead of full sort when pruning bounded caches (pasta) Pull request description: ## Issue being fixed or feature implemented Two bounded caches do strictly more work than they need to when they evict: - `unordered_limitedmap::prune()` (`src/limitedmap.h`) collects an iterator to every entry in the map and fully sorts that vector ascending by mapped value, then throws away everything except the `tooMuch = size - nMaxSize` smallest entries. - `unordered_lru_cache::truncate_if_needed()` (`src/unordered_lru_cache.h`) does the same thing with a full descending sort by access counter, then erases everything past index `maxSize`. In both cases the total order is discarded immediately. All that is actually needed is a partition around the eviction boundary, so the sort is `O(n log n)` work where `O(n)` suffices. This matters because these caches are not small: `unordered_lru_cache` instances in the tree are sized at 10000 (`islockCache`, `txidCache`, `outpointCache`) and 30000 (`hasSigForIdCache`, `hasSigForSessionCache`, `hasSigForHashCache`), and truncation runs on the insert path. This came out of review of #7482, which also touches `src/limitedmap.h`. This PR is deliberately scoped so the two can merge in either order without conflicting: it only rewrites the body of `prune()`, leaves the constructor and the `max_size()` / prune-threshold accessors alone, and does not touch `src/test/limitedmap_tests.cpp` at all. The existing limitedmap tests already pin the prune membership semantics that must be preserved. ## What was done? Replaced `std::sort` with `std::nth_element` in both eviction paths, using the same comparator in each case: - `unordered_limitedmap::prune()`: `tooMuch` is now computed before the partition, and `std::nth_element(begin, begin + tooMuch, end, cmp)` places the `tooMuch` smallest entries in the leading range. The subsequent `resize(tooMuch)` and erase loop are unchanged. - `unordered_lru_cache::truncate_if_needed()`: `std::nth_element(vec.begin(), vec.begin() + maxSize, vec.end(), cmp)` leaves the `maxSize` most recently accessed entries in the leading range, and the existing loop erases everything from `maxSize` onward. This is behaviour preserving for the *set* of evicted entries. The relative order within the evicted group and within the retained group was never observable, and ties between equal values were already broken arbitrarily because `std::sort` is not a stable sort. Two supporting changes: - Added `assert(truncateThreshold >= maxSize)` to the `unordered_lru_cache` constructor. `vec.begin() + maxSize` is only a valid iterator because truncation runs only when `size > truncateThreshold`, so that invariant now has to hold rather than merely happening to hold. Every instantiation in `src/` passes only `maxSize` and therefore gets the default `truncateThreshold = 2 * maxSize`, so no caller is affected. `unordered_limitedmap` already has the equivalent `assert(nPruneAfterSize >= nMaxSize)`. - Added `src/test/unordered_lru_cache_tests.cpp`. The class previously had no unit test coverage at all. ## How Has This Been Tested? Built on macOS arm64 (aarch64-apple-darwin, clang, `--enable-debug --enable-crash-hooks --enable-stacktraces --without-gui --enable-tests`), against the `depends` prefix. - `make -C src test/test_dash` at both commits of this branch, to confirm each commit builds on its own. - `./src/test/test_dash --run_test=limitedmap_tests,unordered_lru_cache_tests` — 6 test cases, no errors. - `./src/test/test_dash` (full unit test suite) — 770 test cases, no errors. This is the check that the new constructor assert does not fire for any existing cache instantiation. - `test/lint/lint-whitespace.py`, `test/lint/lint-git-commit-check.py` (with `COMMIT_RANGE` set to this branch), `test/lint/lint-tests.py`, `test/lint/lint-includes.py`, `test/lint/lint-include-guards.py` — all clean. The new tests cover: no truncation while the size is at or below the truncate threshold (which also pins the default threshold at `2 * max_size`); truncation down to exactly `max_size`, keeping the most recently accessed entries and leaving their values intact; an explicitly supplied truncate threshold; and that both `get()` and `exists()` refresh an entry's recency so an otherwise-oldest entry survives truncation. Not run: functional tests, since no behaviour reachable from RPC or P2P changes. ## Breaking Changes None. Both changes preserve the set of entries evicted, and no consensus, network, or RPC behaviour is affected. ## 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: UdjinM6: utACK b498b67 Tree-SHA512: 4710fd2c2d79f9b6ef98aff9ab798e9eb2c2ddb08c2b6b6e91bc27772f48491d81814c83e7a493fe897ef7c46f3b989ffe57696f587d9c55325eb2810aa32961
knst
reviewed
Aug 5, 2026
| return; | ||
| itTarget->second = v; | ||
| } | ||
| //! Number of elements retained after a prune. |
Collaborator
There was a problem hiding this comment.
technically, not it's not "max size" but "cut-off size".
And prune_after_size became max size now.
| static constexpr size_t MAX_SEEN_CHAINLOCKS{1024}; | ||
| //! Number of recently seen CLSIG hashes retained once `seenChainLocks` is pruned. | ||
| static constexpr size_t SEEN_CHAINLOCKS_RETAINED_SIZE{1024}; | ||
| //! Size `seenChainLocks` may grow to before the next insertion prunes it back down to |
Collaborator
There was a problem hiding this comment.
IMO it's too excessive comment about details of implementation and too easy to become out-dated when code is changed.
| BOOST_REQUIRE_GT(max_size, 0U); | ||
|
|
||
| for (size_t i = 0; i < max_size + 1; ++i) { | ||
| const size_t retained_size = m_node.clhandler->SeenChainLockCacheRetainedSizeForTesting(); |
Collaborator
There was a problem hiding this comment.
this test doesn't test properly functionality, because it retrieve SeenChainLockCacheRetainedSizeForTesting and SeenChainLockCachePruneAfterSizeForTesting from the clhandler
if by mistake setup ridicolous or completely wrong these values, the test will succeed because it retrieve these numbers from clhandler.
Instead, it should be some "sane" constant here.
The "complexity" is not part of functionality in this particular test and should not be tested by regression tests IMO.
I'd propose to just push bunch of chainlocks to clhandler and everytime just be sure that it is not more than 2000 + retrieve couple olds to be sure that they are still on place. This test is too strict about implementation but not really testing that values (prune_after_size, retained_size) are sane at all.
2 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Aug 6, 2026
e5c8384 fix(net): address ChainLock seen-cache follow-up (pasta) Pull request description: ## Issue being fixed or feature implemented Follow-up to #7482 addressing post-merge review feedback. ## What was done? - Renamed limited-map thresholds so `max_size()` is the actual maximum and `cutoff_size()` is the post-prune size. - Removed the detailed ChainLock implementation comments and test-only threshold accessors. - Preserved the existing 1,024/2,048 ChainLock cache configuration. - Reworked the ChainLock test around a fixed 2,500-entry safety bound and recent CLSIG availability, without asserting internal pruning details. ## How Has This Been Tested? - `make -j1` - `./src/test/test_dash --run_test=limitedmap_tests/* --log_level=message` - `./src/test/test_dash --run_test=llmq_chainlock_tests/* --log_level=message` - `test/lint/lint-whitespace.py` - `test/lint/lint-includes.py` ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have added or updated relevant unit/integration/functional/e2e tests This pull request was created by Codex. ACKs for top commit: knst: utACK e5c8384 Tree-SHA512: e53fb00d1eee638de666c3c536093ee39c84c5cd33f0d7f6d68a07d1c9353676793f03aedede61eb6d091825ef6dd7760b159d874ffca65e4bddc4ea9c75b42b
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
4 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.
Issue being fixed or feature implemented
Dash #7424 bounded
ChainlockHandler::seenChainLockswithunordered_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 holdingChainlockHandler::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?
seenChainLockswith 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.unordered_limitedmap's default prune behavior and exposed its configured threshold for focused tests.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.shand./configurewith the existingaarch64-apple-darwindepends prefixmake -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_testsgit diff --check upstream/develop..HEADCOMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.pytest/lint/lint-circular-dependencies.pytest/lint/lint-include-guards.pyship, zero findingsBreaking Changes
None.
Checklist: