perf(evmrpc): eliminate redundant block fetches in simulate backend#3208
perf(evmrpc): eliminate redundant block fetches in simulate backend#3208amir-deris wants to merge 6 commits intomainfrom
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 #3208 +/- ##
==========================================
- Coverage 58.93% 58.78% -0.15%
==========================================
Files 2062 2054 -8
Lines 169019 168230 -789
==========================================
- Hits 99612 98896 -716
+ Misses 60686 60591 -95
- Partials 8721 8743 +22
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
amir-deris
changed the title
…ulate" This reverts commit 04d0539.
evmrpc/simulate.go
Outdated
| return nil, 0, false, err | ||
| } | ||
| return block.Block.Height, false, nil | ||
| return block, block.Block.Height, false, nil |
There was a problem hiding this comment.
If the function returns block, why also return block.Block.Height? The caller can infer what they need from block right?
evmrpc/simulate.go
Outdated
| func (b *Backend) getBlockHeight(ctx context.Context, blockNrOrHash rpc.BlockNumberOrHash) (int64, bool, error) { | ||
| // getResultBlockByNumberOrHash resolves blockNrOrHash to a Tendermint ResultBlock in one RPC path | ||
| // (by hash or by number, including latest). Callers can pass tmBlock to getHeader to avoid a second block fetch. | ||
| func (b *Backend) getResultBlockByNumberOrHash(ctx context.Context, blockNrOrHash rpc.BlockNumberOrHash) (*coretypes.ResultBlock, int64, bool, error) { |
There was a problem hiding this comment.
I would drop Result from the name of this function.
| } | ||
|
|
||
| func (b *Backend) getHeader(blockNumber *big.Int) *ethtypes.Header { | ||
| func (b *Backend) getHeader(ctx context.Context, blockNumber *big.Int, tmBlock *coretypes.ResultBlock) *ethtypes.Header { |
There was a problem hiding this comment.
Similarly, if this function takes block there is no need to also pass in the hight, right? Since it can get what it needs from the block itself.
There was a problem hiding this comment.
I believe the reason to have both blockNumber and tmBlock is in some methods that use the getHeader, the tmBlock is not available and only we have the blockNumber. For example in method CurrentHeader().
2 participants
Summary
Eliminates redundant block fetches in the simulate backend hot path (
eth_call,eth_estimateGas).Single block fetch per request: Added
getResultBlockByNumberOrHashso block number/hash (includinglatest) resolves once. TheResultBlockis passed intogetHeader, removing the secondblockByNumberRespectingWatermarkscall acrossStateAndHeaderByNumberOrHash,HeaderByNumber, andBlockByNumber.Request context in
getHeader:getHeadernow acceptsctxand uses it for the conditional block fetch andblockResultsWithRetry, so deadlines, cancellation, and tracing propagate from the RPC context instead ofcontext.Background().Stricter error handling: If the block cannot be loaded, the error surfaces rather than silently returning a header with
DefaultBlockGasLimit.simulate_testupdated accordingly (bcAlwaysFailClient).Out of scope
BlockByHashstill resolves by hash then callsBlockByNumber(two RPCs). Left unchanged to keep this PR small; can be deduplicated in a follow-up.