Fix multisig partial spends#661
Conversation
WalkthroughThis PR fixes a double-spend bug where change coins from ChangesChange coin double-spend fix
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ShieldedTreasuryStateless
participant sendShielded
participant Ledger
Caller->>ShieldedTreasuryStateless: _send(coin, recipient, amount)
ShieldedTreasuryStateless->>sendShielded: spend coin, pay amount
sendShielded-->>ShieldedTreasuryStateless: result{sent, change}
Note over ShieldedTreasuryStateless: previously re-spent change via sendImmediateShielded (bug)
ShieldedTreasuryStateless-->>Caller: returns result untouched (fixed)
Caller->>Ledger: later spend using result.change
Ledger-->>Caller: succeeds (no double spend)
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
contracts/src/multisig/test/ShieldedTreasuryStateless.test.ts (1)
90-153: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a genuine "persist change, then spend it later" test.
The suite proves the change coin isn't re-spent within the same tx (
isNonceSpent) and can be legitimately routed onward in the same tx (_sendAndRouteChange), but never actually persistsresult.change.valueand feeds it into a later_sendcall to prove it remains spendable — unlikeShieldedTreasury.test.ts's follow-up-spend test. This is the most direct proof of the bug fix and matches the PR's stated acceptance criterion of "deposit, partial spend, then spend of the change."Doing so for the stateless variant requires qualifying the returned
ShieldedCoinInfowith a validmt_indexfor the second_sendcall — worth confirming whether current test-utils support deriving this for a freshly emitted, uncommitted coin.🤖 Prompt for 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. In `@contracts/src/multisig/test/ShieldedTreasuryStateless.test.ts` around lines 90 - 153, The stateless treasury test currently only verifies that change is not double-spent in the same transaction, but it never proves the emitted change coin can be spent later. Update the ShieldedTreasuryStateless test around treasury._send to first capture result.change.value, then use that returned ShieldedCoinInfo in a follow-up _send call to confirm the change remains spendable in a later spend. If the stateless path needs a valid mt_index for the second call, adjust the test setup/helpers so the freshly emitted coin can be qualified correctly before reusing it.
🤖 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.
Nitpick comments:
In `@contracts/src/multisig/test/ShieldedTreasuryStateless.test.ts`:
- Around line 90-153: The stateless treasury test currently only verifies that
change is not double-spent in the same transaction, but it never proves the
emitted change coin can be spent later. Update the ShieldedTreasuryStateless
test around treasury._send to first capture result.change.value, then use that
returned ShieldedCoinInfo in a follow-up _send call to confirm the change
remains spendable in a later spend. If the stateless path needs a valid mt_index
for the second call, adjust the test setup/helpers so the freshly emitted coin
can be qualified correctly before reusing it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7bc5d9e-58dc-4817-b10d-87fe91d823e2
📒 Files selected for processing (12)
contracts/package.jsoncontracts/src/multisig/ForwarderPrivate.compactcontracts/src/multisig/ShieldedTreasury.compactcontracts/src/multisig/ShieldedTreasuryStateless.compactcontracts/src/multisig/test/ForwarderPrivate.test.tscontracts/src/multisig/test/ShieldedTreasury.test.tscontracts/src/multisig/test/ShieldedTreasuryStateless.test.tscontracts/src/multisig/test/mocks/MockForwarderPrivate.compactcontracts/src/multisig/test/mocks/MockShieldedTreasuryStateless.compactcontracts/src/multisig/test/simulators/MockForwarderPrivateSimulator.tscontracts/src/multisig/test/simulators/MockShieldedTreasuryStatelessSimulator.tscontracts/test-utils/zswap.ts
| changeCoin.value | ||
| ); | ||
| _coins.insertCoin(disclose(color), changeCoin, Utils_selfAsRecipient()); | ||
| _coins.insertCoin(disclose(color), result.change.value, Utils_selfAsRecipient()); |
There was a problem hiding this comment.
❔ question (non-blocking): This stores result.change.value with no preceding receiveShielded (unlike _deposit). That's correct only if sendShielded auto-claims its self-routed change, so insertCoin yields a spendable QualifiedShieldedCoinInfo with a valid mt_index. Confirmed on a node? A live spend of the retained change would settle it (see the zswap.ts note).
sent by claude (dev3-midnight-basic-review)
There was a problem hiding this comment.
Confirmed on the local node...if preferred, I can create a new repo or something with the scripts to run so it's verifiable and not "just trust me, bro"
There was a problem hiding this comment.
No need we just need to push the live harness for running tests on local node.
| * nullifiers) and outputs (coin commitments) a circuit produces in the | ||
| * simulator. | ||
| * | ||
| * The dry (in-memory) simulator does NOT enforce the ledger's nullifier set, |
There was a problem hiding this comment.
🔵 followup (non-blocking): These helpers read the dry backend, so the new assertions prove only same-tx non-re-spend. The actual #656 failure was the next tx spending the retained change against a node's nullifier set. A live-backend regression — deposit → partial _send → spend the retained change on a fresh node — is what fully closes #656. (The PR body already flags this pattern isn't sufficient standalone; noting so the live test isn't lost.)
sent by claude (dev3-midnight-basic-review)
| sim?: { circuitContext?: { currentZswapLocalState?: ZswapLocalState } }; | ||
| }; | ||
| } | ||
| )?._backend?.sim?.circuitContext?.currentZswapLocalState; |
There was a problem hiding this comment.
⚪ nitpick (if-minor): Reaches through two undocumented simulator layers — _backend.sim (sim is private on DryBackend) then .circuitContext. It fails loud (throws with a clear message) so it won't rot silently, but consider an upstream ask for a supported zswap-local-state accessor now that the simulator is dual-backend.
sent by claude (dev3-midnight-basic-review)
| export const bytesToHex = (b: Uint8Array): string => | ||
| Buffer.from(b).toString('hex'); | ||
|
|
||
| const toHex = bytesToHex; |
There was a problem hiding this comment.
⚪ nitpick (if-minor): const toHex = bytesToHex is a second name for the same function in one small file — use bytesToHex directly in isNonceSpent and drop the alias.
sent by claude (dev3-midnight-basic-review)
| * `sendShielded` routes the change back to this contract as a self-owned | ||
| * output; `_drain` returns it untouched via `result.change`. The | ||
| * implementing contract chooses what to do with it: leave it dwelling at | ||
| * the contract for a future drain, or spend it onward to a different |
There was a problem hiding this comment.
🔵 followup (non-blocking): This guidance ("spend it onward to a different recipient … via sendImmediateShielded") carries no recipient-type caveat, but for this module routing change to a ContractAddress republishes a contract address in cleartext — the exact leak the module forbids for parent. Add: onward change routing here must stay to a ZswapCoinPublicKey, same reason as the parent. (The generic treasuries don't carry the parent-privacy guarantee, so their identical wording is fine.)
sent by claude (dev3-midnight-basic-review)
There was a problem hiding this comment.
Basic review — PR #661 (Fix multisig partial spends)
Inline comments cover one mechanism question, two followups, and two nitpicks. One more followup that can't be inline (the file isn't in this diff):
🔵 followup (non-blocking): preset docs.
ShieldedMultiSigV2.executeand the forwarder presetdrainnow surface a liveresult.changeto dApp integrators, but their@returnsstill say only "including any change." Mirror the base-module doc improvement: state that the caller now owns the returned change (persist-and-replay, or route onward) and must not treat it as auto-retained.Verdict: ok — 0 blocking, 1 question, 3 followups, 2 nitpicks. Ready to merge; the live-backend regression (noted inline on
zswap.ts) is the item that truly closes #656.sent by claude (dev3-midnight-basic-review)
Thank you @andrew-fleming! LGTM! I will just wait to verify all on local live infra.
0xisk
left a comment
0xisk
left a comment
There was a problem hiding this comment.
LGTM! thank you @andrew-fleming! There will be a following PR that will have all the live infra testing.
2 participants
Resolves #656.
This PR proposes to remove the change routing logic because
sendShieldedalready routes the change coin to the contract. Implementing contracts can use theShieldedSendResultto decide what to do with the change: persist it and pass it back on the next spend, or spend it onward to a different recipient in the same tx viasendImmediateShieldedThe redundant re-send is dropped at all three sites:
ShieldedTreasury._sendrecords the live change coinShieldedTreasuryStateless._sendandForwarderPrivate._drainreturn it untouched for the caller to handleThis PR also adds simulator regression tests that assert on the recorded zswap i/o. The retained change coin's nullifier must not appear among the tx inputs since that's the one thing observable without a node. Note that this only can catch double spends within the same tx and should not be used by itself as a standalone testing pattern
Summary by CodeRabbit
New Features
Bug Fixes