fix: recheck live CoinJoin side coverage before finalizing timeout session - #7622
fix: recheck live CoinJoin side coverage before finalizing timeout session#7622PastaPastaPasta wants to merge 1 commit into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. Walkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to A timed-out session can be reset instead of cleanly remaining available for a final live-coverage check, which may prevent the remaining participant from restoring coverage and leave the session in the wrong state. This should be addressed or explicitly accepted before merging. 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 |
|
🔍 Review in progress — actively reviewing now (commit 531f117) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The exact-head diff correctly revalidates the live participant count and side coverage while holding cs_coinjoin, preventing entry admission from invalidating eligibility before the session transitions to signing. The regression test directly exercises the uncovered 3-demotion/1-promotion state, and no in-scope defects were identified; the targeted test binary was not available in this worktree for execution.
Source: reviewers gpt-5.6-sol (general and dash-core-commit-history); final verifier gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
b091d07 to
3df8074
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The exact-head change revalidates the live participant minimum and side-coverage invariant while holding cs_coinjoin, preventing a concurrently admitted entry from producing an uncovered final transaction. The regression test exercises the 3-demotion/1-promotion state and confirms that the session remains open; no in-scope defects were identified.
Source: reviewers gpt-5.6-sol (general and dash-core-commit-history); final verifier claude-opus-4-6. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
3df8074 to
b6f953e
Compare
PastaPastaPasta
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The live coverage recheck prevents construction of an uncovered final transaction, but its rejection path does not preserve the session in the production scheduler flow. Because the timeout remains expired, the immediately following timeout check charges fees again and resets the session, defeating the stated goal of waiting for the pending counterparty.
Source: reviewers gpt-5.6-sol (general and dash-core-commit-history); final verifier claude-opus-4-6. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 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/coinjoin/server.cpp`:
- [BLOCKING] src/coinjoin/server.cpp:385-388: Rejected finalization is immediately reset by CheckTimeout
This return does not preserve the timed-out session in production. `Schedule()` invokes `CheckPool()` and then `CheckTimeout()` in the same callback. After `CreateFinalTransaction()` rejects the newly uncovered live state, `nState` remains `POOL_STATE_ACCEPTING_ENTRIES` and `nTimeLastSuccessfulStep` remains expired, so `CheckTimeout()` immediately invokes `ChargeFees()` a second time and calls `SetNull()`. The remaining admitted promoter therefore has almost no opportunity to restore coverage, contrary to the PR's stated behavior. Refresh or otherwise resolve the timeout before returning, and extend the regression test to exercise the `CheckPool()` followed by `CheckTimeout()` scheduler sequence rather than calling `CreateFinalTransaction()` alone.
| if (vecEntries.size() < static_cast<size_t>(CoinJoin::GetMinPoolParticipants()) || !sides.IsCovered()) { | ||
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CreateFinalTransaction -- session no longer eligible, entries=%d, sides=%d/%d\n", | ||
| vecEntries.size(), sides.inputs, sides.outputs); | ||
| return; |
There was a problem hiding this comment.
🔴 Blocking: Rejected finalization is immediately reset by CheckTimeout
This return does not preserve the timed-out session in production. Schedule() invokes CheckPool() and then CheckTimeout() in the same callback. After CreateFinalTransaction() rejects the newly uncovered live state, nState remains POOL_STATE_ACCEPTING_ENTRIES and nTimeLastSuccessfulStep remains expired, so CheckTimeout() immediately invokes ChargeFees() a second time and calls SetNull(). The remaining admitted promoter therefore has almost no opportunity to restore coverage, contrary to the PR's stated behavior. Refresh or otherwise resolve the timeout before returning, and extend the regression test to exercise the CheckPool() followed by CheckTimeout() scheduler sequence rather than calling CreateFinalTransaction() alone.
| if (vecEntries.size() < static_cast<size_t>(CoinJoin::GetMinPoolParticipants()) || !sides.IsCovered()) { | |
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CreateFinalTransaction -- session no longer eligible, entries=%d, sides=%d/%d\n", | |
| vecEntries.size(), sides.inputs, sides.outputs); | |
| return; | |
| if (vecEntries.size() < static_cast<size_t>(CoinJoin::GetMinPoolParticipants()) || !sides.IsCovered()) { | |
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CreateFinalTransaction -- session no longer eligible, entries=%d, sides=%d/%d\n", | |
| vecEntries.size(), sides.inputs, sides.outputs); | |
| nTimeLastSuccessfulStep = GetTime(); | |
| return; | |
| } |
source: ['codex']
There was a problem hiding this comment.
Verified the mechanics: correct — Schedule() runs CheckPool() then CheckTimeout() in the same callback, a committed entry never refreshes nTimeLastSuccessfulStep (only SetState/SetNull write it server-side), so the refused session is reset by CheckTimeout() in the same tick. The wording "leave the session open" in the test comment was wrong and has been fixed in 531f117.
Declining the suggested timer refresh, though — resetting the timed-out session is the intended outcome, not a defect:
- The fix's guarantee is "never publish an uncovered final transaction". That holds either way; what's at issue is only the disposition of a session that already blew its full 30s
COINJOIN_QUEUE_TIMEOUT, andCheckTimeout()resetting such a session is the established cleanup path. - Admission requires the declared shapes to be covered, so a lone committed promoter implies at least one more admitted promoter who already had the entire entry window and didn't deliver. Refreshing
nTimeLastSuccessfulStepwould grant everyone another full window on the off chance that straggler shows up, keeping the present participants' inputs and collateral locked for 30 more seconds before the same reset. It would also stamp "last successful step" when nothing succeeded, and let the timeout that already fired be waived. - The double
ChargeFees()roll in that tick is real but bounded: each call proceeds with 33% probability and consumes at most one collateral, from participants who genuinely never submitted.
531f117 documents the deliberate no-refresh at the early return and rewords the test comment to describe the actual behavior (refuse to build, stay out of POOL_STATE_SIGNING, session then falls to CheckTimeout()). Not extending the test to drive the full CheckPool()→CheckTimeout() sequence: the unit under test is the finalization recheck, and the scheduler sequence would drag in ChargeFees/collateral-relay machinery without strengthening the assertion that matters.
A follow-up may relay ERR_SESSION on this path (mirroring the uncovered-full-entries branch in CheckPool()) so clients release their inputs immediately instead of waiting out their own lag timeout.
🤖 Posted autonomously by Claude on behalf of pasta.
…ession The timeout branch in CheckPool decides to finalize based on a snapshot taken before ChargeFees, but entries can commit while that call runs since the session stays in POOL_STATE_ACCEPTING_ENTRIES. This could change a covered 0/3 side into an uncovered 1/3 side, which CreateFinalTransaction previously accepted after only checking the session id/state and then built from the live vecEntries. Validate the live MixSideCounts and minimum entry count under the same cs_coinjoin lock used to build the final transaction so the decision cannot be invalidated between check and construction. Add a unit test exercising the 3-demotion + 1-promotion (1/3) regression that must stay in accepting state until the second promoter arrives.
b6f953e to
531f117
Compare
2 participants
Issue being fixed
Follow-up to #7052 (promotion/demotion).
CheckPooldecides to finalize a timed-outPOOL_STATE_ACCEPTING_ENTRIESsession from a snapshot taken beforeChargeFees(), butAddEntrycan commit a pending DSVIN while that call runs since the session is still accepting entries. For example, a snapshot of 3 demotions is covered (0/3), but if the admitted set also contains 2 promoters, a promoter committing mid-fee-charge changes live sides to1/3.CreateFinalTransactionpreviously only rechecked session identity/state and then published an uncovered transaction with a lone promoter.What was done?
CCoinJoinServer::CreateFinalTransaction, recheck liveGetMixSideCountsLocked().IsCovered()and the minimum entry count under the samecs_coinjoinlock used to build the final transaction and transition toPOOL_STATE_SIGNING.src/test/coinjoin_inouts_tests.cpp:server_finalization_rechecks_live_side_coveragereproducing the3D + 1P = 1/3regression (must stay in accepting state).How Has This Been Tested?
./src/test/test_dash --run_test=coinjoin_inouts_tests/server_finalization_rechecks_live_side_coveragemake -j14This pull request was created by Codex.