cow: settle the idempotency seam on the venue-and-body intent-id - #466
Conversation
lgahdl
left a comment
There was a problem hiding this comment.
The intent_id derivation itself is sound (deterministic, body-scoped, verified different Signed payloads produce different ids), and the restart-regression test genuinely simulates a fresh process (independent MockHost::default(), only the local-store snapshot carried across) rather than just single-process dedup. Three things worth addressing:
| } | ||
| }; | ||
| let journal = Journal::submitted(host); | ||
| if journal.contains(&intent_id)? { |
There was a problem hiding this comment.
This is the real production journal site (submit_ready), and it has the same non-atomic check-then-act shape as the gap I flagged on #454 (videre-sdk::keeper.rs's journal.contains/journal.record): contains(&intent_id) here, then record(&intent_id) later after the actual network submit — two separate host calls with no atomicity between them. Two overlapping sweeps on the same host could both pass contains before either records. This PR's framing ("closes the double-post window") is about the pre-submit-derivability problem (the keeper can't derive a UID once order assembly moves into the adapter) — it doesn't touch this concurrency/atomicity race, which is a different problem and predates this PR (it was already split on the old UID key). Worth flagging as still open rather than implicitly closed by this PR's title.
There was a problem hiding this comment.
Confirmed still valid at the end-of-train tip, and you are right that this PR does not close it. It is already tracked as #538, filed off the same shape you flagged on #454.
The site moves. shepherd-sdk/src/cow/run.rs is deleted by #471, and this logic lands in composable-cow/src/sweep.rs, where the shape is intact at the tip: journal.contains(&intent_id)? at :133, the network submit at :138, journal.record(&intent_id) at :157. I have added that pointer to #538 so the tracker does not go stale against the carve.
One correction on the concurrency framing, which #538 already records in full. Two overlapping sweeps cannot interleave contains and record within an engine process, and it is not lock-based: the runtime moves one owned Supervisor into a single tokio task, the event loop selects one event and awaits dispatch_* to completion before the next, every dispatch and restart path takes &mut self, a timed-out guest call is dropped before dispatch_to returns, and restart replaces the wasmtime Store in place. Two engine processes over one redb state_dir is the only genuine race, and redb is single-process anyway.
Severity is also lower on this L3 CoW path than on the L2 keeper site #538 was filed against, because the venue backstops it. If the record write faults here the sweep logs and carries on by design (:153-159), the next tick re-posts, the orderbook answers with a duplicate error type, classification::is_already_submitted maps it to Refusal::AlreadyHeld (cow-venue/src/adapter.rs:391), post_order returns Posted::AlreadyHeld (:325), and the adapter answers SubmitOutcome::Accepted with assembly::order_uid(chain, order, owner) (:172). The journal write then succeeds on that pass, so the gap self-heals at a cost of one redundant POST rather than a duplicate order.
The L2 videre-sdk keeper site is the one that still bites, since it propagates Err after a submitted-but-unrecorded body and has no venue-side backstop. That remains #538's target, with the decided fix being reserve/commit/release over the existing verbs and no WIT change.
| /// An owner-signed order ready for the orderbook: what a | ||
| /// conditional-order keeper emits after a poll. | ||
| #[derive(BorshSerialize, BorshDeserialize, Clone, Debug, PartialEq, Eq)] | ||
| pub struct SignedOrder { |
There was a problem hiding this comment.
SignedOrder derives BorshSerialize over the whole struct including signature: Vec<u8>, and intent_id hashes the full encoded body — so the signature bytes are part of the dedup key. That's fine for collision-safety, but it means a legitimate re-sign of the same underlying order (same order+owner, fresh EIP-1271 signature after e.g. a keeper restart mid-flight, before the first attempt's journal write landed) produces a different intent_id and defeats dedup rather than falsely triggering it — the opposite failure mode from what this PR is guarding against. Concrete scenario: keeper signs, crashes before journaling, restarts, re-signs the same order, and if the first submit actually reached the orderbook, the second (differently-keyed) attempt isn't recognized as a duplicate. Worth either hashing (venue, owner, order) and excluding signature, or documenting explicitly that this key scopes to "this exact signed payload," not "this economic order."
There was a problem hiding this comment.
The fact is confirmed at the end-of-train tip: the sweep builds CowIntentBody::V1(CowIntent::Signed(SignedOrder { order, owner, signature })) (composable-cow/src/sweep.rs:120) and intent_id hashes the whole encoded body, so the signature bytes really are in the dedup key. Filed as #558.
The concrete scenario does not arise on this path though, for two independent reasons, both checked rather than assumed.
The keeper never signs. submit_ready receives the signature from source.poll's Verdict::Post { order, signature, .. } (composable-cow/src/sweep.rs:60), so the bytes come from the conditional-order source rather than being generated per attempt. For ComposableCoW that is the on-chain proof, and a restart re-reads the same deterministic bytes for the same order and params. There is no "re-sign" step in the loop to produce fresh bytes.
Even a genuinely different signature costs only one redundant POST. The orderbook's identity for an order is signature-independent: assembly::order_uid is order.uid(&chain.settlement_domain(), owner). So a differently-keyed second attempt at the same economic order still lands on the duplicate path, classification::is_already_submitted maps it to Refusal::AlreadyHeld (cow-venue/src/adapter.rs:391), and the adapter answers SubmitOutcome::Accepted with the derived canonical uid (:172). It comes back accepted, not double-posted.
So the failure mode is milder than "defeats dedup": dedup degrades to the venue's own idempotency rather than breaking. What genuinely remains is your second option, documenting that the key scopes to this exact signed payload and not to the economic order, plus the latent hazard that the argument above leans on a venue whose duplicate response is idempotent. #558 carries both, with documenting as the recommendation and excluding the signature from the key as the alternative.
There was a problem hiding this comment.
Revisited this and folded it into the PR rather than leaving it to the tracker, taking your documentation option.
intent_id's rustdoc now says the key covers the encoded body, so a signed payload keys on its signature and scopes to that exact payload rather than the economic order, which dedups only through the venue's duplicate response.
Folding was safe on this car specifically: cow-venue/src/client.rs is touched by no other open car in the train, and the only later change is #479's grouping move, a pure R100 rename with a zero-byte content delta.
The analysis above still stands on why the runtime behaviour is sound today, so this is a precision fix to the contract rather than a behaviour change. #558 stays open only to record the alternative (keying on (venue, owner, order)) should a venue ever be added whose duplicate response is not idempotent, and #466 now carries Closes #558.
| /// The CoW venue marker: every [`CowClient`] call routes to | ||
| /// [`Venue::ID`] and encodes a [`CowIntentBody`]. | ||
| /// [`Venue::ID`] and encodes a [`CowIntentBody`]. An accepted submit's | ||
| /// receipt is the canonical [`OrderUid`](crate::OrderUid) in wire form. |
There was a problem hiding this comment.
This comment (and the PR body) states "SubmitOutcome's accepted receipt is now the canonical 56-byte OrderUid," but nothing in this diff actually wires OrderUid into submit_order's return type or SubmitOutcome — neither symbol is touched here. OrderUid is added with solid conversions and round-trip tests, but it's constructed only in this PR's own unit tests; no production call site builds one from a real submit response yet. Worth either wiring it into the real return path in this PR, or scoping this comment/the PR description to "the type is added, integration follows" so it doesn't read as already-shipped.
There was a problem hiding this comment.
Correct as written, and the diff confirms it: this PR adds exactly that one doc line to client.rs and touches neither submit_order nor SubmitOutcome. OrderUid is built only in this PR's own unit tests.
The wiring lands in the very next car, #467 (feat/m4-cow-adapter-cdylib), which adds cow-venue/src/adapter.rs. There post_order decodes the orderbook's uid from the success response and submit returns SubmitOutcome::Accepted(uid.as_slice().to_vec()) (:174), the already-held path falls back to the derived assembly::order_uid rather than inventing a receipt (:172), the status path rejects anything that is not a 56-byte order uid (:203-204), and the tests assert receipt == uid.as_slice(). So the claim is accurate one car later, and byte-identical at the end-of-train tip where it holds.
I have left the code comment alone rather than spend a re-push and a full CI run on a line that is true from the next car onward, and scoped the PR body instead, which is where it read as already shipped. If you would rather the comment itself carry the "type added here, integration in #467" hedge for the duration of one car, say so and I will fold it into the next ripple.
09ad5b8 to
801b294
Compare
dbadb56 to
f82f4a0
Compare
The submitted: journal now keys on the deterministic intent-id (the generic sweep's venue-and-body submission key over the encoded CowIntentBody), derived pre-submit without assembling OrderCreation. SubmitOutcome's accepted receipt is fixed as the canonical 56-byte order UID (OrderUid), and the CowIntent schema gains the Signed kind a conditional-order keeper emits. A regression test covers resubmit after restart with a single orderbook POST.
f82f4a0 to
fb5d1a8
Compare
The submitted: journal now keys on the deterministic intent-id (the generic sweep's venue-and-body submission key over the encoded CowIntentBody), derived pre-submit without assembling OrderCreation. SubmitOutcome's accepted receipt is fixed as the canonical 56-byte order UID (OrderUid), and the CowIntent schema gains the Signed kind a conditional-order keeper emits. A regression test covers resubmit after restart with a single orderbook POST.
What
Keys the CoW
submitted:journal on the deterministic venue-and-bodyintent_id(cow-venue::intent_id, wrapping the generic sweep'ssubmission_keyover the encodedCowIntentBody) instead of the client-computed order UID. The id is derived pre-submit from the same body bytes a venue submit carries, beforeOrderCreationis assembled. TheOrderUidtype lands here with its conversions and round-trip tests; the adapter wires it intoSubmitOutcome's accepted receipt in #467.intent_id's rustdoc states what the key scopes to: a signed payload keys on its signature, so it covers that exact payload rather than the economic order.CowIntentgains aSignedvariant for a conditional-order keeper's owner-signed order.videre-sdk::keeper::submission_keyis now public so a keeper journalling outsideKeeper::sweepwrites the same key.twap-monitorandshepherd-sdktests and docs follow the rename.Why
Once order assembly moves into the venue adapter, the keeper can no longer derive a UID pre-submit, opening a double-post window across the keeper-to-adapter boundary. The venue-and-body intent-id is derivable before any network work and survives that move.
Testing
intent_iddeterminism and body-scoping,SignedOrderborsh round-trip,OrderUidconversions andDisplay.cargo fmt --all --check,cargo check --workspace --all-features,cargo clippy --workspace --all-targets --all-features -- -D warnings,cargo nextest run --workspace --all-features,cargo test --doc.AI Assistance
Implemented with Claude Code.
Closes #396
Closes #558