fix: getBlockWithTx* fetch block header and txns separately #3322
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors block fetching to separate header and transaction retrieval as part of fixing issue #3261. The change aims to optimize performance by allowing header and transaction data to be fetched independently rather than always fetching the entire block.
Key changes:
- Added new
blockTxnsByNumberhelper method andTransactionsByBlockNumberblockchain reader method to fetch transactions separately - Refactored
BlockWithTxHashesandBlockWithTxsto callblockHeaderByIDandblockTxnsByNumberseparately instead ofblockByID - Updated
blockStatussignature to accept block number directly instead of the full block
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| rpc/v9/helpers.go | Added new blockTxnsByNumber helper to fetch transactions by block number |
| rpc/v9/block.go | Refactored BlockWithTxHashes and BlockWithTxs to use separate header and transaction fetches; updated blockStatus to accept block number |
| blockchain/blockchain.go | Added new TransactionsByBlockNumber method to Reader interface and implementation |
| mocks/mock_blockchain.go | Added mock implementation for TransactionsByBlockNumber method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
brbrr
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3322 +/- ##
==========================================
+ Coverage 76.25% 76.26% +0.01%
==========================================
Files 351 351
Lines 33340 33443 +103
==========================================
+ Hits 25423 25506 +83
- Misses 6103 6108 +5
- Partials 1814 1829 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // https://github.com/starkware-libs/starknet-specs/blob/cce1563eff702c87590bad3a48382d2febf1f7d9/api/starknet_api_openrpc.json#L25 | ||
| func (h *Handler) BlockWithTxHashes(id *rpcv9.BlockID) (*BlockWithTxHashes, *jsonrpc.Error) { | ||
| block, rpcErr := h.blockByID(id) | ||
| header, rpcErr := h.blockHeaderByID(id) |
There was a problem hiding this comment.
question, this PR basically makes a trade-off of reducing fetched data size (get rid of unnecessary receipts field) for the tradeoff of doing 2 DB reads instead of 1 right?
There was a problem hiding this comment.
hm, from my understanding, blockByID does 3 DB reads (header, txns, receipts), where this PR makes only 2 DB reads. I'm not sure how indexed batches work (they're used in blockByID), and potentially we might be performing a single database operation, but still, we are pulling unnecessary receipt data, which I assume will incur some overhead anyway.
| txns := pending.GetTransactions() | ||
| return txns, nil | ||
| default: | ||
| txns, err := h.bcReader.TransactionsByBlockNumber(blockID.Number()) |
There was a problem hiding this comment.
btw, I don't think we have this method anymore. Since we have the new typed buckets, we fetch the transactions by block number and index. I think this will require a new helper function, where we can pass the number of transactions from header
There was a problem hiding this comment.
I think it's there, as I can find it locally and the CI is not complaining 🤷
Signed-off-by: Yaroslav Kukharuk <i.kukharuk@gmail.com>
fixes #3261