-
Notifications
You must be signed in to change notification settings - Fork 201
feat(l1): add TxOrigin enum to thread RPC vs P2P submission through admission #6608
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 1 commit
0aa0d3a
eeb3b89
7622957
3d026d1
44df4a8
da5bb37
1ec1779
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 |
|---|---|---|
|
|
@@ -92,7 +92,7 @@ use ethrex_vm::backends::CachingDatabase; | |
| use ethrex_vm::backends::levm::LEVM; | ||
| use ethrex_vm::backends::levm::db::DatabaseLogger; | ||
| use ethrex_vm::{BlockExecutionResult, DynVmDatabase, Evm, EvmError}; | ||
| use mempool::Mempool; | ||
| use mempool::{Mempool, TxOrigin}; | ||
| use payload::PayloadOrTask; | ||
| use rustc_hash::{FxHashMap, FxHashSet}; | ||
| use std::collections::hash_map::Entry; | ||
|
|
@@ -231,6 +231,11 @@ 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, | ||
| /// If true, locally-submitted transactions are subject to the same admission | ||
| /// policies as P2P-received ones (no exemptions). Defaults to `false`, meaning | ||
| /// `TxOrigin::Local` transactions bypass operator-friendly gates such as the | ||
| /// min-tip floor. | ||
| pub nolocals: bool, | ||
| } | ||
|
|
||
| impl Default for BlockchainOptions { | ||
|
|
@@ -242,6 +247,7 @@ impl Default for BlockchainOptions { | |
| max_blobs_per_block: None, | ||
| precompute_witnesses: false, | ||
| precompile_cache_enabled: true, | ||
| nolocals: false, | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -2308,12 +2314,39 @@ impl Blockchain { | |
| Ok(()) | ||
| } | ||
|
|
||
| /// Add a blob transaction and its blobs bundle to the mempool checking that the transaction is valid | ||
| /// Add a P2P-received blob transaction and its blobs bundle to the mempool. | ||
| /// | ||
| /// The transaction is validated as `TxOrigin::External`, so it is subject to | ||
| /// all admission policies (including operator-configurable floors). | ||
| #[cfg(feature = "c-kzg")] | ||
| pub async fn add_blob_transaction_to_pool( | ||
| &self, | ||
| transaction: EIP4844Transaction, | ||
| blobs_bundle: BlobsBundle, | ||
| ) -> Result<H256, MempoolError> { | ||
| self.add_blob_transaction_to_pool_with_origin(transaction, blobs_bundle, TxOrigin::External) | ||
| .await | ||
| } | ||
|
|
||
| /// Add a locally-submitted blob transaction (e.g. via `eth_sendRawTransaction`) | ||
| /// to the mempool. The transaction is validated as `TxOrigin::Local`, so it may | ||
| /// bypass operator-friendly admission gates unless `--mempool.nolocals` is set. | ||
| #[cfg(feature = "c-kzg")] | ||
| pub async fn add_local_blob_transaction_to_pool( | ||
| &self, | ||
| transaction: EIP4844Transaction, | ||
| blobs_bundle: BlobsBundle, | ||
| ) -> Result<H256, MempoolError> { | ||
| self.add_blob_transaction_to_pool_with_origin(transaction, blobs_bundle, TxOrigin::Local) | ||
| .await | ||
| } | ||
|
|
||
| #[cfg(feature = "c-kzg")] | ||
| async fn add_blob_transaction_to_pool_with_origin( | ||
| &self, | ||
| transaction: EIP4844Transaction, | ||
| blobs_bundle: BlobsBundle, | ||
| origin: TxOrigin, | ||
| ) -> Result<H256, MempoolError> { | ||
| let fork = self.current_fork().await?; | ||
|
|
||
|
|
@@ -2331,7 +2364,10 @@ impl Blockchain { | |
| let sender = transaction.sender(&NativeCrypto)?; | ||
|
|
||
| // Validate transaction | ||
| if let Some(tx_to_replace) = self.validate_transaction(&transaction, sender).await? { | ||
| if let Some(tx_to_replace) = self | ||
| .validate_transaction(&transaction, sender, origin) | ||
| .await? | ||
| { | ||
| self.remove_transaction_from_pool(&tx_to_replace)?; | ||
| } | ||
|
|
||
|
|
@@ -2343,10 +2379,33 @@ impl Blockchain { | |
| Ok(hash) | ||
| } | ||
|
|
||
| /// Add a transaction to the mempool checking that the transaction is valid | ||
| /// Add a P2P-received transaction to the mempool. | ||
| /// | ||
| /// The transaction is validated as `TxOrigin::External`, so it is subject to | ||
| /// all admission policies (including operator-configurable floors). | ||
| pub async fn add_transaction_to_pool( | ||
| &self, | ||
| transaction: Transaction, | ||
| ) -> Result<H256, MempoolError> { | ||
| self.add_transaction_to_pool_with_origin(transaction, TxOrigin::External) | ||
| .await | ||
| } | ||
|
|
||
| /// Add a locally-submitted transaction (e.g. via `eth_sendRawTransaction`) to | ||
| /// the mempool. The transaction is validated as `TxOrigin::Local`, so it may | ||
| /// bypass operator-friendly admission gates unless `--mempool.nolocals` is set. | ||
| pub async fn add_local_transaction_to_pool( | ||
| &self, | ||
| transaction: Transaction, | ||
| ) -> Result<H256, MempoolError> { | ||
| self.add_transaction_to_pool_with_origin(transaction, TxOrigin::Local) | ||
| .await | ||
| } | ||
|
|
||
| async fn add_transaction_to_pool_with_origin( | ||
| &self, | ||
| transaction: Transaction, | ||
| origin: TxOrigin, | ||
| ) -> Result<H256, MempoolError> { | ||
| // Blob transactions should be submitted via add_blob_transaction along with the corresponding blobs bundle | ||
| if matches!(transaction, Transaction::EIP4844Transaction(_)) { | ||
|
|
@@ -2358,7 +2417,10 @@ impl Blockchain { | |
| } | ||
| let sender = transaction.sender(&NativeCrypto)?; | ||
| // Validate transaction | ||
| if let Some(tx_to_replace) = self.validate_transaction(&transaction, sender).await? { | ||
| if let Some(tx_to_replace) = self | ||
| .validate_transaction(&transaction, sender, origin) | ||
| .await? | ||
| { | ||
| self.remove_transaction_from_pool(&tx_to_replace)?; | ||
| } | ||
|
|
||
|
|
@@ -2413,12 +2475,28 @@ impl Blockchain { | |
| 5. Ensure the transactor is able to add a new transaction. The number of transactions sent by an account may be limited by a certain configured value | ||
|
|
||
| */ | ||
| /// Returns the hash of the transaction to replace in case the nonce already exists | ||
| /// Returns the hash of the transaction to replace in case the nonce already exists. | ||
| /// | ||
| /// `origin` records whether the transaction came in via RPC (`TxOrigin::Local`) | ||
| /// or P2P (`TxOrigin::External`). Some admission gates are skipped for local | ||
| /// transactions unless the operator opts out via `--mempool.nolocals` | ||
| /// (see `BlockchainOptions::nolocals`). | ||
| pub async fn validate_transaction( | ||
| &self, | ||
| tx: &Transaction, | ||
| sender: Address, | ||
| origin: TxOrigin, | ||
| ) -> Result<Option<H256>, MempoolError> { | ||
| // `locals_exempt` gates origin-aware exemptions. When the operator sets | ||
| // `--mempool.nolocals`, locally-submitted txs are treated like external | ||
| // ones for admission purposes. | ||
| let _locals_exempt = !self.options.nolocals && origin == TxOrigin::Local; | ||
|
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.
Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/blockchain/blockchain.rs
Line: 2493
Comment:
**Unused variable `_locals_exempt` silences dead-code lint**
`_locals_exempt` is computed but never read; the leading underscore is the only thing preventing a compiler warning. While the PR description explains this is intentional scaffolding for PR #6604, the dead binding means the expression `!self.options.nolocals && origin == TxOrigin::Local` is never exercised in any code path today. If the follow-up PR forgets to remove the underscore when wiring the `if !_locals_exempt` guard, the exemption will silently remain dead. Prefer an explicit `#[allow(unused_variables)]` with an `// until #6604` annotation, or a `let _ = ...;` assignment, to make the intentional deferral even more visible to reviewers.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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. |
||
|
|
||
| // TODO(#6604): when the min-tip floor lands (PR #6604 adds | ||
| // `BlockchainOptions::min_tip_wei` and a `gas_tip_cap < min_tip_wei` | ||
| // rejection), wrap the floor check with `if !_locals_exempt { ... }` so | ||
| // that `TxOrigin::Local` transactions bypass the floor by default. | ||
|
|
||
| let nonce = tx.nonce(); | ||
|
|
||
| if matches!(tx, &Transaction::PrivilegedL2Transaction(_)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -606,18 +606,21 @@ impl RpcHandler for SendRawTransactionRequest { | |
| } | ||
|
|
||
| async fn handle(&self, context: RpcApiContext) -> Result<Value, RpcErr> { | ||
| // RPC-submitted transactions are tagged as `TxOrigin::Local` so they may | ||
| // bypass admission gates (such as the min-tip floor) intended to protect | ||
| // against P2P spam. See `Blockchain::add_local_transaction_to_pool`. | ||
| let hash = if let SendRawTransactionRequest::EIP4844(wrapped_blob_tx) = self { | ||
| context | ||
| .blockchain | ||
| .add_blob_transaction_to_pool( | ||
| .add_local_blob_transaction_to_pool( | ||
| wrapped_blob_tx.tx.clone(), | ||
| wrapped_blob_tx.blobs_bundle.clone(), | ||
| ) | ||
|
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. |
||
| .await | ||
| } else { | ||
| context | ||
| .blockchain | ||
| .add_transaction_to_pool(self.to_transaction()) | ||
| .add_local_transaction_to_pool(self.to_transaction()) | ||
| .await | ||
| }?; | ||
| serde_json::to_value(format!("{hash:#x}")) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -91,6 +91,11 @@ Node options: | |
| [env: ETHREX_MEMPOOL_MAX_SIZE=] | ||
| [default: 10000] | ||
|
|
||
| --mempool.nolocals | ||
| Disable admission exemptions for locally-submitted transactions. When set, RPC-submitted txs are subject to the same admission gates (e.g. min-tip floor) as P2P-received ones. | ||
|
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. |
||
|
|
||
| [env: ETHREX_MEMPOOL_NOLOCALS=] | ||
|
|
||
| --precompute-witnesses | ||
| Once synced, computes execution witnesses upon receiving newPayload messages and stores them in local storage | ||
|
|
||
|
|
@@ -317,6 +322,11 @@ Node options: | |
| [env: ETHREX_MEMPOOL_MAX_SIZE=] | ||
| [default: 10000] | ||
|
|
||
| --mempool.nolocals | ||
| Disable admission exemptions for locally-submitted transactions. When set, RPC-submitted txs are subject to the same admission gates (e.g. min-tip floor) as P2P-received ones. | ||
|
|
||
| [env: ETHREX_MEMPOOL_NOLOCALS=] | ||
|
|
||
| P2P options: | ||
| --bootnodes <BOOTNODE_LIST>... | ||
| Comma separated enode URLs for P2P discovery bootstrap. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ use ethrex_blockchain::constants::{ | |
| TX_INIT_CODE_WORD_GAS_COST, | ||
| }; | ||
| use ethrex_blockchain::error::MempoolError; | ||
| use ethrex_blockchain::mempool::{Mempool, transaction_intrinsic_gas}; | ||
| use ethrex_blockchain::mempool::{Mempool, TxOrigin, transaction_intrinsic_gas}; | ||
| use ethrex_crypto::NativeCrypto; | ||
| use rustc_hash::FxHashMap; | ||
|
|
||
|
|
@@ -242,7 +242,7 @@ async fn transaction_with_big_init_code_in_shanghai_fails() { | |
| }; | ||
|
|
||
| let tx = Transaction::EIP1559Transaction(tx); | ||
| let validation = blockchain.validate_transaction(&tx, Address::random()); | ||
| let validation = blockchain.validate_transaction(&tx, Address::random(), TxOrigin::External); | ||
| assert!(matches!( | ||
| validation.await, | ||
| Err(MempoolError::TxMaxInitCodeSizeError) | ||
|
|
@@ -269,7 +269,7 @@ async fn transaction_with_gas_limit_higher_than_of_the_block_should_fail() { | |
| }; | ||
|
|
||
| let tx = Transaction::EIP1559Transaction(tx); | ||
| let validation = blockchain.validate_transaction(&tx, Address::random()); | ||
| let validation = blockchain.validate_transaction(&tx, Address::random(), TxOrigin::External); | ||
| assert!(matches!( | ||
| validation.await, | ||
| Err(MempoolError::TxGasLimitExceededError) | ||
|
|
@@ -296,7 +296,7 @@ async fn transaction_with_priority_fee_higher_than_gas_fee_should_fail() { | |
| }; | ||
|
|
||
| let tx = Transaction::EIP1559Transaction(tx); | ||
| let validation = blockchain.validate_transaction(&tx, Address::random()); | ||
| let validation = blockchain.validate_transaction(&tx, Address::random(), TxOrigin::External); | ||
| assert!(matches!( | ||
| validation.await, | ||
| Err(MempoolError::TxTipAboveFeeCapError) | ||
|
|
@@ -323,7 +323,7 @@ async fn transaction_with_gas_limit_lower_than_intrinsic_gas_should_fail() { | |
| }; | ||
|
|
||
| let tx = Transaction::EIP1559Transaction(tx); | ||
| let validation = blockchain.validate_transaction(&tx, Address::random()); | ||
| let validation = blockchain.validate_transaction(&tx, Address::random(), TxOrigin::External); | ||
| assert!(matches!( | ||
| validation.await, | ||
| Err(MempoolError::TxIntrinsicGasCostAboveLimitError) | ||
|
|
@@ -350,7 +350,7 @@ async fn transaction_with_blob_base_fee_below_min_should_fail() { | |
| }; | ||
|
|
||
| let tx = Transaction::EIP4844Transaction(tx); | ||
| let validation = blockchain.validate_transaction(&tx, Address::random()); | ||
| let validation = blockchain.validate_transaction(&tx, Address::random(), TxOrigin::External); | ||
| assert!(matches!( | ||
| validation.await, | ||
| Err(MempoolError::TxBlobBaseFeeTooLowError) | ||
|
|
@@ -467,3 +467,71 @@ fn blobs_bundle_insert_and_remove() { | |
| vec![None] | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn validate_transaction_accepts_both_origins() { | ||
| // Threading check: `validate_transaction` must accept both origins. With no | ||
| // origin-gated rules yet wired on `main`, Local and External should still | ||
| // produce the same downstream error for an identical fixture (proving that | ||
| // adding the parameter did not accidentally diverge the validation paths). | ||
| let (config, header) = build_basic_config_and_header(false, false); | ||
| let store = setup_storage(config, header).await.expect("Storage setup"); | ||
| let blockchain = Blockchain::default_with_store(store); | ||
|
|
||
| let tx = EIP1559Transaction { | ||
| nonce: 3, | ||
| max_priority_fee_per_gas: 0, | ||
| max_fee_per_gas: 0, | ||
| gas_limit: 100_000_001, // forces TxGasLimitExceededError before any origin-gated rule could fire | ||
| to: TxKind::Call(Address::from_low_u64_be(1)), | ||
| value: U256::zero(), | ||
| data: Bytes::default(), | ||
| access_list: Default::default(), | ||
| ..Default::default() | ||
| }; | ||
| let tx = Transaction::EIP1559Transaction(tx); | ||
| let sender = Address::random(); | ||
|
|
||
| let local = blockchain | ||
| .validate_transaction(&tx, sender, TxOrigin::Local) | ||
| .await; | ||
| let external = blockchain | ||
| .validate_transaction(&tx, sender, TxOrigin::External) | ||
| .await; | ||
|
|
||
| assert!(matches!(local, Err(MempoolError::TxGasLimitExceededError))); | ||
| assert!(matches!( | ||
| external, | ||
| Err(MempoolError::TxGasLimitExceededError) | ||
| )); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn add_local_transaction_to_pool_routes_through_validation() { | ||
| // Threading check: the RPC entry point must route through validation. A | ||
| // transaction whose gas limit exceeds the block's must be rejected with | ||
| // `TxGasLimitExceededError`, proving that we did not accidentally bypass | ||
| // `validate_transaction` in `add_local_transaction_to_pool`. | ||
| let (config, header) = build_basic_config_and_header(false, false); | ||
| let store = setup_storage(config, header).await.expect("Storage setup"); | ||
| let blockchain = Blockchain::default_with_store(store); | ||
|
|
||
| // A canonical legacy tx (sender derivable from signature) with `gas_limit` | ||
| // beyond the test block's limit (100_000_000). Reused from the existing | ||
| // mempool fixtures (`test_filter_mempool_transactions`). | ||
| let tx = Transaction::decode_canonical(&hex::decode("f86d80843baa0c4082f618946177843db3138ae69679a54b95cf345ed759450d870aa87bee538000808360306ba0151ccc02146b9b11adf516e6787b59acae3e76544fdcd75e77e67c6b598ce65da064c5dd5aae2fbb535830ebbdad0234975cd7ece3562013b63ea18cc0df6c97d4").unwrap()).unwrap(); | ||
|
|
||
| let result = blockchain.add_local_transaction_to_pool(tx).await; | ||
| // The fixture tx has chain_id None, so it should hit NotEnoughBalance | ||
| // (sender not in storage) — same outcome as `add_transaction_to_pool`. | ||
| // The point of the assertion is that the call returns an error from | ||
| // `validate_transaction` rather than silently inserting. | ||
| assert!(result.is_err(), "local tx must be rejected by validation"); | ||
|
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. |
||
| } | ||
|
|
||
| #[test] | ||
| fn blockchain_options_default_keeps_local_exemption() { | ||
| // Default policy: locals are exempt from origin-gated rules. | ||
| let opts = ethrex_blockchain::BlockchainOptions::default(); | ||
| assert!(!opts.nolocals); | ||
| } | ||
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.
eeb3b89