core/txpool: allow tx and authority regardless of admission order #31373
Conversation
|
Hi @lightclient. I haven't managed to get CI to run in |
|
@LuisPH3 This is a known timeout/flaky test. We will not block merging PRs on this test not passing |
LuisPH3
force-pushed
the
setcode_changes
branch
3 times, most recently
from
71fc749 to
9d3b06d
Compare
lightclient
force-pushed
the
setcode_changes
branch
from
9d3b06d to
1986308
Compare
There was a problem hiding this comment.
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.
|
My concern is that if we also consider the blobPool, the new rule will become even more complex.
|
lightclient
force-pushed
the
setcode_changes
branch
from
b5a6703 to
d28d39e
Compare
lightclient
force-pushed
the
setcode_changes
branch
from
c11a8ee to
42d1156
Compare
lightclient
changed the title
| // 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 | ||
| } |
There was a problem hiding this comment.
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.
|
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. |
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
{ from: B, auths [ A ] }: is accepted.{ from: A }: Is accepted: it becomes the one in-flight transaction allowed.Second case
{ from: A }: is accepted{ 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.
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.