-
Notifications
You must be signed in to change notification settings - Fork 863
feat: add ledger cache layer for receipt store #2788
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2788 +/- ##
==========================================
- Coverage 46.94% 46.91% -0.03%
==========================================
Files 1965 1969 +4
Lines 160525 160784 +259
==========================================
+ Hits 75363 75438 +75
- Misses 78637 78801 +164
- Partials 6525 6545 +20
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| type cacheWarmupProvider interface { | ||
| warmupReceipts() []ReceiptRecord |
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.
Does warm up means we will load some receipts into cache during initialization?
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.
yes it is used for parquet to replay from WAL
| s.cacheNextRotate = blockNumber + s.cacheRotateInterval | ||
| return | ||
| } | ||
| for blockNumber >= s.cacheNextRotate { |
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.
Why not use blockNumber % interval == 0?
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.
we could miss the rotation if the blockNumber % interval == 0 block does not have EVM receipts for some reason.
| receipt, found := blockReceipts[txHash] | ||
| if found { | ||
| // Callers (e.g. RPC response formatting) may normalize TransactionIndex in-place. | ||
| // Clone to avoid mutating the cached receipt and corrupting future lookups. |
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.
For perf reason, do we want to default to zero copy? As long as we make sure the codebase doesn't modify the receipt after calling get, we should be able to enable zeroCopy
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.
I think there are other places we do modify the receipt actually for example:
there are multiple mutations in evmrpc/tx.go:
Lines 154-165 (for failed txs that used 0 gas):
receipt.From = from.Hex()
receipt.To = etx.To().Hex()
receipt.ContractAddress = ""
receipt.TxType = uint32(etx.Type())
receipt.Status = uint32(ethtypes.ReceiptStatusFailed)
receipt.GasUsed = 0
Line 456 (tx index normalization):
receipt.TransactionIndex = uint32(evmTxIndex)
yzang2019
left a 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.
Overall LGTM
|
|
||
| type receiptGetter func(ctx sdk.Context, txHash common.Hash) (*types.Receipt, error) | ||
|
|
||
| func filterLogsFromReceipts(ctx sdk.Context, blockHeight int64, blockHash common.Hash, txHashes []common.Hash, crit filters.FilterCriteria, applyExactMatch bool, getReceipt receiptGetter) ([]*ethtypes.Log, error) { |
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.
is filterLogsFromReceipts being used?
| blockHash := common.BytesToHash(block.BlockID.Hash) | ||
|
|
||
| // Fetch receipts individually and filter logs locally | ||
| var logIndex uint |
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.
it looks we are fetches every receipt and iterates every log with matchesCriteria skipping bloom filter checks. for blocks with many transactions and few matching logs, could this introduce some performance regression?
| log.TxIndex = uint(evmTxIndex) //nolint:gosec | ||
| log.BlockNumber = uint64(blockHeight) //nolint:gosec | ||
| log.BlockHash = blockHash | ||
| if isLogExactMatch(log, crit) { |
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.
what's the difference between matchesCriteria() and isLogExactMatch()
| require.NoError(t, err) | ||
| // FilterLogs now takes fromBlock, toBlock range | ||
| collectedLogs, err := store.FilterLogs(ctx, 2, 2, filters.FilterCriteria{}) | ||
| // Note: this test was using pebble backend which now returns ErrRangeQueryNotSupported |
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.
do we have testings to cover original test's behavior like a test verified that TransactionIndex was set correctly during log collection?
Summary
This PR introduces an in-memory cache layer on top of the pebbledb receipt store backend, improving
GetReceiptperformance for recently accessed receipts.cachedReceiptStorewrapper that caches receipts in rotating chunksFilterLogsAPI: change from per-block signature to range-based(fromBlock, toBlock, crit)FilterLogsreturnsErrRangeQueryNotSupported, signaling callers to fetch receipts individuallyevmrpc/filter.gowith fallback logic for backends that don't support range queriesReceiptStoreConfigby removing unused pebble optionsThe cache rotates every 500 blocks (configurable) and keeps 3 chunks, providing a sliding window of recent receipts for fast lookups.
Test plan
ErrRangeQueryNotSupportedfor pebble backend