-
Notifications
You must be signed in to change notification settings - Fork 37
fix(contract): drop the deposit for submit_participant_info
#3940
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
Changes from 21 commits
3790000
c0886f0
d35db9b
ba694a9
6b4cbeb
c471323
c67cea5
e7128f2
b1ab8eb
c8d62f9
3efa6e4
8af54e5
7b7444c
e693f1c
0ca1cc9
29e7520
c88ff3e
afacfef
296539d
5a164a8
e845e90
158e8e4
74ea6a0
a80955c
1428b4d
d12a0c8
aae164f
d8c3579
26c343b
bf3fe13
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 |
|---|---|---|
|
|
@@ -117,11 +117,9 @@ const MINIMUM_CKD_REQUEST_DEPOSIT: NearToken = NearToken::from_yoctonear(1); | |
| /// node key cannot invoke these methods. | ||
| pub const MINIMUM_NODE_MANAGEMENT_DEPOSIT: NearToken = NearToken::from_yoctonear(1); | ||
|
|
||
| /// Flat fee a node attaches to [`MpcContract::submit_participant_info`] for its | ||
| /// stored attestation entry. The entry is bounded, so the fee is fixed and | ||
| /// nothing is refunded; its margin over the true cost absorbs storage-price and | ||
| /// layout changes. A unit test asserts it covers the worst-case entry. | ||
| const MINIMUM_ATTESTATION_STORAGE_DEPOSIT: NearToken = NearToken::from_millinear(100); | ||
| /// Caps the contract-funded storage cost of a single stored attestation entry, guarding against a | ||
| /// future schema/attestation-size change bloating an entry. Does not bound total attestation storage. | ||
| pub(crate) const MAX_ATTESTATION_ENTRY_STORAGE_COST: NearToken = NearToken::from_millinear(100); | ||
|
|
||
| /// Entries to scan in the post-reshare `clean_invalid_attestations` sweep. External | ||
| /// callers may pick a different value; this only governs the automatic invocation. | ||
|
|
@@ -784,9 +782,8 @@ impl MpcContract { | |
| /// `verify_quote` call, with [`Self::resolve_verification`] chained as its | ||
| /// callback to run the post-DCAP checks and store the attestation. | ||
| /// | ||
| /// The caller must attach a flat 0.1 NEAR fee for the stored entry; the whole | ||
| /// fee is kept on success and refunded if the attestation is not accepted. | ||
| #[payable] | ||
| /// Storage is funded by the contract's own balance, so a node submits with no deposit via its | ||
| /// function-call access key, for its first attestation and re-attestations alike. | ||
| #[handle_result] | ||
|
Comment on lines
+781
to
783
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 effectively the same as the pre-#3714 version of the contract. IIUC, v3.13.0 charged the measured delta, but I’m not quite sure what a better funding model for the new attestations would be, given that the nodes’ function-call access keys can’t pay for storage.
Collaborator
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. Agreed this isn't a regression and shouldn't block. One correction to the framing though, since #3972 is being written against it: this needs no TEE at all.
They're also permanent: So the Slack framing of "an arbitrary entity with a single TEE can slowly drain" is understating it. Worth #3972 pricing out the mock path explicitly — and gating |
||
| pub fn submit_participant_info( | ||
| &mut self, | ||
|
|
@@ -822,15 +819,6 @@ impl MpcContract { | |
| account_public_key, | ||
| }; | ||
|
|
||
| let attached = env::attached_deposit(); | ||
| if attached < MINIMUM_ATTESTATION_STORAGE_DEPOSIT { | ||
| return Err(InvalidParameters::InsufficientDeposit { | ||
| attached: attached.as_yoctonear(), | ||
| required: MINIMUM_ATTESTATION_STORAGE_DEPOSIT.as_yoctonear(), | ||
| } | ||
| .into()); | ||
| } | ||
|
|
||
| match proposed_participant_attestation { | ||
| Attestation::Mock(mock) => { | ||
| let tee_upgrade_deadline_duration = | ||
|
|
@@ -871,7 +859,6 @@ impl MpcContract { | |
| .then( | ||
| Self::ext(env::current_account_id()) | ||
| .with_static_gas(Gas::from_tgas(self.config.resolve_verification_tera_gas)) | ||
| .with_attached_deposit(env::attached_deposit()) | ||
| .resolve_verification(VerificationContext { | ||
| node_id, | ||
| attestation, | ||
|
|
@@ -2311,11 +2298,10 @@ impl MpcContract { | |
| } | ||
| } | ||
|
|
||
| /// Verify-quote callback: on a verifier verdict it runs the post-DCAP | ||
| /// checks and stores the attestation, refunding the flat fee if the | ||
| /// attestation is not accepted. | ||
| /// Verify-quote callback: on a verifier verdict it runs the post-DCAP checks and stores the | ||
| /// attestation (storage funded by the contract's balance). On any failure it fails the | ||
| /// submitter's transaction. | ||
| #[private] | ||
| #[payable] | ||
| pub fn resolve_verification( | ||
| &mut self, | ||
| #[serializer(borsh)] context: VerificationContext, | ||
|
|
@@ -2347,9 +2333,8 @@ impl MpcContract { | |
| match attestation_result { | ||
| Ok(()) => PromiseOrValue::Value(()), | ||
| Err(err) => { | ||
| refund_to(&account_id, env::attached_deposit()); | ||
| // Fail the submitter's transaction from a separate receipt so | ||
| // the refund above commits (a panic here would roll it back) | ||
| // Fail the submitter's transaction from a separate receipt so any prior state | ||
| // commits (a panic here would roll it back) | ||
| let promise = Promise::new(env::current_account_id()).function_call( | ||
| method_names::FAIL_ATTESTATION_SUBMISSION.to_string(), | ||
| borsh::to_vec(&err.to_string()) | ||
|
|
@@ -2363,9 +2348,7 @@ impl MpcContract { | |
| } | ||
|
|
||
| /// Runs the post-DCAP checks and stores the attestation for a | ||
| /// [`VerificationResult::Verified`] response. The deposit was already | ||
| /// checked against the flat fee in [`Self::submit_participant_info`], so this | ||
| /// only verifies and stores. | ||
| /// [`VerificationResult::Verified`] response. Storage is funded by the contract's balance. | ||
| fn verify_post_dcap_and_store( | ||
| &mut self, | ||
| context: &VerificationContext, | ||
|
|
@@ -2384,7 +2367,6 @@ impl MpcContract { | |
| log!("post-DCAP check failed for {account_id}: {err}"); | ||
| return Err(err.into()); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
|
|
@@ -4191,7 +4173,6 @@ mod tests { | |
| let participant_context = VMContextBuilder::new() | ||
| .signer_account_id(account_id.clone()) | ||
| .predecessor_account_id(account_id.clone()) | ||
| .attached_deposit(NearToken::from_near(1)) | ||
| .build(); | ||
| testing_env!(participant_context); | ||
|
|
||
|
|
@@ -4620,7 +4601,6 @@ mod tests { | |
| let ctx = VMContextBuilder::new() | ||
| .signer_account_id(participant_id.clone()) | ||
| .predecessor_account_id("outsider.near".parse().unwrap()) | ||
| .attached_deposit(NearToken::from_near(1)) | ||
| .build(); | ||
| testing_env!(ctx); | ||
|
|
||
|
|
@@ -4635,7 +4615,6 @@ mod tests { | |
| let context = VMContextBuilder::new() | ||
| .current_account_id(contract_account_id.clone()) | ||
| .predecessor_account_id(contract_account_id) | ||
| .attached_deposit(MINIMUM_ATTESTATION_STORAGE_DEPOSIT) | ||
| .block_timestamp(VALID_ATTESTATION_TIMESTAMP * 1_000_000_000) | ||
| .build(); | ||
| testing_env!(context); | ||
|
|
@@ -4717,7 +4696,6 @@ mod tests { | |
| let ctx = VMContextBuilder::new() | ||
| .signer_account_id(outsider_id.clone()) | ||
| .predecessor_account_id(outsider_id.clone()) | ||
| .attached_deposit(NearToken::from_near(1)) | ||
| .build(); | ||
| testing_env!(ctx); | ||
|
|
||
|
|
@@ -4779,7 +4757,6 @@ mod tests { | |
| VMContextBuilder::new() | ||
| .signer_account_id(outsider_id.clone()) | ||
| .predecessor_account_id(outsider_id.clone()) | ||
| .attached_deposit(NearToken::from_near(1)) | ||
| .build() | ||
| ); | ||
|
|
||
|
|
@@ -8095,43 +8072,48 @@ mod tests { | |
| assert!(configs.contains_key(&tls_key_b), "node B config must exist"); | ||
| } | ||
|
|
||
| // Catches only entry-size growth: fails if a schema change makes the stored entry | ||
| // cost more than the fee at today's storage_byte_cost. It cannot see a future | ||
| // storage_byte_cost increase on a live contract; the fee's margin covers that. | ||
| #[test] | ||
| fn minimum_attestation_storage_deposit__should_cover_worst_case_entry() { | ||
| // Given: the largest entry a submission can store. NEAR caps an account id | ||
| // at 64 bytes; every other field is fixed-size, so this is the worst case. | ||
| const MAX_HASH: [u8; 32] = [0xff; 32]; | ||
|
|
||
| #[rstest] | ||
| #[case::dstack(VerifiedAttestation::Dstack(ValidatedDstackAttestation { | ||
| mpc_image_hash: MAX_HASH.into(), | ||
| launcher_compose_hash: MAX_HASH.into(), | ||
| expiry_timestamp_seconds: u64::MAX, | ||
| measurements: default_measurements()[0], | ||
| }))] | ||
| #[case::mock(VerifiedAttestation::Mock(MpcMockAttestation::WithConstraints { | ||
| mpc_docker_image_hash: Some(MAX_HASH.into()), | ||
| launcher_docker_compose_hash: Some(MAX_HASH.into()), | ||
| expiry_timestamp_seconds: Some(u64::MAX), | ||
| expected_measurements: Some(default_measurements()[0]), | ||
| }))] | ||
| fn submit_participant_info__should_bound_worst_case_entry_cost( | ||
| #[case] verified_attestation: VerifiedAttestation, | ||
| ) { | ||
| testing_env!(VMContextBuilder::new().build()); | ||
| // NEAR caps an account id at 64 bytes; every other NodeId field is fixed-size. | ||
| let node_id = create_node_id( | ||
| &"a".repeat(64).parse().unwrap(), | ||
| &bogus_ed25519_public_key(), | ||
| ); | ||
| let worst_case = NodeAttestation { | ||
| node_id: node_id.clone(), | ||
| verified_attestation: VerifiedAttestation::Dstack(ValidatedDstackAttestation { | ||
| mpc_image_hash: [0xff; 32].into(), | ||
| launcher_compose_hash: [0xff; 32].into(), | ||
| expiry_timestamp_seconds: u64::MAX, | ||
| measurements: default_measurements()[0], | ||
| }), | ||
| }; | ||
|
|
||
| // When: the entry is inserted and flushed, so storage_usage reflects it. | ||
| let mut tee_state = TeeState::default(); | ||
| let storage_before = env::storage_usage(); | ||
| tee_state | ||
| .stored_attestations | ||
| .insert(node_id.tls_public_key.clone(), worst_case); | ||
| let before = env::storage_usage(); | ||
| tee_state.stored_attestations.insert( | ||
| node_id.tls_public_key.clone(), | ||
| NodeAttestation { | ||
| node_id, | ||
| verified_attestation, | ||
| }, | ||
| ); | ||
| tee_state.stored_attestations.flush(); | ||
| let bytes_grown = env::storage_usage() - storage_before; | ||
| let worst_case_cost = env::storage_byte_cost().saturating_mul(u128::from(bytes_grown)); | ||
| let bytes_grown = env::storage_usage() - before; | ||
| let cost = env::storage_byte_cost().saturating_mul(u128::from(bytes_grown)); | ||
|
|
||
| // Then: the flat fee covers the worst-case cost with headroom to spare. | ||
| assert!( | ||
| MINIMUM_ATTESTATION_STORAGE_DEPOSIT >= worst_case_cost, | ||
| "flat fee {MINIMUM_ATTESTATION_STORAGE_DEPOSIT} must cover the worst-case entry \ | ||
| ({bytes_grown} bytes, {worst_case_cost}) at today's storage price" | ||
| cost <= MAX_ATTESTATION_ENTRY_STORAGE_COST, | ||
| "worst-case entry cost ({bytes_grown} bytes, {cost}) must stay under \ | ||
| {MAX_ATTESTATION_ENTRY_STORAGE_COST} at today's storage price" | ||
| ); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,5 @@ | ||||||||||||||||||||
| use crate::{ | ||||||||||||||||||||
| MAX_ATTESTATION_ENTRY_STORAGE_COST, | ||||||||||||||||||||
| primitives::{key_state::AuthenticatedParticipantId, participants::Participants}, | ||||||||||||||||||||
| storage_keys::StorageKey, | ||||||||||||||||||||
| tee::measurements::{ | ||||||||||||||||||||
|
|
@@ -19,7 +20,7 @@ use mpc_attestation::{ | |||||||||||||||||||
| }; | ||||||||||||||||||||
| use mpc_primitives::hash::{LauncherDockerComposeHash, LauncherImageHash}; | ||||||||||||||||||||
| use near_mpc_contract_interface::types::{self as dtos, Ed25519PublicKey}; | ||||||||||||||||||||
| use near_sdk::{env, near, store::IterableMap}; | ||||||||||||||||||||
| use near_sdk::{NearToken, env, near, store::IterableMap}; | ||||||||||||||||||||
| use std::time::Duration; | ||||||||||||||||||||
| use tee_verifier_interface::VerifiedReport; | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -45,6 +46,8 @@ pub enum AttestationSubmissionError { | |||||||||||||||||||
| "TLS public key is already registered to a different account; only the owning account may update it" | ||||||||||||||||||||
| )] | ||||||||||||||||||||
| TlsKeyOwnedByOtherAccount, | ||||||||||||||||||||
| #[error("stored attestation entry costs {cost} which exceeds the maximum {max}")] | ||||||||||||||||||||
| EntryStorageCostExceeded { cost: NearToken, max: NearToken }, | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| #[derive(Debug)] | ||||||||||||||||||||
|
|
@@ -228,13 +231,32 @@ impl TeeState { | |||||||||||||||||||
| return Err(AttestationSubmissionError::TlsKeyOwnedByOtherAccount); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let previous = self.stored_attestations.insert( | ||||||||||||||||||||
| tls_pk, | ||||||||||||||||||||
| NodeAttestation { | ||||||||||||||||||||
| node_id, | ||||||||||||||||||||
| verified_attestation, | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| ); | ||||||||||||||||||||
| let entry = NodeAttestation { | ||||||||||||||||||||
| node_id, | ||||||||||||||||||||
| verified_attestation, | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let before = env::storage_usage(); | ||||||||||||||||||||
| let previous = self.stored_attestations.insert(tls_pk.clone(), entry); | ||||||||||||||||||||
| self.stored_attestations.flush(); | ||||||||||||||||||||
| let cost = | ||||||||||||||||||||
| env::storage_byte_cost().saturating_mul(u128::from(env::storage_usage() - before)); | ||||||||||||||||||||
|
Collaborator
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.
On an update, three of the four size contributors are pinned:
Reachable shrinks:
The contract is always built with That's still a valid A well-behaved mainnet node never hits this — it always submits
Suggested change
A shrink then reads as cost 0, which is the correct answer — the contract's storage stake went down, so there is nothing to cap. |
||||||||||||||||||||
|
|
||||||||||||||||||||
| if cost > MAX_ATTESTATION_ENTRY_STORAGE_COST { | ||||||||||||||||||||
| match previous { | ||||||||||||||||||||
| Some(prev) => { | ||||||||||||||||||||
| self.stored_attestations.insert(tls_pk, prev); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| None => { | ||||||||||||||||||||
| self.stored_attestations.remove(&tls_pk); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| self.stored_attestations.flush(); | ||||||||||||||||||||
| return Err(AttestationSubmissionError::EntryStorageCostExceeded { | ||||||||||||||||||||
| cost, | ||||||||||||||||||||
| max: MAX_ATTESTATION_ENTRY_STORAGE_COST, | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
Collaborator
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. Nothing exercises this branch. It's the only thing stopping the dstack callback from committing an oversized entry — Easiest fix is to make the cap injectable rather than a module const — the same shape as fn store_verified_attestation(
&mut self,
node_id: NodeId,
verified_attestation: VerifiedAttestation,
max_entry_cost: NearToken,
) -> Result<ParticipantInsertion, AttestationSubmissionError>Production passes the const; a unit test passes something tiny and asserts both halves that matter: the Worth doing whichever way the cap-sizing discussion above lands, as long as the runtime check stays. |
||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Ok(match previous { | ||||||||||||||||||||
| Some(_) => ParticipantInsertion::UpdatedExistingParticipant, | ||||||||||||||||||||
|
|
@@ -846,6 +868,30 @@ mod tests { | |||||||||||||||||||
| ); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| #[test] | ||||||||||||||||||||
| fn store_verified_attestation__should_accept_entry_within_cap() { | ||||||||||||||||||||
| // Given | ||||||||||||||||||||
| testing_env!(VMContextBuilder::new().build()); | ||||||||||||||||||||
| let mut tee_state = TeeState::default(); | ||||||||||||||||||||
| let node_id = node_id_for(&"alice.near".parse().unwrap()); | ||||||||||||||||||||
| let verified_attestation = VerifiedAttestation::Mock(MockAttestation::Valid); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // When | ||||||||||||||||||||
| let result = tee_state.store_verified_attestation(node_id.clone(), verified_attestation); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Then | ||||||||||||||||||||
| assert_matches!(result, Ok(ParticipantInsertion::NewlyInsertedParticipant)); | ||||||||||||||||||||
| let stored = tee_state | ||||||||||||||||||||
| .stored_attestations | ||||||||||||||||||||
| .get(&node_id.tls_public_key) | ||||||||||||||||||||
| .expect("attestation must be stored"); | ||||||||||||||||||||
| assert_eq!(stored.node_id, node_id); | ||||||||||||||||||||
| assert_matches!( | ||||||||||||||||||||
| stored.verified_attestation, | ||||||||||||||||||||
| VerifiedAttestation::Mock(MockAttestation::Valid) | ||||||||||||||||||||
| ); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| #[test] | ||||||||||||||||||||
| fn verify_and_store_mock__should_index_by_tls_key() { | ||||||||||||||||||||
| // given | ||||||||||||||||||||
|
|
||||||||||||||||||||
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.
This cap is ~16× looser than the thing it guards, and it bounds growth rather than size.
0.1 NEAR permits a 10,000-byte entry. The real worst case is 604 bytes / 0.006 NEAR — measured by instrumenting
submit_participant_info__should_bound_worst_case_entry_costin this PR. An entry would have to bloat 16× before either the runtime check or that test noticed, so neither really delivers the "guards against a future schema/attestation-size change" claim. The test asserts against this same constant, so it inherited the blind spot from the oldMINIMUM_ATTESTATION_STORAGE_DEPOSIT.Separately, on an update the measured delta is
new − old, so the check constrains the increment, not the entry — which is not what this doc comment says. That matters for #3972, whose AC is "the flat deposit is derived from the per-entry storage cap": a deposit can't be sized off a delta-bound.@gilcu3's point in Slack holds — the type is fixed-size, so a static bound is the natural tool here. Either:
One trap either way: the maximum is a mock entry, not a real one.
Mock::WithConstraints{all Some}is 450 bytes vsDstack's 445. Any tightened bound has to be sized off the mock branch as long asAttestation::Mockis ungated in production.