chore(rpc): Fetch only block number for TransactionByBlockIDAndIndex #3296
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes the TransactionByBlockIDAndIndex method by fetching only the block number instead of the full block header. It introduces a new BlockNumberByHash method in the blockchain reader interface to support this optimization. However, the implementation has critical bugs that break existing functionality for latest and l1Accepted block ID types.
- Adds
BlockNumberByHashmethod to blockchain reader interface for efficient block number lookup - Refactors
TransactionByBlockIDAndIndexto avoid fetching full headers for hash-based lookups - Introduces bugs for
latestandl1Acceptedblock ID types
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| rpc/v9/transaction.go | Refactors TransactionByBlockIDAndIndex to use block number directly instead of fetching full header, but incorrectly returns ErrBlockNotFound for latest and l1Accepted block types |
| blockchain/blockchain.go | Adds BlockNumberByHash interface method and implementation to retrieve block number from hash without fetching full header |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rpc/v9/transaction.go
Outdated
| } | ||
| case number: | ||
| blockNumber = blockID.Number() | ||
| case l1Accepted, preConfirmed, latest: |
There was a problem hiding this comment.
The latest and l1Accepted cases should resolve to their respective block numbers, not return ErrBlockNotFound. Based on blockHeaderByID in helpers.go (lines 81-93), latest should use HeadsHeader() to get the block number, and l1Accepted should call L1Head() to get the block number. The current implementation breaks existing functionality for these block ID types.
| case l1Accepted, preConfirmed, latest: | |
| case l1Accepted: | |
| header := h.L1Head() | |
| if header == nil { | |
| return nil, rpccore.ErrBlockNotFound | |
| } | |
| blockNumber = header.Number | |
| case latest: | |
| header := h.HeadsHeader() | |
| if header == nil { | |
| return nil, rpccore.ErrBlockNotFound | |
| } | |
| blockNumber = header.Number | |
| case preConfirmed: |
There was a problem hiding this comment.
l1Accepted should be like this, but use ChainHeight() for latest and PendingData() for preconfirmed.
There was a problem hiding this comment.
preConfirmed case is covered above the switch case. Also, it seems that we could follow the same logic for latest (assuming h.bcReader.Head() is better than following default code path)
| } | ||
|
|
||
| txn, err := h.bcReader.TransactionByBlockNumberAndIndex(header.Number, uint64(txIndex)) | ||
| txn, err := h.bcReader.TransactionByBlockNumberAndIndex(blockNumber, uint64(txIndex)) |
There was a problem hiding this comment.
We're having 4 versions in v6, v7, v8 and v9, please also fix for the other versions.
There was a problem hiding this comment.
yup, I'll update other versions, once the v9 implementation is good enough :)
There was a problem hiding this comment.
No need v8, v9 and v10 only please
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3296 +/- ##
==========================================
- Coverage 76.24% 76.18% -0.07%
==========================================
Files 349 349
Lines 33092 33147 +55
==========================================
+ Hits 25231 25252 +21
- Misses 6063 6094 +31
- Partials 1798 1801 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
brbrr
changed the title
rodrodros
force-pushed
the
chore/getTransactionByBlockIdAndIndex-fetch-only-block-number
branch
from
008aa87 to
7c449a5
Compare
Co-authored-by: Dat Duong <DuongDat.Informatics@rocketmail.com> Signed-off-by: Yaroslav Kukharuk <i.kukharuk@gmail.com>
rodrodros
force-pushed
the
chore/getTransactionByBlockIdAndIndex-fetch-only-block-number
branch
from
7c449a5 to
85c4b99
Compare
fixes #3257