fix: bound ChainLock seen cache#7424
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR changes Estimated code review effort: 3 (Moderate) | ~20 minutes 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 |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 59fafb0) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff09613ed6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(failed), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed), codex/dash-core-commit-history=gpt-5.5(completed); verifier=gpt-5.5
The PR is focused and the main bounded-cache behavior matches its stated goal, including avoiding retention of stale same-height CLSIGs. I verified one in-scope regression: because the bounded cache is now the only source for AlreadyHave(), attacker-controlled invalid future CLSIGs can evict the current best CLSIG hash and make later inventory for an object the node already stores appear new.
🔴 0 blocking | 🟡 1 suggestion(s) | 💬 0 nitpick(s)
Verified Findings
- [SUGGESTION]
src/chainlock/handler.cpp:75-76: Check the stored best ChainLock when the seen cache misses
AlreadyHave()now answers only fromseenChainLocks, but that cache is intentionally evictable and future-height CLSIGs are inserted before signature validation. A peer can churn more thanMAX_SEEN_CHAINLOCKSinvalid higher-height CLSIGs, evict the hash for the current best ChainLock, and make laterMSG_CLSIGinventory for that stored ChainLock look new.net_processingwill then request the CLSIG even thoughChainlocks::GetChainLockByHash()can already serve it. Falling back to the authoritative best-ChainLock store on a cache miss preserves the bounded duplicate cache without forgetting the one CLSIG the node actually retains.
Note: GitHub does not allow me to approve/request changes on my own PR, so this exact-SHA self-review is posted as a COMMENT while preserving the verified findings.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Handled the ChainLock seen-cache follow-up in What changed:
Validation:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior finding prior-1 is fixed: AlreadyHave() now falls back to Chainlocks::GetChainLockByHash() after a seen-cache miss, and the new regression test covers best-CLSIG availability after cache eviction. The only new latest-delta finding is commit-history hygiene: the second commit directly repairs behavior introduced by the first commit and should be folded before merge.
Source: reviewers: opus/claude general failed (extra-usage quota), gpt-5.5 codex general completed, opus/claude dash-core-commit-history failed (extra-usage quota), gpt-5.5 codex dash-core-commit-history completed; verifier: gpt-5.5 codex.
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:f807b437>`:
- [SUGGESTION] <commit:f807b437>:1: Fold the best-CLSIG repair into the bounded-cache commit
The PR stack has `ff09613ed6c` adding the bounded ChainLock seen-cache behavior, followed by `f807b437bd7` fixing that same behavior by adding the authoritative best-CLSIG fallback and regression coverage. Dash PR hygiene expects each commit to be a complete logical change, and keeping these as separate commits records the first commit as an intermediate state where the bounded cache can forget the stored best CLSIG. Squash `f807b437bd7` into `ff09613ed6c`, or amend the original commit, so the history contains one complete `fix: bound ChainLock seen cache` change.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/chainlock/handler.cpp`:
- Around line 49-50: Update the seenChainLocks initialization in the chain lock
handler so the unordered_limitedmap uses a larger prune-after-size instead of
defaulting to MAX_SEEN_CHAINLOCKS. The issue is that the current constructor
only sets the max size, causing prune() to sort and evict on every insert once
full. Follow the pattern used by g_already_asked_for in net_processing.cpp: keep
the existing max size for seenChainLocks, but pass a larger second capacity to
batch evictions and reduce work while cs is held.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8ecda1f5-0cb6-4a79-81d2-26a0dc048b4d
📒 Files selected for processing (3)
src/chainlock/handler.cppsrc/chainlock/handler.hsrc/test/llmq_chainlock_tests.cpp
thepastaclaw
force-pushed
the
fix/chainlock-seen-cache-bound
branch
from
f807b43 to
67deff2
Compare
|
Resolved the commit-history feedback in |
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
No carried-forward prior findings remain: prior-f807-commit-history is fixed because the current PR stack at 67deff2 is a single fix: bound ChainLock seen cache commit rather than a corrective follow-up commit. The latest delta from f807b43 is empty, and the cumulative ChainLock cache changes match the PR goal: stale CLSIGs are not retained, the seen cache is bounded, and AlreadyHave() falls back to the stored best ChainLock after cache eviction.
Source: reviewers claude/general=opus(failed), codex/general=gpt-5.5(ok), claude/dash-core-commit-history=opus(failed), codex/dash-core-commit-history=gpt-5.5(ok); verifier codex=gpt-5.5.
Note: posted as a COMMENT review because GitHub does not allow approving my own PR.
thepastaclaw
force-pushed
the
fix/chainlock-seen-cache-bound
branch
from
67deff2 to
d0202da
Compare
|
Fixed the CI failure by keeping all tests in Validation:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/test/llmq_chainlock_tests.cpp`:
- Around line 196-207: The seen_chainlock_cache_is_bounded test only checks that
SeenChainLockCacheSizeForTesting() stays under max_size, so it can pass even if
ProcessNewChainLock never inserts anything. Update the test in
seen_chainlock_cache_is_bounded to verify the cache actually grows after the
first CreateChainLock/ProcessNewChainLock call, and ensure the handler is in an
active state (as in the other chainlock tests) before exercising the cache
bounds. Keep the existing bounded-size assertion, but make the test fail if
insertion does not happen.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5b1c7dab-432d-4aee-a183-c2b66dfe5256
📒 Files selected for processing (3)
src/chainlock/handler.cppsrc/chainlock/handler.hsrc/test/llmq_chainlock_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/chainlock/handler.h
- src/chainlock/handler.cpp
|
Addressed CodeRabbit’s test-hardening finding in Validation:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(failed: extra-usage quota), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed: extra-usage quota), codex/dash-core-commit-history=gpt-5.5(completed); verifier=codex gpt-5.5.
Carried-forward prior findings: none. The prior exact-head review at 67deff2c had no issues, and the latest delta to d0202da8 changes the test fixture scoping rather than reintroducing any earlier implementation or commit-history problem.
New findings in latest delta: one test-strength suggestion. Cumulative current-head validation still shows the bounded-cache behavior is sound, but the new bounded-cache test can pass even if the seen-cache insertion path stops being exercised. CodeRabbit's same-SHA top-level review raised the same concern; there was no inline comment id available for an automated reaction.
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/test/llmq_chainlock_tests.cpp`:
- [SUGGESTION] src/test/llmq_chainlock_tests.cpp:201-205: Assert the bounded cache is actually populated
This test only checks that the seen ChainLock cache size stays at or below the configured maximum. If `ProcessNewChainLock()` regressed to return before inserting every non-stale CLSIG into `seenChainLocks`, the cache would stay at size 0 and this test would still pass, so it would not prove the duplicate-tracking path that this PR keeps while bounding the cache. The test should verify that processed hashes are visible through `AlreadyHave()` before eviction and that the cache reaches the configured bound.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(failed), codex/general=gpt-5.5(ok), claude/dash-core-commit-history=opus(failed), codex/dash-core-commit-history=gpt-5.5(ok); verifier codex=gpt-5.5.
Prior finding reconciliation:
- FIXED
prior-d0202-cache-population(src/test/llmq_chainlock_tests.cpp:201-205):d0477eebnow activates the ChainLock handler before the bounded-cache loop and asserts the cache is non-empty after the first processed non-stale CLSIG, so the prior vacuous-pass concern no longer applies.
Carried-forward prior findings: none.
New findings in latest delta: one commit-history suggestion. The current code review has no in-scope implementation findings, and CodeRabbit supplied no actionable inline findings, so no CodeRabbit reactions were emitted.
Suggestion:
- [SUGGESTION]
<commit:d0477eebec7>: Squash the test follow-up into the ChainLock cache fix
Commitd0477eebec7(test: assert ChainLock seen cache population) only adds a five-line review-feedback patch tosrc/test/llmq_chainlock_tests.cpp, in the same test case introduced by the immediately precedingfix: bound ChainLock seen cachecommit. This is not an independently useful history step; it is part of the test coverage for the original unmerged fix. Squash it intod0202da8085before merge to keep the PR as one logical change.
Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:d0477eebec7>`:
- [SUGGESTION] <commit:d0477eebec7>:1: Squash the test follow-up into the ChainLock cache fix
Commit d0477eebec7 (`test: assert ChainLock seen cache population`) only adds a five-line review-feedback patch to `src/test/llmq_chainlock_tests.cpp`, in the same test case introduced by the immediately preceding `fix: bound ChainLock seen cache` commit. This is not an independently useful history step; it is part of the test coverage for the original unmerged fix. Squash it into d0202da8085 before merge to keep the PR as one logical change.
d0477ee to
fe728a7
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(failed: extra-usage quota), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed: extra-usage quota), codex/dash-core-commit-history=gpt-5.5(completed); verifier=codex gpt-5.5.
Carried-forward prior finding: prior-d0477-commit-history is FIXED because the prior two-commit stack at d0477ee now appears as a single coherent commit, fe728a7. New delta: I verified the changed ChainLock handler and focused tests against the checked-out head and found no in-scope defects. CodeRabbit had no actionable inline findings, so there are no reactions.
Prior finding reconciliation:
- FIXED
prior-d0477-commit-history: Separate test follow-up commit should be folded into the ChainLock cache fix — The prior SHA d0477ee contained two commits,fix: bound ChainLock seen cachefollowed bytest: assert ChainLock seen cache population; the current head fe728a7 contains a singlefix: bound ChainLock seen cachecommit with the handler changes and tests folded together.
Carried-forward prior findings: none.
New findings in latest delta: none.
No in-scope findings. CodeRabbit had no actionable inline findings, so no reactions were emitted.
Note: GitHub does not allow me to approve/request changes on my own PR, so this exact-SHA clean self-review is posted as a COMMENT.
thepastaclaw
force-pushed
the
fix/chainlock-seen-cache-bound
branch
from
fe728a7 to
47578dc
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(failed), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed), codex/dash-core-commit-history=gpt-5.5(completed); verifier=codex gpt-5.5
Prior finding reconciliation: prior review at fe728a7d found no issues, so there are no carried-forward prior findings. New findings in latest delta fe728a7d..47578dc9: none. Cumulative current-head review found no in-scope defects.
Prior reconciliation: the previous review had no findings, so there are no resolved or carried-forward prior findings. Latest-delta and cumulative verification found no in-scope issues in PR #7424; the two reported functional-test risks exist in the current tree but predate this ChainLock cache PR and are outside its changed surface.
Prior finding reconciliation:
- The previous exact-SHA review at
fe728a7dfound no issues, so there are no carried-forward prior findings.
Carried-forward prior findings: none.
New findings in latest delta fe728a7d..47578dc9: none.
CodeRabbit had no actionable inline findings at this head, so no reactions were emitted.
Note: GitHub does not allow me to approve/request changes on my own PR, so this exact-SHA clean self-review is posted as a COMMENT.
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
force-pushed
the
fix/chainlock-seen-cache-bound
branch
from
47578dc to
59fafb0
Compare
|
Rebased onto current develop so the branch includes the merged asset-lock functional-test stabilization from #7411; the red linux64-test job was running the stale pre-#7411 head and failed in feature_asset_locks.py. Validation passed locally after the rebase:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(failed: extra-usage quota), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed: extra-usage quota), codex/dash-core-commit-history=gpt-5.5(completed); verifier=codex gpt-5.5.
Prior finding reconciliation: the previous exact-SHA automated review at 47578dc9 had no verified findings.
Carried-forward prior findings: none.
New findings in latest delta 47578dc9..59fafb02: none. That delta is base movement from f084efb7 to cbab2549 in asset-lock functional-test files, not a new PR-authored ChainLock change.
Cumulative current-head review found no in-scope defects in the ChainLock seen-cache patch. CodeRabbit had no actionable inline findings, so no reactions were emitted.
Note: GitHub does not allow me to approve/request changes on my own PR, so this exact-SHA clean self-review is posted as a COMMENT.
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Issue being fixed or feature implemented
Bounds ChainLock seen-CLSIG duplicate tracking so unvalidated or non-current CLSIG inventory cannot grow retained handler state without limit.
This improves defensive handling for peer-supplied ChainLock messages while preserving the existing best ChainLock validation and update flow.
What was done?
seenChainLocksmap with a boundedunordered_limitedmap.How Has This Been Tested?
Tested locally on macOS in a fresh worktree rebased on current
upstream/develop:make -C src test/test_dash -j$(sysctl -n hw.ncpu)src/test/test_dash --run_test=llmq_chainlock_tests,chainlock_handler_testsgit diff --check upstream/develop..HEADCOMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.pyupstream/develop..HEAD: no significant issues found; recommendation ship.Breaking Changes
None.
Checklist: