fix(node): confirm submit_participant_info via advancing attestation expiry - #3736
Conversation
…expiry The post-submit confirmation reconstructed the attestation's creation time as `stored_expiry - DEFAULT_EXPIRATION_DURATION_SECONDS`, which breaks when the node and contract compile different values for that constant (version skew): the reconstructed age far exceeds the freshness bound, so every submission is treated as NotExecuted and retried indefinitely. Confirm instead that the stored expiry advanced past the value observed before submitting: a successful submit re-stamps expiry forward, a failed one leaves it unchanged. This drops the shared-constant dependency and holds under the planned verifier-rotation expiry cap (#3734), where stored expiry is no longer `creation + constant`. - Capture the pre-submit expiry baseline (read_pre_submit_attestation_expiry) - Decide via submitted_attestation_landed (Dstack: expiry advanced; Mock: equality) - Log the check result with both expiry values, and the not-stored path - Point the certificate-timestamp follow-up at the correct issue (#1639) - Unit-test the Dstack and Mock decision paths
62c307f to
98d9bc5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
The prior wording implied None means nothing was stored, but None also arises from a stored Mock; state the pure invariant instead.
This comment was marked as outdated.
This comment was marked as outdated.
kevindeforth
left a comment
kevindeforth
left a comment
There was a problem hiding this comment.
Thank you, requesting changes for now:
- querying the tee expiration status for all transactions is unnecessary overhead. I also think it's the wrong place, that information should be retrieved once, possibly before we call
submit_remote_attestation. - code comments are not very useful, please take a look at our engineering standards.
pbeza
left a comment
pbeza
left a comment
There was a problem hiding this comment.
There’s a corner case we don’t handle properly, and I don’t think we document it anywhere: if we shrink the contract’s attestation expiry window (DEFAULT_EXPIRATION_DURATION_SECONDS), this check breaks:
mpc/crates/node/src/indexer/tx_sender.rs
Line 223 in f89aaf1
Right now, to confirm that a submit landed, we check that the stored expiry is later than it was before the submit. That works because every successful submit sets expiry = block_now + DEFAULT_EXPIRATION_DURATION_SECONDS, so a fresh submit should normally push the expiry forward, assuming the duration stays the same.
But if a contract upgrade makes DEFAULT_EXPIRATION_DURATION_SECONDS smaller, for example 7 days → 1 day, the next submit can set an expiry that is earlier than the old one already stored on-chain. So the submit actually succeeds, but the expiry goes down instead of up, and our check thinks it failed and keeps retrying forever.
So basically the same retry storm as #3686, but triggered by shrinking the expiry window instead of a node/contract version mismatch.
In this case, the first submission would succeed and shorten the expiry, but the node will think it failed and re-try. so we will get only one false negative. which is fine by me. another option, is to still read once, but then ask the operators to just re-start the node. |
Address review feedback on the attestation submit-confirmation: - Move the confirmation into a dedicated confirm_participant_info_submission fn; observe_tx_result no longer takes a pre-submit baseline, keeping it off the generic signature. - Read the pre-submit baseline only for submit_participant_info. - Confirm via expiry *changed* (!=) rather than *advanced* (>), so a landed submit is detected even if a lowered expiry constant sets an earlier expiry; correctness no longer depends on re-reading per attempt. - Rewrite comments to explain why (invariant, Mock-vs-Dstack asymmetry). - TODO(#3746): note the retry loop reuses a stale attestation and stops on timeout; to be split into inner+outer loops.
Keep ensure_send_transaction / observe_tx_result generic (no per-transaction
logic), per review. The pre-submit attestation expiry is now read once per
round by the caller and carried on the request variant:
- SubmitParticipantInfo becomes a struct variant { args, pre_submit_expiry };
#[serde(flatten)] + #[serde(skip)] keep the on-chain args byte-identical.
- Add a ReadAttestationExpiry trait (real impl over the view client, fake stub);
periodic_attestation_submission / monitor_attestation_removal read the
baseline once and pass it down. Read error skips the round; no stored
attestation (e.g. first submit) is a normal Ok(None) that proceeds.
- observe_tx_result reads the baseline from the request; ensure_send_transaction
has no submit-specific code. Confirmation still uses expiry-changed (!=).
Tighten the doc comments the review flagged as verbose/distracting.
Per review: attestation_expiry_changed is a plain comparison, so drop the system-behavior doc from it and state the assumption once at the decision site (submitted_attestation_landed), linking DEFAULT_EXPIRATION_DURATION_SECONDS so the cross-crate reference is checked. Trim remaining where-used / paraphrasing comments.
kevindeforth
left a comment
kevindeforth
left a comment
There was a problem hiding this comment.
thank you! Just a nit and a comment regarding control flow
… task args Address review on the attestation-submission tasks: - On a pre-submit baseline read error, submit anyway (baseline = None) instead of skipping the round: a broken read must not block attestation submission, which would let the entry lapse and evict the node. Confirmation just runs without a baseline that round. - Bundle the shared task inputs into an AttestationSubmitter struct, dropping the too_many_arguments on periodic_attestation_submission / monitor_attestation_removal.
Assert the request serializes to the bare on-chain args with pre_submit_expiry omitted, protecting the #[serde(flatten)]/#[serde(skip)] invariant.
Covers the read-error path: a failing pre-submit baseline read must not block attestation submission.
kevindeforth
left a comment
kevindeforth
left a comment
There was a problem hiding this comment.
Thank you, this is very nice!
…submit-confirmation
|
I took the liberty to resolve the merge conflict. Would be nice to get this merged today. |
Closes #3686
The node confirmed
submit_participant_infoby reconstructing the attestation's creation time asstored_expiry − DEFAULT_EXPIRATION_DURATION_SECONDS. That only works when node and contract share the constant; under version skew (contract 3.12 = 7d, node 3.13 = 1d) the reconstructed age is ~6 days, always exceeding the 120s freshness bound, so every submission is treated asNotExecutedand retried indefinitely.Instead, confirm the submission by checking that the stored attestation expiry advanced past the value observed before submitting.
expiry − constantage reconstruction,MAX_ATTESTATION_AGE, and the shared-constant importattestation_expiry_advanced+ unit testsExpiry only ever advances via the node's own submit for its key, so this never yields a false positive. It also holds under the verifier-rotation expiry cap in #3734 (where stored expiry ≠
creation + constant), which lists #3686 as a prerequisite.Design discussion