fix(net): follow up on ChainLock cache feedback - #7548
Conversation
|
✅ Final review complete — no blockers (commit e5c8384) |
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)
Comment |
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 216-237: Update the chainlock cache recency logic exercised by
ProcessNewChainLock so entries sharing the same seconds-resolution timestamp are
ordered by insertion order, preserving the newest RECENT_CHAINLOCKS_TO_RETAIN
entries during pruning. Extend the test around the process lambda with a
same-timestamp burst, assert the required recent-hash retention and cache-size
bounds, and keep the existing distinct-timestamp coverage intact.
🪄 Autofix
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 Plus
Run ID: cda9fa71-c20c-4405-b7b3-edd340ebc2e5
📒 Files selected for processing (5)
src/chainlock/handler.cppsrc/chainlock/handler.hsrc/limitedmap.hsrc/test/limitedmap_tests.cppsrc/test/llmq_chainlock_tests.cpp
1c6054b to
e5c8384
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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.h`:
- Around line 57-58: Update the SEEN_CHAINLOCKS_CUTOFF_SIZE and
SEEN_CHAINLOCKS_MAX_SIZE constants in the chainlock handler to use the specified
bounds of 1000 and 2000, respectively, rather than deriving them from the
current 1024 cutoff.
🪄 Autofix
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 Plus
Run ID: 7112e897-4fea-4b28-b0ea-9596aef77819
📒 Files selected for processing (5)
src/chainlock/handler.cppsrc/chainlock/handler.hsrc/limitedmap.hsrc/test/limitedmap_tests.cppsrc/test/llmq_chainlock_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test/llmq_chainlock_tests.cpp
- src/test/limitedmap_tests.cpp
- src/chainlock/handler.cpp
- src/limitedmap.h
Issue being fixed or feature implemented
Follow-up to #7482 addressing post-merge review feedback.
What was done?
max_size()is the actual maximum andcutoff_size()is the post-prune size.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=messagetest/lint/lint-whitespace.pytest/lint/lint-includes.pyBreaking Changes
None.
Checklist:
This pull request was created by Codex.