-
Notifications
You must be signed in to change notification settings - Fork 37
feat(contract): auto-expire unused launcher image hashes #3564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 22 commits
99f6904
d7c0c65
a8694aa
cb955fb
074a701
c435dc5
9759f11
d329413
13fbc1f
6e3039e
096dfb1
d725003
e80673a
07de77b
2aa024e
c95e32d
e1d8955
04254ac
37c0218
eb593e5
df57fd8
4485904
3fc93c7
883dfad
797407a
6ffd125
1748925
02a05c3
503a627
9cf1bd1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,10 @@ const DEFAULT_VERIFIER_TERA_GAS: u64 = 200; | |
| /// Prepaid gas for the `resolve_verification` callback. Carries the bulk of the | ||
| /// post-DCAP work (allowlist match, RTMR3 replay, app-compose validation, store). | ||
| const DEFAULT_RESOLVE_VERIFICATION_TERA_GAS: u64 = 60; | ||
| /// Default TTL after which a launcher image hash unused by any participant is evicted. | ||
| const DEFAULT_LAUNCHER_HASH_UNUSED_TTL_SECONDS: u64 = 14 * 24 * 60 * 60; // 14 days | ||
| /// Prepaid gas for a `clean_expired_launcher_hashes` call. | ||
| const DEFAULT_CLEAN_EXPIRED_LAUNCHER_HASHES_TERA_GAS: u64 = 5; | ||
|
|
||
| /// Config for V2 of the contract. | ||
| #[near(serializers=[borsh, json])] | ||
|
|
@@ -81,6 +85,10 @@ pub(crate) struct Config { | |
| pub(crate) verifier_tera_gas: u64, | ||
| /// Prepaid gas for the `resolve_verification` callback. | ||
| pub(crate) resolve_verification_tera_gas: u64, | ||
| /// TTL after which a launcher image hash unused by any participant is evicted. | ||
| pub(crate) launcher_hash_unused_ttl_seconds: u64, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see no test covering enlarging the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Good point — added |
||
| /// Prepaid gas for a `clean_expired_launcher_hashes` call. | ||
| pub(crate) clean_expired_launcher_hashes_tera_gas: u64, | ||
| } | ||
|
|
||
| impl Default for Config { | ||
|
|
@@ -110,6 +118,23 @@ impl Default for Config { | |
| DEFAULT_REMOVE_NON_PARTICIPANT_TEE_VERIFIER_VOTES_TERA_GAS, | ||
| verifier_tera_gas: DEFAULT_VERIFIER_TERA_GAS, | ||
| resolve_verification_tera_gas: DEFAULT_RESOLVE_VERIFICATION_TERA_GAS, | ||
| launcher_hash_unused_ttl_seconds: DEFAULT_LAUNCHER_HASH_UNUSED_TTL_SECONDS, | ||
| clean_expired_launcher_hashes_tera_gas: DEFAULT_CLEAN_EXPIRED_LAUNCHER_HASHES_TERA_GAS, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl Config { | ||
| /// Invariant: a launcher hash backing a still-valid attestation must never expire, | ||
| /// so its unused-TTL must be at least the attestation validity window. | ||
|
SimonRastikian marked this conversation as resolved.
|
||
| pub(crate) fn validate(&self) -> Result<(), &'static str> { | ||
| if self.launcher_hash_unused_ttl_seconds | ||
| < mpc_attestation::attestation::DEFAULT_EXPIRATION_DURATION_SECONDS | ||
| { | ||
| return Err( | ||
| "launcher_hash_unused_ttl_seconds must be >= DEFAULT_EXPIRATION_DURATION_SECONDS", | ||
| ); | ||
| } | ||
| Ok(()) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -826,11 +826,31 @@ impl MpcContract { | |
| Attestation::Mock(mock) => { | ||
| let tee_upgrade_deadline_duration = | ||
| Duration::from_secs(self.config.tee_upgrade_deadline_duration_seconds); | ||
| 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(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: better do define vars right before they are used, unless the compiler restricts it ofc |
||
|
|
||
| self.tee_state.verify_and_store_mock( | ||
| node_id, | ||
| mock, | ||
| tee_upgrade_deadline_duration, | ||
| launcher_unused_ttl, | ||
| )?; | ||
|
|
||
| // A `WithConstraints` mock may reference a launcher hash; refresh-on-use keeps | ||
| // it alive, matching the dstack path. No-op for mocks without a launcher. | ||
| if let Some(participant) = &authenticated_participant { | ||
| self.tee_state | ||
| .refresh_launcher_usage(&tls_public_key_for_refresh, participant); | ||
| } | ||
|
|
||
| Ok(PromiseOrValue::Value(())) | ||
| } | ||
| Attestation::Dstack(attestation) => Ok(PromiseOrValue::Promise( | ||
|
|
@@ -921,6 +941,7 @@ impl MpcContract { | |
| let validation_result = self.tee_state.reverify_and_cleanup_participants( | ||
| proposal.participants(), | ||
| tee_upgrade_deadline_duration, | ||
| Duration::from_secs(self.config.launcher_hash_unused_ttl_seconds), | ||
| ); | ||
|
|
||
| let proposed_participants = proposal.participants(); | ||
|
|
@@ -1472,10 +1493,10 @@ impl MpcContract { | |
| Duration::from_secs(self.config.tee_upgrade_deadline_duration_seconds); | ||
|
|
||
| if votes >= self.threshold()?.value() { | ||
| let added = self | ||
| let outcome = self | ||
| .tee_state | ||
| .add_launcher_image(launcher_hash, tee_upgrade_deadline_duration); | ||
| log!("launcher hash add result: {}", added); | ||
| log!("launcher hash {:?}: {:?}", outcome, launcher_hash); | ||
| } | ||
|
|
||
| Ok(()) | ||
|
|
@@ -1716,12 +1737,24 @@ impl MpcContract { | |
| }; | ||
| let current_params = running_state.parameters.clone(); | ||
|
|
||
| // Physical eviction runs in a detached self-call so it can never fail this | ||
| // transaction. | ||
| Promise::new(env::current_account_id()) | ||
| .function_call( | ||
| method_names::CLEAN_EXPIRED_LAUNCHER_HASHES.to_string(), | ||
| vec![], | ||
| NearToken::from_yoctonear(0), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it from_yoctonear(0) because it's a function call?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's just a zero attached deposit for the detached self-call — |
||
| Gas::from_tgas(self.config.clean_expired_launcher_hashes_tera_gas), | ||
| ) | ||
| .detach(); | ||
|
|
||
| let tee_upgrade_deadline_duration = | ||
| Duration::from_secs(self.config.tee_upgrade_deadline_duration_seconds); | ||
|
|
||
| match self.tee_state.reverify_and_cleanup_participants( | ||
| current_params.participants(), | ||
| tee_upgrade_deadline_duration, | ||
| Duration::from_secs(self.config.launcher_hash_unused_ttl_seconds), | ||
| ) { | ||
| TeeValidationResult::Full => { | ||
| self.accept_requests = true; | ||
|
|
@@ -1835,6 +1868,21 @@ impl MpcContract { | |
| Ok(()) | ||
| } | ||
|
|
||
| /// Private endpoint to evict launcher image hashes unused past their TTL. | ||
| /// Spawned as a detached promise from `verify_tee`; safe to fail (read-time | ||
| /// filtering already enforces expiry). | ||
| #[private] | ||
| #[handle_result] | ||
| pub fn clean_expired_launcher_hashes(&mut self) -> Result<(), Error> { | ||
| log!( | ||
| "clean_expired_launcher_hashes: signer={}", | ||
| env::signer_account_id() | ||
| ); | ||
| let ttl = Duration::from_secs(self.config.launcher_hash_unused_ttl_seconds); | ||
| self.tee_state.clean_expired_launcher_images(ttl); | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Prunes up to `max_scan` stored attestations that fail re-verification (expired or | ||
| /// referencing stale whitelists). Returns the number of entries removed. Callable by | ||
| /// anyone while the protocol is in `Running`. | ||
|
|
@@ -1852,9 +1900,11 @@ impl MpcContract { | |
| } | ||
| let tee_upgrade_deadline_duration = | ||
| Duration::from_secs(self.config.tee_upgrade_deadline_duration_seconds); | ||
| 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, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't expect this change, will need to look deeper on why it is needed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It needed the launcher TTL because |
||
| )) | ||
| } | ||
|
|
||
| /// Private endpoint to clean up foreign chain policy votes and node configurations | ||
|
|
@@ -1961,6 +2011,13 @@ impl MpcContract { | |
| let initial_participants = parameters.participants(); | ||
| let tee_state = TeeState::with_mocked_participant_attestations(initial_participants); | ||
|
|
||
| let config: Config = init_config.map(Into::into).unwrap_or_default(); | ||
| config | ||
| .validate() | ||
| .map_err(|reason| InvalidParameters::MalformedPayload { | ||
| reason: reason.to_string(), | ||
| })?; | ||
|
|
||
| Ok(Self { | ||
| protocol_state: ProtocolContractState::Running(RunningContractState::new( | ||
| DomainRegistry::default(), | ||
|
|
@@ -1974,7 +2031,7 @@ impl MpcContract { | |
| StorageKey::PendingVerifyForeignTxRequestsV2, | ||
| ), | ||
| proposed_updates: ProposedUpdates::default(), | ||
| config: init_config.map(Into::into).unwrap_or_default(), | ||
| config, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIUC this is a bit of a behavior change. Apparently before we were taking the default in case of conversion problems, now we are returning an error. Not saying this is bad, but we should be careful with such changes |
||
| tee_state, | ||
| accept_requests: true, | ||
| node_migrations: NodeMigrations::default(), | ||
|
|
@@ -2040,8 +2097,15 @@ impl MpcContract { | |
| let initial_participants = parameters.participants(); | ||
| let tee_state = TeeState::with_mocked_participant_attestations(initial_participants); | ||
|
|
||
| let config: Config = init_config.map(Into::into).unwrap_or_default(); | ||
| config | ||
| .validate() | ||
| .map_err(|reason| InvalidParameters::MalformedPayload { | ||
| reason: reason.to_string(), | ||
| })?; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should make the validation part of the into conversion to make this more ergonomic, and avoid someone forgetting to call validate
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done — |
||
|
|
||
| Ok(MpcContract { | ||
| config: init_config.map(Into::into).unwrap_or_default(), | ||
| config, | ||
| protocol_state: ProtocolContractState::Running(RunningContractState::new( | ||
| domains, | ||
| keyset, | ||
|
|
@@ -2118,11 +2182,17 @@ impl MpcContract { | |
| } | ||
|
|
||
| pub fn allowed_launcher_compose_hashes(&self) -> Vec<LauncherDockerComposeHash> { | ||
| self.tee_state.get_allowed_launcher_compose_hashes() | ||
| self.tee_state | ||
| .get_allowed_launcher_compose_hashes(Duration::from_secs( | ||
| self.config.launcher_hash_unused_ttl_seconds, | ||
| )) | ||
| } | ||
|
|
||
| pub fn allowed_launcher_image_hashes(&self) -> Vec<LauncherImageHash> { | ||
| self.tee_state.get_allowed_launcher_hashes() | ||
| self.tee_state | ||
| .get_allowed_launcher_hashes(Duration::from_secs( | ||
| self.config.launcher_hash_unused_ttl_seconds, | ||
| )) | ||
| } | ||
|
|
||
| /// Returns the current launcher hash votes, showing each participant's vote. | ||
|
|
@@ -2350,17 +2420,36 @@ impl MpcContract { | |
| let account_id = &context.node_id.account_id; | ||
| let tee_upgrade_deadline_duration = | ||
| Duration::from_secs(self.config.tee_upgrade_deadline_duration_seconds); | ||
| 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. | ||
| // The signer is preserved across the verifier promise, so this reflects the | ||
| // original submitter. | ||
| let authenticated_participant = self | ||
| .protocol_state | ||
| .threshold_parameters() | ||
| .ok() | ||
| .and_then(|params| AuthenticatedParticipantId::new(params.participants()).ok()); | ||
| let tls_public_key_for_refresh = context.node_id.tls_public_key.clone(); | ||
|
|
||
| if let Err(err) = self.tee_state.verify_and_store_dstack( | ||
| context.node_id.clone(), | ||
| &context.attestation, | ||
| report, | ||
| tee_upgrade_deadline_duration, | ||
| launcher_unused_ttl, | ||
| ) { | ||
| log!("post-DCAP check failed for {account_id}: {err}"); | ||
| return Err(err.into()); | ||
| } | ||
|
|
||
| // Refresh-on-use: a current participant's successful submission keeps the launcher | ||
| // hash its attestation references from expiring. | ||
| if let Some(participant) = &authenticated_participant { | ||
| self.tee_state | ||
| .refresh_launcher_usage(&tls_public_key_for_refresh, participant); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pbeza heads-up on the The launcher check + refresh-on-use now live here in the |
||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
|
|
@@ -2444,7 +2533,11 @@ impl MpcContract { | |
|
|
||
| #[private] | ||
| 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; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — fixed in d7c0c65. |
||
| } | ||
|
|
||
| /// Get our own account id as a voter. Returns an error if we are not a participant. | ||
|
|
@@ -2719,6 +2812,7 @@ impl MpcContract { | |
| self.tee_state.reverify_participants( | ||
| &node_id, | ||
| Duration::from_secs(self.config.tee_upgrade_deadline_duration_seconds), | ||
| Duration::from_secs(self.config.launcher_hash_unused_ttl_seconds), | ||
| ), | ||
| TeeQuoteStatus::Valid | ||
| )) { | ||
|
|
@@ -4002,6 +4096,33 @@ mod tests { | |
| (contract, participants, first_participant_id) | ||
| } | ||
|
|
||
| #[test] | ||
| fn init_rejects_launcher_ttl_below_attestation_validity() { | ||
| let participants = gen_participants(3); | ||
| let signer = participants.participants()[0].0.clone(); | ||
| testing_env!( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is PR wide, we are encouraging the use of "given, when, then" in our engineering standards, would be good to adopt here (and relatively cheap for LLMs)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done — added
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, although apparently in this test where the comment lives nothing changed 🤔 |
||
| VMContextBuilder::new() | ||
| .signer_account_id(signer.clone()) | ||
| .predecessor_account_id(signer) | ||
| .attached_deposit(NearToken::from_near(1)) | ||
| .build() | ||
| ); | ||
| let parameters = ThresholdParameters::new(participants, Threshold::new(2)).unwrap(); | ||
| let bad_config = dtos::InitConfig { | ||
| launcher_hash_unused_ttl_seconds: Some( | ||
| mpc_attestation::attestation::DEFAULT_EXPIRATION_DURATION_SECONDS - 1, | ||
| ), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let err = MpcContract::init((¶meters).into_dto_type(), Some(bad_config)) | ||
| .expect_err("init must reject a launcher TTL below the attestation validity window"); | ||
| assert!( | ||
| format!("{err:?}").contains("launcher_hash_unused_ttl_seconds"), | ||
| "error should point at the invalid config field, got: {err:?}" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| #[expect(non_snake_case)] | ||
| fn vote_tee_verifier_change__should_apply_candidate_when_threshold_reached() { | ||
|
|
@@ -5250,6 +5371,7 @@ mod tests { | |
| }, | ||
| valid_participant_attestation, | ||
| tee_upgrade_duration, | ||
| Duration::MAX, | ||
| ); | ||
| assert_matches::assert_matches!( | ||
| insertion_result, | ||
|
|
@@ -5899,7 +6021,12 @@ mod tests { | |
| }; | ||
| contract | ||
| .tee_state | ||
| .verify_and_store_mock(node_id, expiring_attestation, TEE_UPGRADE_DURATION) | ||
| .verify_and_store_mock( | ||
| node_id, | ||
| expiring_attestation, | ||
| TEE_UPGRADE_DURATION, | ||
| Duration::MAX, | ||
| ) | ||
| .expect("mock attestation is not yet expired and valid"); | ||
|
|
||
| // Capture the running state before verify_tee for comparison | ||
|
|
@@ -6014,7 +6141,12 @@ mod tests { | |
| }; | ||
| contract | ||
| .tee_state | ||
| .verify_and_store_mock(node_id, expiring_attestation, TEE_UPGRADE_DURATION) | ||
| .verify_and_store_mock( | ||
| node_id, | ||
| expiring_attestation, | ||
| TEE_UPGRADE_DURATION, | ||
| Duration::MAX, | ||
| ) | ||
| .expect("mock attestation is not yet expired and valid"); | ||
|
|
||
| let (first_account_id, _, _) = &participant_list[0]; | ||
|
|
@@ -7461,6 +7593,7 @@ mod tests { | |
| }, | ||
| MpcMockAttestation::Valid, | ||
| Duration::from_secs(contract.config.tee_upgrade_deadline_duration_seconds), | ||
| Duration::MAX, | ||
| ) | ||
| .expect("attestation insertion should succeed"); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why
5TGas: this is the budget for the detached,#[private]clean_expired_launcher_hashesself-call spawned fromverify_tee(physical eviction of launcher hashes past their TTL).Measured via a sandbox (near-workspaces) benchmark: the call burns ~3.0 TGas (function-call receipt) / ~3.35 TGas total on a small allowlist. The cost is dominated by contract state load/store + the base function-call charge, not the launcher count (the eviction loop over a handful of operator-curated entries is negligible), so it stays flat as the allowlist changes. That leaves ~40% headroom under 5 TGas.
It's also consistent with the sibling detached-cleanup budgets that do comparable small work (
clean_foreign_chain_data,remove_non_participant_*are all 5), and it's fail-safe: the call is detached, so a shortfall can't failverify_tee, and expiry is already enforced at read time — physical eviction is just GC that retries next round. Tunable viaConfigif a benchmark on production-sized state ever warrants it.