feat(consensus): activate native multisig accounts - #7238
Conversation
✅ Changelog found on PR. |
935ca4e to
2988c3d
Compare
2988c3d to
dc6102c
Compare
📊 Tempo Precompiles CoverageprecompilesCoverage: 6454/11165 lines (57.81%) File details
contractsCoverage: 1/202 lines (0.50%) File details
Total: 6455/11367 lines (56.79%) |
dc6102c to
402b6ae
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 activates native multisig accounts at T11 and wires their authorization and gas accounting into transaction validation. One medium-severity subblock liveness issue and three lower-severity validation or maintenance issues are included inline.
Reviewer Callouts
- ⚡ Subblock policy: Prefer an allowlist of authorization forms whose validity cannot change within a block, so future signature types are denied by default.
- ⚡ Subblock error recovery: Document that non-fee validation failures abort payload construction; every subblock-eligible authorization mechanism must preserve that invariant.
- ⚡ Reserved namespaces:
is_valid_multisig_accountexcludes TIP-20 addresses but not the similarly prefixed ZonePortal namespace; confirm whether that omission is intentional.
50a2c67 to
c50709e
Compare
c50709e to
b12cf5a
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 activates native multisig accounts at T11 and generally implements the authorization and consensus invariants carefully. One verified medium-severity resource-exhaustion issue remains; see the inline finding.
Reviewer Callouts
- ⚡ Simulation owner-count hint (
crates/revm/src/handler.rs:3231): state-derived config gas is skipped based on the presence ofsimulation_config_owner_count, notExecutionContext::Simulation. Gate the hint on simulation context before an RPC path can set it. - ⚡ Dead alternate-account branch (
crates/revm/src/handler.rs:1424): if the caller-equality guard is relaxed later, the alternate config load is not represented in the state-dependent gas calculation. Remove the dead branch or calculate gas for the signature account. - ⚡ RPC authorization-list normalization (
crates/revm/src/handler.rs:1069): AA simulation checkstempo_authorization_list, while pre-execution can fall back to the standard authorization list. Normalize or assert the two representations to prevent gas under-estimation. - ⚡ TIP-1060 storage-credit accounting (
crates/revm/src/handler.rs:1948): bootstrap slots are written with accounting disabled but can later be cleared with accounting enabled. Confirm that minting credits for those slots is intentional.
b12cf5a to
496a290
Compare
496a290 to
bbcbc52
Compare
bbcbc52 to
341e0cc
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" |
341e0cc to
dc88db8
Compare
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This change activates TIP-1061 native multisig accounts at T11 with stateful configuration and authorization validation. The consensus-side implementation is generally careful, but one verified Medium-severity issue remains on the current head after drift.
🚨 [SECURITY] Multisig state changes leave dependent transactions stale in the pool
Severity: Medium
File: crates/transaction-pool/src/maintain.rs:123
TempoPoolUpdates::from_chain handles TIP-20, keychain, fee-manager, and TIP-403 events but ignores MultisigInitialized and MultisigConfigUpdated. Native-multisig authorization is checked against the current stored configuration and version, so updates can invalidate direct, key-authorization, and nested multisig approvals without changing the affected sender's nonce. These transactions remain eligible across nonce lanes and are repeatedly reconsidered by payload builders until generic stale eviction. Initialization can likewise invalidate transactions that name the newly registered account in a multisig-restricted role. This violates the pool-revalidation requirement at tips/tip-1061.md:321.
Recommended Fix: Decode native-multisig initialization and configuration-update logs in TempoPoolUpdates, then remove and fully revalidate every transaction whose authorization depends on the changed account. Cover direct outer signatures, multisig key-authorization signatures, nested owner nodes, newly invalid access-key IDs and keychain authorization-list authorities, and both the 2D/expiring-nonce and protocol-nonce pools.
Reviewer Callouts
- ⚡ Parallel storage-action replay: Account nonce/code-hash facts checked at
crates/revm/src/handler.rs:1292are not replay assertions. No current mutation path was found, but revisit this if payment-lane authorization rules change. - ⚡ Duplicate configuration gas:
crates/revm/src/handler.rs:1200can charge owner-configuration validation twice when both outer and key-authorization signatures are multisig for the same caller, despite loading the configuration once. - ⚡ Fee-check ordering: Key authorization and keychain authorization-list entries can trigger registry reads at
crates/revm/src/handler.rs:1092before insufficient-funds rejection. Confirm the bounded asymmetry is intentional. - ⚡ Bootstrap simulation nonce: Bootstrap nonce validation does not honor disabled nonce checks. Confirm strict failure for nonzero protocol nonces is intended for RPC estimation and pool admission.
1771bd5 to
3cc126b
Compare
3cc126b to
365c7f4
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 activates TIP-1061 native multisig accounts at T11 and integrates bootstrap, authorization, role restrictions, and gas accounting into transaction validation. The core paths are carefully implemented; one Low defense-in-depth gap remains in transaction-scoped precompile initialization.
Reviewer Callouts
- ⚡ Simulation authorization boundary:
ExecutionContext::Simulationskips native-multisig owner-quorum verification while still staging bootstrap state. Add an invariant or test proving simulation-marked transactions cannot reach committing block execution. - ⚡ State-measured intrinsic gas:
native_multisig_registry_validation_gasderives consensus intrinsic gas from live journal warm/cold state. Add a test showing a discarded transaction or access-list prewarming cannot change the subsequent transaction's measured intrinsic gas unexpectedly.
| keychain.set_tx_origin(ctx.tx.caller())?; | ||
|
|
||
| let mut multisig = NativeMultisig::new(); | ||
| multisig.set_tx_origin(ctx.tx.caller())?; |
There was a problem hiding this comment.
🛡️ [DEFENSE-IN-DEPTH] Reset the native-multisig bootstrap guard at transaction start
seed_precompile_tx_context resets the keychain transaction_key and seeds NativeMultisig.tx_origin, but does not reset the adjacent transient bootstrapped_account slot. store_initial_config writes that slot, and update_multisig_config rejects updates when it equals the caller. Current revm finalization clears transient storage, so this is fail-safe today; however, a driver or future refactor that preserves transient state across transactions would incorrectly block a later owner rotation and could create pool/execution divergence.
Recommended Fix:
Expose a crate-private setter and set bootstrapped_account to Address::ZERO here alongside the other transaction-scoped resets. Add a two-transaction regression test using one EVM instance, including the pool-validation/discard path.
2 participants
Activates native multisig accounts at T11 with registration, bootstrap writes, stateful validation, role restrictions, and complete gas accounting. Enforces the no-code invariant across direct, access-key, and nested authorization paths.
Stacked on #7237; extracted from #4069.