feat: migrate setcode tx upstream changes #1175
Conversation
WalkthroughThis update introduces stricter and refactored logic for handling delegated accounts and transaction concurrency within the transaction pool. A new error and a dedicated method enforce that delegated or pending delegated accounts can have only one in-flight executable transaction, disallowing gapped nonces except for transaction replacements. The access list tracer is refactored to accept a consolidated exclusion map, and the API layer now excludes authorized addresses from access list tracking and validates authorization list size based on available gas. The test suite is expanded to cover these scenarios, and the software patch version is incremented. Changes
Sequence Diagram(s)sequenceDiagram
participant API
participant TxPool
participant AccessListTracer
API->API: Validate AuthorizationList size
API->AccessListTracer: Create exclusion map (sender, receiver, precompiles, authorities)
API->AccessListTracer: Initialize tracer with exclusion map
API->TxPool: Submit transaction
TxPool->TxPool: checkDelegationLimit(from, tx)
TxPool->TxPool: Validate transaction ordering and in-flight limit
TxPool->API: Return error if limit exceeded or out-of-order
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
core/vm/access_list_tracer.go (1)
118-131: Handle niladdressesToExcludedefensively
addressesToExcludeis dereferenced immediately.
A nil map lookup is safe in Go, but later you keep a reference to the same map viaa.excl.
If the caller accidentally passesnil, subsequent writes likeaddressesToExclude[addr] = struct{}{}(which happen outside this function) will still panic.Consider normalising to an empty map when
nilis supplied:-func NewAccessListTracer(acl types.AccessList, addressesToExclude map[common.Address]struct{}) *AccessListTracer { - list := newAccessList() +func NewAccessListTracer(acl types.AccessList, addressesToExclude map[common.Address]struct{}) *AccessListTracer { + if addressesToExclude == nil { + addressesToExclude = make(map[common.Address]struct{}) + } + list := newAccessList()This keeps the API nil-safe while preserving current behaviour.
internal/ethapi/api.go (1)
1551-1556: Minor: overflowing-nonce test is unclearauth.Nonce + 1 < auth.NonceThis expression only turns
trueon uint64 overflow and is hard to parse at a glance.
Explicitly comparing againstmath.MaxUint64makes the intent clearer:if auth.Nonce == math.MaxUint64 { continue // would overflow on increment }Purely cosmetic but improves readability for future maintainers.
core/tx_pool.go (1)
99-102: Error constant is fine, but documentation can be clearer
ErrOutOfOrderTxFromDelegatedis self-descriptive, however the comment above it should explain why gapped nonces are dangerous for delegated accounts (e.g. the sweep-able balance problem). That additional context will be useful for future maintainers.core/tx_pool_test.go (3)
2717-2732: Consider turning the table-driven loop intot.Runsub-testsAll cases are enumerated in a slice and executed sequentially inside the outer test that already runs in parallel.
Wrapping the body withfor _, tt := range cases { tt := tt // capture t.Run(tt.name, func(t *testing.T) { t.Parallel() … }) }would give:
- Individual result lines in
go testoutput – failures are easier to pinpoint.- True parallel execution of the independent scenarios (they only share global constants), reducing wall-clock time.
- Proper scoping of defers (
pool.Stop()) – currently all defers are stacked and executed after every case finishes, which may keep dozens of pools alive longer than necessary and inflate memory consumption.
2733-2748: Userequire.ErrorIs/NoErrorfor clearer intent and shorter codeAssertion blocks repeat the same error-checking pattern:
if err := pool.addRemoteSync(...); !errors.Is(err, ErrX) { … }The
stretchr/testify/requirehelpers already imported earlier give you one-liners that immediately abort on failure and make the expected condition obvious:require.ErrorIs(t, pool.addRemoteSync(...), core.ErrOutOfOrderTxFromDelegated)(or
require.NoErrorfor success cases).Besides readability this stops the test at the first unexpected state, preventing cascading follow-up failures.
2803-2818: Nit: duplicated wording in failure message
failed to add with pending delegationis reused for several different
operations (second tx insert, delegation tx insert, etc.).
Using a more specific phrase (e.g. “sending delegation after two
in-flight txs”) helps future readers understand which step broke without
needing to locate the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/tx_pool.go(2 hunks)core/tx_pool_test.go(2 hunks)core/vm/access_list_tracer.go(1 hunks)internal/ethapi/api.go(2 hunks)params/version.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
core/tx_pool_test.go (3)
core/tx_pool.go (4)
TxPool(285-330)ErrOutOfOrderTxFromDelegated(101-101)ErrInflightTxLimitReached(97-97)ErrAuthorityReserved(106-106)core/types/setcode_tx.go (1)
DelegationPrefix(35-35)core/vm/opcodes.go (3)
ADDRESS(78-78)PUSH0(126-126)SSTORE(116-116)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
params/version.go (1)
24-29: Version bump looks consistent with upstream sync
VersionPatchwas incremented from 40 → 41 to advertise the new feature set.
No additional code or semantic changes are introduced in this file, so the bump is correct and harmless.internal/ethapi/api.go (1)
1591-1592: Reuse previously built exclusion map to avoid inadvertent driftYou correctly pass
addressesToExcludeinto both the initial and subsequent tracers.
Just flagging that if the map is mutated between iterations (it currently isn’t), both tracers will see the mutated contents because they share the same reference.
If later code starts appending toaddressesToExclude, consider cloning it per tracer to keep iterations independent.core/tx_pool_test.go (1)
2754-2756: Delegation clean-up might still leave residual contract codeYou reset
addrA’s code to nil, but the helper contract (aa) deployed at
common.Address{0xaa, 0xaa}remains in state.
If a later case re-uses the same StateDB instance (e.g. by accident when a new
test forgets to create a fresh pool) the stale storage contract could falsify
results.Either:
- Also clear
statedb.SetCode(aa, nil)here; or- Call
pool.requestReset(nil, nil)so the pool refreshescurrentState
from a brand-new database.
1. Purpose or design rationale of this PR
This PR migrates upstream fixes and enhancements:
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
Chores