feat(pool): revalidate native multisig transactions - #7240
Conversation
7f72761 to
1116aec
Compare
2a56d67 to
d0fa80d
Compare
joshieDo
changed the title
d0fa80d to
1319fda
Compare
📊 Tempo Precompiles CoverageprecompilesCoverage: 6426/11165 lines (57.55%) File details
contractsCoverage: 1/202 lines (0.50%) File details
Total: 6427/11367 lines (56.54%) |
|
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 transaction-pool revalidation for native-multisig initialization and configuration updates. One verified Medium security finding remains: initialization changes a key-authorization dependency that the new signature-tree matcher does not enumerate.
Reviewer Callouts
- ⚡ Initialization dependency coverage: Compare the pool's dependency enumeration with every
NativeMultisig::is_multisig_accountcheck in execution; signature-tree membership alone is sufficient for owner-config rotations but not for account initialization.
42fd714 to
55676f5
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 transaction-pool revalidation after initialization and configuration updates. One verified medium-severity availability finding remains after deduplication and verification.
Reviewer Callouts
- ⚡ Pool scan budget:
crates/transaction-pool/src/maintain.rs:695adds another full-pool scan to a sequential maintenance thread whose lagged canonical-state notifications are not replayed; consider indexing affected transactions or enforcing a bounded per-block maintenance budget. - ⚡ Invalidation predicate consistency:
crates/transaction-pool/src/maintain.rs:291does not include the new multisig update sets inhas_invalidation_events(), althoughis_empty()does. The dedicated path makes this harmless today, but a future generalization could silently skip multisig updates.
55676f5 to
9811211
Compare
9811211 to
96e6037
Compare
96e6037 to
6577c56
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" |
6577c56 to
eb22aba
Compare
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This change revalidates pooled transactions affected by native-multisig initialization or configuration updates. One verified medium-severity availability race remains on the current head.
Reviewer Callouts
- ⚡ Public invalidation wrapper:
evict_invalidated_transactions()passes no multisig revalidation output and can silently skip multisig updates if a production caller is added. Route affected transactions to removal or document/restrict this API. - ⚡ Maintenance write lock: The maintenance path calls
remove_transactionseven when no multisig transaction was selected, causing an unnecessary AA 2D pool write-lock acquisition every block. Guard the call with!to_revalidate.is_empty(). - ⚡ Fire-and-forget re-admission: Multisig removals are not added to
removed_this_iteration. This is safe only while this remains the final snapshot-based scan; record the hashes to preserve that invariant if later maintenance phases are added.
| .iter() | ||
| .filter(|tx| !removed_this_iteration.contains(tx.hash())), | ||
| Some(tip_timestamp.saturating_add(EVICTION_BUFFER_SECS)), | ||
| Some(&mut multisig_revalidated), |
There was a problem hiding this comment.
🚨 [SECURITY] Point-in-time multisig scans miss transactions admitted from an older state snapshot
This scan only covers transactions already present in all_txs. An AA 2D batch can finish validation against the pre-update state after this scan and then be inserted by add_transactions without any canonical-tip or state-generation check. Those stale multisig signatures remain in the pool and can be retried across payload builds, degrading proposer throughput.
Recommended Fix:
Bind stateful validation outcomes to the canonical tip hash or a monotonic state generation, and revalidate at insertion whenever that snapshot no longer matches. Apply the same barrier to maintenance reinsertion.
eb22aba to
9df0583
Compare
9df0583 to
9afb28e
Compare
9afb28e to
dadc596
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
No new actionable findings remain after verification and deduplication. The verified validation/insertion race was already reported in the prior Cyclops review; the reverted-bootstrap receipt finding was rejected because pre-checkpoint logs are retained in reverted execution results.
Reviewer Callouts
- ⚡ Non-AA EIP-7702 authority recovery (
crates/transaction-pool/src/transaction.rs:257-262): Confirm whether cached non-AA transaction environments retain signed rather than recovered authorizations. If so, each multisig initialization can trigger repeatedecrecoverwork across every pooled authorization; pre-recover authorities or enforce an authorization-list bound.
2 participants
Revalidates pooled transactions when applied or reverted configuration events affect their outer account, nested owners, or key-authorization multisig tree. Still-valid transactions return to the pool.
Stacked on #7239; extracted from #4069.