fix(contract): drop the deposit for submit_participant_info - #3940
Conversation
submit_participant_info kept the whole 0.1 NEAR deposit on success. Re-submissions that change no stored bytes overpaid, and the flat fee mis-charged when the stored attestation entry changed size. Charge only the measured storage delta (storage_byte_cost * bytes_grown) and refund the excess, keeping the up-front minimum-deposit floor. The floor covers the worst-case entry, so the charge can never exceed the deposit and the refund cannot underflow, which is why no insert-then-revert machinery is needed. store_verified_attestation now flushes so env::storage_usage() reflects the insert before the delta is measured. Closes #3857
There was a problem hiding this comment.
Pull request overview
Adjusts submit_participant_info deposit handling in mpc-contract so successful submissions only retain the storage staking cost for the actual on-chain storage delta and refund any excess, addressing overcharging on identical re-submissions and better tracking entry-size changes.
Changes:
- Charge
submit_participant_infoby measuredenv::storage_usage()delta and refund the remainder (Mock path immediately; Dstack path inresolve_verification). - Ensure attestation inserts are flushed (
IterableMap::flush) so storage deltas are measurable. - Update unit + sandbox tests and interface/docs commentary to reflect “keep delta, refund rest”.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/design/attestation-verifier-contract.md | Updates design doc wording around deposit behavior (but needs alignment with how the callback receives deposit). |
| crates/near-mpc-contract-interface/src/deposits.rs | Updates deposit constant documentation to reflect refunding excess. |
| crates/contract/tests/sandbox/utils/mpc_contract.rs | Adds a helper to measure “kept deposit” in sandbox tests. |
| crates/contract/tests/sandbox/tee.rs | Updates/extends sandbox tests to assert non-overcharge on first submit and identical resubmit. |
| crates/contract/src/tee/tee_state.rs | Flushes attestation inserts so storage delta measurement works; adds a unit test guarding this. |
| crates/contract/src/lib.rs | Implements “keep storage delta + refund rest” logic and updates docs/tests around minimum deposit sizing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Make MINIMUM_ATTESTATION_STORAGE_DEPOSIT pub so the public submit_participant_info doc can link to it (broken_intra_doc_links is denied), matching the sibling MINIMUM_NODE_MANAGEMENT_DEPOSIT. Sandbox measure-kept-deposit helper now asserts the balance decreased and covers gas, using plain subtraction so it fails loudly instead of masking an inverted balance. Log the unreachable cost-exceeds-deposit branch for observability. Correct the design-doc bullet: the deposit is forwarded onto the callback receipt, not stashed.
… test helper The submit_participant_info doc edits changed the ABI doc strings; regenerate the snapshot. The sandbox measure-kept-deposit helper uses checked_sub with a panicking fallback so an inverted balance or gas exceeding spend fails loudly instead of saturating to zero.
…ion-charge comments Convert minimum_attestation_storage_deposit__should_cover_worst_case_entry to an rstest parametrized over the Dstack and Mock attestation variants, so each reports as its own case. The per-variant assertion (deposit covers each variant's cost) is equivalent to the previous max-over-variants assertion. Also tighten the surrounding comments: drop the stale 'submission' framing on keep_storage_delta_and_refund_rest, name the test that pins the deposit floor on the unreachable None branch, and shorten the flush comment in store_verified_attestation.
…-participants submit_participant_info required a deposit unconditionally, which broke nodes signing with a function-call access key: such keys cannot attach any deposit, so every re-attestation failed at tx validation with DepositWithFunctionCall. Restore the pre-#3714 conditional: a participant re-attesting an existing entry attaches no deposit and is charged nothing (so the node's function-call key can re-attest), while a new entry or a non-participant caller still pays the storage delta. The client and node stop attaching the deposit; an operator's full-access key funds a first-time (join) submission. Adds a sandbox regression test that drives submit_participant_info through a real function-call access key (zero deposit succeeds; a deposit is rejected). Closes #3925
…age-delta # Conflicts: # crates/near-mpc-contract-interface/src/client.rs
… onboarding The fixed MINIMUM_ATTESTATION_STORAGE_DEPOSIT floor rejected zero-deposit submissions outright. Restore the pre-#3714 behavior: charge only the measured storage delta and reject only when the attached deposit is below it. A participant re-attesting an unchanged entry grows nothing, so it pays nothing and its function-call key works; a new entry costs the measured delta, funded by a deposit-capable caller. In e2e, fund each new participant's first attestation before resharing, signed with the node's near_signer_key so the stored account_public_key matches the key the node later uses for key-event votes (an operator key would store the wrong key and stall resharing). Add MpcNodeState::near_signer_key for this. Repurpose the worst-case test to assert the entry cost stays under a bounded ceiling, and regenerate the ABI snapshot.
submit_participant_info takes no deposit; the contract's own balance stakes the bounded attestation entry. A node self-submits with its function-call access key for its first attestation and re-attestations alike, fixing onboarding: #3714 required a deposit, which a function-call key cannot attach. This makes intentional what pre-#3714 did by accident. Pre-#3714 measured the storage delta right after inserting into the IterableMap, before the deferred write was flushed, so the delta read as 0 and the caller was never charged. #3714 added the flush (kept here), which exposed the real cost and started rejecting the node's zero-deposit submit. Storage is bounded and reclaimed by clean_invalid_attestations, so the contract-funded cost is bounded and self-healing. Removes the charge path, the payable modifiers, deposit forwarding, and the e2e funding workaround; updates tests and the ABI snapshot.
Once storage is contract-funded, nothing reads env::storage_usage() after the store, so the explicit flush in store_verified_attestation is dead: IterableMap flushes on Drop at end-of-call, so persistence is unchanged. Remove the flush, its comments, and the test that only guarded immediate delta visibility. Also reword one stale comment referring to the removed caller-funded model.
submit_participant_info and resolve_verification no longer take a deposit, so the attached_deposit(1 NEAR) set in their unit-test contexts is dead context; remove it. Rename the tee_verifier sandbox refund helper/test to reflect that a failed submission now costs only gas (no deposit to refund).
…hen/then markers Revert the sandbox submit_participant_info helper to the MpcContractHandle-based form (it attaches zero deposit, which the contract-funded method accepts), and add the Given/When/Then markers to the function-call-key test for consistency with the rest of the file.
…orage The 'attached deposit' bullet described the removed delta-charge/refund/ deposit-forwarding design. Attestation storage is now contract-funded, so submit_participant_info attaches no deposit and none is forwarded or refunded.
| // Zero: the node's function-call key cannot attach a deposit; the contract only charges new entries. | ||
| deposit: NearToken::from_yoctonear(0), |
There was a problem hiding this comment.
Stale as of this PR — the contract no longer "charges new entries"; it funds them from its own balance. Copilot flagged this one too.
| // Zero: the node's function-call key cannot attach a deposit; the contract only charges new entries. | |
| deposit: NearToken::from_yoctonear(0), | |
| // The node's function-call key cannot attach a deposit; attestation storage is | |
| // funded by the contract's own balance. | |
| deposit: NearToken::from_yoctonear(0), |
| - **The submitter's `Attestation::Dstack` payload** — the RTMR3 event log, app-compose, and report-data the post-DCAP checks consume. | ||
| - **The submitter's TLS public key** — the callback hashes it with the submitter's account public key and compares to the quote's `report_data` field, proving the enclave produced the quote for this specific submitter. | ||
| - **The attached deposit** — covers storage staking on success, refunded to the signer of the original `submit_participant_info` transaction on failure. `env::attached_deposit()` is not visible from the callback receipt, so the value is stashed at submit time and the recipient `AccountId` is the same one used to key the entry (set by `Self::assert_caller_is_signer()`). | ||
| - No deposit is stashed: attestation storage is funded by the contract's own balance, so `submit_participant_info` attaches nothing and there is no deposit to forward to the callback or refund. |
There was a problem hiding this comment.
Three things in this file, in increasing order of scope.
1. This bullet (line 142). It's a negation dropped into a list of things PendingAttestation holds, and the only unbolded entry in it. Reads better as prose right after the list than as a list item.
2. Line 108 is still stale. It reads "...and the deposit stays locked because the refund is part of the cleanup the contract never got around to." Outside this PR's diff so I can't suggest inline, but it needs the same treatment you gave :142.
3. Pre-existing, so your call — this whole section describes a design that never shipped. PendingAttestation doesn't exist in the code:
$ grep -rn "PendingAttestation\|on_attestation_verified\|pending_attestation" crates/ --include=*.rs
(no matches)
submit_participant_info passes VerificationContext directly into a plain .then(resolve_verification) chain — no pending map, no data_id, no promise_yield_create (the one at lib.rs:341 is the generic enqueue_yield_request helper used by sign/CKD/foreign-tx, not this flow). So lines ~136-146 and the "Handling failures" discussion above describe a superseded mechanism. Copilot noticed this too.
Not introduced by this PR, and I wouldn't ask you to rewrite the section here. But AGENTS.md says a design doc describing a superseded design has to be updated, removed, or prominently marked rather than left silently stale — so the cheap out is a Status: superseded by #NNNN banner plus a follow-up issue, rather than editing individual bullets inside a section that no longer matches the code.
There was a problem hiding this comment.
Direction is right — contract-funded storage is the only model that works with fc keys, and deferring the rework to #3972 makes sense. Requesting changes on one thing.
Blocker: underflow in the new entry-size cap — #3940 (comment). storage_usage() - before is a signed delta read as unsigned, so any submission that shrinks the entry wraps and permanently pins that account to its larger entry. Two ordinary txs, no TEE. release-contract sets no overflow-checks, so it wraps silently rather than panicking.
More generally, I think a static check is enough here — and better, given the bug. NodeAttestation is bounded by construction (only account_id varies, capped at 64B), so nothing can vary at runtime. The runtime check costs gas on every submit, adds an untested rollback path (r3656731057), and at 0.1 NEAR against a measured 604-byte worst case wouldn't catch bloat anyway (r3656725797).
Suggestion: revert e845e90, keep the unit test, tighten it to ~0.02 NEAR. Closes all three in one move.
@kevindeforth your call, since it counters your runtime-check preference. If it stays, it needs all three: underflow fix, tighter constant, rejection-path coverage.
Also, doc-alignment rule: stale comment at client.rs:126, stale deposit sentence at attestation-verifier-contract.md:108.
Reverts the three commits that introduced it, returning store_verified_attestation to the state approved in afacfef: e845e90 refactor(contract): measure real storage delta for the attestation entry cap 5a164a8 test(contract): drop tautological entry-cost test, inline single-use helper 296539d refactor(contract): reuse storage-cost idiom for attestation entry cap NodeAttestation is bounded by construction - every field is fixed-width except account_id, which NEAR caps at 64 bytes - so no runtime-variable input exists for the check to catch. It only guarded against a future schema change, which submit_participant_info__should_bound_worst_case_entry_cost already covers, and it would have caught that in production by rejecting live submissions rather than in CI. It also carried an underflow: env::storage_usage() - before reads a signed delta as unsigned, so any submission that shrank the stored entry wrapped and permanently pinned that account to its larger entry.
With the runtime cap reverted, these tests are the only guard on how much storage the contract funds per attestation entry. Pin the exact measured size of each VerifiedAttestation variant (599 bytes Dstack, 604 bytes Mock) so any layout change fails CI and forces a deliberate decision, rather than silently changing what the contract pays per node. Add WORST_CASE_ENTRY_BYTES as the pinned worst case, and a doc comment stating what must be revisited before the numbers are updated. Tighten WORST_CASE_ENTRY_COST_CEILING from 0.1 to 0.01 NEAR. The old value was inherited from the flat deposit MINIMUM_ATTESTATION_STORAGE_DEPOSIT, where wide margin was wanted; as a regression ceiling it left 16x slack and would not have caught a schema change short of a 16x bloat. Extract the shared measurement into measure_stored_entry_bytes so both tests share one definition of the worst case.
barakeinav1
left a comment
barakeinav1
left a comment
There was a problem hiding this comment.
Approving — the blocker is resolved.
- Runtime entry-size cap removed in 158e8e4.
NodeAttestationis bounded by construction, so there was no runtime-variable input for it to catch; it only guarded against a future schema change, and it carried an underflow (storage_usage() - beforereading a signed delta as unsigned). The revert lands byte-identical to afacfef, the version @gilcu3 approved. - Replaced with stricter build-time checks in 74ea6a0. The exact stored size of each attestation variant is now pinned — 599 bytes Dstack, 604 bytes Mock — so a layout change fails CI instead of surfacing in production by rejecting live submissions.
WORST_CASE_ENTRY_COST_CEILINGtightened from 0.1 to 0.01 NEAR; the old value was inherited from the flat deposit, where wide margin was wanted, and left 16x slack as a regression ceiling.
Net effect is a strictly tighter guard than the runtime check it replaces, caught earlier and at no gas cost.
Two doc items are still open and I'm happy to push them myself: the stale comment at client.rs:126 and the stale deposit sentence at docs/design/attestation-verifier-contract.md:108. Not blocking.
…age-delta # Conflicts: # crates/near-mpc-contract-interface/src/client.rs
| ``` | ||
|
|
||
| _Note_: submit_participant_info - can be called either by the node or by the operator. | ||
| _Note_: submit_participant_info - can be called either by the node or by the operator. Attestation storage is funded by the contract's own balance, so submissions attach no deposit and the node's function-call access key works for both a first-time (join) submission and re-attestations. |
There was a problem hiding this comment.
our current flow is for node submission only.
IIRC we check the reportdata that contains the hash of public key hash, against who submitted 'submit_participant_info'
| - **The submitter's `Attestation::Dstack` payload** — the RTMR3 event log, app-compose, and report-data the post-DCAP checks consume. | ||
| - **The submitter's TLS public key** — the callback hashes it with the submitter's account public key and compares to the quote's `report_data` field, proving the enclave produced the quote for this specific submitter. | ||
| - **The attached deposit** — covers storage staking on success, refunded to the signer of the original `submit_participant_info` transaction on failure. `env::attached_deposit()` is not visible from the callback receipt, so the value is stashed at submit time and the recipient `AccountId` is the same one used to key the entry (set by `Self::assert_caller_is_signer()`). | ||
| - No deposit is stashed: attestation storage is funded by the contract's own balance, so `submit_participant_info` attaches nothing and there is no deposit to forward to the callback or refund. |
There was a problem hiding this comment.
add follow-up issue here
The 'Handling failures' and 'Contract state changes' sections describe a yield-resume design that was replaced by the no-yield promise chain: PendingAttestation, pending_attestations, data_id, the 'already pending' guard and on_attestation_verified do not exist in the code. #3825 tracks rewriting them; mark them until it lands so readers are not misled.
The note claimed the call could be made by the node or the operator. The quote's report_data binds hash(tls_public_key, account_public_key) to env::signer_account_pk(), and verify_report_data enforces it, so a submission signed by an operator key fails verification. assert_caller_is_signer rules out a proxy contract as well.
|
@claude review |
Pull request overview
Changes:
Reviewed changesPer-file summary
FindingsBlocking (must fix before merge):
Non-blocking (nits, follow-ups, suggestions):
✅ Approved on the code; |
…nner The operator runbook told first-time joiners to call submit_participant_info with --deposit. submit_participant_info is no longer payable, so near-sdk's generated guard panics with 'doesn't accept deposit' on any attached amount, and the 'Attached deposit is lower than required' error it documented can no longer come from this method. Remove that bullet and the 'contract's deposit logic' clause above it. Replace the per-subsection TODO(#3825) in the attestation-verifier design doc with a file-level Status banner. The subsection marker covered two of the roughly fifteen stale yield-resume and deposit references; the rest -- the submission-flow prose, the mermaid diagrams, and the mpc-contract API proposal -- sit outside it and read as current design.
Both tests insert straight into tee_state.stored_attestations and never call submit_participant_info, so the old prefix promised coverage of an entry point they do not exercise. The doc comment also claimed to pin every attestation variant while only the largest of each kind is cased.
…deposit Dropping #[payable] makes near-sdk's generated guard panic on any nonzero deposit rather than merely making it optional. Nothing pinned that: the old below-fee sandbox test was deleted with the deposit gate and no test asserted either direction. Assert the rejection and that nothing is stored.
|
Went through all five. Three fixed, one rejected with reasoning, one moot.
Fixed in aae164f — removed that bullet, which took the stale #903 pointer with it, plus the "the contract's deposit logic" clause at :2070. #903 is now
Correct, and my mistake — I grepped with too narrow a pattern and only inspected the section I was editing. A plain Fixed in aae164f by replacing the subsection TODO with a file-level
Keeping non-payable, deliberately. Restoring Checked what actually breaks first. Pre-#3714 nodes attached zero:
So this restores the pre-#3714 node path line-for-line. The 0.1 NEAR in
Fair. Added
Moot on both conditions. No contract carrying #3714 is deployed — mainnet and the testnet cluster in #3925 are both on 3.13.2. And the verifier is not a party to the deposit in any case:
Both fixed in d8c3579 — renamed to |
scripts/check-todo-format.sh requires a colon immediately after the issue reference; the banner used a period, failing the fast CI checks.
gilcu3
left a comment
gilcu3
left a comment
There was a problem hiding this comment.
LGTM
One thing to flag, apparently we no longer need the fail_attestation_submission method, or its usage, because we are no longer accepting deposits that would need roll-back. If you agree, feel free to clean that up here, or open an issue for a follow-up (if none exists)
| /// 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. |
There was a problem hiding this comment.
nit: this seems a bit LLM verbose, but I don't mind
| **Status:** Partially superseded — TODO(#3825). The `mpc-contract` side shipped as a no-yield promise chain, so every mention below of yield-resume, `pending_attestations` / `PendingAttestation`, `data_id`, the "already pending" guard, `on_attestation_verified`, and the attached deposit and its refunds describes a design that was not built. The verifier-contract split, governance, crate layout, and the verifier's own API are accurate. | ||
|
|
There was a problem hiding this comment.
what do you mean a no-yield promise chain? Afaik we still have the yields, else how are we handling the cross-contract return call?
| ``` | ||
|
|
||
| _Note_: submit_participant_info - can be called either by the node or by the operator. Attestation storage is funded by the contract's own balance, so submissions attach no deposit and the node's function-call access key works for both a first-time (join) submission and re-attestations. | ||
| _Note_: submit_participant_info must be called by the node itself - the quote's report_data binds hash(tls_public_key, account_public_key) to env::signer_account_pk(), so a submission signed by any other key fails verification. Attestation storage is funded by the contract's own balance, so submissions attach no deposit and the node's function-call access key works for both a first-time (join) submission and re-attestations. |
…ragraph The background implied contract-funded storage started with #3940. It did not: the deployed 3.13.0 contains charging code that never collects, because it reads the storage delta before the write is flushed, and #3714 -- the only version that genuinely required a deposit -- never shipped. So this has been true of every deployed version, and #3940 only made it intentional. That matters because it means the drain is not a regression to be reverted but a gap that was always there. Also split the run-on sentence about the security implication into three bullets: unlimited entries, roughly 7x cost asymmetry, and the contract being unable to write state at all once its balance is gone.
Closes #3925.
Closes #3857.
Closes #903.
submit_participant_infois no longer#[payable]— it takes no deposit and rejects one if attached, and the contract funds attestation storage from its own balance. A node self-submits with its function-call access key for its first attestation and re-attestations alike.This fixes onboarding: #3714 introduced a deposit, which a function-call key cannot attach (
DepositWithFunctionCall), so nodes could no longer attest.Why removing the deposit is safe
Pre-#3714 the contract had "charge the caller for a new entry" logic that never collected: it read
env::storage_usage()immediately after inserting into theIterableMap, before the deferred write was flushed, so the delta read as 0 and nothing was charged. New entries were already contract-funded, by accident. This makes that intentional.The cost is bounded by construction — every field of the stored entry is fixed-width except
account_id, which NEAR caps at 64 bytes. Its exact size is pinned at 604 bytes (~0.006 NEAR) bysubmit_participant_info__should_store_exactly_the_pinned_entry_size, under a 0.01 NEAR ceiling, so a schema change that bloats it fails CI.Dstackentries expire and are reclaimed byclean_invalid_attestations.Mock::Validnever expires and is never reclaimed — pre-existing (but will be fix in #3785 -) , and the same exposurerelease/v3.13shipped with; see #3972.Behavior after this change
Follow-ups
Referenced, not closed by this PR:
report_databinds the quote toenv::signer_account_pk()(details).fail_attestation_submissiononly existed so a failingresolve_verificationcould commit its refund first. With the deposit gone nothing needs to survive the error branch, so the method and itsConfiggas knob can go — a state-schema change, hence not folded in here. Sequence it after design: funding model for attestation storage #3972.submit_participant_infodesign #3825 —docs/design/attestation-verifier-contract.mdstill describes the superseded yield-resume design. Out of scope here, so the affected sections carry aTODO(#3825). docs: align TEE attestation docs with the no-yield design #3826 is the in-flight fix and needs rebasing onto this PR, since it documents the deposit machinery removed here.