feat(rpc): support native multisig simulation - #7239
Conversation
5c5872d to
4b070c1
Compare
✅ Changelog found on PR. |
5a15408 to
13c1bc9
Compare
13c1bc9 to
f023ebe
Compare
📊 Tempo Precompiles CoverageprecompilesCoverage: 6459/11165 lines (57.85%) File details
contractsCoverage: 1/202 lines (0.50%) File details
Total: 6460/11367 lines (56.83%) |
|
cyclops audit fast note="this belongs to the GH stack #7243 and the TIP 1061 is specified in the PR 7242. any bug not introduced by this PR should be clearly marked" |
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This change adds state-derived recursive mock approvals so RPC simulation can estimate native-multisig transactions conservatively. One verified low-severity correctness issue remains: the duplicated quorum cost model omits a real intrinsic-gas charge and can under-estimate valid nested-multisig transactions.
Reviewer Callouts
- ⚡ Simulation context boundary:
ExecutionContext::Simulationskips native-multisig authorization, and empty transaction encoding can mark a transaction as RPC simulation. This behavior predates the PR, but a human should confirm no consensus or payload-building path can construct aTempoTxEnvwith empty encoding or its default simulation context. - ⚡ WebAuthn hint ranking: Explicit WebAuthn hints are ranked at maximum WebAuthn gas while mock construction uses the supplied key-data size. The node currently emits only
UnknownPrimitiveorMultisig, but in-process or SDK hint producers could violate the conservative-ranking invariant. - ⚡ Signed-request round trip:
From<AASigned> for TempoTransactionRequestemitsmultisig_signature_countwhile leavingfromunset. Confirm no server or SDK path feeds that request back into simulation, where it now fails withmultisig account cannot be zero.
f023ebe to
e575087
Compare
e575087 to
35fbe63
Compare
|
cyclops audit fast note="this belongs to the GH stack #7243 and the TIP 1061 is specified in the PR 7242. any bug not introduced by this PR should be clearly marked" |
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This change adds native-multisig gas modeling to Tempo RPC simulations. The state-aware call path is careful and conservative, but simulateV1 bypasses it and can understate intrinsic gas; hint derivation also adds bounded attacker-shaped work before EVM gas accounting.
Reviewer Callouts
- ⚡ Contract-creation calls from registered multisigs: Populating a hint makes
has_aa_fields()true, while AA conversion requires a non-empty calls list. Verify thateth_callwith notostill simulates CREATE rather than failing withempty calls list. - ⚡ Conservative owner key assumptions: The state-derived path prices unknown owners as maximum-size WebAuthn. Confirm that this intentional overestimate does not make otherwise valid simulations fail the caller balance check.
35fbe63 to
04d8e75
Compare
b37ee58 to
c511a17
Compare
|
cyclops audit fast note="this belongs to the GH stack #7243 and the TIP 1061 is specified in the PR 7242. any bug not introduced by this PR should be clearly marked" |
c511a17 to
6f64329
Compare
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
Rechecked against current head 6f643292333cac24a649cc8c8d019e0e4067a466 after head drift. Three actionable findings remain.
Reviewer Callouts
- ⚡ Conservative primitive-owner pricing: State-derived hints still emit
UnknownPrimitivefor every non-multisig owner, pricing each as maximal WebAuthn. A common registered 1-of-1 secp256k1 estimate is inflated by about 35,720 gas; consider a way to provide exact per-owner key hints. - ⚡ Hint-builder work bound: A state override can create the maximum nested owner graph without on-chain setup cost, causing thousands of storage reads and millions of bounded DP iterations for one RPC call. Benchmark and rate-limit this path.
- ⚡ Storage-reader invariant parity: The RPC reader uses
InitMultisig::validate()while the precompile usesvalidate_for_account(account). Keep these readers aligned so future state sources cannot make self-owned configurations acceptable only to simulation.
| mut request: TempoTransactionRequest, | ||
| mut db: impl Database<Error: Into<EthApiError>>, | ||
| ) -> Result<TxEnvFor<Self::Evm>, Self::Error> { | ||
| populate_native_multisig_simulation_hints(&mut request, &mut db) |
There was a problem hiding this comment.
🚨 [SECURITY] Block simulation bypasses state-aware multisig hint construction
This preprocessing runs only through Call::create_txn_env; eth_simulateV1 and tempo_simulateV1 resolve raw requests through TryIntoSimTx. A normal registered-multisig request therefore stays non-AA and omits the TIP-1061 surcharge, while a request with multisigSignatureCount fabricates primitive approvals without loading the stored threshold or nested owner tree. Under T11 simulation this can report success and gas for an authorization shape that cannot execute on chain.
Recommended Fix:
Run the same state-aware preprocessing for each transaction at its simulated block position before build_simulate_v1_transaction, or reject count-only registered-multisig simulation when state-aware preprocessing is unavailable.
6f64329 to
e8a27db
Compare
e8a27db to
fe282e5
Compare
fe282e5 to
e060cd3
Compare
|
cyclops audit fast note="this belongs to the GH stack #7243 and the TIP 1061 is specified in the PR 7242. any bug not introduced by this PR should be clearly marked" |
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This change adds native-multisig gas modeling to Tempo RPC simulations. After deduplicating against prior reviews, one verified Low-severity correctness issue remains: the simulateV1 bootstrap path uses a prefix-greedy quorum model that can undercharge intrinsic gas or reject valid multisig configurations.
Reviewer Callouts
- ⚡ Cross-RPC quorum parity: Confirm the same multisig bootstrap configuration produces a conservative approval count across
eth_simulateV1,tempo_simulateV1, andeth_estimateGasafter the fix.
| }; | ||
|
|
||
| let account = init.account().map_err(MultisigConfigError::as_str)?; | ||
| let signature_count = multisig_signature_count_for_threshold( |
There was a problem hiding this comment.
🚨 [SECURITY] Bootstrap simulation uses a prefix-greedy quorum model
create_mock_native_multisig_sig sizes the mock approval list with multisig_signature_count_for_threshold, which accumulates owners in address order and stops at the first quorum. Consensus accepts any ascending subset of at most eight owners. A valid [2,1,1] configuration at threshold 2 is therefore modeled with one approval even though a two-approval quorum is valid and costs more gas. Conversely, eight weight-1 owners followed by a weight-100 owner at threshold 100 is a valid configuration, but this helper returns TooManySignatures before considering the heavy owner. Thus eth_simulateV1 and tempo_simulateV1 can understate intrinsic gas or reject valid bootstrap requests.
Recommended Fix:
Reuse the worst-case quorum selector used by the state-aware estimation path, or move an equivalent selector into a shared module and use it in both paths. Add regression tests for both configurations above.
2 participants
Adds request hints and state-derived recursive mock approvals for conservative multisig gas estimation, while preserving access-key simulation and fill/sign behavior.
Stacked on #7238; extracted from #4069.