core/txpool,types: add support for SetCode transactions #31073 #1922
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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. Comment |
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
liam-lai and
wanwiset25
There was a problem hiding this comment.
Pull request overview
This PR adds support for EIP-7702 SetCode transactions to the XDC network. The implementation introduces a LazyTransaction wrapper to optimize transaction handling, moves transaction ordering logic from the types package to the miner package, and implements an address reservation mechanism to prevent cross-subpool conflicts.
Key changes include:
- Introduction of SetCode transaction type support with authorization tracking and validation
- LazyTransaction wrapper that defers full transaction resolution until needed
- Address reservation system ensuring exclusive account ownership per subpool
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| miner/ordering.go | Implements new transaction ordering logic with price/nonce sorting for LazyTransaction |
| miner/ordering_test.go | Comprehensive tests for transaction ordering with EIP-1559 and special transaction handling |
| miner/worker.go | Updated to use LazyTransaction wrapper and new ordering function |
| core/txpool/txpool.go | Adds address reservation mechanism for subpool exclusivity |
| core/txpool/subpool.go | Defines LazyTransaction struct and updated SubPool interface |
| core/txpool/validation.go | Adds SetCode transaction validation rules and authorization conflict checks |
| core/txpool/legacypool/legacypool.go | Integrates address reservation, authorization tracking, and SetCode transaction support |
| core/txpool/legacypool/legacypool_test.go | Extensive tests for SetCode transaction scenarios and delegation handling |
| core/txpool/legacypool/legacypool2_test.go | Updates Init calls with new AddressReserver parameter |
| core/txpool/errors.go | Adds new error types for account limits and authority reservation |
| core/types/transaction.go | Removes old ordering code, adds SetCodeAuthorities and Time methods |
| core/types/transaction_test.go | Removes relocated ordering tests |
| core/types/tx_setcode.go | Changes SignSetCode parameter order (breaking change) |
| eth/protocol.go | Updates txPool interface to return LazyTransaction |
| eth/sync.go | Resolves LazyTransaction when syncing transactions |
| eth/api_backend.go | Resolves LazyTransaction when retrieving pool transactions |
| eth/helper_test.go | Updates test implementation to return LazyTransaction |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for from, accTxs := range txs { | ||
| var normalTxs []*txpool.LazyTransaction | ||
| for _, lazyTx := range accTxs { | ||
| if tx := lazyTx.Resolve(); tx.Tx.IsSpecialTransaction() { |
There was a problem hiding this comment.
Potential nil pointer dereference. When calling lazyTx.Resolve() on line 133, the result could be nil if the transaction is no longer available in the pool. However, the code immediately dereferences it with tx.Tx.IsSpecialTransaction() without checking if tx is nil. This could lead to a panic. A nil check should be added before dereferencing, or the transaction should be skipped if Resolve returns nil.
| if tx := lazyTx.Resolve(); tx.Tx.IsSpecialTransaction() { | |
| tx := lazyTx.Resolve() | |
| if tx == nil { | |
| // Transaction no longer available in the pool; skip it. | |
| continue | |
| } | |
| if tx.Tx.IsSpecialTransaction() { |
| if tx := s.txs[i].tx.Resolve(); tx != nil && tx.Tx.To() != nil { | ||
| if _, ok := s.payersSwap[*tx.Tx.To()]; ok { | ||
| i_price = common.TRC21GasPrice | ||
| } |
There was a problem hiding this comment.
Potential nil pointer dereference. The code calls tx.Resolve() and then immediately dereferences the result with tx.Tx.To() without checking if tx is nil. If the transaction is no longer available in the pool, Resolve() could return nil, leading to a panic. A nil check should be added before dereferencing.
| t.Fatalf("%s: failed to add with remote setcode transaction: %v", name, err) | ||
| } | ||
| if err := pool.addRemoteSync(pricedTransaction(0, 100000, new(big.Int).Set(minGasPrice), keyC)); err != nil { | ||
| t.Fatalf("%s: failed to add with pending delegatio: %v", name, err) |
There was a problem hiding this comment.
The comment has a typo: "delegatio" should be "delegation". The phrase should read "failed to add with pending delegation".
| t.Fatalf("%s: failed to add with pending delegatio: %v", name, err) | |
| t.Fatalf("%s: failed to add with pending delegation: %v", name, err) |
|
|
||
| // SignSetCode creates a signed SetCode authorization. | ||
| func SignSetCode(auth SetCodeAuthorization, prv *ecdsa.PrivateKey) (SetCodeAuthorization, error) { | ||
| func SignSetCode(prv *ecdsa.PrivateKey, auth SetCodeAuthorization) (SetCodeAuthorization, error) { |
There was a problem hiding this comment.
The parameter order in the function signature was changed from (auth, prv) to (prv, auth), which is a breaking API change. While this may be intentional to improve consistency (private key first is a common pattern), this change should be carefully documented in the PR description as it could break existing code that calls this function.
| func SignSetCode(prv *ecdsa.PrivateKey, auth SetCodeAuthorization) (SetCodeAuthorization, error) { | |
| func SignSetCode(auth SetCodeAuthorization, prv *ecdsa.PrivateKey) (SetCodeAuthorization, error) { |
| // removeTx removes a single transaction from the queue, moving all subsequent | ||
| // transactions back to the future queue. | ||
| // | ||
| // In unreserve is false, the account will not be relinquished to the main txpool |
There was a problem hiding this comment.
The comment is missing proper documentation about the "In" parameter mentioned. The text says "In unreserve is false", but "In" appears to be a typo. It should read "If unreserve is false" to properly describe the parameter behavior.
| // In unreserve is false, the account will not be relinquished to the main txpool | |
| // If unreserve is false, the account will not be relinquished to the main txpool |
| return fmt.Errorf("%w: balance %v, queued cost %v, tx cost %v, overshot %v", core.ErrInsufficientFunds, balance, spent, cost, new(big.Int).Sub(need, newBalance)) | ||
| } | ||
| // Transaction takes a new nonce value out of the pool. Ensure it doesn't | ||
| // overflow the number of permitted transactions from a single accoun |
There was a problem hiding this comment.
The comment has a typo: "accoun" should be "account". The full phrase should read "overflow the number of permitted transactions from a single account".
| // overflow the number of permitted transactions from a single accoun | |
| // overflow the number of permitted transactions from a single account |
| i_price := s.txs[i].fees | ||
| if tx := s.txs[i].tx.Resolve(); tx != nil && tx.Tx.To() != nil { | ||
| if _, ok := s.payersSwap[*tx.Tx.To()]; ok { | ||
| i_price = common.TRC21GasPrice | ||
| } | ||
| } | ||
|
|
||
| j_price := s.txs[j].fees | ||
| if tx := s.txs[j].tx.Resolve(); tx != nil && tx.Tx.To() != nil { | ||
| if _, ok := s.payersSwap[*tx.Tx.To()]; ok { | ||
| j_price = common.TRC21GasPrice | ||
| } | ||
| } | ||
|
|
||
| // If the prices are equal, use the time the transaction was first seen for | ||
| // deterministic sorting | ||
| cmp := i_price.Cmp(j_price) | ||
| if cmp == 0 { | ||
| return s.txs[i].tx.Time.Before(s.txs[j].tx.Time) | ||
| } | ||
| return cmp > 0 |
There was a problem hiding this comment.
The variable naming uses snake_case (i_price, j_price) which is inconsistent with Go naming conventions. These should be renamed to camelCase (iPrice, jPrice) to follow Go style guidelines.
| // Init sets the gas price needed to keep a transaction in the pool and the chain | ||
| // head to allow balance / nonce checks. The transaction journal will be loaded | ||
| // from disk and filtered based on the provided starting settings. The internal | ||
| // goroutines will be spun up and the pool deemed operational afterwards. |
There was a problem hiding this comment.
The function documentation is missing a description of the "reserve" parameter. The Init function now takes three parameters but only describes the purpose of gasTip and head in its documentation. The new "reserve" parameter should be documented to explain its purpose (address reservation callback).
| // goroutines will be spun up and the pool deemed operational afterwards. | |
| // goroutines will be spun up and the pool deemed operational afterwards. The | |
| // reserve callback is used to reserve addresses whose transactions are handled | |
| // by the pool, ensuring exclusive access to those accounts while they are | |
| // being processed. |
| // Allow at most one in-flight tx for delegated accounts or those with | ||
| // a pending authorization. |
There was a problem hiding this comment.
The comment lacks sufficient detail about what makes delegated accounts or pending authorizations special. While the inline comment mentions "at most one in-flight tx for delegated accounts or those with a pending authorization," the surrounding documentation should better explain why this restriction exists (e.g., security concerns about account sweeping). This would improve code maintainability.
| // Allow at most one in-flight tx for delegated accounts or those with | |
| // a pending authorization. | |
| // For accounts with non-empty code or with a pending authorization we only | |
| // allow at most one in-flight transaction at a time. | |
| // | |
| // These accounts can act as delegated executors or rely on reusable | |
| // authorizations (e.g. signatures that permit spending or other actions | |
| // on behalf of another party). If the pool were to admit many concurrent | |
| // transactions for such an account, the executor could rapidly submit a | |
| // sequence of transfers before intermediate state changes (balance or | |
| // authorization revocation) are fully reflected in the pool. That pattern | |
| // can effectively "sweep" all available funds/allowances once a single | |
| // authorization exists, and makes it harder to reason about which | |
| // transactions are still safe to execute. | |
| // | |
| // By limiting these accounts to a single in-flight transaction, we | |
| // serialize their execution from the pool's perspective and avoid | |
| // admitting additional transactions until the effect of the previous one | |
| // is known, which mitigates these risks. |
| // price sorted transactions in a nonce-honouring way. | ||
| // | ||
| // Note, the input map is reowned so the caller should not interact any more with | ||
| // if after providing it to the constructor. |
There was a problem hiding this comment.
The comment has a typo: "if after" should be "it after". The sentence should read "Note, the input map is reowned so the caller should not interact any more with it after providing it to the constructor."
| // if after providing it to the constructor. | |
| // it after providing it to the constructor. |
1 participant
Proposed changes
Ref: ethereum#31073
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which part of the codebase this PR will touch base on,
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that