In create_order.rs, compute the recovery ID before returning the StateWrapper#1412
In create_order.rs, compute the recovery ID before returning the StateWrapper#1412
Conversation
…efore returning state_intent to avoid subtraction underflow if the nullifier is incorrectly believed to be 0
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
akirillo
left a comment
There was a problem hiding this comment.
Summary
- Overall: This PR calls
compute_recovery_id()on theDarkpoolStateIntentat order creation time increate_private_intent_state_wrapper, advancing the stored recovery stream index from 0 to 1. The intent is to fix au64subtraction underflow incompute_nullifier()(state_wrapper.rs:191) when called on a Ring 1 intent before its first fill. However, this fix introduces a recovery stream double-advancement: settlement also callsadvance_recovery_stream()(ring1.rs:345), so after the first fill the stored index would be 2 instead of the expected 1. - Concerns: The double-advancement shifts the recovery ID sequence — the first recovery ID emitted onchain would be
H(seed || 1)instead ofH(seed || 0), breaking the sequential derivation that state recovery relies on. The root cause is thatcompute_nullifier()unconditionally doesindex - 1, which underflows when no recovery ID has been emitted yet (index == 0). The fix should target that assumption rather than mutating the stored intent's stream state at creation time. - Architecture: The API server's
compute_recovery_id()call inhelpers.rs:148-153is on a throwaway intent used only for computing the signed commitment — it does not affect the stored state and should not be mirrored on the stored intent.
Rating: needs changes
Areas for Manual Review
- Identify which code path calls
compute_nullifier()on a Ring 1 intent before first fill — this is the actual trigger for the underflow and determines which alternative fix is appropriate.
| Ok(DarkpoolStateIntent::new(self.intent.clone(), share_stream.seed, recovery_stream.seed)) | ||
| let mut state_intent = | ||
| DarkpoolStateIntent::new(self.intent.clone(), share_stream.seed, recovery_stream.seed); | ||
| state_intent.compute_recovery_id(); |
There was a problem hiding this comment.
This advances the stored intent's recovery stream index from 0 to 1 at creation time. Since ring1.rs:345 also calls advance_recovery_stream() at first-fill settlement, the index ends up at 2 after the first fill instead of the expected 1. This also shifts the recovery ID values: the first-fill validity proof (intent_only.rs:85-86) clones the stored intent and calls compute_recovery_id() on the clone, so with stored index=1 the first onchain recovery ID would be H(seed || 1) instead of H(seed || 0).
The underlying bug is in state_wrapper.rs:191:
let index = self.recovery_stream.index - 1; // u64 underflow when index == 0compute_nullifier() assumes at least one recovery ID has been computed. For a Ring 1 intent before first fill, that assumption doesn't hold — there's no onchain state and no meaningful nullifier.
Suggested alternatives:
- Guard
compute_nullifier()— return a sentinel or error whenindex == 0, since a pre-first-fill Ring 1 intent has no onchain nullifier. - Fix the caller — whatever code path computes the nullifier of a pre-first-fill Ring 1 intent should check
has_been_filled(or equivalent) first and skip the nullifier computation.
This fixes ring1 order cancellation. The problem is that in
crates/darkpool-types/src/state_wrapper.rs:191, we have:This underflows when
self.recovery_stream.indexis 0, which is the case whencrates/workers/task-driver/src/tasks/create_order.rs:379creates a new state wrapper with default values.