chore (evmrpc): fix eth_getBlockTransactionCountByHash hash lookup consistency#3069
Conversation
mojtaba-esk
changed the title
|
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 #3069 +/- ##
==========================================
+ Coverage 58.41% 58.58% +0.16%
==========================================
Files 2081 2080 -1
Lines 171790 171579 -211
==========================================
+ Hits 100352 100520 +168
+ Misses 62504 62140 -364
+ Partials 8934 8919 -15
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
masih
left a comment
masih
left a comment
There was a problem hiding this comment.
No blockers, left a few suggestions.
Also worth looking at:
GetBlockReceiptsandGetBlockNumberByNrOrHashwhich are not covered by fixes here. If a client passes the genesis hash to eth_getBlockReceipts, it will fail with the same error this PR fixes for the other two endpoints.GetBlockTransactionCountByNumber("0x0")also needs fixing right?GetBlockByNumberExcludeTraceFailneeds genesis check too.
🚀
evmrpc/block.go
Outdated
| // must recognize this so that count/block-by-hash stay consistent with block-by-number. | ||
| var GenesisBlockHash = common.HexToHash("0xF9D3845DF25B43B1C6926F3CEDA6845C17F5624E12212FD8847D0BA01DA1AB9E") | ||
|
|
||
| func encodeGenesisBlock() map[string]interface{} { |
There was a problem hiding this comment.
use any instead of interface{} whenever you can. I am hoping to slowly phase out the old syntax.
evmrpc/block.go
Outdated
| func encodeGenesisBlock() map[string]interface{} { | ||
| return map[string]interface{}{ | ||
| "number": (*hexutil.Big)(big.NewInt(0)), | ||
| "hash": "0xF9D3845DF25B43B1C6926F3CEDA6845C17F5624E12212FD8847D0BA01DA1AB9E", |
There was a problem hiding this comment.
Extract the hash as a const and use in both places?
There was a problem hiding this comment.
yeah b/c it used more than once, it makes sense to do it.
evmrpc/block.go
Outdated
| startTime := time.Now() | ||
| defer recordMetricsWithError(fmt.Sprintf("%s_getBlockTransactionCountByHash", a.namespace), a.connectionType, startTime, returnErr) | ||
| if blockHash == GenesisBlockHash { | ||
| return a.getEvmTxCount(nil, 0), nil |
There was a problem hiding this comment.
This is correct but i worry that it is fragile. a simple refactor in getEvmTxCount could cause panics given the nil txs passed in.
Why not use hexutil.Uint(0) directly, or whatever that results in? You can even define it as a var/const.
There was a problem hiding this comment.
Well when it is nil it does not go to the loop, but you're right in sense of being fragile so I will have a package-level *hexutil.Uint(0) (genesisBlockTxCount) for the genesis block only and so avoid depending on its handling of nil.
…sh-hash-lookup-consistency
Co-authored-by: Masih H. Derkani <m@derkani.org>
Thank you Masih jan for the insight, yes I think it worth tracking them and make sure we get consistent behaviour across the endpoints. |
2 participants
Describe your changes and provide context
Closes PLT-164
Ref: PLT-164
Testing performed to validate your change
=== RUN TestEVMRPCSpec/eth_getBlockTransactionCountByHash/get-block-n.iox === RUN TestEVMRPCSpec/eth_getBlockTransactionCountByHash/get-genesis.iox --- PASS: TestEVMRPCSpec/eth_getBlockTransactionCountByHash/get-block-n.iox (0.00s) --- PASS: TestEVMRPCSpec/eth_getBlockTransactionCountByHash/get-genesis.iox (0.00s)