fix(contract): make mock attestations cleanable via expiry - #3785
Conversation
The DstackAttestation example references Quote, Collateral, TcbInfo, and ExpectedMeasurements without imports, breaking the --all-features doctest. Mark the block rust,ignore since it only illustrates the struct shape.
MockAttestation::Valid passed re-verification unconditionally, so its stored entry could never be evicted by clean_invalid_attestations and lingered in get_tee_accounts after a key migration (#3293). - Stamp DEFAULT_EXPIRATION_DURATION_SECONDS on every accepted mock, as Dstack attestations already do, converting a bare Valid into an expiring WithConstraints (explicit expiries are preserved). - Migrate existing stored mock entries during upgrade so pre-existing stale Valid entries also become cleanable.
With the contract now stamping an expiry on accepted mocks, a submitted
Mock::Valid is stored as WithConstraints{expiry}. The node confirmed mock
submissions by identity (stored == submitted), which would never match the
re-stamped form, so a node on the new contract would treat every mock
submit as not-landed and retry until it errored.
Assuming node-is-upgraded-before-contract, the upgraded node must work
with both contract versions:
- submitted_attestation_landed: confirm an expiry-carrying mock via the
same expiry-changed heuristic used for Dstack; fall back to identity for
the bare Valid form stored by older contracts.
- read_stored_attestation_expiry (renamed from _dstack): also return the
mock expiry so the pre-submit baseline is accurate.
- add VerifiedAttestation/MockAttestation::expiry_timestamp_seconds helpers.
…y tests - Use get_mut in stamp_expiry_on_legacy_mocks instead of clone-and-reinsert; drop the now-unneeded Clone derive on NodeAttestation. - Add unit tests for MockAttestation::with_expiry (public API). - Use intra-doc links in the new doc comments per engineering standards.
Resolve migration conflict in v3_13_0_state.rs (keep both main's OldConfig->Config conversion and the legacy-mock expiry stamping), adapt the new test to main's verify_and_store_mock API, and apply review fixups: - drop #3293 references from code comments (kept the explanations) - use TeeState::current_time_seconds() in the migration instead of an inline block_timestamp_ms()/1000 - add TODO(#3978) marking stamp_expiry_on_legacy_mocks as transitional - shorten the AcceptedAttestation::mock doc comment
There was a problem hiding this comment.
Pull request overview
This PR makes mock attestations eligible for cleanup by stamping them with an expiry (matching the existing Dstack expiry window), migrates already-stored legacy MockAttestation::Valid entries during contract upgrade, and updates the node + shared DTOs so an upgraded node can confirm submissions against both old (identity-stored mocks) and new (expiry-stamped mocks) contract behavior.
Changes:
- Stamp
DEFAULT_EXPIRATION_DURATION_SECONDSexpiry onto accepted mock attestations (preserving any explicit caller-provided expiry) and add a migration to stamp expiry onto legacy stored mocks. - Update node confirmation logic to treat expiry-stamped mocks like Dstack (expiry-change heuristic), while retaining identity fallback for old-contract / genesis-sentinel
Mock::Valid. - Add helpers/tests across
mpc-attestation, contract, and node to cover the new expiry-based behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/node/src/tee/remote_attestation.rs | Switches node baseline read to the generalized attestation-expiry reader. |
| crates/node/src/indexer/tx_sender.rs | Extends “landed” confirmation to expiry-stamped mocks; simplifies stored-expiry extraction; adds tests. |
| crates/node/src/indexer/fake.rs | Updates fake expiry reader method name to match renamed trait API. |
| crates/node/src/indexer.rs | Renames/expands expiry reader trait to cover both Dstack and expiry-carrying mocks. |
| crates/near-mpc-contract-interface/src/types/attestation.rs | Adds expiry_timestamp_seconds() helpers on shared DTOs (VerifiedAttestation, MockAttestation). |
| crates/mpc-attestation/tests/test_attestation_verification.rs | Updates verification test to assert mock acceptance produces WithConstraints{expiry}. |
| crates/mpc-attestation/src/attestation.rs | Implements mock expiry stamping (AcceptedAttestation::mock + MockAttestation::with_expiry) and unit tests. |
| crates/contract/src/v3_13_0_state.rs | Runs one-time migration during upgrade to stamp expiry on legacy stored mock attestations. |
| crates/contract/src/tee/tee_state.rs | Adds migration helper stamp_expiry_on_legacy_mocks and tests proving mocks become cleanable after expiry. |
| crates/contract/README.md | Marks a Rust doc snippet as rust,ignore to avoid doctest compilation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Pull request overviewMakes mock attestations cleanable by stamping them with the same Changes:
Reviewed changesPer-file summary
FindingsNon-blocking (nits, follow-ups, suggestions):
✅ Approved Overall: the design is clean (single |
Previously with_expiry preserved a caller-supplied mock expiry, so a submitter could set an arbitrarily long (uncleanable) expiry via MockAttestation::WithConstraints. The contract now owns the maximum mock lifetime — capping the expiry at now + DEFAULT_EXPIRATION_DURATION_SECONDS (a shorter caller value is kept) — matching the Dstack path where the contract sets the expiry outright. The migration likewise caps oversized legacy entries.
|
PR title type suggestion: This PR adds new functionality (expiry mechanism for mock attestations), so the type prefix should probably be |
- Clarify that a mock with no expiry can be an older contract OR a genesis sentinel (VerifiedAttestation dto + node ReadAttestationExpiry trait doc). - Note stamp_expiry_on_legacy_mocks stamps all stored mocks incl. genesis sentinels. - Use an intra-doc link for MockAttestation::Valid in AcceptedAttestation::mock. - Drop a redundant set_block_timestamp in the legacy-mock cleanup test.
|
PR title type suggestion: This PR adds a new capability to make mock attestations cleanable via expiry. Since it's adding new functionality rather than fixing a bug, the type prefix should be Suggested title: |
|
Thanks — dispositions per finding (addressed in a874994):
|
|
Keeping |
These tests submitted mocks with Some(u64::MAX) expiries and asserted get_attestation returned them verbatim. Now that the contract caps a mock's expiry at now + DEFAULT_EXPIRATION_DURATION_SECONDS, use in-window expiries (computed from the current block time) so they are preserved and stay distinct.
haiyuechen-nearone
left a comment
haiyuechen-nearone
left a comment
There was a problem hiding this comment.
I think the general flow LGTM, left a question about the comments in crates/node/src/indexer/tx_sender.rs
…nership); defer full alignment to #1639
|
PR title type suggestion: The phrasing 'make mock attestations cleanable' suggests this is adding new functionality rather than fixing a bug. Consider using Suggested title: |
netrome
left a comment
netrome
left a comment
There was a problem hiding this comment.
The change looks correct, but it feels hacky to overwrite expiry times on mock attestations on the verify call. That alone is not a hard blocker though, but the migration helper should be moved and the comment on submitted_attestation_landed should be simplified so it's not confusing people navigating the code base.
| fn mock(mock_attestation: &MockAttestation, current_timestamp_seconds: u64) -> Self { | ||
| let expiry_timestamp_seconds = | ||
| current_timestamp_seconds + DEFAULT_EXPIRATION_DURATION_SECONDS; | ||
| Self { | ||
| attestation: VerifiedAttestation::Mock(mock_attestation.clone()), | ||
| attestation: VerifiedAttestation::Mock( | ||
| mock_attestation | ||
| .clone() | ||
| .with_expiry(expiry_timestamp_seconds), | ||
| ), | ||
| advisory_ids: Vec::new(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Hmm definitely not a fan of overwriting the inner expiry timestamp on the verify call. This feels like it opens the door to bugs and risks, but I see you raised #4005 to tackle this so I won't consider it a hard blocker.
There was a problem hiding this comment.
I don't think the location of this is wrong.
After verifying an attestation, we store it. In this case we cap the expiry at our default now + DEFAULT_EXPIRATION_DURATION_SECONDS (if the submitted one was larger). This is deliberate: it stops a submitter from storing an arbitrarily long, uncleanable expiry (the goal of this PR).
For dstack we always set now + DEFAULT_EXPIRATION_DURATION_SECONDS, since extracting the actual expiry from the cert chain was hard (#1639).
#4005 is meant to align both dstack/mock to the same logic.
There was a problem hiding this comment.
Ah right I see the .with_expiry method performs a min between the existing expiry and the cap. That's a bit confusing though. I'd expect .with_expiry to be a plain setter. Perhaps worth renaming it to .cap_expiry() or something similar?
There was a problem hiding this comment.
- Move stamp_expiry_on_legacy_mocks (+ its test) into the v3_13_0_state migration module, since it is migration-only (netrome). - Simplify submitted_attestation_landed docstring: a changed stored expiry is enough to conclude the submit landed; drop the confusing 'identity' wording (the legacy equality fallback is covered by TODO(#3786)).
- Restore the TODO(#3978) on the moved migration helper (it was dropped when the helper moved to v3_13_0_state). - The simplified landing docstring referenced `TODO(#3786)` in prose, which the todo-format check flags as a malformed TODO; reword to drop it (the inline fallback TODO is self-sufficient).
|
PR title type suggestion: This PR adds a new capability to clean mock attestations via expiry, which is a feature addition rather than a bug fix. The type prefix should probably be Suggested title: |
It caps (min of existing and provided), not sets, so the old name read as a pure setter and misled readers (netrome). Renamed the method, its two call sites (AcceptedAttestation::mock, the migration helper), and its tests.
Reflect the refactor from last_used + read-time TTL to a stored expires_at (stamped now+TTL at write time), and note the migration now also stamps expiry on legacy mocks (main #3785).
…ependency Two facts from the full bot review that the earlier summary did not surface: - #3785 is merged (2026-07-29), so "land it first or alongside" described a dependency that no longer exists. Replaced with what actually remains: mock entries are sweepable now, but TeeState::with_mocked_participant_attestations still stores bare non-expiring Mock::Valid sentinels at init, which never fail re-verification and so are never swept or granted. - The legacy grandfather does not work the way we assumed when deciding to accept it. A live node re-attests under rule 1, so its entry never fails re-verification, is never swept, and yields no grant -- operators currently running nodes get nothing and need nothing. Only *abandoned* entries convert to grants. The number is still negligible today (14 mainnet, 31 testnet, nearly all live) and the deploy-time count check still bounds it, but the rationale is "abandoned entries are rare", not "it rewards our existing operators". Also: say why prepay keeps the remainder instead of following the contract's require_deposit + refund_to convention, and note that making the fee votable needs the ConfigExt DTO plumbing and a borsh-schema snapshot, not just the migration.
- Status line removed; the design is settled, not a draft for review. - Testing removed outright: the test plan belongs in the implementation PR. - Implementation notes removed, but two of its items were costs of design decisions rather than implementation chores, so they moved next to the decisions that cause them instead of disappearing: rule 3 now carries its own sweep gas-budget caveat, and the votable-Config row in Decisions carries the state migration, ConfigExt plumbing and snapshot regeneration. The non-universal sweepability caveat moved to the Security residual about swept entries, which is what it qualifies. Dropped entirely: the #3785 dependency note (merged, so there is nothing to sequence) and the runbook-coordination reminder (Operator UX already says operators must prepay first). Also repaired the API table, which an earlier edit had split by inserting a paragraph between its rows. 259 -> 144 lines across this and the previous trim.
Closes #3293
Implements the expiry-based approach from the issue thread: give mock attestations the same expiry window real (Dstack) ones already get, so the existing
clean_invalid_attestationsflow can evict them.Contract
AcceptedAttestation::mocknow stampsDEFAULT_EXPIRATION_DURATION_SECONDSon every accepted mock, converting a bareMockAttestation::Validinto an otherwise-unconstrained expiringWithConstraints(an explicit caller-set expiry is preserved). No borsh-layout change —WithConstraintsalready exists.stamp_expiry_on_legacy_mocks), so the staleMock::Validentries already on-chain also become cleanable.Node (upgrade-flow compatibility)
Assumes the node is upgraded before the contract, so the upgraded node must work against both the old and new contract.
The node confirmed mock submissions landed by identity (
stored == submitted). Once the contract stores a submittedMock::ValidasWithConstraints{expiry}, identity never matches, so a node on the new contract would treat every mock submit as not-landed and retry until it errored (and my expiry change also makes mocks expire, which triggers resubmission). Fixed:submitted_attestation_landed: confirm an expiry-carrying mock via the same expiry-changed heuristic already used for Dstack; fall back to identity for the bareValidform stored by older contracts.read_stored_attestation_expiry(renamed fromread_stored_dstack_expiry): also return the mock expiry so the pre-submit baseline is accurate.VerifiedAttestation/MockAttestation::expiry_timestamp_secondshelpers on the shared dtos.Compatibility matrix (new node):
Mock::Valid(no expiry) → identity match → landed ✓WithConstraints{expiry}→ expiry-changed heuristic → landed ✓Tests
with_expiry__should_*(Valid→expiring constraints, fill missing expiry + keep other constraints, preserve explicit expiry, leave Invalid unchanged); updatedvalid_mock_attestation_succeeds_verificationfor the new stored shapeclean_invalid_attestations__should_remove_accepted_mock_valid_after_expiry,stamp_expiry_on_legacy_mocks__should_make_valid_mock_cleanablesubmitted_attestation_landed__should_confirm_mock_with_changed_expiry,..._should_reject_mock_with_unchanged_expiry(old-contract identity path still covered by the existing mock tests)Out of scope / open question
The genesis sentinel path (
with_mocked_participant_attestations, used byinit/init_running) still stores non-expiringValidmocks. The migration converts existing ones, but fresh inits create new non-expiring entries. Stamping those too would change the #1087 placeholder semantics (participants would auto-expire if they never submit a real attestation), so I left it out of this PR.