feat(primitives): add native multisig wire types - #7234
Conversation
✅ Changelog found on PR. |
|
cyclops audit fast note="extracted from #4069, check that PR for the spec" |
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This change adds bounded native-multisig wire types and a public precompile ABI while keeping execution inactive. One critical authorization bypass remains actionable on the current head after rechecking the drifted diff; details are inline.
Reviewer Callouts
- ⚡ Polymorphic signer recovery: Audit every consumer of
TempoSignature::recover_signer; multisig recovery returns a claimed account rather than cryptographically proving authority. - ⚡ Multisig verification gas: The zero-cost assumption is sound only if every carrier path is rejected before intrinsic gas; add per-approval pricing before activation.
📊 Tempo Precompiles CoverageprecompilesCoverage: 6435/10593 lines (60.75%) File details
contractsCoverage: 1/202 lines (0.50%) File details
Total: 6436/10795 lines (59.62%) |
5369c61 to
e5e7666
Compare
971ab78 to
407184c
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
There was a problem hiding this comment.
👁️ Cyclops Review
This change introduces TIP-1061 native-multisig wire types, validation helpers, and pre-activation handling. One verified High-severity consensus flaw remains: the pre-T11 activation gate covers only the outer AA signature, allowing type 0x05 through authorization-list entries and creating transaction-validity divergence between upgraded and non-upgraded nodes.
Reviewer Callouts
- ⚡ Reserved address namespaces (
crates/primitives/src/transaction/multisig.rs:450–472):InitMultisig::account()rejects only the zero address, while TIP-1061 also requires excluding virtual, active-precompile, and TIP-20 namespaces. Add those checks before bootstrap activation at T11. - ⚡ Nested signature size (
crates/primitives/src/transaction/multisig.rs:949–958): nested multisig approvals bypassMAX_MULTISIG_OWNER_SIGNATURE_BYTES; structural limits permit an encoding near 131 KB, slightly above reth's default 128 KB transaction-input limit. Confirm pool and consensus size limits are aligned before T11. - ⚡ Infallible constructors (
crates/primitives/src/transaction/multisig.rs:511–518,588–590):MultisigAddress::account()andMultisigSignature::new()panic if validation invariants are violated. Ensure the T11 precompile uses fallible construction and cannot introduce an unchecked path.
407184c to
237be9d
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
There was a problem hiding this comment.
👁️ Cyclops Review
This change adds TIP-1061 native-multisig parsing while keeping execution disabled. One verified high-severity activation-guard gap can split consensus before T11; two low-severity validation gaps are currently latent.
Reviewer Callouts
- ⚡ Nested payload bounds:
validate_shapecaps only primitive approvals individually; add a total encoded-size bound before T11 activation. - ⚡ 65-byte round-trip ambiguity: An exactly 65-byte multisig encoding serializes as
0x05but decodes as secp256k1, which is unsafe for compact database round trips. - ⚡ Deferred authorization invariants: Before enabling execution, enforce initialized-only nested signatures, owner membership, quorum, and rejection of signatures after quorum.
5ef2802 to
3ed239f
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
There was a problem hiding this comment.
👁️ Cyclops Review
This PR adds TIP-1061 native-multisig wire types, decoding, and ABI declarations while keeping execution disabled behind NativeMultisigNotActive. The activation gate prevents a live authorization exploit, but three stateless validation and construction invariants remain under-enforced.
Reviewer Callouts
- ⚡ Activation gas accounting:
crates/revm/src/signature_gas.rs:45charges zero for multisig verification. The activation change must add bounded per-owner verification gas in the same change that removesNativeMultisigNotActive. - ⚡ Unauthenticated stateless recovery: Initialized multisig recovery returns the account supplied by the payload before stateful authorization. Future pool, RPC, and subblock consumers must not treat that recovered address as authenticated until the stateful verifier succeeds.
- ⚡ Payload bounds: Nested approvals are exempt from the primitive owner-signature byte cap, while the authorization-list length is only pool-bounded. Reassess consensus-level transaction payload and verification-work bounds before activation.
- ⚡ RPC compatibility: The hand-written
TempoSignatureJSON deserializer replaces the prior derived untagged behavior for every signature type. Run compatibility coverage for existing primitive and keychain JSON forms.
3ed239f to
d718839
Compare
Defines bounded native multisig configuration and signature wire formats together with the public precompile ABI. Execution remains inactive behind an explicit validation guard; extracted from #4069.
Stacked on #7242.
Do not merge: this branch temporarily pins mpp-rs #391 through Foundry #16213. Merge both upstream PRs, restore the Foundry checkouts in
specs.ymltomaster, and rerun CI first.