Skip to content

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

Merged
sonhv0212 merged 3 commits into
ronin-chain:mainfrom
sonhv0212:setcode-changes
Jun 2, 2025
Merged

core/txpool: allow tx and authority regardless of admission order#94
sonhv0212 merged 3 commits into
ronin-chain:mainfrom
sonhv0212:setcode-changes

Conversation

@sonhv0212
Copy link
Copy Markdown
Contributor

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
Notice that replacement shall be allowed, and behavior shall remain
coherent with the table, according to the replaced transaction.

LuisPH3 and others added 2 commits May 27, 2025 14:03
…1373)

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 <lightclient@protonmail.com>
Comment thread core/txpool/legacypool/legacypool.go
Comment thread core/txpool/legacypool/legacypool.go
@sonhv0212 sonhv0212 merged commit 4368d02 into ronin-chain:main Jun 2, 2025
2 checks passed
@trantienduchn
Copy link
Copy Markdown
Contributor

ref: ethereum/go-ethereum#31373

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.

5 participants