Skip to content

fix(txpool): stop turning fee-token state-read failures into verdicts - #204

Closed
panos-xyz wants to merge 1 commit into
mainfrom
fix/token-fee-propagate-state-errors
Closed

panos-xyz wants to merge 1 commit into
mainfrom
fix/token-fee-propagate-state-errors

Conversation

@panos-xyz

@panos-xyz panos-xyz commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

A failed state read is not a statement about the transaction. Two places treated it as one.

1. A database error came back as a zero balance

query_balance_via_system_call (crates/revm/src/token_fee.rs) ended with:

Ok(_) => Ok(U256::ZERO),
Err(_) => Ok(U256::ZERO),

The second arm swallows EVMError::Database along with everything else, which made the arm one level up — in read_token_balance_with_fallback — dead code:

Err(EVMError::Database(e)) => Err(e),   // never reached

So for a call-mode fee token, a failed storage read reported "this account holds no tokens" and the caller rejected the transaction for insufficient funds.

Now the database error is reported. A revert or an unusable return value still maps to a zero balance — those are statements about the token, and they match what the caller should conclude.

Reachability is not theoretical: the maintenance task holds a state provider bound to one tip and then walks every MorphTx in the pool, one storage read at a time.

2. A missing state provider marked the transaction known-bad

validate_morph_tx_balance wraps a failure to obtain a state provider as TokenInfoFetchFailed, and the caller mapped every MorphTxError to TransactionValidationOutcome::Invalid.

Invalid is a verdict. The pool emits an invalid event for it, which is what the network layer uses to hold the sending peer responsible — for a transaction that may be entirely valid and simply could not be checked. Error emits discarded instead: the attempt is dropped, nobody is blamed, and the sender can retry.

The mapping is now explicit in morph_tx_validation_outcome, so the distinction is stated in one place rather than implied by a fall-through.

3. TokenInfoFetchFailed::token_id is now Option<u16>

The provider failure happens before any token ID is known, and the call site passed a hardcoded 0 with a // token_id not yet extracted comment. The error rendered as "failed to fetch token info for ID 0" — pointing at a token that had nothing to do with it.

Tests

  • balance_of_fallback_reports_a_failed_state_read_instead_of_a_zero_balance — the same call-mode token is read twice, once from readable state and once with the token's storage failing. Confirmed to fail before this change.
  • an_unreadable_fee_token_state_is_an_error_not_an_invalid_transaction / a_real_fee_token_failure_is_still_an_invalid_transaction — pins both directions of the mapping.

make lint, cargo test --all and cargo test --doc pass.

Relationship to the other maintenance PRs

Independent of #202 and #203, and safe to merge in any order. #202 is what makes the maintenance task act on the distinction this PR restores — before it, the task removed the transaction either way.

Not in this PR

The balance query still runs against a fabricated block environment (BlockEnv::default(), chain_id: 1) and with SYSTEM_ADDRESS as msg.sender at a 30M gas limit, where morph-geth uses the real header, the real chain config, the user as sender and a 200k cap. That is a consensus-facing difference for any call-mode token whose balanceOf reads block context or msg.sender, and it gets its own PR — it touches the execution path.

Found by an external review of the pool maintenance path.

https://claude.ai/code/session_01PiUjd47Da71WG2BFkDQz9q

Summary by CodeRabbit

  • Bug Fixes

    • Database errors encountered while checking token balances are now reported accurately instead of being interpreted as zero balances.
    • Temporary token information or balance lookup failures no longer incorrectly mark transactions as invalid or blame the sender.
    • Token-related validation errors now provide clearer details, including cases where the token could not be identified.
  • Tests

    • Added regression coverage for failed token storage reads and validation outcome handling.

`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
@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 099c597a-92dc-4c46-a0b0-b8be744ae312

📥 Commits

Reviewing files that changed from the base of the PR and between dfae5d4 and f1b2f0b.

📒 Files selected for processing (4)
  • crates/revm/src/token_fee.rs
  • crates/txpool/src/error.rs
  • crates/txpool/src/morph_tx_validation.rs
  • crates/txpool/src/validator.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The change propagates token database read errors, represents unknown token IDs explicitly, and maps token state read failures to validation errors instead of invalid transactions.

Changes

Token state error handling

Layer / File(s) Summary
Balance lookup error propagation
crates/revm/src/token_fee.rs
Database errors from token balance reads are propagated. Reverted or unusable token responses still return zero. Tests cover successful reads and database failures.
Token fetch error contract
crates/txpool/src/error.rs, crates/txpool/src/morph_tx_validation.rs
TokenInfoFetchFailed now stores an optional token ID. Display formatting and constructors cover both known and unknown IDs.
Validation outcome mapping
crates/txpool/src/validator.rs
Token state read failures produce TransactionValidationOutcome::Error; other MorphTx errors produce Invalid. Tests cover both outcomes.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to f1b2f

Token state-read failures are handled without an identified unresolved merge risk.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.87% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: fee-token state-read failures no longer become transaction validation verdicts.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/token-fee-propagate-state-errors

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@panos-xyz

Copy link
Copy Markdown
Contributor Author

Folded into #200 at the author's request — the commit is preserved there unchanged (cherry-picked, -x trailer intact). Branch kept for now; delete once #200 merges.

@panos-xyz panos-xyz closed this Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant