Conversation
jxom
changed the title
|
This PR has been marked stale due to 7 days of inactivity. |
| /// @param configId The permanent config ID for msg.sender. It is not a config version. | ||
| /// @param threshold The new threshold. Must be between 1 and the total owner weight. | ||
| /// @param owners The new sorted owner signature types, key IDs, and weights. | ||
| function updateMultisigConfig( |
There was a problem hiding this comment.
Nitpick: we know this is the multisig precompile, so we don't need to put "multisig" in all the names.
| account || | ||
| config_id | ||
| ) | ||
| ``` |
There was a problem hiding this comment.
Unless we have a good reason, I think owners should probably sign over the multisig's configuration as it exists at signature time along with its permanent ID. Otherwise my old signature under one config can become valid in a new one.
A classic Safe mostly handles this because of its sequential nonce. Since config changes bump the nonce, an old signature can't accidentally become valid in a new config. (It does not totally handle this because a signature for a future nonce can become valid later, but that's a much weirder thing to do)
A simple option is an incrementing config version that we bump on every update.
There was a problem hiding this comment.
Since we removed the concept of config_id don't think this applies directly anymore.
But won't the owners also have to authorize the config change, in which case they might still want their signatures to be valid?
There was a problem hiding this comment.
still a bit weird no? i sign a tx back when it's 2-2 (say), then parallel nonce sign a tx making the multisig a 1-2, and depending on how it's ordered we could end up with my sig being sufficient?
tempoxyz-cyclops-bot
left a comment
tempoxyz-cyclops-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
Found 1 actionable issue in the draft TIP text. The issue is inline on the affected invariant wording.
Reviewer Callouts
- ⚡ Gas accounting before activation:
tips/tip-1061.md:529-541andtips/tip-1061.md:943-945leave gas accounting as a TODO. Before implementation/activation, the gas schedule should deterministically price up to 10 owner verifications and large WebAuthn approvals to avoid CPU/DoS underpricing. - ⚡ Optional-tail decoder disambiguation:
tips/tip-1061.md:150-159adds a second optional transaction tail. Future implementation should spell out and test the exactkey_authorizationvs.multisig_initRLP-shape disambiguation so malformed tails cannot be accepted differently by different clients. - ⚡ Signature byte parsing precedence:
tips/tip-1061.md:170-183reserves outer signature byte0x05for multisig. Future decoder changes should preserve existing raw 65-byte secp256k1 precedence so legacy signatures whose first byte is0x05are not misclassified. - ⚡ Top-level precompile frame enforcement:
tips/tip-1061.md:657-675andtips/tip-1061.md:737-770requireupdateMultisigConfigto run only in a protocol-created top-level frame. The eventual precompile implementation will need explicit call-depth/frame context to enforce this exactly.
| ### Signature Verifier Restrictions | ||
|
|
||
| The TIP-1020 signature verifier precompile remains a stateless primitive signature verifier. | ||
|
|
||
| This TIP updates TIP-1020 by adding `TempoSignature::Multisig` to the rejected stateful signature forms. | ||
|
|
||
| Signature verifier rules: | ||
|
|
||
| - `ISignatureVerifier.recover` MUST reject `TempoSignature::Multisig` with `InvalidFormat()`. | ||
| - `ISignatureVerifier.verify` MUST reject `TempoSignature::Multisig` with `InvalidFormat()`. | ||
| - Multisig authorization MUST be performed by transaction validation, not by the signature verifier precompile. |
There was a problem hiding this comment.
So a native multisig cannot sign messages at all? Safes have a mechanism for this, seems like we might need parity here.
There was a problem hiding this comment.
We just need to add a verify function to SignatureVerifier, then I think it should be gtg. We can do in a follow up TIP for T9
| | `✗` | `✔︎` | `multisig_init` | | ||
| | `✔︎` | `✔︎` | `key_authorization, multisig_init` | | ||
|
|
||
| - `InitMultisig` is encoded as `rlp([threshold, owners])`. |
There was a problem hiding this comment.
| - `InitMultisig` is encoded as `rlp([threshold, owners])`. | |
| - `InitMultisig` is encoded as `rlp([tag, threshold, owners])`. |
Should we not add a tag here so it is unambiguous whether to try to parse as key_authorization or multisig_init ?
|
This PR has been marked stale due to 7 days of inactivity. |
|
This PR has been marked stale due to 7 days of inactivity. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ff9b0bf8b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| 11. **Native multisig sender.** A transaction from a native multisig account MUST use `TempoSignature::Multisig` as the outer signature. | ||
|
|
||
| 12. **Top-level multisig signatures.** `TempoSignature::Multisig` MUST be accepted only as the outer transaction signature. |
There was a problem hiding this comment.
This invariant says TempoSignature::Multisig is accepted only as the outer transaction signature, but the main owner-approval rules explicitly allow nested multisig owner approvals and recursively validate them. Taken literally, implementers following the invariants would reject every nested multisig approval, breaking the parent -> child -> grandchild authorization path and the nested-signature test cases defined earlier in the TIP.
Useful? React with 👍 / 👎.
|
|
||
| 19. **No config update signature.** Config updates MUST NOT accept an additional signature parameter. | ||
|
|
||
| ## Copyright |
|
|
||
| Canonical EVM multisigs are generally contract accounts with an owner set, owner weights, and a threshold. Tempo can provide the same core threshold-control model through native account and transaction validation, without requiring a contract wallet deployment. | ||
|
|
||
| ## Specification |
There was a problem hiding this comment.
remove? none of the other TIPs have this.
| `derive_multisig_account(init) == signature.account`. | ||
| - Nested multisig owner approvals MUST NOT carry `init`; nested multisig accounts MUST already be initialized. | ||
|
|
||
| Flat M-of-N multisigs are represented by assigning every owner weight `1` and setting `threshold` to `M`. Because `threshold`, owner weights, and the owner count are `u8`, flat M-of-N configs can express thresholds up to 255 and owner counts up to 255. |
There was a problem hiding this comment.
are we worried at all about the validation work of that many sigs (times 3 layers)? this is basically unpaid at the txpool level right?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c8e3f97ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| TempoSignature::default() |
There was a problem hiding this comment.
When bootstrap is carried by key_authorization.signature, the request uses keyId for the newly authorized key and does not set the top-level multisigInit; none of these branches examines key_id, so this fallback creates a primitive outer signature. Even with the simulation sentinel, validation still requires a sidecar bootstrap to have a V2 keychain outer signature, causing both eth_simulateV1 and the delegating tempo_simulateV1 path to reject this valid authorize-and-use flow. Construct the keychain mock from keyId here as try_into_tempo_tx_env does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0963527edf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for owner in &config.owners { | ||
| if self.read_stored_owner_weight(account, owner.owner)? != owner.weight { | ||
| return Err(NativeMultisigError::invalid_config().into()); |
There was a problem hiding this comment.
When T11 starts from custom-genesis or pre-existing storage containing an otherwise valid ordered config plus a nonzero owner_weights[account][attacker] row, this validation passes because it checks direct weights only for owners present in config.owners. NativeMultisigAuthConfig::owner_weight later trusts the attacker's direct row without checking ordered-list membership, allowing an unconfigured signer to contribute weight. Fresh evidence beyond the earlier stored-config comment is that the new complete-config validation still cannot detect these extraneous direct-weight rows; authorization must establish membership from the ordered config or activation must sanitize them.
Useful? React with 👍 / 👎.
| let requires_native_multisig_state = outer_multisig_signature.is_some() | ||
| || key_authorization_multisig_signature.is_some() | ||
| || key_authorization_key_id.is_some() | ||
| || tx.has_fee_payer_signature() | ||
| || has_authorization_list; |
There was a problem hiding this comment.
When a custom genesis or pre-T11 state contains a registered multisig account with bytecode or an EIP-7702 delegation and a valid AccountKeychain key, an ordinary access-key transaction has only an outer keychain signature and therefore leaves requires_native_multisig_state false. The later caller code/delegation check is never reached, so the key executes as the code-bearing multisig account despite the no-code invariant; include keychain senders in the registry/code validation path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59d62306a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| impl NativeMultisig { | ||
| /// Creates the EVM precompile for this type. | ||
| pub fn create_precompile(env: &PrecompileEnv) -> DynPrecompile { | ||
| tempo_precompile!("NativeMultisig", env: env, |input| { Self::new() }) |
There was a problem hiding this comment.
When updateConfig is invoked through DELEGATECALL, this generic precompile wrapper rejects the call before ABI dispatch and returns DelegateCallNotAllowed, while TIP-1061 explicitly requires the UnauthorizedCaller error for an indirect configuration update. Contracts that decode INativeMultisig.UnauthorizedCaller therefore receive an unexpected selector; the native multisig wrapper needs to preserve the specified error for this path.
Useful? React with 👍 / 👎.
# Conflicts: # crates/transaction-pool/src/maintain.rs # crates/transaction-pool/src/tempo_pool.rs
Moves existing primitive and keychain signature gas calculations out of `handler.rs` into a dedicated internal module without changing gas values or public behavior. This leaves the handler focused on orchestration and makes additional signature schemes easier to add. Extracted while reviewing tempoxyz#4069.
# Conflicts: # crates/revm/src/handler.rs # crates/revm/src/handler/tests.rs # crates/revm/src/signature_gas.rs # crates/revm/src/tx.rs # crates/transaction-pool/src/tempo_pool.rs
Marks Reth `eth_simulateV1` transactions as simulations when their `WithEncoded` wrapper carries the empty envelope used by the block-simulation pipeline. Applies the same rule to envelope and bare AA conversions while preserving each transaction’s replay and channel identity. Extracted while reviewing tempoxyz#4069.
|
continued in a PR stack: #7242 |
| --- | ||
|
|
||
| # Specification | ||
|
|
There was a problem hiding this comment.
could we add some text here explaining at a high-level. basically above you've explained what the idea is, and the threat model etc. next thing i read is types and limits and bootstrap. some more orientation for the humans still reading this manually could be useful.
8 participants
Adds a TIP for native multisig accounts using weighted threshold primitive owner signatures. The draft defines stable derived account identity, bootstrap validation, config updates through the multisig precompile, and consensus validation rules for current owner and threshold authorization.