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.58% 58.59%
=======================================
Files 2080 2080
Lines 171656 171661 +5
=======================================
+ Hits 100567 100577 +10
+ Misses 62166 62159 -7
- Partials 8923 8925 +2
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.
🚀
| // 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.
|
|
||
| // GenesisBlockHash is the block hash returned by GetBlockByNumber("0x0"). Hash-based lookups | ||
| // must recognize this so that count/block-by-hash stay consistent with block-by-number. | ||
| var GenesisBlockHash = common.HexToHash("0xF9D3845DF25B43B1C6926F3CEDA6845C17F5624E12212FD8847D0BA01DA1AB9E") |
There was a problem hiding this comment.
I recommend not exporting this variable if we don't need to.
| var GenesisBlockHash = common.HexToHash("0xF9D3845DF25B43B1C6926F3CEDA6845C17F5624E12212FD8847D0BA01DA1AB9E") | |
| var genesisBlockHash = common.HexToHash("0xF9D3845DF25B43B1C6926F3CEDA6845C17F5624E12212FD8847D0BA01DA1AB9E") |
| 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?
| 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.
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)