Conversation
…sactions The MorphTx revalidation task runs alongside reth's own pool maintenance and both subscribe to the canonical state stream independently, so this task can observe a pool snapshot that still contains transactions the new block already executed. It read the sender's account only for the balance and discarded the nonce, so those already-executed transactions were charged against the new (already reduced) post-state balance a second time, and the sender's next, genuinely affordable transaction was evicted. Read the nonce alongside the balance and skip everything below it, as upstream's `AllTransactions::update` and go-ethereum's `demoteUnexecutables` both do. Removal used `remove_transactions_and_descendants`, which deletes every higher-nonce transaction of the sender — including plain ETH-fee transactions that are affordable on their own and only depend on the removed one through the nonce sequence. `remove_transactions` parks them instead (upstream's `remove_transaction_by_hash` calls `park_descendant_transactions`), matching what go-ethereum does by re-enqueueing its `invalids`. A failed token state read was wrapped as `TokenInfoFetchFailed` and handled like any other validation failure, so a transient read error removed a perfectly valid transaction. The rest of this task already skips on a failed state provider, L1 block info fetch or ETH balance read; token reads now follow the same rule. go-ethereum drops the transaction in this case (`executableTxFilter`, core/tx_pool.go:1690) and that is deliberately not mirrored. Also skip ahead to the newest queued notification before starting a round: a round costs one state read per transaction, so the chain can advance while it runs, and the verdicts are a pure function of the latest state. The per-round decision is extracted into `collect_removable_transactions` so it can be driven directly against a hand-built state, which is what the three new regression tests do; each was confirmed to fail before this change. Claude-Session: https://claude.ai/code/session_01PiUjd47Da71WG2BFkDQz9q
…udget
The revalidation walk applied one rolling sender budget across every MorphTx in
the pool, pending and queued alike. A transaction sitting behind a nonce gap was
therefore charged against whatever the sender's executable transactions had left
over — but the transactions filling the gap are not in the pool, so how much of
the balance is actually still owed by the time the gapped one executes is
unknown. An unrelated block was enough to evict a future-nonce transaction that
had passed admission on its own.
Stop the walk at the first nonce discontinuity, which is what upstream's
`AllTransactions::update` does ("If there's a nonce gap, we can shortcircuit,
because there's nothing to update yet"). go-ethereum reaches the same place from
the other direction: `promoteExecutables` only ever applies a per-transaction
cost check to the queue and discards `FilterF`'s `invalids`.
Nothing is lost by leaving those transactions alone: without `NO_NONCE_GAPS`
they sit in the queued sub-pool, which is exactly what reth's own stale eviction
reaps.
Claude-Session: https://claude.ai/code/session_01PiUjd47Da71WG2BFkDQz9q
`query_balance_via_system_call` mapped every error, `EVMError::Database` included, to a zero balance. A failed state read therefore came back as "this account holds no tokens" and the transaction was rejected for insufficient funds — and the `Err(EVMError::Database(e)) => Err(e)` arm in `read_token_balance_with_fallback`, which exists precisely to propagate it, was unreachable. Report the database error and leave the revert / short-return cases as a zero balance, which are genuine statements about the token. At admission a failure to even get a state provider became `TransactionValidationOutcome::Invalid`. That is a verdict on the transaction: the pool records it as known-bad and the network layer holds the sending peer responsible for something that may be perfectly valid and merely could not be checked. Route `TokenInfoFetchFailed` to `TransactionValidationOutcome::Error` instead, which discards the attempt without blaming anyone. `TokenInfoFetchFailed::token_id` becomes `Option<u16>`: the provider failure happens before any token ID is known and was reporting a hardcoded `0`, so the error read "failed to fetch token info for ID 0" for a token that had nothing to do with it. Claude-Session: https://claude.ai/code/session_01PiUjd47Da71WG2BFkDQz9q
The token-fee handler resolved the caller's ERC20 balance by building a throwaway `MorphEvm` over the raw database. That EVM carries `BlockEnv::default()` and `CfgEnv::default()` — block 0, timestamp 1, chain id 1, zero coinbase and base fee, `u64::MAX` gas limit — and `system_call_one` issues the call from `SYSTEM_ADDRESS` with a 30M gas cap. go-ethereum reads the same balance through `st.evm` (`GetAltTokenBalanceHybrid`, core/token_gas.go:43), so the call sees the real header, the real chain config, the user as `msg.sender` and a 200k cap. For any call-mode token whose `balanceOf` reads block context or `msg.sender`, the two clients were computing different balances for the same transaction — and that balance both caps `fee_limit` and becomes the `from_balance_before` the post-transfer equality check is measured against, so it decides whether the transaction is valid at all. Resolve it against the executing EVM instead. Slot mode keeps reading storage directly: there is no environment to get wrong, and an `sload` would warm a slot the deduction below is careful to leave cold. `evm_call_balance_of` now queries as the account being asked about, matching `sender := vm.AccountRef(userAddress)`, and returns a `Result` so a failed state read propagates rather than being reported as a zero balance — an I/O failure must not decide a block's contents. A revert or unusable return value stays a zero balance, which produces the same rejection go-ethereum reaches by erroring out of `buyAltTokenGas`. The receipt-field fallback in the block executor switches to `load_storage_only`: it only reads `price_ratio` and `scale`, both plain registry storage, and was spinning up a temporary EVM to resolve a balance it discards. No currently registered fee token is affected — every call-mode token on mainnet and hoodi is a FiatTokenV2_2 or OZ ERC20 whose `balanceOf` is a plain storage read — so this closes a latent divergence rather than an active one. Claude-Session: https://claude.ai/code/session_01PiUjd47Da71WG2BFkDQz9q
The pool resolved a call-mode fee token's balance through a temporary EVM built from `MorphContext::new(db, hardfork)`, which carries `BlockEnv::default()` and `CfgEnv::default()` — block 0, timestamp 1, chain id 1 — and queried it from `SYSTEM_ADDRESS` with a 30M gas cap. go-ethereum's pool builds a real `vm.BlockContext` from the head header and calls `balanceOf` as the user with a 200k cap (`pool.getBalanceFunc`, core/tx_pool.go:330). So for a token whose `balanceOf` reads block context or `msg.sender`, admission and maintenance were answering a different question than the execution layer — admitting transactions that cannot execute, or rejecting ones that would. Thread the block's `EvmEnv` through `MorphTxValidationInput` instead. The validator caches it alongside the L1 block info, built by `ConfigureEvm::evm_env` for the head, and the maintenance task builds it for each canonical tip, so both use exactly what execution would. `read_token_balance_with_fallback` now stands its EVM up in that environment and delegates to the same `evm_call_balance_of` the handler uses, leaving one implementation of the query rather than two that can drift. `evm_call` takes the context error after running the frame group. A database failure inside a frame is recorded on the context and surfaces as a halt; running the frames directly skips the step that normally converts it, so an I/O failure was indistinguishable from the token reverting — which would have silently undone the propagation this relies on. `query_erc20_balance` and `query_balance_via_system_call` are removed: they were the only remaining way to ask this question in the wrong environment. Claude-Session: https://claude.ai/code/session_01PiUjd47Da71WG2BFkDQz9q
|
Warning Review limit reachedNext included review available in 36 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge last. This branch contains #202, #203, #204 and #205 as merge parents because it builds on all of them — the shared
collect_removable_transactionsfrom #202/#203 and the unifiedevm_call_balance_offrom #205. Once those land, the diff here is just the commit at the tip. Review that commit; the rest is already under review elsewhere.The problem
#205 fixed the execution layer. The pool has the same defect.
morph_tx_validation.rsresolves a call-mode fee token's balance throughTokenFeeInfo::load_for_caller, which built a temporary EVM fromMorphContext::new(db, hardfork)—BlockEnv::default(),CfgEnv::default(), block 0, timestamp 1, chain id 1 — and queried it fromSYSTEM_ADDRESSwith a 30M gas cap.morph-geth's pool does not do this. It builds a real
vm.BlockContextfrom the head header and the real chain config before callingbalanceOf, as the user, with a 200k cap:(core/tx_pool.go:330)
For a token whose
balanceOfreads block context ormsg.sender, admission and maintenance were answering a different question than the execution layer: admitting transactions that cannot execute, or — worse for the user — rejecting ones that would have been fine, and having maintenance remove them afterwards.No consensus impact: block building already skips a transaction that fails execution via
mark_invalid. This is about the pool agreeing with execution.The fix
MorphTxValidationInputcarries the block'sEvmEnv.ConfigureEvm::evm_envfor the head — the same call that produces the environment execution runs in, so there is no second implementation to drift.MorphEvmConfig, whichMorphPoolBuilderalready has.read_token_balance_with_fallbackstands its EVM up in that environment and delegates to the sameevm_call_balance_ofthe handler uses, which also brings the pool the user-as-caller and 200k gas cap from fix(revm): read fee-token balances under the executing block environment #205.query_erc20_balanceandquery_balance_via_system_callare deleted — they were the only remaining way to ask this question in the wrong environment.A bug this surfaced
evm_callnow takes the context error after running the frame group:A database failure inside a frame is recorded on the context and surfaces as a halt, not an
Err. Running the frame group directly (asevm_calldoes) skips the step that normally converts it. Without this, routing the pool throughevm_call_balance_ofwould have silently undone #204 — the regression test from that PR caught it, and it also affectstransfer_erc20_with_evm, where a read failure was being reported asTokenTransferFailed.Tests
call_mode_balance_is_read_under_the_supplied_block_environment— abalanceOfreturningTIMESTAMP, read through the pool's entry point; previously answered1.balance_of_fallback_reports_a_failed_state_read_instead_of_a_zero_balance(from fix(txpool): stop turning fee-token state-read failures into verdicts #204) now also covers theevm_callpath, and was the test that caught the context-error bug above.make lint,cargo test --all,cargo test --docandmake test-e2e(128/128) pass.Generic bounds
MorphTransactionValidatorgainsEvmFactoryFor<Evm>: EvmFactory<Spec = MorphHardfork, BlockEnv = MorphBlockEnv>andClient: BlockReaderIdExt<Header = HeaderTy<Evm::Primitives>>, which pin the cached environment to Morph's. Both already held for every instantiation in the tree; they are now stated.https://claude.ai/code/session_01PiUjd47Da71WG2BFkDQz9q