-
Notifications
You must be signed in to change notification settings - Fork 69
core, eth, miner: 4844 blob transaction pool #26940 #1911
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR integrates EIP-4844 blob transaction pool functionality by introducing a modular transaction pool architecture with specialized subpools. The main changes include refactoring the legacy transaction pool into a subpool implementation and creating a new aggregator pool that manages multiple transaction type-specific subpools.
Key changes:
- Introduced
SubPoolinterface andLazyTransactionwrapper for deferred transaction resolution - Refactored existing transaction pool code from
txpoolpackage tolegacypoolpackage - Created new aggregator
TxPoolthat manages multiple subpools and routes transactions accordingly
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| miner/worker.go | Updated to use LazyTransaction wrapper and new ordering functions |
| miner/ordering_test.go | Added comprehensive tests for transaction ordering with lazy loading |
| miner/ordering.go | Implemented new transaction ordering logic with miner fee calculations |
| core/txpool/txpool.go | Replaced legacy pool with new aggregator pool managing subpools |
| core/txpool/subpool.go | Defined SubPool interface and LazyTransaction structure |
| core/txpool/errors.go | Extracted error definitions to shared package |
| core/txpool/validation.go | Added UsedAndLeftSlots validation callback |
| core/txpool/txpool_test.go | Moved and updated tests to legacypool package |
| eth/backend.go | Updated to instantiate new modular pool architecture |
| Multiple files | Package renames from txpool to legacypool |
Comments suppressed due to low confidence (1)
miner/ordering.go:1
- Corrected 'if' to 'it' for grammatical accuracy.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6a8b8ca to
04746b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
💡 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() { |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential nil pointer dereference: lazyTx.Resolve() can return nil, and then accessing tx.Tx.IsSpecialTransaction() will panic. The code should check if tx is nil before accessing tx.Tx.
| if tx := lazyTx.Resolve(); tx.Tx.IsSpecialTransaction() { | |
| tx := lazyTx.Resolve() | |
| if tx == nil { | |
| // Skip entries that failed to resolve to a concrete transaction | |
| continue | |
| } | |
| if tx.Tx.IsSpecialTransaction() { |
| txset, _ := newTransactionsByPriceAndNonce(signer, groups, map[common.Address]*big.Int{}, baseFee) | ||
|
|
||
| txs := types.Transactions{} | ||
| for tx := txset.Peek(); tx != nil; tx = txset.Peek() { |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential nil pointer dereference: tx.Tx could be nil if Resolve() returns a non-nil wrapper but with nil Tx field. The code should check if tx.Tx is not nil before accessing tx.Tx.Tx.
| for tx := txset.Peek(); tx != nil; tx = txset.Peek() { | |
| for tx := txset.Peek(); tx != nil; tx = txset.Peek() { | |
| if tx.Tx == nil || tx.Tx.Tx == nil { | |
| t.Fatalf("unexpected nil transaction in txset") | |
| } |
| txset, _ := newTransactionsByPriceAndNonce(signer, groups, map[common.Address]*big.Int{}, nil) | ||
|
|
||
| txs := types.Transactions{} | ||
| for tx := txset.Peek(); tx != nil; tx = txset.Peek() { |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential nil pointer dereference: tx.Tx could be nil if Peek() returns a LazyTransaction with nil Tx field. The code should check if tx.Tx is not nil before accessing tx.Tx.Tx.
| for tx := txset.Peek(); tx != nil; tx = txset.Peek() { | |
| for tx := txset.Peek(); tx != nil; tx = txset.Peek() { | |
| if tx.Tx == nil || tx.Tx.Tx == nil { | |
| t.Fatalf("unexpected nil transaction in txset") | |
| } |
|
|
||
| warped := lazyTx.Resolve() | ||
| if warped == nil || warped.Tx == nil { | ||
| break |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential nil pointer dereference: if lazyTx.Resolve() returns a non-nil wrapper but with tx.Tx being nil, the code breaks. However, when warped is nil or warped.Tx is nil, the function breaks out of the loop. This could prematurely terminate transaction processing when there might be more valid transactions in the queue. Consider calling txs.Shift() or txs.Pop() instead of breaking, to move past the invalid transaction and continue processing other transactions.
| break | |
| log.Warn("Skipping invalid transaction wrapper") | |
| txs.Pop() | |
| continue |
Proposed changes
Ref: ethereum#26940
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