feat(contract): auto-expire unused launcher image hashes - #3564
feat(contract): auto-expire unused launcher image hashes#3564barakeinav1 wants to merge 30 commits into
Conversation
2c212db to
ba05472
Compare
| pub(crate) launcher_hash: LauncherImageHash, | ||
| pub(crate) compose_hashes: Vec<LauncherDockerComposeHash>, | ||
| pub(crate) added: Timestamp, | ||
| pub(crate) last_attested: Timestamp, |
There was a problem hiding this comment.
Updated (superseded by the last_used refactor in c435dc5):
last_used is a single keep-alive signal — stamped when the hash is voted in / re-voted, and refreshed on each attestation by a current participant. An entry is expired when last_used + TTL < now; the all-expired fallback keeps the most-recently-used entry.
(Originally this was two fields, added + last_attested, with expiry on max(...). Collapsed to one per Patrick's suggestion — the distinction was not load-bearing.)
There was a problem hiding this comment.
I have a feeling it's an overkill to have both in. I feel you only need the last_attested and potentially a bool to state whether it should benefit from full grace or partial one. Then this last_attested is as done currently updated along with the boolean. The expiry would be if bool=true then check last_attested<full grace period , otherwise check last_attested < partial_grace.
Of course this is assuming you have two different grace periods. If you have only one then the logic is even simpler
There was a problem hiding this comment.
Fixed already — collapsed to a single last_used in c435dc5 (we have one TTL / one grace period, so no bool needed). Sorry for the churn on this thread.
ba05472 to
b2d72fe
Compare
Launcher image hashes accumulated forever; removal required a unanimous vote. This adds usage-based expiry: - AllowedLauncherImage gains added/last_attested timestamps; an entry is expired when max(added, last_attested) + TTL < now - last_attested is refreshed on a successful attestation, but ONLY for a current participant (enforced by requiring an AuthenticatedParticipantId); a prospective/non-participant node cannot keep a stale launcher alive - reads filter out expired entries, with a newest-entry fallback so the allowed set never goes empty - verify_tee spawns a detached self-call to a new #[private] clean_expired_launcher_hashes that sweeps expired entries from storage - re-voting an existing launcher hash refreshes its added timestamp - new config launcher_hash_unused_ttl_seconds (default 14d), validated >= DEFAULT_EXPIRATION_DURATION_SECONDS - state migration (v3_12_0_state) initializes timestamps for existing entries Updates the design doc status to Implemented and regenerates the borsh-schema and ABI snapshots for the new fields/method. Closes #3381
b2d72fe to
99f6904
Compare
There was a problem hiding this comment.
Pull request overview
Implements the approved “auto-expire unused launcher image hashes” design by adding usage-based expiry for allowed launcher images in the contract, including config/ABI updates, state migration for the new borsh layout, and test coverage for refresh/expiry/cleanup behavior.
Changes:
- Add
added/last_attestedtimestamps to launcher allowlist entries and filter expired entries at read/verify time (with newest-entry fallback). - Refresh
last_attestedon successful participant submissions and add a detached private cleanup self-call to physically evict expired entries. - Introduce new config knobs (
launcher_hash_unused_ttl_seconds,clean_expired_launcher_hashes_tera_gas) with interface + DTO mapping + snapshots + migration/tests updated.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/design/auto-remove-launcher-hashes-design.md | Marks the design as implemented and updates wording/invariants/decisions to match the shipped behavior. |
| crates/test-utils/src/contract_types.rs | Extends dummy config builder with the new TTL + cleanup gas fields. |
| crates/near-mpc-contract-interface/src/types/config.rs | Adds the new config fields to InitConfig/Config and updates serialization tests. |
| crates/near-mpc-contract-interface/src/method_names.rs | Adds the clean_expired_launcher_hashes method name constant. |
| crates/mpc-attestation/src/attestation.rs | Exposes the launcher compose hash from a verified attestation to support refresh-on-use. |
| crates/contract/tests/snapshots/abi__abi_has_not_changed.snap | Updates ABI snapshot for the new private cleanup method and config fields. |
| crates/contract/tests/sandbox/upgrade_from_current_contract.rs | Updates sandbox upgrade test config to include the new TTL + cleanup gas fields. |
| crates/contract/tests/sandbox/contract_configuration.rs | Updates sandbox init config test to include the new TTL + cleanup gas fields. |
| crates/contract/src/v3_12_0_state.rs | Adds 3.12.0 shadow types for launcher allowlist migration and a migration round-trip test. |
| crates/contract/src/tee/tee_state.rs | Threads launcher TTL through verification paths and adds refresh-on-use + cleanup hooks. |
| crates/contract/src/tee/proposal.rs | Implements TTL filtering, fallback selection, refresh-on-use, re-vote refresh, and cleanup for launcher allowlist entries (with tests). |
| crates/contract/src/snapshots/mpc_contract__tests__mpc_contract_borsh_schema_has_not_changed.snap | Updates borsh schema snapshot for new fields. |
| crates/contract/src/lib.rs | Wires TTL into submit/verify/read paths, spawns detached cleanup self-call, adds private cleanup endpoint, and validates config updates. |
| crates/contract/src/dto_mapping.rs | Maps new config fields between DTOs and contract config. |
| crates/contract/src/config.rs | Adds new config fields, defaults, and a validation invariant tying TTL to attestation expiry window. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn update_config(&mut self, config: dtos::Config) { | ||
| self.config = config.into(); | ||
| let new_config: Config = config.into(); | ||
| if let Err(e) = new_config.validate() { | ||
| env::panic_str(e); | ||
| } | ||
| self.config = new_config; |
There was a problem hiding this comment.
Good catch — fixed in d7c0c65. init and init_running now build the config, call config.validate(), and propagate the error (both are #[handle_result]), so the contract can no longer be initialized with a TTL below the attestation validity window. Added init_rejects_launcher_ttl_below_attestation_validity to lock it in.
Pull request overviewImplements usage-based expiry for Changes:
Reviewed changesPer-file summary
FindingsBlocking (must fix before merge):
Non-blocking (nits, follow-ups, suggestions):
|
Address review feedback: - init/init_running now validate the config (not just update_config), so the contract cannot be initialized with launcher_hash_unused_ttl_seconds below the attestation validity window; add a regression test. - dummy_config: give clean_expired_launcher_hashes_tera_gas a unique offset (was duplicating remove_non_participant_tee_verifier_votes_tera_gas).
Address non-blocking review comments: - is_expired: note why overflow returns not-expired (never panic on a bogus timestamp) - add(): note re-vote refreshes only the clock; compose hashes managed separately - refresh_launcher_usage: note the AuthenticatedParticipantId is a capability token
|
Thanks for the thorough review. Responses inline:
Fixed in d7c0c65 — both init paths now call
Done (a8694aa): added a comment explaining the deadline is unrepresentably far off and we must never panic on a bogus timestamp.
Done (a8694aa): the re-vote branch now notes compose hashes are maintained separately via
Done (a8694aa): docstring now states it is a capability token with the value intentionally unused.
Left as-is: the proposal-level
Left as-is — micro / the list is tiny, as you noted. |
…and design doc DEFAULT_EXPIRATION_DURATION_SECONDS was changed 7d -> 1d in #3626. Update the stale '7 days' wording; reference the constant instead of hardcoding a day-count in test comments to avoid future drift. The MPC docker-image grace period (DEFAULT_TEE_UPGRADE_DEADLINE_DURATION_SECONDS) remains 7 days.
Good catch — fixed in cb955fb. It was deliberately changed 7d → 1d in #3626, so the wording was stale. Updated the comments/design doc to "1 day", and to avoid this drift recurring I dropped the hardcoded day-count in the test comments (they now just reference the constant). The design-doc safety argument is now phrased around the constant ( |
| pub(crate) added: Timestamp, | ||
| pub(crate) last_attested: Timestamp, |
There was a problem hiding this comment.
Wouldn’t it be simpler to just have last_used instead of both added and last_attested? I don’t see why we need that distinction. last_used could then be refreshed both when re-voting for the launcher hash and when the attestation is used.
There was a problem hiding this comment.
Wouldn't it be simpler to just have
last_usedinstead of bothaddedandlast_attested?
Agreed — collapsed to a single last_used (refreshed on both re-vote and participant attestation) in c435dc5.
For context on why I originally split them: I wanted to distinguish governance liveness (when the hash was voted in) from usage liveness (last participant attestation), so the all-expired fallback would deliberately retain the most-recently-voted-in hash. But the only readers are the expiry check (max, unchanged) and that fallback — and "keep the most-recently-used" is just as sensible there (arguably better). So the distinction isn't load-bearing; one field is simpler and equivalent.
There was a problem hiding this comment.
Oups sorry commented the same thing. Just noticed you had the same comment
| pub(crate) added: Timestamp, | ||
| pub(crate) last_attested: Timestamp, |
There was a problem hiding this comment.
It’s a bit confusing to use the Timestamp type here while we use u64 in other places. We should probably align on one type unless there’s a specific reason not to:
mpc/crates/mpc-attestation/src/attestation.rs
Lines 206 to 208 in 187fffa
There was a problem hiding this comment.
It's a bit confusing to use the
Timestamptype here while we useu64in other places.
Kept Timestamp here: in this file it matches the sibling AllowedMpcDockerImage.added: Timestamp, and gives checked_add/now/Ord for free. The u64 you linked is in the mpc-attestation crate (raw unix seconds in the attestation DTO) — switching the launcher fields to u64 would make them inconsistent with their neighbor here. The broader u64-vs-Timestamp alignment across crates is a real but pre-existing, separate cleanup.
| /// Prepaid gas for a `remove_non_participant_tee_verifier_votes` call. | ||
| pub(crate) remove_non_participant_tee_verifier_votes_tera_gas: u64, | ||
| /// TTL after which a launcher image hash unused by any participant is evicted. | ||
| pub(crate) launcher_hash_unused_ttl_seconds: u64, |
There was a problem hiding this comment.
I see no test covering enlarging the launcher_hash_unused_ttl_seconds config while entries are stored: a bigger TTL should bring back an entry a smaller TTL had hidden, since filtering only hides (doesn't delete — only cleanup_expired does). Existing tests only advance time at a fixed TTL.
There was a problem hiding this comment.
I see no test covering enlarging the
launcher_hash_unused_ttl_seconds... a bigger TTL should bring back an entry a smaller TTL had hidden
Good point — added enlarging_ttl_unhides_previously_expired_entry in c435dc5, asserting a larger TTL re-surfaces an entry a smaller TTL hid (filtering hides, only cleanup_expired deletes).
|
The design doc [docs/design/auto-remove-launcher-hashes-design.md](https://github.com/near/mpc/blob/main/docs/design/auto-remove-launcher-hashes-design.md still says it's a Draft |
I should have written "design had been reviewed (by @pbeza and @netrome) ) |
… simplify live_indices Address review feedback: - AllowedLauncherImage: replace added + last_attested with one last_used, refreshed on both re-vote and participant attestation. Expiry and the all-expired fallback (now newest-by-last_used) are unchanged in behavior. - live_indices: drop the .expect()/empty-guard in favor of max_by_key(...).unwrap_or_default(). - Add a test that enlarging the TTL un-hides an entry a smaller TTL filtered (read-time filtering hides, never deletes). - Regenerate borsh-schema snapshot for the field change.
SimonRastikian
left a comment
SimonRastikian
left a comment
There was a problem hiding this comment.
Partial review only. Will do the second part most likely later today
| // Overflow means the deadline is unrepresentably far in the future, so the | ||
| // entry is not expired. Never panic here: a bogus timestamp must not evict a hash. | ||
| None => false, |
There was a problem hiding this comment.
That's interesting, not sure if I strongly like it or strongly dislike it. I guess this should be fine and cannot be called adversarially. or can it? If it can then better evict the hash (even on bogus timestamp)
There was a problem hiding this comment.
Not adversarial: last_used is stamped by the contract via Timestamp::now() (block time, seconds) — never user-supplied — so last_used + ttl cannot approach u64::MAX for ~centuries. Overflow is unreachable in practice, so returning "not expired" is safe (and we prefer never to evict a hash on an arithmetic edge). Kept as-is with the explanatory comment.
There was a problem hiding this comment.
@barakeinav1 @SimonRastikian I think we should add a log! here. If this ever gets printed, it would be a red flag that something is very wrong. Without logging it, we might never catch it.
SimonRastikian
left a comment
SimonRastikian
left a comment
There was a problem hiding this comment.
Partial review only. Will do the second part most likely later today
…c to last_used
Address review feedback (SimonRastikian):
- add -> add_or_refresh returning enum AddOutcome { Added, Refreshed }; the
bool return was always true. Thread the outcome through add_launcher_image
and make vote_add_launcher_hash log added vs refreshed. (internal only; not
in state/ABI)
- fix all_compose_hashes/launcher_hashes docstrings: they return live entries
(non-expired, or the most-recently-used fallback when all expired)
- fix stale refresh_launcher_usage docstring (last_attested -> last_used)
- rewrite the design doc to the single last_used model (struct, expiry,
mermaid, migration) and correct the stale 1-day attestation-validity wording
There was a problem hiding this comment.
I briefly re-scanned the PR after you rebased it on the latest main, with #3714 and #3715 merged in, and it looks good to me. But I’d prefer to re-review the whole PR from scratch, since it’s been a while since I last approved it and this is a big PR that has now been rebased on top of several other large PRs.
So ideally, someone else (@gilcu3?) could also take another look next week before we merge it, since I’ll be OoO the whole week.
I can take a look next Tuesday when I am back. This is a tricky PR indeed, so the more eyes the better. |
|
@claude review |
1 similar comment
|
@claude review |
Pull request overviewImplements usage-based expiry for the launcher-image allowlist. The core mechanics look right to me: the signer is genuinely preserved across the verifier promise so the capability gate holds in the async callback; Changes:
Reviewed changesPer-file summary
FindingsBlocking (must fix before merge):
Non-blocking (nits, follow-ups, suggestions):
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/test-utils/src/contract_types.rs:23
dummy_configis documented as producing different values for eachConfigfield, butclean_expired_launcher_hashes_tera_gascurrently duplicatesverifier_tera_gas(value + 14). This contradicts the docstring and reduces the usefulness of the dummy config in tests.
clean_expired_launcher_hashes_tera_gas: value + 14,
- docs: express the attestation-validity window via DEFAULT_EXPIRATION_DURATION_SECONDS instead of a hardcoded day count (design doc); update README launcher views to note expiry filtering; update operator guide (auto-expiry vs. immediate vote-remove, and the query now hides expired digests) - test: add participant-gate coverage above TeeState — verify_and_store_mock rejects an expired launcher hash; resolve_verification refreshes for a participant and not for a non-participant - verify_tee: guard the detached sweep with has_expired() to skip a wasted receipt when nothing is expired - is_expired: drop the per-entry overflow log - clean_expired_launcher_hashes: drop the always-Ok Result - proposal.rs: document newest_index tie behavior; remove dead set_block_secs; rename re_add_refresh test after the added->last_used rename
|
Thanks — thorough review. Addressed across Blocking
Not a regression by this PR — the branch had merged
Done. README launcher views now describe expiry filtering; operator guide explains auto-expiry vs. immediate
Both gate sites now have negative tests in this PR:
New tests use the Non-blocking
Dropped the per-entry
Same fix (log removed). Skipped adding an upper bound: the field is set only by the deployer at init or a participant threshold vote (
Done —
Done — dropped
Removed the dead line. (Left the two scenarios in one test.) ✅
Documented the tie behavior on
Renamed to |
check-assert-matches.sh flags assert!(matches!(..)) unless the matched type lacks Debug and is annotated; PromiseOrValue doesn't implement Debug.
…t refresh Covers submit_participant_info's mock arm (the second refresh gate site): a non-participant's WithConstraints mock stores but does not extend the referenced launcher's last_used.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
docs/running-an-mpc-node-in-tdx-external-guide.md:1829
- This line states an unused digest “is evicted automatically once no node has attested with it” after TTL. In the implementation, expiry is enforced by read-time filtering with a newest-entry fallback (so one digest can still be returned even if all are expired), and physical eviction is done later by an internal cleanup. The wording should match those semantics.
An unused launcher manifest digest now auto-expires after the configured TTL (`launcher_hash_unused_ttl_seconds`, default 14 days) and is evicted automatically once no node has attested with it for that window, so no vote is needed for routine rotation. The unanimous `vote_remove_launcher_hash` is only needed to remove a still-valid digest *immediately* (before its TTL lapses), for example a compromised launcher.
docs/running-an-mpc-node-in-tdx-external-guide.md:1739
- This sentence suggests expired digests are always removed from results (“auto-evicted”), but the contract keeps a newest-entry fallback when all entries are expired, and physical eviction happens later via a detached cleanup call. The guide should reflect the read-time filtering + newest fallback semantics rather than implying immediate removal.
This issue also appears on line 1829 of the same file.
The contract method is named `allowed_launcher_image_hashes` for historical reasons, but the values returned are manifest digests. The query returns only non-expired digests; digests that have aged out past their TTL are hidden and auto-evicted.
crates/contract/src/lib.rs:4156
- Test name doesn’t follow the repository’s required pattern
fn <system_under_test>__should_<assertion>()from AGENTS.md, which the surrounding tests in this module adhere to.
fn init_rejects_launcher_ttl_below_attestation_validity() {
crates/contract/src/v3_13_0_state.rs:209
- Test name doesn’t follow the repository’s required pattern
fn <system_under_test>__should_<assertion>()from AGENTS.md. Using the standard naming makes it easier to scan and keep consistency across the suite.
fn migrating_launcher_images_preserves_hashes_and_stamps_timestamps() {
gilcu3
left a comment
gilcu3
left a comment
There was a problem hiding this comment.
Looks quite good, although it takes a lot of time to go through this. Added some comments, still need to review changes in crates/contract/src/tee/tee_state.rs. Will continue tomorrow
| //! | ||
| //! Relative to `3.13.0`, this release adds two `Config` fields | ||
| //! (`launcher_hash_unused_ttl_seconds`, `clean_expired_launcher_hashes_tera_gas`) and a | ||
| //! `last_used` timestamp to each `AllowedLauncherImage`. Those are the only borsh-layout | ||
| //! changes, so only `Config` and `TeeState` are shadowed here; every other field reuses the | ||
| //! real (byte-identical) type. |
There was a problem hiding this comment.
this comment can be removed, as it is explaining something that is very visible in the code already
There was a problem hiding this comment.
Done — removed.
| /// The launcher compose hash this attestation was validated against, if it carries one. | ||
| pub fn launcher_compose_hash(&self) -> Option<LauncherDockerComposeHash> { | ||
| match self { | ||
| Self::Dstack(a) => Some(a.launcher_compose_hash), |
There was a problem hiding this comment.
nit:
| Self::Dstack(a) => Some(a.launcher_compose_hash), | |
| Self::Dstack(attestation) => Some(attestation.launcher_compose_hash), |
There was a problem hiding this comment.
Done — renamed the binding to attestation.
| /// The `3.13.0` launcher layout (no timestamp) must deserialize under the shadow and | ||
| /// migrate: launcher hash + compose hashes preserved, and `last_used` set to the | ||
| /// migration time (NOT the borsh/epoch default, which would immediately expire every | ||
| /// migrated hash). Two entries + a short TTL defeat the newest-only read fallback, so | ||
| /// both surviving proves the timestamp was stamped to "now". |
There was a problem hiding this comment.
nit: this is also not needed. Sorry for being annoying on this one, we are trying to keep comments to the necessary as they tend to add bloat.
There was a problem hiding this comment.
Done — removed.
| Ok(self | ||
| .tee_state | ||
| .clean_invalid_attestations(tee_upgrade_deadline_duration, max_scan as usize)) | ||
| Ok(self.tee_state.clean_invalid_attestations( | ||
| tee_upgrade_deadline_duration, | ||
| Duration::from_secs(self.config.launcher_hash_unused_ttl_seconds), | ||
| max_scan as usize, |
There was a problem hiding this comment.
I didn't expect this change, will need to look deeper on why it is needed
There was a problem hiding this comment.
It needed the launcher TTL because clean_invalid_attestations re-verifies stored attestations against the (then read-time-filtered) launcher allowlist. This is now moot: the expires_at refactor removed the read-time TTL entirely, so clean_invalid_attestations no longer takes a launcher-TTL param (02a05c3).
| let config: Config = init_config.map(Into::into).unwrap_or_default(); | ||
| config | ||
| .validate() | ||
| .map_err(|reason| InvalidParameters::MalformedPayload { | ||
| reason: reason.to_string(), | ||
| })?; |
There was a problem hiding this comment.
we should make the validation part of the into conversion to make this more ergonomic, and avoid someone forgetting to call validate
There was a problem hiding this comment.
Done — InitConfig/Config → Config are now TryFrom impls that validate, so init/init_running/update_config can't skip validation (02a05c3).
| VMContextBuilder::new() | ||
| .signer_account_id(non_participant.clone()) | ||
| .predecessor_account_id(non_participant) | ||
| .attached_deposit(MINIMUM_ATTESTATION_STORAGE_DEPOSIT) |
There was a problem hiding this comment.
is this just out of date? Or we have not removed the deposit yet on main?
There was a problem hiding this comment.
this is before we removed the deposit, I need to rebase main.
There was a problem hiding this comment.
Good catch — the deposit was dropped on main (#3940). Re-merged main, so MINIMUM_ATTESTATION_STORAGE_DEPOSIT is gone from these tests too.
| pub(crate) fn new( | ||
| launcher_hash: LauncherImageHash, | ||
| compose_hashes: Vec<LauncherDockerComposeHash>, | ||
| ) -> Self { | ||
| Self { | ||
| launcher_hash, | ||
| compose_hashes, | ||
| last_used: Timestamp::now(), | ||
| } | ||
| } | ||
|
|
||
| fn is_expired(&self, ttl: Duration, now: Timestamp) -> bool { | ||
| match self.last_used.checked_add(ttl) { | ||
| Some(deadline) => deadline < now, | ||
| // Overflow ⇒ deadline unrepresentably far off, so treat as not expired rather than | ||
| // panic: a bogus timestamp (or an enormous TTL) must never evict a hash. | ||
| None => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
shouldn't is_expired do the same as new above and take the current time from Timestamp::now? Else the function semantics is a bit off
There was a problem hiding this comment.
Addressed by the expires_at refactor — is_expired(now) is now just expires_at < now, and Timestamp::now() is only used at write time when stamping expires_at. The new vs is_expired asymmetry you flagged is gone.
| log!("launcher hash already in allowed list"); | ||
| return false; | ||
| existing.last_used = Timestamp::now(); | ||
| return AllowedLauncherImageInsertion::Refreshed; |
There was a problem hiding this comment.
ok I see in another thread that it was moved, should be fine
| fn is_expired(&self, ttl: Duration, now: Timestamp) -> bool { | ||
| match self.last_used.checked_add(ttl) { | ||
| Some(deadline) => deadline < now, | ||
| // Overflow ⇒ deadline unrepresentably far off, so treat as not expired rather than | ||
| // panic: a bogus timestamp (or an enormous TTL) must never evict a hash. | ||
| None => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
having to pass the ttl to many methods seems like a code smell. As I explained in slack this does not seem necessary, as we could simply store the expire at time instead, like we do for attestations.
There was a problem hiding this comment.
Done — implemented the stored-expires_at model you suggested (02a05c3): each entry stores expires_at = now + TTL, stamped at write time. Expiry is a plain expires_at < now, so the TTL no longer threads through the ~20 read/verify sites — only the 3 write sites (vote-in, refresh, migration) take it (still have it for tests though) . Thanks for the suggestion.
| /// Refreshes the `last_used` timestamp of the launcher image referenced by the stored | ||
| /// attestation for `tls_public_key`. The [`AuthenticatedParticipantId`] is an unused | ||
| /// capability token — requiring it means only a current participant can refresh. | ||
| pub(crate) fn refresh_launcher_usage( | ||
| &mut self, | ||
| tls_public_key: &Ed25519PublicKey, | ||
| _authenticated_participant: &AuthenticatedParticipantId, | ||
| ) { |
There was a problem hiding this comment.
that's a very strange (or advanced) way of using the type system :)
There was a problem hiding this comment.
It's a capability-token / proof-by-construction pattern (suggested by claude) : an AuthenticatedParticipantId can only be built via ::new(participants), which succeeds only when env::signer_account_id() is a current participant. So requiring one as a parameter means refresh_launcher_usage cannot be called unless the caller has proven participant status — a compile-time gate.
The type is already used across the contract (vote_launcher, vote_measurement, CodeHashesVotes::vote, …), but in those its value is used (the participant identity). Here it's a pure gate, so the value is unused (_authenticated_participant) — which is what reads oddly.
The runtime gate already lives at the lib layer (if let Some(participant) = AuthenticatedParticipantId::new(...).ok()), so this param is a belt-and-suspenders compile-time backstop against a future caller forgetting the check. I'm happy either way: keep it (defensive) or drop the unused param and rely on the lib-layer gate + a doc comment. Do you have a preference?
There was a problem hiding this comment.
I was just thinking if there would be a more natural way of achieving the same. But we can leave that for future coders, like it is now is also fine IMO
…cher-hashes # Conflicts: # crates/contract/src/lib.rs # crates/contract/src/v3_13_0_state.rs
Reynaldo's main suggestion: store a single expires_at timestamp per launcher image (stamped now+TTL at write time) instead of last_used + a read-time TTL. Expiry becomes a plain expires_at < now, and the TTL no longer threads through the ~20 read/verify call sites — only the 3 write sites (vote-in, refresh, migration) take it. Overflow saturates at Timestamp::MAX. Also: - fold Config validation into the DTO->Config conversion (TryFrom) so it can't be forgotten - Given/When/Then + clearer constants on the new launcher tests - drop two redundant comments in v3_13_0_state; tidy an attestation binding - regenerate the borsh schema snapshot (last_used -> expires_at)
Our launcher shadow-conversion and main's stamp_expiry_on_legacy_mocks were each tested in isolation. Add a test exercising the full From<MpcContract> sequence on one TeeState: a launcher image gets its expires_at stamped AND a legacy Valid mock becomes cleanable, confirming the two steps compose correctly.
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).
gilcu3
left a comment
gilcu3
left a comment
There was a problem hiding this comment.
Dropping some more comments.
Main one is the use of a promise for the cleanup, that seems not needed taking into account what I explain in the comment.
Hopefully we can get this merged on Monday when the other reviewers are back!
| testing_env!( | ||
| VMContextBuilder::new() | ||
| .signer_account_id(signer) | ||
| .block_timestamp(10 * 1_000_000_000) | ||
| .build() | ||
| ); | ||
| let authenticated = AuthenticatedParticipantId::new(&participants).unwrap(); | ||
|
|
||
| let mut tee_state = TeeState::default(); | ||
| tee_state | ||
| .allowed_launcher_images | ||
| .add_or_refresh(launcher_1, &[mpc_hash]); | ||
|
|
||
| // A second (newer) launcher so the list is never empty — this defeats the | ||
| // newest-only read fallback and lets us observe real expiry. | ||
| testing_env!( | ||
| VMContextBuilder::new() | ||
| .block_timestamp(20 * 1_000_000_000) |
There was a problem hiding this comment.
we need to name this constants to clarify what's the aim
| /// Refreshes the `last_used` timestamp of the launcher image referenced by the stored | ||
| /// attestation for `tls_public_key`. The [`AuthenticatedParticipantId`] is an unused | ||
| /// capability token — requiring it means only a current participant can refresh. | ||
| pub(crate) fn refresh_launcher_usage( | ||
| &mut self, | ||
| tls_public_key: &Ed25519PublicKey, | ||
| _authenticated_participant: &AuthenticatedParticipantId, | ||
| ) { |
There was a problem hiding this comment.
I was just thinking if there would be a more natural way of achieving the same. But we can leave that for future coders, like it is now is also fine IMO
| fn from(value: near_mpc_contract_interface::types::Config) -> Self { | ||
| Config { | ||
| impl TryFrom<near_mpc_contract_interface::types::Config> for Config { | ||
| type Error = &'static str; |
There was a problem hiding this comment.
this should be Error or ConversionError, as used in other imp in this file
| let launcher_unused_ttl = | ||
| Duration::from_secs(self.config.launcher_hash_unused_ttl_seconds); | ||
|
|
||
| // Capability token: only a current participant may keep its launcher hash alive. | ||
| let authenticated_participant = self | ||
| .protocol_state | ||
| .threshold_parameters() | ||
| .ok() | ||
| .and_then(|params| AuthenticatedParticipantId::new(params.participants()).ok()); | ||
| let tls_public_key_for_refresh = node_id.tls_public_key.clone(); |
There was a problem hiding this comment.
nit: better do define vars right before they are used, unless the compiler restricts it ofc
| if let Some(participant) = &authenticated_participant { | ||
| self.tee_state.refresh_launcher_usage( | ||
| &tls_public_key_for_refresh, | ||
| participant, | ||
| launcher_unused_ttl, | ||
| ); |
There was a problem hiding this comment.
nit: I believe this is an accepted pattern in rust
| if let Some(participant) = &authenticated_participant { | |
| self.tee_state.refresh_launcher_usage( | |
| &tls_public_key_for_refresh, | |
| participant, | |
| launcher_unused_ttl, | |
| ); | |
| if let Some(authenticated_participant) = &authenticated_participant { | |
| self.tee_state.refresh_launcher_usage( | |
| &tls_public_key_for_refresh, | |
| authenticated_participant, | |
| launcher_unused_ttl, | |
| ); |
| let current_params = running_state.parameters.clone(); | ||
|
|
||
| // Physical eviction runs in a detached self-call so it can never fail this | ||
| // transaction. Skip it when nothing is expired to avoid a wasted receipt on the | ||
| // common (timer-driven) path. | ||
| if self.tee_state.has_expired_launcher_images() { | ||
| Promise::new(env::current_account_id()) | ||
| .function_call( | ||
| method_names::CLEAN_EXPIRED_LAUNCHER_HASHES.to_string(), | ||
| vec![], | ||
| NearToken::from_yoctonear(0), | ||
| Gas::from_tgas(self.config.clean_expired_launcher_hashes_tera_gas), | ||
| ) | ||
| .detach(); | ||
| } |
There was a problem hiding this comment.
It felt strange to have this here, and I found out why. For the docker mpc-node hashes, we do the cleanup inline in
mpc/crates/contract/src/tee/tee_state.rs
Lines 278 to 284 in f5c0f4a
Notice that when this function runs, these values are already in memory, not lazy loaded, and we expect the amount to be pretty small. Therefore, I think we should move this code to reverify_and_cleanup_participants, right after the line:
self.allowed_docker_image_hashes
.cleanup_expired_hashes(tee_upgrade_deadline_duration);
and then remove the promise and the new config value, that seem no needed in this case.
| #[test] | ||
| fn init_rejects_launcher_ttl_below_attestation_validity() { | ||
| let participants = gen_participants(3); | ||
| let signer = participants.participants()[0].0.clone(); | ||
| testing_env!( |
There was a problem hiding this comment.
Nice, although apparently in this test where the comment lives nothing changed 🤔
| /// Returns all allowed launcher image hashes. | ||
| pub fn get_allowed_launcher_hashes(&self) -> Vec<LauncherImageHash> { | ||
| self.allowed_launcher_images.launcher_hashes() | ||
| } |
There was a problem hiding this comment.
nit: this was probably not removed on purpose
| /// Whether a `clean_expired_launcher_images` sweep would evict anything. | ||
| pub fn has_expired_launcher_images(&self) -> bool { | ||
| self.allowed_launcher_images.has_expired() | ||
| } |
There was a problem hiding this comment.
if dropping the cleanup promise for launcher hashes as I suggested in another comment, this will also disappear, so it will be a good simplification
| /// Index of the entry with the latest expiry, if any. On ties (e.g. right after a | ||
| /// migration stamps every entry with the same `expires_at`) this returns the last such | ||
| /// entry — deterministic, but the specific choice among equal entries is arbitrary. | ||
| fn newest_index(&self) -> Option<usize> { |
There was a problem hiding this comment.
nit: the name is now a bit misleading, because the ttl is a dynamic parameter. so latest_expiry_index would probably be a more accurate name
9cf1bd1 to
93c9ab5
Compare
|
PR title type suggestion: This PR changes only configuration files, assets, and dependencies—no source code changes. The type prefix should probably be Suggested title: |
93c9ab5 to
9cf1bd1
Compare
Implements the approved design (docs/design/auto-remove-launcher-hashes-design.md, PR #3488).
Usage-based expiry for
allowed_launcher_image_hashes, replacing unanimous-vote-only removal for routine cleanup.Model. Each
AllowedLauncherImagestores anexpires_at: Timestamp, stampednow + TTLat write time (vote-in, re-vote, and on a successful attestation). An entry is expired whenexpires_at < now. Because expiry is fixed at write time — mirroring how attestations store their own expiry — reads are a plain comparison and the TTL only touches the write sites; it isn't threaded through the ~20 read/verify paths.expires_at = now + TTL, but only for a current participant (enforced by requiring anAuthenticatedParticipantIdcapability token), so a prospective/non-participant node cannot keep a launcher alive. Applies to both attestation paths: the mock path (MockAttestation::WithConstraintsmay reference a launcher) and the Dstack path (refresh runs in the asyncresolve_verificationcallback; the signer is preserved across the verifier promise). No node-side changes.verify_teespawns a detached#[private] clean_expired_launcher_hashesself-call (guarded byhas_expired(), so it's skipped when nothing is expired; gas viaclean_expired_launcher_hashes_tera_gas); never removes the last entry; safe to fail since read-time filtering already enforces expiry.vote_add_launcher_hashon an already-present hash restamps itsexpires_at(threshold vote), recovering a never-adopted hash.launcher_hash_unused_ttl_seconds(default 14d), validated>= DEFAULT_EXPIRATION_DURATION_SECONDS(the attestation validity window). Validation is folded into the DTO→ConfigTryFrom, soinit/init_running/update_configcan't skip it.Migration
Shadows the live 3.13.0 baseline in
v3_13_0_state.rs:OldConfigdeserializes the old config and defaults the new fields;OldTeeStatedeserializes launcher entries without a timestamp and stampsexpires_at = migration_time + TTL. OnlyConfigandallowed_launcher_imageschanged borsh layout; every other field reuses the real (byte-identical) type. Combined withmain's #3785, the same migration also stamps an expiry on legacyMockAttestation::Validentries so they become cleanable — both steps run together and are covered by tests (including a combined round-trip).Tests
Expiry filtering, newest fallback,
cleanup_expired, re-vote refresh; expired-launcher rejection end-to-end; refresh gating (participant vs. non-participant, both mock and dstack arms); migration round-trips (launcher, legacy-mock, and combined).Follow-ups
AllowedLauncherImagesentries by hash instead of aVecInitConfigboilerplate (relax / derive fromConfig)_tera_gas→_tgasrenameCloses #3381