feat(tx-pool): migrate upstream setcode transaction changes #1148
Conversation
WalkthroughThis pull request refactors the transaction pool’s error handling and validation logic. It replaces the previous account limit error with a new error for in-flight transaction limits and introduces a dedicated authorization check method. The changes update function signatures for managing transaction authorities, improve testing—especially for reorganization scenarios—and ensure duplicate authorities in transactions are filtered. Additionally, the patch version has been incremented. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TxPool
participant Validator
participant Transaction
Client->>TxPool: Submit Transaction
TxPool->>TxPool: validateTx()
TxPool->>Validator: validateAuth(from, tx)
Validator->>Transaction: Retrieve authority list
Transaction-->>Validator: Return unique authorities
Validator-->>TxPool: Validation result
TxPool->>Client: Accept/Reject Transaction
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)
🪧 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: 1
🧹 Nitpick comments (1)
core/tx_pool_test.go (1)
163-176: Handle potential signature errors.The
MustSignNewTxcall panics on failure. Consider validating or returning the error to avoid unexpected panics. Also, verify that the hard-codedValue: uint256.NewInt(100)is intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/tx_pool.go(5 hunks)core/tx_pool_test.go(8 hunks)core/types/transaction.go(1 hunks)params/version.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
params/version.go (1)
27-27: Incrementing the patch version looks good.This minor version bump from 25 to 26 aligns with the feature additions in the transaction handling logic, and there do not appear to be any issues with this change.
core/types/transaction.go (1)
487-496: Ensure uniqueness logic is correct and efficient.The introduction of the
marksmap to prevent duplicate authorities is a straightforward and efficient solution. It correctly skips already-seen addresses while building the final list. The time complexity is O(n), which is acceptable for typical authorization lists, and there is no risk of data races since this method only reads local data.core/tx_pool.go (4)
1887-1911: Check single in-flight transaction rule logic carefully.The check combining:
- A non-empty code hash (account has a deployed delegation) or
- Existing in-flight authorization (tracked in
pool.all.auths[from])
to limit the account to one transaction is clearly implemented here.
However, consider clarifying behavior for partially overlapping transactions (same nonce but different gas prices) vs. truly distinct in-flight transactions (different nonces). This enforcement might cause confusion if an account wants to replace an existing delegated transaction with a higher gas price.Could you confirm that overlapping transactions (i.e., same nonce) are allowed as “replace” transactions as intended?
1914-1920: Validate reserved authority logic.This logic prevents conflicting authorities from different transactions in-flight. It ensures that if a SetCode authorization is already pending or queued for a particular address, no other transaction can attempt to use that same address for authorization. This effectively maintains one unique authority usage at a time in the pool. This approach appears correct for the delegated scenario.
2170-2170: Switching to removeAuthorities(tx) is consistent.Switching from removing authorities by hash to removing them using the full
txobject ensures the code can accurately clear the relevant addresses, especially now that you track multiple authorities within a single transaction. This implements the logic more robustly, preventing partial or incorrect lookups by hash alone.
95-97:✅ Verification successful
Validate new error usage.
The newly added
ErrInflightTxLimitReachederror is clearly named and scoped to the delegated account case. Ensure it’s handled consistently in the rest of the codebase so that calling sites properly differentiate between in-flight limit violations and other capacity constraints.To confirm coverage, please search for usage of
ErrInflightTxLimitReachedacross the codebase:
🏁 Script executed:
#!/bin/bash # Searching all code for occurrences of ErrInflightTxLimitReached rg "ErrInflightTxLimitReached"Length of output: 1505
Usage Verified: ErrInflightTxLimitReached is Consistently Handled
After verifying the repository, it’s clear that the new error is not only defined appropriately in
core/tx_pool.gobut also consistently used in the test suite (core/tx_pool_test.go). The tests correctly compare the error viaerrors.Is, ensuring that calling sites can differentiate in-flight limit violations from other error conditions.
- Definition Check: The error is defined with a clear message in
core/tx_pool.go.- Return & Handling: It is returned from functions as expected and is validated in multiple test cases.
- Test Coverage: Tests in
core/tx_pool_test.goverify that the error is properly used when the in-flight transaction limit is reached.No further changes are required.
core/tx_pool_test.go (6)
27-27: Confirm Go 1.21+ compatibility.You've introduced the
slicespackage at line 27. Please ensure the build environment uses Go 1.21 or higher, otherwise this import may cause compilation issues.
160-160: No issues with new function call.Replacing inline logic with a call to
pricedSetCodeTxWithAuthappears clean and consistent. No immediate concerns.
2737-2742: Test coverage for in-flight transaction limit.These lines properly confirm that any additional transactions for the same account trigger
ErrInflightTxLimitReachedonce the limit is reached. The logic looks sound.
2776-2777: Clarify in-flight transaction limit for delegated accounts.Same pattern of verifying
ErrInflightTxLimitReached. The approach ensures delegated or authority-based constraints. No immediate issues.
2882-2908: Verify authority tracker cleanup logic.You're testing that replaced SetCode transactions remove their old hashes from the authority tracker. It might be worth confirming that no partial references remain in other data structures. Otherwise, this is a helpful test scenario.
2937-2990: Reorg handling for SetCode transactions.This test thoroughly verifies state transitions for delegations and checks that older authorizations no longer block new transactions. The logic is clear, and the coverage is good.
1. Purpose or design rationale of this PR
This PR migrates some recent upstream changes:
removeAuthorities: core/txpool: fix error logs flood caused by removeAuthorities ethereum/go-ethereum#31249setCodeTxreorg test: core/txpool/legacypool: add setCodeTx reorg test ethereum/go-ethereum#312062. 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