[PastaPastaPasta]

Issue being fixed or feature implemented

Concurrent first calls can regenerate the watch-connection seed while another caller reads it. A missing quorum during signature-share collection leaves a partial batch that the caller dispatches to code using quorums.at(). ChainLock shutdown also returns before its enforcement thread exits, allowing enforcement to overlap validation-interface teardown.

What was done?

• Initialize the immutable watch-connection seed with thread-safe function-local static initialization.
• Clear both batch outputs when quorum lookup fails, so the caller cannot dispatch an incomplete batch.
• Join the ChainLock scheduler thread in Stop() and reuse that method from the destructor, guarding repeated joins with joinable().

Reviewed every handler stop/destruction path: production shutdown joins before stopping the main scheduler, with no chain or handler lock held; the full unit fixture calls Stop() before later destruction; reindex resets the handler without those locks while the main scheduler runs. The simpler base fixture leaves the handler dormant. No stop/destructor runs on the handler's own scheduler thread.

How Has This Been Tested?

• Debug build with warnings treated as errors: make -C src -j4 dashd test/test_dash.
• ./src/test/test_dash --run_test='llmq*' --log_level=message: all 84 cases passed.
• ./src/test/test_dash --run_test='validation_chainstatemanager_tests/chainstatemanager,chainstatemanager_snapshot_discarded_on_reindex' --log_level=message: both cases passed, covering base-fixture teardown and reindex destruction/recreation.
• RAM-disk functional runner with an isolated port range and -j1 --timeout-factor=3: feature_llmq_chainlocks.py, feature_llmq_signing.py, and its --spork21 variant all passed.
• Whitespace lint, git diff --check, and self-review passed.

The seed and partial-batch failure paths were verified by code inspection. No TSAN run or deterministic forced-race reproducer is claimed.

Breaking Changes

None.

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation (not needed for these internal fixes)
• I have assigned this pull request to a milestone

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You'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 5 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 87a00d13-db9d-4dc6-a3c4-66b8ce6702f1

Commits

Reviewing files that changed from the base of the PR and between 7d85e33 and a3933fa.

Files selected for processing: 3

• src/chainlock/handler.cpp
• src/llmq/signing_shares.cpp
• src/llmq/utils.cpp

Pre-merge checks: 2

Failed checks: 2 inconclusive

Check name	Status	Explanation	Resolution
Title check		This assessment has not completed. The check will be retried on the next pre-merge check run.
Description check		This assessment has not completed. The check will be retried on the next pre-merge check run.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[thepastaclaw]

✅ Final review complete — no blockers (commit a3933fa)

[github-actions]

Potential PR merge conflicts

This is advisory only. It does not block CI. This PR and each open PR below change the same files and cannot be merged together cleanly, so whichever of the two merges second will likely need a rebase:

• #8074: fix: keep short rotation quarters as selected Changed files: src/llmq/utils.cpp.

[thepastaclaw]

Final validation — Phase 2 only (queue backlog)

The reviewed changes correctly address the three stated concurrency and shutdown defects, and no blocking correctness issue is evident. Add deterministic regression coverage for the missing-quorum batch contract; the relevant build, lint, and sanitizer checks were still queued in the supplied CI snapshot.

🟡 1 suggestion(s)

Approved: nothing here blocks the merge. The non-blocking findings are optional: fix them forward, in this PR or a follow-up, without waiting for another review round.

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: normal by gpt-6.1-sol (effort low) — Although small, the diff changes concurrent initialization, batch failure handling, and shutdown thread joining, requiring reasoning about caller invariants and lifecycle ordering rather than straightforward local verification.
• Phase 1 reviewers: not run (skipped for throughput: 12 PRs queued, above the 10 limit)
• Second Phase-2 pass: on this first full review every Phase-2 reviewer also ran a second, independent time, and the verifier weighed both sets
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review (PastaClaw). How to work through them:
- Verify each finding against the code; fix only what is real, as narrowly as the finding.
- If a finding is wrong, decline it on its thread in one or two sentences; the bot answers once and stops.
- To defer one, link a tracking issue on its thread (or reply asking the bot to file one).
- Suggestions don't block the merge, but most are worth fixing in this PR; judge each on its merits.
- Keep the PR's scope; don't grow it to satisfy hardening suggestions.
- Carried items were raised in an earlier round: read the thread first, and don't re-fix what you already fixed, declined or deferred there. Disputed and deferred items are already handled and are not listed.

In `src/llmq/signing_shares.cpp`:
- [SUGGESTION] src/llmq/signing_shares.cpp:599-600: Add deterministic regression coverage for incomplete batch suppression
  The fix correctly clears both output maps when quorum lookup fails, which prevents NetSigning::WorkThreadDispatcher from dispatching an incomplete batch. However, no test exercises this contract. Add a focused regression that collects a batch containing a quorum unavailable to GetQuorum, verifies that both output maps are empty, and confirms that a later valid batch can still be collected. This is important because the caller uses the boolean return only as a more-work indicator and dispatches whenever the shares map is nonempty; returning false alone would not prevent the original quorums.at() failure.