core/txpool: allow tx and authority regardless of admission order #31373

LuisPH3 · 2025-03-13T08:38:54Z

This PR proposes a change to the authorizations' validation introduced in commit cdb66c8. These changes make the expected behavior independent of the order of admission of authorizations, improving the predictability of the resulting state and the usability of the system with it.

The current implementation behavior is dependent on the transaction submission order: This issue is related to authorities and the sender of a transaction, and can be reproduced respecting the normal nonce rules.

The issue can be reproduced by the two following cases:
First case

Given an empty pool.
Submit transaction { from: B, auths [ A ] }: is accepted.
Submit transaction { from: A }: Is accepted: it becomes the one in-flight transaction allowed.

Second case

Given an empty pool.
Submit transaction { from: A }: is accepted
Submit transaction { from: B, auths [ A ] }: is rejected since there is already a queued/pending transaction from A.

The expected behavior is that both sequences of events would lead to the same sets of accepted and rejected transactions.

Proposed changes
The queued/pending transactions issued from any authority of the transaction being validated have to be counted, allowing one transaction from accounts submitting an authorization.

Notice that the expected behavior was explicitly forbidden in the case "reject-delegation-from-pending-account", I believe that this behavior conflicts to the definition of the limitation, and it is removed in this PR. The expected behavior is tested in "accept-authorization-from-sender-of-one-inflight-tx".
Replacement tests have been separated to improve readability of the acceptance test.
The test "allow-more-than-one-tx-from-replaced-authority" has been extended with one extra transaction, since the system would always have accepted one transaction (but not two).
The test "accept-one-inflight-tx-of-delegated-account" is extended to clean-up state, avoiding leaking the delegation used into the other tests. Additionally, replacement check is removed to be tested in its own test case.

Expected behavior
The expected behavior of the authorizations' validation shall be as follows:

Notice that replacement shall be allowed, and behavior shall remain coherent with the table, according to the replaced transaction.

LuisPH3 · 2025-03-14T08:52:31Z

Hi @lightclient. I haven't managed to get CI to run in Image: Visual Studio 2019; Environment: GETH_ARCH=386, GETH_MINGW=C:\msys64\mingw32. I have noticed that other PRs fail in the same step.
How can I solve this issue?

MariusVanDerWijden · 2025-03-14T09:46:16Z

@LuisPH3 This is a known timeout/flaky test. We will not block merging PRs on this test not passing

lightclient

Generally okay with the idea here. It doesn't seem terribly important though since we shouldn't really expect that a user will be sending both a delegation and tx at the same time. Since the change is small though, I don't see a reason to not do it.

Pushed a small change to simplify the logic for counting the in-flight txs.

core/txpool/legacypool/legacypool_test.go

rjl493456442 · 2025-04-02T12:34:23Z

My concern is that if we also consider the blobPool, the new rule will become even more complex.

delegation || pending auth:
=> at most one non-blob tx
=> at most one blob tx

at most one non-blob tx && at most one blob tx:
=> accept pending auth

more than one non-blob tx || more than one blob tx:
=> reject pending auth

core/txpool/legacypool/legacypool.go

lightclient · 2025-04-08T21:12:57Z

core/txpool/blobpool/blobpool_test.go

+// reserver is a utility struct to sanity check that accounts are
+// properly reserved by the blobpool (no duplicate reserves or unreserves).
+type reserver struct {
+	accounts map[common.Address]struct{}
+	lock     sync.RWMutex
+}
+
+func newReserver() txpool.Reserver {
+	return &reserver{accounts: make(map[common.Address]struct{})}
+}
+
+func (r *reserver) Hold(addr common.Address) error {
+	r.lock.Lock()
+	defer r.lock.Unlock()
+	if _, exists := r.accounts[addr]; exists {
+		panic("already reserved")
+	}
+	r.accounts[addr] = struct{}{}
+	return nil
+}
+
+func (r *reserver) Release(addr common.Address) error {
+	r.lock.Lock()
+	defer r.lock.Unlock()
+	if _, exists := r.accounts[addr]; !exists {
+		panic("not reserved")
+	}
+	delete(r.accounts, addr)
+	return nil
+}
+
+func (r *reserver) Has(address common.Address) bool {
+	r.lock.RLock()
+	defer r.lock.RUnlock()
+	_, exists := r.accounts[address]
+	return exists
+}


In #31373 I removed the test-only reserver and in hindsight I think this is a mistake. It's useful to panic when some assumptions about double holds and double releases fail. Adding it back here.

lightclient · 2025-04-08T21:33:16Z

I think this change is small enough that it's okay to include. Allowing tx and authority regardless of order doesn't seem that important too me, but considering the change itself is only around 10 lines of code and the reset is fixing up some tests and me refactoring the Reserver object, it's reasonable.

The blobpool will still require specific order if the user wants to pool both a delegation and a blob tx. I think this is okay since we expect blob users to generally be EOAs managed by rollups.

…hereum#31373) This PR proposes a change to the authorizations' validation introduced in commit cdb66c8. These changes make the expected behavior independent of the order of admission of authorizations, improving the predictability of the resulting state and the usability of the system with it. The current implementation behavior is dependent on the transaction submission order: This issue is related to authorities and the sender of a transaction, and can be reproduced respecting the normal nonce rules. The issue can be reproduced by the two following cases: **First case** - Given an empty pool. - Submit transaction `{ from: B, auths [ A ] }`: is accepted. - Submit transaction `{ from: A }`: Is accepted: it becomes the one in-flight transaction allowed. **Second case** - Given an empty pool. - Submit transaction `{ from: A }`: is accepted - Submit transaction `{ from: B, auths [ A ] }`: is rejected since there is already a queued/pending transaction from A. The expected behavior is that both sequences of events would lead to the same sets of accepted and rejected transactions. **Proposed changes** The queued/pending transactions issued from any authority of the transaction being validated have to be counted, allowing one transaction from accounts submitting an authorization. - Notice that the expected behavior was explicitly forbidden in the case "reject-delegation-from-pending-account", I believe that this behavior conflicts to the definition of the limitation, and it is removed in this PR. The expected behavior is tested in "accept-authorization-from-sender-of-one-inflight-tx". - Replacement tests have been separated to improve readability of the acceptance test. - The test "allow-more-than-one-tx-from-replaced-authority" has been extended with one extra transaction, since the system would always have accepted one transaction (but not two). - The test "accept-one-inflight-tx-of-delegated-account" is extended to clean-up state, avoiding leaking the delegation used into the other tests. Additionally, replacement check is removed to be tested in its own test case. **Expected behavior** The expected behavior of the authorizations' validation shall be as follows: ![image](https://github.com/user-attachments/assets/dbde7a1f-9679-4691-94eb-c197a0cbb823) Notice that replacement shall be allowed, and behavior shall remain coherent with the table, according to the replaced transaction. --------- Co-authored-by: lightclient <[email protected]>

LuisPH3 marked this pull request as ready for review March 13, 2025 08:39

LuisPH3 requested review from holiman and rjl493456442 as code owners March 13, 2025 08:39

fjl assigned fjl and lightclient and unassigned fjl Mar 13, 2025

LuisPH3 force-pushed the setcode_changes branch 3 times, most recently from 71fc749 to 9d3b06d Compare March 27, 2025 14:12

lightclient force-pushed the setcode_changes branch from 9d3b06d to 1986308 Compare April 1, 2025 20:00

lightclient reviewed Apr 1, 2025

View reviewed changes

core/txpool/legacypool/legacypool_test.go Outdated Show resolved Hide resolved

rjl493456442 added the status:triage label Apr 3, 2025

lightclient force-pushed the setcode_changes branch from b5a6703 to d28d39e Compare April 4, 2025 20:37

jwasinger reviewed Apr 8, 2025

View reviewed changes

core/txpool/legacypool/legacypool.go Outdated Show resolved Hide resolved

fjl removed the status:triage label Apr 8, 2025

core/txpool: allow tx and authority regardless of admission order

42d1156

lightclient force-pushed the setcode_changes branch from c11a8ee to 42d1156 Compare April 8, 2025 17:56

lightclient changed the title ~~Improve transaction pool validation of SetCode transactions: behavior independent of admission order.~~ core/txpool: allow tx and authority regardless of admission order Apr 8, 2025

core/txpool: rename reserver handle to reservation handle

32169a2

lightclient reviewed Apr 8, 2025

View reviewed changes

core/txpool/legacypool: clear both pending and queue in Clear()

52d8e1e

lightclient previously approved these changes Apr 8, 2025

View reviewed changes

core/txpool/legacypool: improve clarity of test comments for setcode

65752ba

lightclient dismissed their stale review via 65752ba April 9, 2025 20:46

core/txpool: make Has docs more clear about the result

70e4835

lightclient approved these changes Apr 9, 2025

View reviewed changes

lightclient merged commit 0c2ad07 into ethereum:master Apr 10, 2025
4 checks passed

colinlyguo mentioned this pull request Apr 29, 2025

feat: migrate setcode tx upstream changes scroll-tech/go-ethereum#1175

Merged

3 tasks

trantienduchn mentioned this pull request May 26, 2025

core/txpool: allow tx and authority regardless of admission order (#31373) trantienduchn/ronin#1

Closed

LuisPH3 deleted the setcode_changes branch June 10, 2025 07:12

facuMH mentioned this pull request Jun 11, 2025

Update validateAuth 0xsoniclabs/sonic#295

Merged

trantienduchn mentioned this pull request Jun 30, 2025

core/txpool: allow tx and authority regardless of admission order ronin-chain/ronin#94

Merged

qianhh mentioned this pull request Sep 1, 2025

Merge Geth [v1.15.0...v1.15.11] bane-labs/go-ethereum#499

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

core/txpool: allow tx and authority regardless of admission order #31373

core/txpool: allow tx and authority regardless of admission order #31373

Uh oh!

LuisPH3 commented Mar 13, 2025

Uh oh!

LuisPH3 commented Mar 14, 2025

Uh oh!

MariusVanDerWijden commented Mar 14, 2025

Uh oh!

lightclient left a comment

Uh oh!

Uh oh!

rjl493456442 commented Apr 2, 2025

Uh oh!

Uh oh!

lightclient Apr 8, 2025

Uh oh!

lightclient commented Apr 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

core/txpool: allow tx and authority regardless of admission order #31373

core/txpool: allow tx and authority regardless of admission order #31373

Uh oh!

Conversation

LuisPH3 commented Mar 13, 2025

Uh oh!

LuisPH3 commented Mar 14, 2025

Uh oh!

MariusVanDerWijden commented Mar 14, 2025

Uh oh!

lightclient left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rjl493456442 commented Apr 2, 2025

Uh oh!

Uh oh!

lightclient Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

lightclient commented Apr 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants