-
Notifications
You must be signed in to change notification settings - Fork 201
feat(l1): cap 7702-delegated EOAs at 1 in-flight mempool tx #6630
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 4 commits
b4039af
5c0c53a
5f60d95
93414ea
fccaa73
c42e2be
7c68254
50f59d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,8 +52,8 @@ pub mod vm; | |
|
|
||
| use ::tracing::{debug, error, info, instrument, warn}; | ||
| use constants::{ | ||
| AMSTERDAM_MAX_INITCODE_SIZE, MAX_INITCODE_SIZE, MAX_TRANSACTION_DATA_SIZE, | ||
| POST_OSAKA_GAS_LIMIT_CAP, | ||
| AMSTERDAM_MAX_INITCODE_SIZE, DEFAULT_DELEGATED_SENDER_CAP, MAX_INITCODE_SIZE, | ||
| MAX_TRANSACTION_DATA_SIZE, POST_OSAKA_GAS_LIMIT_CAP, | ||
| }; | ||
| use error::MempoolError; | ||
| use error::{ChainError, InvalidBlockError}; | ||
|
|
@@ -68,7 +68,8 @@ use ethrex_common::types::block_execution_witness::ExecutionWitness; | |
| use ethrex_common::types::fee_config::FeeConfig; | ||
| use ethrex_common::types::{ | ||
| AccountInfo, AccountState, AccountUpdate, Block, BlockHash, BlockHeader, BlockNumber, | ||
| ChainConfig, Code, Receipt, Transaction, WrappedEIP4844Transaction, validate_block_body, | ||
| ChainConfig, Code, EIP7702_DELEGATION_CODE_LEN, Receipt, Transaction, | ||
| WrappedEIP4844Transaction, is_eip7702_delegation, validate_block_body, | ||
| }; | ||
| use ethrex_common::types::{ELASTICITY_MULTIPLIER, P2PTransaction}; | ||
| use ethrex_common::types::{Fork, MempoolTransaction}; | ||
|
|
@@ -231,6 +232,13 @@ pub struct BlockchainOptions { | |
| /// warmer thread and the executor. Set to false (via `--no-precompile-cache`) to | ||
| /// disable the cache for benchmarking purposes. | ||
| pub precompile_cache_enabled: bool, | ||
| /// Maximum number of pending transactions an EIP-7702 delegated EOA may | ||
| /// keep in the mempool. Defaults to [`DEFAULT_DELEGATED_SENDER_CAP`]. | ||
| /// | ||
| /// Delegated EOAs are held to a tighter cap than regular senders because | ||
| /// their delegate contract can be invoked to act on behalf of multiple | ||
| /// identities, amplifying the spam surface of a single signer. | ||
| pub delegated_sender_cap: u64, | ||
| } | ||
|
|
||
| impl Default for BlockchainOptions { | ||
|
|
@@ -242,6 +250,7 @@ impl Default for BlockchainOptions { | |
| max_blobs_per_block: None, | ||
| precompute_witnesses: false, | ||
| precompile_cache_enabled: true, | ||
| delegated_sender_cap: DEFAULT_DELEGATED_SENDER_CAP, | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -2486,7 +2495,7 @@ impl Blockchain { | |
|
|
||
| let maybe_sender_acc_info = self.storage.get_account_info(header_no, sender).await?; | ||
|
|
||
| if let Some(sender_acc_info) = maybe_sender_acc_info { | ||
| let sender_code_hash = if let Some(sender_acc_info) = &maybe_sender_acc_info { | ||
| if nonce < sender_acc_info.nonce || nonce == u64::MAX { | ||
| return Err(MempoolError::NonceTooLow); | ||
| } | ||
|
|
@@ -2498,10 +2507,11 @@ impl Blockchain { | |
| if tx_cost > sender_acc_info.balance { | ||
| return Err(MempoolError::NotEnoughBalance); | ||
| } | ||
| sender_acc_info.code_hash | ||
| } else { | ||
| // An account that is not in the database cannot possibly have enough balance to cover the transaction cost | ||
| return Err(MempoolError::NotEnoughBalance); | ||
| } | ||
| }; | ||
|
|
||
| // Check the nonce of pendings TXs in the mempool from the same sender | ||
| // If it exists check if the new tx has higher fees | ||
|
|
@@ -2514,9 +2524,53 @@ impl Blockchain { | |
| return Err(MempoolError::InvalidChainId(config.chain_id)); | ||
| } | ||
|
|
||
| // EIP-7702 delegated EOAs are capped at a tighter number of pending | ||
| // transactions because their delegate contract can be invoked to act | ||
| // on behalf of multiple identities. Replacements (`tx_to_replace_hash` | ||
| // is `Some`) bypass the cap: they swap a slot rather than consuming a | ||
| // new one. | ||
| // | ||
| // TODO: once an atomic version of the per-sender pending-tx cap lands | ||
| // (mirroring the count + insert pair under the mempool write lock), | ||
| // move the effective-cap computation to be passed into | ||
| // `Mempool::add_transaction` and enforce the check there to close the | ||
| // TOCTOU window between this read and insertion. | ||
| if tx_to_replace_hash.is_none() && self.is_sender_delegated(sender_code_hash)? { | ||
| let cap = self.options.delegated_sender_cap; | ||
| let pending = self.mempool.pending_tx_count_for_sender(sender)?; | ||
| if pending >= cap { | ||
| return Err(MempoolError::MaxDelegatedPendingTxsExceeded(cap)); | ||
| } | ||
|
Comment on lines
+2585
to
+2590
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/blockchain/blockchain.rs
Line: 2538-2543
Comment:
**`delegated_sender_cap = 0` silently blocks every delegated-sender tx**
When `delegated_sender_cap` is set to `0`, the condition `pending >= cap` evaluates to `0 >= 0 = true` for the very first transaction from a delegated EOA — so *no* delegated sender can ever enter the mempool. This is probably unintentional for operators who set the value thinking "0 means unlimited" (a common convention), but the code treats it as "ban all delegated senders." There is no guard, clamp, or documentation in the CLI help or doc-comment that explains this edge case. If the intent is that `0` means "no delegated senders allowed," that should be explicitly called out in the `--mempool.delegated-sender-cap` help text.
How can I resolve this? If you propose a fix, please make it concise.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } | ||
|
Comment on lines
+2580
to
+2591
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| Ok(tx_to_replace_hash) | ||
| } | ||
|
|
||
| /// Returns `true` when the account identified by `code_hash` is an | ||
| /// EIP-7702 delegated EOA (its code is exactly `0xef0100 || address`). | ||
| /// | ||
| /// Uses a length pre-check via [`Store::get_code_metadata`] so that the | ||
| /// full bytecode is only fetched when the code length matches the | ||
| /// delegation designation (23 bytes). | ||
| fn is_sender_delegated(&self, code_hash: H256) -> Result<bool, MempoolError> { | ||
| let Some(metadata) = self.storage.get_code_metadata(code_hash)? else { | ||
| return Ok(false); | ||
| }; | ||
| if metadata.length != EIP7702_DELEGATION_CODE_LEN as u64 { | ||
| return Ok(false); | ||
| } | ||
| let Some(code) = self.storage.get_account_code(code_hash)? else { | ||
| // The metadata table claimed the code exists at the expected | ||
| // length but the code table is missing it — surface this as a | ||
| // store error rather than silently treating the sender as | ||
| // non-delegated. | ||
| return Err(MempoolError::StoreError(StoreError::Custom(format!( | ||
| "code metadata present for {code_hash:?} but bytecode missing" | ||
| )))); | ||
| }; | ||
| Ok(is_eip7702_delegation(&code.bytecode)) | ||
| } | ||
|
|
||
| /// Marks the node's chain as up to date with the current chain | ||
| /// Once the initial sync has taken place, the node will be considered as sync | ||
| pub fn set_synced(&self) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -423,6 +423,18 @@ impl Mempool { | |
| Ok(pool_lock.len() as u64) | ||
| } | ||
|
|
||
| /// Returns the number of pending transactions in the pool for `sender`. | ||
| /// | ||
| /// Used by per-sender admission caps (see [`Blockchain::validate_transaction`]). | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| pub fn pending_tx_count_for_sender(&self, sender: Address) -> Result<u64, MempoolError> { | ||
| let inner = self.read()?; | ||
| let count = inner | ||
| .txs_by_sender_nonce | ||
| .range((sender, 0)..=(sender, u64::MAX)) | ||
| .count(); | ||
| Ok(count as u64) | ||
| } | ||
|
|
||
| pub fn contains_sender_nonce( | ||
| &self, | ||
| sender: Address, | ||
|
|
@@ -574,3 +586,75 @@ pub fn transaction_intrinsic_gas( | |
|
|
||
| Ok(gas) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use ethrex_common::types::{EIP1559Transaction, TxKind}; | ||
|
|
||
| const TEST_MEMPOOL_MAX_SIZE: usize = 64; | ||
|
|
||
| fn mempool_tx(sender: Address, nonce: u64) -> (H256, MempoolTransaction) { | ||
| let tx = EIP1559Transaction { | ||
| nonce, | ||
| max_priority_fee_per_gas: 1, | ||
| max_fee_per_gas: 1, | ||
| gas_limit: 21_000, | ||
| to: TxKind::Call(Address::from_low_u64_be(1)), | ||
| value: U256::zero(), | ||
| data: Default::default(), | ||
| access_list: Default::default(), | ||
| ..Default::default() | ||
| }; | ||
| let tx = Transaction::EIP1559Transaction(tx); | ||
| // Fabricate a unique hash per (sender, nonce) — `add_transaction` does | ||
| // not recompute, it stores by the hash we pass in. We avoid collisions | ||
| // with random bytes derived from the inputs. | ||
| let mut bytes = [0u8; 32]; | ||
| bytes[..20].copy_from_slice(sender.as_bytes()); | ||
| bytes[24..].copy_from_slice(&nonce.to_be_bytes()); | ||
| let hash = H256::from(bytes); | ||
| let mempool_tx = MempoolTransaction::new(tx, sender); | ||
| (hash, mempool_tx) | ||
| } | ||
|
|
||
| #[test] | ||
| fn pending_tx_count_isolates_senders() { | ||
| let mempool = Mempool::new(TEST_MEMPOOL_MAX_SIZE); | ||
| let alice = Address::from_low_u64_be(0xA); | ||
| let bob = Address::from_low_u64_be(0xB); | ||
|
|
||
| for nonce in 0..3 { | ||
| let (hash, tx) = mempool_tx(alice, nonce); | ||
| mempool.add_transaction(hash, alice, tx).unwrap(); | ||
| } | ||
| let (hash, tx) = mempool_tx(bob, 0); | ||
| mempool.add_transaction(hash, bob, tx).unwrap(); | ||
|
|
||
| assert_eq!(mempool.pending_tx_count_for_sender(alice).unwrap(), 3); | ||
| assert_eq!(mempool.pending_tx_count_for_sender(bob).unwrap(), 1); | ||
| assert_eq!( | ||
| mempool | ||
| .pending_tx_count_for_sender(Address::from_low_u64_be(0xC)) | ||
| .unwrap(), | ||
| 0 | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn pending_tx_count_decrements_on_remove() { | ||
| let mempool = Mempool::new(TEST_MEMPOOL_MAX_SIZE); | ||
| let sender = Address::from_low_u64_be(0xA); | ||
|
|
||
| let (hash0, tx0) = mempool_tx(sender, 0); | ||
| let (hash1, tx1) = mempool_tx(sender, 1); | ||
| mempool.add_transaction(hash0, sender, tx0).unwrap(); | ||
| mempool.add_transaction(hash1, sender, tx1).unwrap(); | ||
| assert_eq!(mempool.pending_tx_count_for_sender(sender).unwrap(), 2); | ||
|
|
||
| mempool.remove_transaction(&hash0).unwrap(); | ||
| assert_eq!(mempool.pending_tx_count_for_sender(sender).unwrap(), 1); | ||
| mempool.remove_transaction(&hash1).unwrap(); | ||
| assert_eq!(mempool.pending_tx_count_for_sender(sender).unwrap(), 0); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO describes a future state — "once an atomic version of the per-sender pending-tx cap lands… move the effective-cap computation to be passed into
Mempool::add_transactionand enforce the check there" — but the atomic version IS landing in this PR (mempool.rs:154-172does exactly that: takesmax_pending_txs_per_account: u64, checks count under the write lock, refuses if at cap).So the TODO is stale on arrival. Per the PR description, this read-lock check at
:2548-2554is deliberately kept as defense-in-depth (and to keep the existing integration tests exercising the rejection path), which is a different story than the TODO tells.Swap the TODO for a note explaining the actual rationale, e.g.:
Non-blocking, but future readers will trip on the current TODO.