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 (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. Walkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR prevents finalization when live CoinJoin side coverage has changed during fee charging. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
⛔ Blockers found — Opus deferred (commit b6f953e) |
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)
…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.
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']
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.