-
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
Merged
Merged
Changes from 29 commits
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
3790000
fix(contract): charge attestation storage by actual delta
pbeza c0886f0
fix(contract): make attestation deposit const pub, address review
pbeza d35db9b
Merge remote-tracking branch 'origin/main' into 3857-attestation-stor…
pbeza ba694a9
fix(contract): regenerate ABI snapshot, use fail-loud balance math in…
pbeza 6b4cbeb
test(contract): parametrize worst-case storage test, tighten attestat…
pbeza c471323
Merge remote-tracking branch 'origin/main' into 3857-attestation-stor…
pbeza c67cea5
fix(contract): charge attestation storage only for new entries or non…
pbeza e7128f2
Merge remote-tracking branch 'origin/main' into 3857-attestation-stor…
pbeza b1ab8eb
fix(contract): charge attestation storage by measured delta; fund e2e…
pbeza c8d62f9
Merge remote-tracking branch 'origin/main' into 3857-attestation-stor…
pbeza 3efa6e4
fix(contract): fund attestation storage from the contract balance
pbeza 8af54e5
Merge remote-tracking branch 'origin/main' into 3857-attestation-stor…
pbeza 7b7444c
refactor(contract): drop now-vestigial attestation-store flush
pbeza e693f1c
test(contract): drop dead deposit context, fix stale refund naming
pbeza 0ca1cc9
test(contract): restore MpcContractHandle sandbox helper, add given/w…
pbeza 29e7520
docs: correct attestation-verifier deposit note to contract-funded st…
pbeza c88ff3e
Merge remote-tracking branch 'origin/main' into 3857-attestation-stor…
pbeza afacfef
refactor(node): drop deposit threading from the tx path
pbeza 296539d
refactor(contract): reuse storage-cost idiom for attestation entry cap
pbeza 5a164a8
test(contract): drop tautological entry-cost test, inline single-use …
pbeza e845e90
refactor(contract): measure real storage delta for the attestation en…
pbeza 158e8e4
revert(contract): drop the runtime attestation entry-size cap
barakeinav1 74ea6a0
test(contract): pin the attestation entry size, tighten the cost ceiling
barakeinav1 a80955c
Merge remote-tracking branch 'origin/main' into 3857-attestation-stor…
barakeinav1 1428b4d
docs: mark the superseded yield-resume sections with TODO(#3825)
barakeinav1 d12a0c8
docs: submit_participant_info is node-submission only
barakeinav1 aae164f
docs: drop the stale deposit guidance, widen the superseded-design ba…
barakeinav1 d8c3579
test(contract): name the entry-size tests after their subject
barakeinav1 26c343b
test(contract): pin that submit_participant_info rejects an attached …
barakeinav1 bf3fe13
docs: use the required TODO(#NNNN): format in the status banner
barakeinav1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,12 +117,6 @@ 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); | ||
|
|
||
| /// Entries to scan in the post-reshare `clean_invalid_attestations` sweep. External | ||
| /// callers may pick a different value; this only governs the automatic invocation. | ||
| const RESHARE_CLEAN_INVALID_ATTESTATIONS_MAX_SCAN: u32 = 100; | ||
|
|
@@ -784,9 +778,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] | ||
| pub fn submit_participant_info( | ||
| &mut self, | ||
|
|
@@ -822,15 +815,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 +855,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, | ||
|
|
@@ -2320,11 +2303,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, | ||
|
|
@@ -2356,9 +2338,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()) | ||
|
|
@@ -2372,9 +2353,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, | ||
|
|
@@ -2393,7 +2372,6 @@ impl MpcContract { | |
| log!("post-DCAP check failed for {account_id}: {err}"); | ||
| return Err(err.into()); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
|
|
@@ -4201,7 +4179,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); | ||
|
|
||
|
|
@@ -4630,7 +4607,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); | ||
|
|
||
|
|
@@ -4645,7 +4621,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); | ||
|
|
@@ -4727,7 +4702,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); | ||
|
|
||
|
|
@@ -4789,7 +4763,6 @@ mod tests { | |
| VMContextBuilder::new() | ||
| .signer_account_id(outsider_id.clone()) | ||
| .predecessor_account_id(outsider_id.clone()) | ||
| .attached_deposit(NearToken::from_near(1)) | ||
| .build() | ||
| ); | ||
|
|
||
|
|
@@ -8152,43 +8125,104 @@ 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]; | ||
|
|
||
| /// Charged storage of the largest attestation entry the contract can store, in bytes: | ||
| /// a 64-byte account id (NEAR's cap) plus fixed-width keys and the largest | ||
| /// [`VerifiedAttestation`] variant, including the `IterableMap` record overhead. | ||
| const WORST_CASE_ENTRY_BYTES: u64 = 604; | ||
|
|
||
| /// Ceiling on one entry's storage cost at today's price, with headroom over | ||
| /// [`WORST_CASE_ENTRY_BYTES`] for storage-price changes. | ||
| const WORST_CASE_ENTRY_COST_CEILING: NearToken = NearToken::from_millinear(10); | ||
|
|
||
| fn worst_case_dstack_attestation() -> VerifiedAttestation { | ||
| VerifiedAttestation::Dstack(ValidatedDstackAttestation { | ||
| mpc_image_hash: MAX_HASH.into(), | ||
| launcher_compose_hash: MAX_HASH.into(), | ||
| expiry_timestamp_seconds: u64::MAX, | ||
| measurements: default_measurements()[0], | ||
| }) | ||
| } | ||
|
|
||
| fn worst_case_mock_attestation() -> VerifiedAttestation { | ||
| 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]), | ||
| }) | ||
| } | ||
|
|
||
| /// Storage the contract pays for one stored attestation entry, measured the way the runtime | ||
| /// charges it. NEAR caps an account id at 64 bytes; every other `NodeId` field is fixed-size, | ||
| /// so this is the worst case for the given attestation variant. | ||
| fn measure_stored_entry_bytes(verified_attestation: VerifiedAttestation) -> u64 { | ||
| testing_env!(VMContextBuilder::new().build()); | ||
| 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)); | ||
|
|
||
| // Then: the flat fee covers the worst-case cost with headroom to spare. | ||
| env::storage_usage() - before | ||
| } | ||
|
|
||
| /// Pins the exact stored size of the largest variant of each attestation kind. | ||
| /// | ||
| /// Do not update these numbers just to make a failing case pass. The contract funds every | ||
| /// entry from its own balance, so a size change alters what the contract pays per node, and | ||
| /// everything derived from it must be revisited in the same change — today | ||
| /// [`WORST_CASE_ENTRY_BYTES`] and [`WORST_CASE_ENTRY_COST_CEILING`]. | ||
| /// | ||
| /// TODO(#3972): the flat onboarding deposit will be derived from these sizes too. | ||
|
Comment on lines
+8181
to
+8188
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: this seems a bit LLM verbose, but I don't mind |
||
| #[rstest] | ||
| #[case::dstack(599, worst_case_dstack_attestation())] | ||
| #[case::mock(604, worst_case_mock_attestation())] | ||
| fn stored_attestation_entry__should_have_the_pinned_size( | ||
| #[case] expected_bytes: u64, | ||
| #[case] verified_attestation: VerifiedAttestation, | ||
| ) { | ||
| // Given / When | ||
| let bytes_stored = measure_stored_entry_bytes(verified_attestation); | ||
|
|
||
| // Then | ||
| assert_eq!( | ||
| bytes_stored, expected_bytes, | ||
| "stored entry size changed; see this test's doc comment before updating the number" | ||
| ); | ||
| assert!( | ||
| bytes_stored <= WORST_CASE_ENTRY_BYTES, | ||
| "entry ({bytes_stored} bytes) exceeds the pinned worst case \ | ||
| ({WORST_CASE_ENTRY_BYTES} bytes)" | ||
| ); | ||
| } | ||
|
|
||
| #[rstest] | ||
| #[case::dstack(worst_case_dstack_attestation())] | ||
| #[case::mock(worst_case_mock_attestation())] | ||
| fn stored_attestation_entry__should_stay_under_the_cost_ceiling( | ||
| #[case] verified_attestation: VerifiedAttestation, | ||
| ) { | ||
| // Given / When | ||
| let bytes_grown = measure_stored_entry_bytes(verified_attestation); | ||
| let cost = env::storage_byte_cost().saturating_mul(u128::from(bytes_grown)); | ||
|
|
||
| // Then | ||
| 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 <= WORST_CASE_ENTRY_COST_CEILING, | ||
| "worst-case entry cost ({bytes_grown} bytes, {cost}) must stay under \ | ||
| {WORST_CASE_ENTRY_COST_CEILING} at today's storage price" | ||
| ); | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
It’s effectively the same as the pre-#3714 version of the contract. IIUC, v3.13.0 charged the measured delta, but
add_participantnever flushed the storage, so the delta was always read as0and newMockentries were effectively free. The drain existed there too.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.
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.
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.
MockAttestation::ValidreturnsOk(())unconditionally fromverify_constraints,submit_participant_infohas no participant gate (justassert_caller_is_signer, which only rules out cross-contract calls), and entries are keyed bytls_public_keywith no per-account limit — so a single ordinary account can mint unbounded entries with no attestation hardware, for gas only.They're also permanent:
clean_invalid_attestationsremoves only entries that fail re-verification, andMock::Validre-verifies as valid forever with no expiry field to age out. So the "bounded and self-healing, reclaimed byclean_invalid_attestations" argument holds forDstack(which does expire and does get swept) but not for the cheap path.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
Attestation::Mockbehind a feature flag would remove the cheap variant independently of the deposit work.