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)
Co-authored-by: Dat Duong <[email protected]> Signed-off-by: Yaroslav Kukharuk <[email protected]>
| } | ||
|
|
||
| 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 :)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3296 +/- ##
==========================================
- Coverage 76.20% 76.17% -0.04%
==========================================
Files 346 346
Lines 32690 32745 +55
==========================================
+ Hits 24912 24942 +30
- Misses 5987 6007 +20
- Partials 1791 1796 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
brbrr
changed the title
…only-block-number
…only-block-number
fixes #3257