feat(contract): operator-prepaid attestation storage (grant counter) - #4016
Conversation
54e2e95 to
93c9ab5
Compare
93c9ab5 to
54e2e95
Compare
59b9b69 to
1affa28
Compare
1affa28 to
1f80a7e
Compare
1f80a7e to
263426a
Compare
There was a problem hiding this comment.
Pull request overview
Implements the operator-prepaid attestation storage “grant counter” model in the mpc-contract, shifting the cost of stored attestation entries from contract-funded storage to operator prepayment while keeping nodes self-submitting using deposit-less function-call keys.
Changes:
- Adds an attestation-storage grant counter (
attestation_grants) plusprepay_attestation_storageandavailable_attestation_grantscontract methods, and wires grant consumption/return into attestation insertions and sweep cleanup. - Extends contract configuration and DTO plumbing with
attestation_storage_fee_millinear(defaulting to 20 milliNEAR) and updates migration/state keys accordingly. - Updates docs and expands sandbox/in-process tests and ABI snapshots to cover the new prepay-and-consume behavior.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/running-an-mpc-node-in-tdx-external-guide.md | Adds an operator prepayment step and updates the submit note to reflect prepaid storage requirements. |
| crates/test-utils/src/contract_types.rs | Extends dummy config with attestation_storage_fee_millinear. |
| crates/near-mpc-contract-interface/src/types/config.rs | Adds attestation_storage_fee_millinear to InitConfig/Config plus serialization tests. |
| crates/contract/tests/snapshots/abi__abi_has_not_changed.snap | Updates ABI snapshot for new public methods and config field. |
| crates/contract/tests/sandbox/utils/mpc_contract.rs | Adds sandbox helper to prepay grants and updates submit helper to prepay when needed. |
| crates/contract/tests/sandbox/upgrade_from_current_contract.rs | Updates config update test data to include the new fee field. |
| crates/contract/tests/sandbox/tee_verifier.rs | Adjusts sandbox verifier tests to separate prepayment from submission for balance assertions. |
| crates/contract/tests/sandbox/contract_configuration.rs | Includes attestation_storage_fee_millinear in initialization config test. |
| crates/contract/tests/inprocess/attestation_submission.rs | Adds in-process coverage for grant rejection, consumption, multi-entry, TLS-key ownership, exact deposit multiple, and sweep grant return. |
| crates/contract/src/v3_13_0_state.rs | Migration initializes the new attestation_grants map and updates cleanup return type usage. |
| crates/contract/src/tee/tee_state.rs | Makes sweep return entry owners (not just a count) and adds attestation_owner helper for early grant classification. |
| crates/contract/src/storage_keys.rs | Adds a storage key variant for the new grants map. |
| crates/contract/src/lib.rs | Core implementation: grant accounting, prepay entrypoint, early precondition, callback re-check, and sweep crediting. |
| crates/contract/src/errors.rs | Adds new InvalidParameters variants for exact-deposit mismatch and missing grants. |
| crates/contract/src/dto_mapping.rs | Wires fee field between contract config and interface DTOs. |
| crates/contract/src/config.rs | Adds default attestation_storage_fee_millinear to contract config. |
| crates/contract/README.md | Documents the new prepay and grants view endpoints in the contract API list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c8eb6e9 to
70bc171
Compare
|
@claude review |
Pull request overviewShifts the cost of a stored attestation entry from the contract's balance to an operator prepayment, keeping the node self-submitting with its deposit-less function-call key. Changes:
Reviewed changesPer-file summary
FindingsBlocking (must fix before merge):
Non-blocking (nits, follow-ups, suggestions):
|
3c6e9d1 to
770a269
Compare
Addresses the review on #4016. Doc drift (CLAUDE.md treats this as review-blocking): five places still said attestation storage is funded by the contract's balance, which this PR inverts -- submit_participant_info, resolve_verification and verify_post_dcap_and_store docstrings, plus securing-mpc-with-tee-design-doc.md and attestation-verifier-contract.md. Each now says a new entry consumes a prepaid grant and a re-attestation consumes none. The first two are embedded in the ABI, so the snapshot is re-accepted. The sandbox test called should_store_new_entry_with_zero_deposit claimed "the contract's own balance funds the storage" and only passed because the shared helper prepaid for it. Renamed to ...should_store_a_new_entry_against_a_prepaid_grant, with a separate account prepaying explicitly and an assertion that the grant reaches zero. "Zero deposit" is about the submitting node, whose function-call key cannot attach one -- not about the storage being free. Also: assert the concrete TlsKeyOwnedByOtherAccount rather than Err(_), which held even with the precondition removed; reject a configured fee of zero, since a zero required deposit is one a function-call key can attach, which would let anyone mint grants; and drop a duplicated comment in the sandbox helper.
Review follow-ups on #4016. - return_attestation_storage_grant now uses checked_add, matching the credit in prepay_attestation_storage; a saturating add at u32::MAX would silently drop a grant that was genuinely returned. - A private grants_for(&AccountId) does the lookup and the public view delegates to it, removing the clone the owned parameter forced at every internal call site. The view keeps an owned parameter because NEAR deserialises view arguments by value. - A configured fee of zero stays permitted: the value is for governance to choose. - Drop the "Rule 3:" prefix, which numbered a rule that exists only in the design doc rather than in the code. - Operator guide: say outright that a grant prepaid to a mistyped account cannot be recovered or redirected, since nothing is refunded and there is no withdrawal.
11011fb to
5cab95b
Compare
Implements the design in docs/design/operator-prepaid-attestation-storage.md (#4015). An operator prepays for attestation-entry storage in a separate transaction; the node keeps self-submitting with its deposit-less function-call key. Payment and submission have to be separate: a function-call key cannot attach a deposit, and report_data binds the quote to env::signer_account_pk(), so neither party can do both halves. One prepayment buys one grant -- permission to hold one stored attestation entry -- and the grant returns when that entry is reclaimed, so it is a slot the operator keeps rather than a per-attestation charge. Contract: - prepay_attestation_storage(account_id, grants), payable and permissionless, requiring exactly fee x grants so there is no remainder to keep or refund. - available_attestation_grants(account_id) view. The fee is read from config(); there is deliberately no dedicated view for it. - available_attestation_grants: LookupMap<AccountId, u32> holds available grants; the row is removed at zero so the map does not accumulate rows for accounts holding none. - Config.attestation_storage_fee_millinear defaults to 20 (0.02 NEAR) and is votable through ConfigExt. Zero stays permitted: the value is governance's to choose. Charging rules: a re-attestation under a key the caller already owns consumes nothing; a new entry consumes one grant; clean_invalid_attestations returns one grant to the owner of each entry it removes. The precondition is read-only and runs before any verification, and compares the owning account rather than just testing key presence -- otherwise a submission for somebody else's key would be classified as needing no grant and would still reach verify_quote. It is re-checked inside resolve_verification, since that callback runs in a later receipt where the grant may since have been consumed. Entries that predate the fee need no handling: they already hold a slot no grant was bought for, and re-attestation is free, so those operators need no grant and no action. Migration just initialises the map. TeeState::clean_invalid_attestations now returns the owners of the entries it removed rather than a count, so the caller can credit them; MpcContract still returns the count, leaving the external interface unchanged. Docs: operator guide gains the prepayment step after Create a NEAR Account for Your Node, where the operator still holds that account's full-access key and the node has not started yet; it reads the fee from config() rather than hard-coding it, and warns that a grant prepaid to a mistyped account cannot be recovered. Drops the stale "will incur a cost (TBD, XXX NEAR)" note citing the closed #903. Both new methods are documented in the contract README's User API. Five docstrings that still claimed storage is contract-funded now describe the grant instead. Tests cover the guards as well as the happy path: exact-deposit rejection either side by one yocto, zero grants, one account funding another, rejection without a grant, a key owned by another account rejected before verification by its concrete error, and a swept entry returning a grant that is then spendable without paying again. Both sandbox and in-process harnesses prepay only when a submission would actually consume a grant, and the e2e harness prepays for every node in the cluster -- a node joining by resharing attests from its own process with a key that cannot attach a deposit, so its grant must exist beforehand. Sweep gas is out of scope and tracked in #4035: this adds a per-removal write that takes the marginal cost from 0.347 to 0.504 TGas, but the budget was already short of RESHARE_CLEAN_INVALID_ATTESTATIONS_MAX_SCAN on main.
5cab95b to
a9970fe
Compare
|
Went through all of these. Two were already stale, three fixed, one deferred with reasoning, and the non-blocking list mostly done. Everything is in the single squashed commit.
Fixed, and your diagnosis was exactly right including the "every node, not just participants" part — a node joining by resharing attests from its own process with a function-call key, so its grant has to exist beforehand. Took several rounds: the fee could not be read from
Fixed, all six.
Fixed. Renamed to You also caught a real flaw underneath this: both harnesses prepaid whenever an account had no grant, including for re-attestations that need none. Since participants already hold sentinel entries from init, every setup submission left a stray unused grant — which is why the function-call-key sandbox test was passing without ever prepaying. Both now prepay only when a submission would actually consume a grant.
Stale — #4011 merged and the doc is on
Deferred to #4035 deliberately, and the severity is lower than we both had it. Non-blocking
|
|
@claude review |
|
@copilot lite review |
`prepay_attestation_storage`'s exact-multiple and zero-grants cases are pure input validation: they need a contract and a deposit context, nothing about participants or protocol state, so they belong in `src` rather than the in-process harness.
gilcu3
left a comment
gilcu3
left a comment
There was a problem hiding this comment.
Thanks for the fixes!
Left some nits (and a tiny blocker about a contract method name constant but I am not blocking for that one)
There was a problem hiding this comment.
I still believe some of these tests could still be unit tests, but we can leave that as a follow up, to not overcomplicate this one
| /// Like [`Self::call`], but waits for the block to be final, so a `view` issued | ||
| /// afterwards sees the state this call wrote. | ||
| pub async fn call_final( | ||
| &self, | ||
| method: &str, | ||
| args: serde_json::Value, | ||
| ) -> anyhow::Result<FinalExecutionOutcome> { | ||
| self.client | ||
| .call(&self.contract_id, method) | ||
| .args(args) | ||
| .gas(MAX_GAS) | ||
| .wait_until::<Final>() | ||
| .await | ||
| .map_err(|e| anyhow::anyhow!("contract call `{method}` failed: {e}")) | ||
| } |
There was a problem hiding this comment.
thought we could just reuse the call function above this one, but that one is not Final. It feels we could organize this a bit better to avoid so much duplicate code and to ensure the caller knows exactly what they are calling, but for now this is fine.
| /// call site costs a failed transaction rather than an entry nobody paid for. | ||
| fn consume_attestation_storage_grant(&mut self, account_id: &AccountId) { | ||
| let remaining = self | ||
| .grants_for(account_id) | ||
| .checked_sub(1) | ||
| .expect("caller must establish an available grant before consuming one"); | ||
| if remaining == 0 { | ||
| self.available_attestation_grants.remove(account_id); | ||
| } else { | ||
| self.available_attestation_grants | ||
| .insert(account_id.clone(), remaining); | ||
| } | ||
| } | ||
|
|
||
| fn return_attestation_storage_grant(&mut self, account_id: &AccountId) { | ||
| let available = self.grants_for(account_id); | ||
| // Checked, not saturating: at `u32::MAX` a returned grant would be dropped silently. | ||
| let Some(returned) = available.checked_add(1) else { | ||
| log!("grant counter for {account_id} is saturated; not returning a grant"); | ||
| return; | ||
| }; | ||
| self.available_attestation_grants | ||
| .insert(account_id.clone(), returned); | ||
| } |
There was a problem hiding this comment.
I see a small foot gun here related to the fact that at 0 the entry is deleted, then in the function below apparently that would cause that the grant is not returned. This is not the case because of the unwrap_or(0) inside grants_for but I wonder if there is a better way. I have no explicit suggestion though 😿
netrome
left a comment
netrome
left a comment
There was a problem hiding this comment.
Quick sweep, looks alright but I'm questioning if we want to use a LookupMap vs an iterable map for the attestation grants. Don't need to block merging this PR, but could consider changing this as a follow-up.
| tee_verifier_votes: TeeVerifierVotes, | ||
| /// A row is removed at zero, so the map holds no entry for an account with none. | ||
| available_attestation_grants: LookupMap<AccountId, u32>, |
There was a problem hiding this comment.
Yeah, it feels like we should spend more time thinking how to organize the contract code. I guess you're about to do this soon @gilcu3 so probably could move this in the same go.
Scanning an entry costs gas whether or not it is removed, so RESHARE_CLEAN_INVALID_ATTESTATIONS_MAX_SCAN and clean_invalid_attestations_tera_gas have to be sized against each other. They were not: max_scan 100 needs ~28 TGas to walk the list with zero removals, against a 10 TGas budget. The promise is detached, so exceeding it rolls back every removal silently, which a live sweep on v1.signer-prod.testnet confirmed by burning 13.1 TGas. Lower max_scan 100 -> 30 and raise the budget 10 -> 15 TGas. A 30-entry scan costs ~9.9 TGas before any removal; 15 TGas covers ~7 removals on top, measured with #4016's storage-grant return in place (which took the per-removal cost from 0.362 to 0.588 TGas). Reset that field during migration rather than carrying the deployed value forward, so it applies on upgrade without a governance config vote. Every scanned entry being removable stays unfunded (~22 TGas); the permissionless entry point is the recovery path. Closes #4035
Scanning an entry costs gas whether or not it is removed, so RESHARE_CLEAN_INVALID_ATTESTATIONS_MAX_SCAN and clean_invalid_attestations_tera_gas have to be sized against each other. They were not: max_scan 100 needs ~28 TGas to walk the list with zero removals, against a 10 TGas budget. The promise is detached, so exceeding it rolls back every removal silently, which a live sweep on v1.signer-prod.testnet confirmed by burning 13.1 TGas. Lower max_scan 100 -> 30 and raise the budget 10 -> 15 TGas. A 30-entry scan costs ~9.9 TGas before any removal; 15 TGas covers ~7 removals on top, measured with #4016's storage-grant return in place (which took the per-removal cost from 0.362 to 0.588 TGas). Reset that field during migration rather than carrying the deployed value forward, so it applies on upgrade without a governance config vote. Every scanned entry being removable stays unfunded (~22 TGas); the permissionless entry point is the recovery path. Closes #4035
Scanning an entry costs gas whether or not it is removed, so RESHARE_CLEAN_INVALID_ATTESTATIONS_MAX_SCAN and clean_invalid_attestations_tera_gas have to be sized against each other. They were not: max_scan 100 needs ~28 TGas to walk the list with zero removals, against a 10 TGas budget. The promise is detached, so exceeding it rolls back every removal silently, which a live sweep on v1.signer-prod.testnet confirmed by burning 13.1 TGas. Lower max_scan 100 -> 30 and raise the budget 10 -> 15 TGas. A 30-entry scan costs ~9.9 TGas before any removal; 15 TGas covers ~7 removals on top, measured with #4016's storage-grant return in place (which took the per-removal cost from 0.362 to 0.588 TGas). Reset that field during migration rather than carrying the deployed value forward, so it applies on upgrade without a governance config vote. Every scanned entry being removable stays unfunded (~22 TGas); the permissionless entry point is the recovery path. Closes #4035
Scanning an entry costs gas whether or not it is removed, so RESHARE_CLEAN_INVALID_ATTESTATIONS_MAX_SCAN and clean_invalid_attestations_tera_gas have to be sized against each other. They were not: max_scan 100 needs ~28 TGas to walk the list with zero removals, against a 10 TGas budget. The promise is detached, so exceeding it rolls back every removal silently, which a live sweep on v1.signer-prod.testnet confirmed by burning 13.1 TGas. Lower max_scan 100 -> 30 and raise the budget 10 -> 15 TGas. A 30-entry scan costs ~9.9 TGas before any removal; 15 TGas covers ~7 removals on top, measured with #4016's storage-grant return in place (which took the per-removal cost from 0.362 to 0.588 TGas). Reset that field during migration rather than carrying the deployed value forward, so it applies on upgrade without a governance config vote. Every scanned entry being removable stays unfunded (~22 TGas); the permissionless entry point is the recovery path. Closes #4035
Scanning an entry costs gas whether or not it is removed, so RESHARE_CLEAN_INVALID_ATTESTATIONS_MAX_SCAN and clean_invalid_attestations_tera_gas have to be sized against each other. They were not: max_scan 100 needs ~28 TGas to walk the list with zero removals, against a 10 TGas budget. The promise is detached, so exceeding it rolls back every removal silently, which a live sweep on v1.signer-prod.testnet confirmed by burning 13.1 TGas. Lower max_scan 100 -> 30 and raise the budget 10 -> 15 TGas. A 30-entry scan costs ~9.9 TGas before any removal; 15 TGas covers ~7 removals on top, measured with #4016's storage-grant return in place (which took the per-removal cost from 0.362 to 0.588 TGas). Reset that field during migration rather than carrying the deployed value forward, so it applies on upgrade without a governance config vote. Every scanned entry being removable stays unfunded (~22 TGas); the permissionless entry point is the recovery path. Closes #4035
Closes #4015. Implements
docs/design/operator-prepaid-attestation-storage.md, agreed in #3972 / #4011 — read that for the model, the alternatives considered and the fee derivation.An operator prepays for attestation-entry storage in a separate transaction; the node keeps self-submitting with its deposit-less function-call key. One prepayment buys one grant, and the grant returns when the entry it paid for is reclaimed.
prepay_attestation_storage(account_id, grants)— payable, permissionless, requires exactlyfee × grants.available_attestation_grants(account_id)view. The fee is read fromconfig(); there is deliberately no dedicated view (why).Config.attestation_storage_fee_millinear, default 20 (0.02 NEAR), votable viaConfigExt.clean_invalid_attestationsreturns one grant per entry it removes.Worth a reviewer's attention
verify_quote. It is re-checked insideresolve_verification, because that callback runs a receipt later, where the grant may since have been consumed.TeeState::clean_invalid_attestationsnow returns the owners of the entries it removed rather than a count, so the caller can credit them.MpcContract::clean_invalid_attestationsstill returns the count, so the external interface is unchanged.Out of scope
Two follow-ups, neither affecting the contract:
initmeans they never consume it. Wasteful, not harmful.clean_invalid_attestations_tera_gaswas already short ofRESHARE_CLEAN_INVALID_ATTESTATIONS_MAX_SCANon main. Nothing to do here, though: those two constants govern only the promisevote_resharedschedules.clean_invalid_attestationsis permissionless and takesmax_scanas a parameter, so an external caller is bound by neither — one manual call at ~300 TGas clears roughly 550 entries. The worst this PR contributes is a manual invocation, not a stuck backlog.