fix(contract): address review on async attestation flow

- refund the attached deposit when a participant refreshes an existing
  attestation (charge_attestation_storage early-return kept it silently)
- add v3_13_0_state migration shadow so migrate() can upgrade from a
  deployed 3.13.0 layout, not just 3.12.0
- collapse the two per-arm debug_asserts in on_attestation_verified into
  one guarding both resolved paths, with a note on why cleanup stays in
  resolve_verification (its remove-before-store gates the timeout race)
- nits: from_yoctonear(0) on the fail-call; drop a redundant yield
  comment; reframe the promise_yield_resume comment; PendingAttestation
  fields pub(crate); document the verify_quote wire args; benchmark TODOs
  on the new gas defaults
- TODO(#3720): stash only tcb_info in PendingAttestation (follow-up)

Author: pbeza

crates/contract/src/lib.rs

@@ -17,6 +17,7 @@ pub mod update;
 pub mod utils;
 
 pub mod v3_12_0_state;
+pub mod v3_13_0_state;
 
 #[cfg(feature = "bench-contract-methods")]
 mod bench;
@@ -904,9 +905,6 @@ impl MpcContract {
             },
         );
 
-        // The yield is the method's return value: `enqueue_yield_request` called
-        // `promise_return` as the final host call, so returning unit here adds no
-        // `value_return` that would override it.
         Ok(())
     }
 
@@ -920,8 +918,9 @@ impl MpcContract {
     ) -> Result<(), Error> {
         let is_new_attestation =
             matches!(insertion, ParticipantInsertion::NewlyInsertedParticipant);
-        // A participant refreshing an existing attestation is not charged.
+
         if caller_is_participant && !is_new_attestation {
+            refund_attestation_deposit(account_id, attached);
             return Ok(());
         }
 
@@ -2161,6 +2160,14 @@ impl MpcContract {
     pub fn migrate() -> Result<Self, Error> {
         log!("migrating contract");
 
+        match try_state_read::<v3_13_0_state::MpcContract>() {
+            Ok(Some(state)) => return Ok(state.into()),
+            Ok(None) => return Err(InvalidState::ContractStateIsMissing.into()),
+            Err(err) => {
+                log!("failed to deserialize state into 3.13.0 state: {:?}", err);
+            }
+        };
+
         match try_state_read::<v3_12_0_state::MpcContract>() {
             Ok(Some(state)) => return Ok(state.into()),
             Ok(None) => return Err(InvalidState::ContractStateIsMissing.into()),
@@ -2415,7 +2422,7 @@ impl MpcContract {
             refund_attestation_deposit(&account_id, pending.attached_deposit);
         }
         // MUST be the last host call: anything after could panic and roll back
-        // the state mutations above.
+        // the state mutations above
         env::promise_yield_resume(
             &pending.data_id,
             serde_json::to_vec(&attestation_result)
@@ -2482,8 +2489,17 @@ impl MpcContract {
         #[callback_result] result: Result<AttestationResult, PromiseError>,
     ) -> PromiseOrValue<()> {
         let reason = match result {
-            Ok(AttestationResult::Ok) => return PromiseOrValue::Value(()),
-            Ok(AttestationResult::Err(reason)) => reason,
+            Ok(resolved) => {
+                // resolve_verification already removed the entry and refunded on failure. It
+                // removes before storing, so a verifier reply that arrives after the
+                // ~200-block yield-resume timeout (handled by the Err arm below) bails
+                // instead of storing an attestation whose deposit was already refunded.
+                debug_assert!(!self.pending_attestations.contains_key(&account_id));
+                match resolved {
+                    AttestationResult::Ok => return PromiseOrValue::Value(()),
+                    AttestationResult::Err(reason) => reason,
+                }
+            }
             Err(_promise_err) => {
                 // Timeout: the resolution callback never resumed us, so the
                 // pending entry is still here. Clean it up and refund.
@@ -2500,7 +2516,7 @@ impl MpcContract {
         let promise = Promise::new(env::current_account_id()).function_call(
             method_names::FAIL_ATTESTATION_SUBMISSION.to_string(),
             borsh::to_vec(&reason).expect("borsh serialization of reason must succeed"),
-            NearToken::from_near(0),
+            NearToken::from_yoctonear(0),
             Gas::from_tgas(self.config.fail_attestation_submission_tera_gas),
         );
         PromiseOrValue::Promise(promise.as_return())

crates/contract/src/tee/pending_attestation.rs

@@ -13,20 +13,22 @@ use near_sdk::{CryptoHash, NearToken, near};
 /// One in-flight verification per submitter account.
 #[derive(Debug, BorshSerialize, BorshDeserialize)]
 pub struct PendingAttestation {
-    /// The submitted payload the post-DCAP checks consume once the verifier
-    /// returns its report.
-    pub dstack: DstackAttestation,
+    /// The submitted payload. Only `tcb_info` is read after the verifier callback;
+    /// `quote`/`collateral` are consumed once for the pre-callback `verify_quote`
+    /// call, so storing the whole struct holds ~2-8 KiB of dead bytes per in-flight
+    /// entry. TODO(#3720): stash only `tcb_info`.
+    pub(crate) dstack: DstackAttestation,
     /// Checked against the quote's report-data during the post-DCAP checks.
-    pub tls_public_key: Ed25519PublicKey,
+    pub(crate) tls_public_key: Ed25519PublicKey,
     /// Stashed because the deposit is not visible from the callback receipt:
     /// consumed for storage on success, refunded on failure.
-    pub attached_deposit: NearToken,
+    pub(crate) attached_deposit: NearToken,
     /// Participant status at submit time, which decides whether the caller pays
     /// for storage. Captured because the callback receipt is no longer the
     /// caller, so it can no longer be re-derived.
-    pub caller_is_participant: bool,
+    pub(crate) caller_is_participant: bool,
     /// Yield handle, read back by the callback to resume the yield.
-    pub data_id: CryptoHash,
+    pub(crate) data_id: CryptoHash,
 }
 
 #[near(serializers = [json])]

crates/contract/src/v3_13_0_state.rs

@@ -0,0 +1,144 @@
+//! ## Overview
+//! Shadows the contract state written by the `3.13.0` release so [`crate::migrate`]
+//! can upgrade from it. See [`crate::v3_12_0_state`] for the rationale and guideline.
+//!
+//! `3.13.0` differs from the live layout only by the two fields this version adds:
+//! `Config::fail_attestation_submission_tera_gas` (and the three verifier gas knobs,
+//! all defaulted here) and the `MpcContract::pending_attestations` map.
+
+use borsh::{BorshDeserialize, BorshSerialize};
+use near_mpc_contract_interface::types::{Metrics, VerifyForeignTransactionRequest};
+use near_sdk::{
+    AccountId, env,
+    store::{Lazy, LookupMap},
+};
+
+use crate::{
+    SupportedForeignChainsByNode,
+    foreign_chains_metadata::ForeignChainsMetadata,
+    node_migrations::NodeMigrations,
+    primitives::{
+        ckd::CKDRequest,
+        domain::max_reconstruction_threshold,
+        signature::{SignatureRequest, YieldIndex},
+        thresholds::ThresholdParameters,
+    },
+    state::{ProtocolContractState, running::RunningContractState},
+    storage_keys::StorageKey,
+    tee::{tee_state::TeeState, verifier_votes::TeeVerifierVotes},
+    update::ProposedUpdates,
+};
+
+/// The `Config` layout written by the `3.13.0` contract, before
+/// `fail_attestation_submission_tera_gas` and the verifier gas knobs were added.
+#[derive(Debug, BorshSerialize, BorshDeserialize)]
+pub struct OldConfig {
+    key_event_timeout_blocks: u64,
+    tee_upgrade_deadline_duration_seconds: u64,
+    contract_upgrade_deposit_tera_gas: u64,
+    sign_call_gas_attachment_requirement_tera_gas: u64,
+    ckd_call_gas_attachment_requirement_tera_gas: u64,
+    return_signature_and_clean_state_on_success_call_tera_gas: u64,
+    return_ck_and_clean_state_on_success_call_tera_gas: u64,
+    fail_on_timeout_tera_gas: u64,
+    clean_tee_status_tera_gas: u64,
+    clean_invalid_attestations_tera_gas: u64,
+    cleanup_orphaned_node_migrations_tera_gas: u64,
+    remove_non_participant_update_votes_tera_gas: u64,
+    clean_foreign_chain_data_tera_gas: u64,
+    remove_non_participant_tee_verifier_votes_tera_gas: u64,
+}
+
+impl From<OldConfig> for crate::Config {
+    fn from(old: OldConfig) -> Self {
+        crate::Config {
+            key_event_timeout_blocks: old.key_event_timeout_blocks,
+            tee_upgrade_deadline_duration_seconds: old.tee_upgrade_deadline_duration_seconds,
+            contract_upgrade_deposit_tera_gas: old.contract_upgrade_deposit_tera_gas,
+            sign_call_gas_attachment_requirement_tera_gas: old
+                .sign_call_gas_attachment_requirement_tera_gas,
+            ckd_call_gas_attachment_requirement_tera_gas: old
+                .ckd_call_gas_attachment_requirement_tera_gas,
+            return_signature_and_clean_state_on_success_call_tera_gas: old
+                .return_signature_and_clean_state_on_success_call_tera_gas,
+            return_ck_and_clean_state_on_success_call_tera_gas: old
+                .return_ck_and_clean_state_on_success_call_tera_gas,
+            fail_on_timeout_tera_gas: old.fail_on_timeout_tera_gas,
+            clean_tee_status_tera_gas: old.clean_tee_status_tera_gas,
+            clean_invalid_attestations_tera_gas: old.clean_invalid_attestations_tera_gas,
+            cleanup_orphaned_node_migrations_tera_gas: old
+                .cleanup_orphaned_node_migrations_tera_gas,
+            remove_non_participant_update_votes_tera_gas: old
+                .remove_non_participant_update_votes_tera_gas,
+            clean_foreign_chain_data_tera_gas: old.clean_foreign_chain_data_tera_gas,
+            remove_non_participant_tee_verifier_votes_tera_gas: old
+                .remove_non_participant_tee_verifier_votes_tera_gas,
+            // New in this version: the attestation fail-call and verifier-call gas
+            // knobs, added alongside the async attestation flow.
+            ..crate::Config::default()
+        }
+    }
+}
+
+/// Keep this module in sync with [`crate::MpcContract`]: it is the `3.13.0` layout,
+/// which differs only by the appended `pending_attestations` map.
+#[derive(Debug, BorshSerialize, BorshDeserialize)]
+pub struct MpcContract {
+    protocol_state: ProtocolContractState,
+    pending_signature_requests: LookupMap<SignatureRequest, Vec<YieldIndex>>,
+    pending_ckd_requests: LookupMap<CKDRequest, Vec<YieldIndex>>,
+    pending_verify_foreign_tx_requests: LookupMap<VerifyForeignTransactionRequest, Vec<YieldIndex>>,
+    proposed_updates: ProposedUpdates,
+    node_foreign_chain_support: SupportedForeignChainsByNode,
+    config: OldConfig,
+    tee_state: TeeState,
+    accept_requests: bool,
+    node_migrations: NodeMigrations,
+    metrics: Metrics,
+    foreign_chains: Lazy<ForeignChainsMetadata>,
+    tee_verifier_account_id: Option<AccountId>,
+    tee_verifier_votes: TeeVerifierVotes,
+}
+
+impl From<MpcContract> for crate::MpcContract {
+    fn from(old: MpcContract) -> Self {
+        if let ProtocolContractState::Running(running) = &old.protocol_state {
+            validate_threshold_relation_on_migration(running);
+        }
+
+        crate::MpcContract {
+            protocol_state: old.protocol_state,
+            pending_signature_requests: old.pending_signature_requests,
+            pending_ckd_requests: old.pending_ckd_requests,
+            pending_verify_foreign_tx_requests: old.pending_verify_foreign_tx_requests,
+            proposed_updates: old.proposed_updates,
+            node_foreign_chain_support: old.node_foreign_chain_support,
+            config: old.config.into(),
+            tee_state: old.tee_state,
+            accept_requests: old.accept_requests,
+            node_migrations: old.node_migrations,
+            metrics: old.metrics,
+            foreign_chains: old.foreign_chains,
+            tee_verifier_account_id: old.tee_verifier_account_id,
+            tee_verifier_votes: old.tee_verifier_votes,
+            pending_attestations: LookupMap::new(StorageKey::PendingAttestations),
+        }
+    }
+}
+
+fn validate_threshold_relation_on_migration(running: &RunningContractState) {
+    let num_participants = running.parameters.participants().len() as u64;
+    let max_reconstruction_threshold = max_reconstruction_threshold(running.domains.domains());
+    if let Err(err) = ThresholdParameters::validate_governance_against_reconstruction(
+        num_participants,
+        running.parameters.threshold(),
+        max_reconstruction_threshold,
+    ) {
+        env::panic_str(&format!(
+            "Migration aborted: existing state violates the GovernanceThreshold/ReconstructionThreshold relation ({err:?}). num_participants={}, governance_threshold={}, max_reconstruction_threshold={:?}. Correct it via vote_new_parameters before upgrading.",
+            num_participants,
+            running.parameters.threshold().value(),
+            max_reconstruction_threshold.map(|t| t.inner()),
+        ));
+    }
+}