Repository navigation
fix: gate signing intake before quorum retrieval - #7833
PastaPastaPasta merged 1 commit into
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 12 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (2)
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 |
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If these PRs merge firstThis PR will likely need a rebase:
|
|
🕓 Review not started yet because the new head is waiting for the 30-minute push debounce.
Commit 438708e. Normal review starts when eligible; priority review starts as soon as a slot is available. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied reviewer evidence against head 68ad268. The admission gates preserve synchronized quota enforcement and existing-session refreshes, with no blocking defects identified; one minor loss of quota-rejection diagnostics is confirmed. This was a static review: the supplied CI snapshot shows preliminary checks passing, but container builds remain queued and CI build/test validation is not established.
💬 1 nitpick(s)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The diff adds small, contained admission checks using existing quota and active-quorum predicates, preserves the locked capacity check, and adds focused regression coverage without changing cryptography or wire formats. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 94% left, weekly 95% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
🤖 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/llmq/signing_shares.cpp`:
- [NITPICK] src/llmq/signing_shares.cpp:262-264: Preserve the quota-rejection diagnostic in the early gate
The new early gate handles ordinary at-capacity announcement rejection without emitting the existing "too many sessions" diagnostic. The final synchronized check still logs that reason, but announcements rejected here never reach it, so operators using the LLMQ_SIGS debug category see the announcement without its quota-drop reason. Add the same diagnostic to this branch to preserve the previous debugging information.
| if (it != nodeStates.end() && !it->second.CanCreateSessionFromAnn(ann, maxSessions)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
✅ Resolved at
f8c08d71; see the replies below.
💬 Nitpick: Preserve the quota-rejection diagnostic in the early gate
The new early gate handles ordinary at-capacity announcement rejection without emitting the existing "too many sessions" diagnostic. The final synchronized check still logs that reason, but announcements rejected here never reach it, so operators using the LLMQ_SIGS debug category see the announcement without its quota-drop reason. Add the same diagnostic to this branch to preserve the previous debugging information.
| if (it != nodeStates.end() && !it->second.CanCreateSessionFromAnn(ann, maxSessions)) { | |
| return true; | |
| } | |
| if (it != nodeStates.end() && !it->second.CanCreateSessionFromAnn(ann, maxSessions)) { | |
| LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- too many sessions. cnt=%d, max=%d, llmqType=%d, node=%d\n", | |
| __func__, it->second.GetAnnouncementSessionCount(llmqType), maxSessions, static_cast<int>(llmqType), pfrom.GetId()); | |
| return true; | |
| } |
source: glm-5.3-flash (phase1-reviewer: general)
There was a problem hiding this comment.
Resolved (re-reviewed at f8c08d71): Removing the silent early capacity check restores the existing mutex-protected capacity check and its 'too many sessions' diagnostic at lines 270–276. The remaining early gate checks only quorum activity, so announcements for retained active quorums still reach the quota-rejection diagnostic.
thepastaclaw
added
the
pastaclaw:commented
68ad268 to
f8c08d7
Compare
thepastaclaw
removed
the
pastaclaw:commented
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
The complete diff at f8c08d7 has no confirmed in-scope defects: both handlers check retained-quorum activity before resolving the peer-selected quorum, and the batched path retains its independent activity check. The regression test observes receiving-session admission through public handlers, covering stale rejection, oldest-retained-quorum acceptance, and session-ID refresh; the prior quota-diagnostic finding is fixed. Verification was static only: reported local tests were not independently reproduced, and the supplied CI snapshot shows successful formatting and preliminary checks, with container builds and PR-title validation still queued.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The diff adds two existing active-quorum checks before retrieval in signing intake handlers and a focused regression test, making it small and well-contained rather than large or intricate. - Phase 1 reviewers: not run (skipped for throughput: 12 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
f8c08d7 to
438708e
Compare
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
approved; there was a test added; but I think the regression is so narrow it wasn't worth it
2 participants
Issue being fixed or feature implemented
Signing-session announcements (
QSIGSESANN) and direct sig shares (QSIGSHARE) resolved the referenced quorum before checking whether it belongs to the retained active signing set. A peer could therefore make a node build or load historical quorums, and an announcement for a stale quorum created a receiving session. The reachable admission defect is reproduced; CPU/exhaustion magnitude is unmeasured.What was done?
Run the existing active-quorum predicate (
IsQuorumActive) before quorum retrieval inProcessMessageSigSesAnnandProcessMessageSigShare. The per-peer session capacity check under the session mutex is unchanged.PreVerifySigShareQuorumkeeps its own active-quorum check on purpose, because the batched path depends on it. This is a Dash-specific change with no corresponding Bitcoin backport.How Has This Been Tested?
On macOS arm64 with the existing depends configuration,
makeofdashd,dash-cli,dash-walletandtest_dash; the fullllmq_utils_testssuite, include lint, circular dependency lint andgit diff --checkpassed.feature_llmq_signing.py(both variants),feature_llmq_is_retroactive.pyandp2p_quorum_data.pypassed. No dedicated unit test is included: the change only reorders an existing active-quorum check ahead of quorum retrieval. No CPU benchmark, sanitizer stress or operational exhaustion demonstration is claimed.Breaking Changes
None. Existing active-quorum policy and wire formats are preserved.
Checklist:
No user-facing documentation change is required.