-
Notifications
You must be signed in to change notification settings - Fork 104
fix(rpc): align jsonrpc apis with geth v1.16.3 #599
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
- TestGetBlockTransactionCountByHash - TestGetBlockTransactionCountByNumber - TestGetEthBlockFromComet
GetLogs(blockHash common.Hash) ([][]*ethtypes.Log, error) | ||
GetLogsByHeight(*int64) ([][]*ethtypes.Log, error) | ||
BlockBloom(blockRes *coretypes.ResultBlockResults) (ethtypes.Bloom, error) | ||
BlockBloomFromCometBlock(blockRes *coretypes.ResultBlockResults) (ethtypes.Bloom, 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.
why did we need to change this name?
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 wanted to separate data conversion functions and real rpc backend query functions.
In case of BlockBloom
, there is no api like eth_getBlockBloom
and it is just util function that converse cometbft block result to ethereum block bloom.
It should be removed from Backend interface. But now, to avoid confusion, I just rename function.
I think we need to discuss how we can store ethereum backend data like block header, transactionRoots, receiptsRoot, blockBlooms, ... at indexer level after v0.5.0
. Then I think this conversion function can be located in indexer/utils.
// GetHeaderByNumber returns the requested canonical block header. | ||
// - When blockNr is -1 the chain pending header is returned. | ||
// - When blockNr is -2 the chain latest header is returned. | ||
// - When blockNr is -3 the chain finalized header is returned. | ||
// - When blockNr is -4 the chain safe header is returned. | ||
func (e *PublicAPI) GetHeaderByNumber(ethBlockNum rpctypes.BlockNumber) (map[string]interface{}, error) { | ||
e.logger.Debug("eth_getHeaderByNumber", "number", ethBlockNum) | ||
return e.backend.GetHeaderByNumber(ethBlockNum) | ||
} | ||
|
||
// GetHeaderByHash returns the requested header by hash. | ||
func (e *PublicAPI) GetHeaderByHash(hash common.Hash) (map[string]interface{}, error) { | ||
e.logger.Debug("eth_getHeaderByHash", "hash", hash.Hex()) | ||
return e.backend.GetHeaderByHash(hash) | ||
} |
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.
should we define type Header = map[string]any
to be used here?
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 don't think so.
I think the effect of defining a type is that it forces other packages or clients to import and use that type, while also allowing the use of interfaces like Validate, Marshal/Unmarshal, and Encode/Decode.
However, headers, blocks, and other data returned by the Ethereum RPC server can vary in their fields depending on the ChainConfig (hardfork config), and even when the fields are the same, as with transactions, the representation may differ (e.g., common.Hash or a full Tx struct) depending on the query parameters (e.g. fullTx
). Because of this, I don’t think we can enforce a type here, nor would it provide much benefit.
This design is consistent with the current go-ethereum implementation, and I don’t see a reason to implement it differently.
- **WebSocket**: <LOCAL_HOST>:8546 | ||
- **Cosmos REST**: <LOCAL_HOST>:1317 | ||
- **Tendermint RPC**:<LOCAL_HOST>:26657 | ||
- **gRPC**: localhost:9090 |
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 update this too?
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 is because ci (markdown link check) fails (ref)
It says this link is not available in github-ci instance.
I don't know the reason why it did not fail in past but fail currently...
|
||
- **JSON-RPC**: http://localhost:8547 | ||
- **JSON-RPC**: <LOCAL_HOST>:8547 | ||
- **WebSocket**: ws://localhost:8548 |
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.
same
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.
same (#599 (comment))
Description
Closes: #446 #447
Overview
The primary purpose of this PR was to make the return values of the Ethereum APIs compatible with go-ethereum v1.16.3.
However, as I progressed with the work, I found that methods for obtaining the same values had been developed in a disorganized way without clear separation of layers. It was obvious that leaving them as-is would make it very difficult to maintain compatibility with go-ethereum.
Therefore, I refactored the backend methods to be properly separated by layer
comet block query → comet-to-eth conversion → eth-to-rpc conversion
, making it easier to later migrate thecomet block subscription → comet-to-eth conversion & indexing
process to the indexer.Please take this into consideration during the code review.
What is fixed?
1. Add missing apis
eth_getHeaderByNumber
eth_getHeaderByHash
2. Fix return values of
eth
namespace apisBlock Header
withdrawals ([]*Withdrawal)
,header.excessBlobGas (*uint64)
,header.blobGasUsed (*common.Hash)
,header.parentBeaconBlockRoot (*common.Hash)
,requestsHash (*common.Hash)
,header.withdrawalsRoot (*common.Hash)
receiptsRoot (*common.Hash)
transactionsRoot (common.Hash)
,Time
transactionRoot
: Remove cosmos tx from root hash calculation,Time
: change from local timestamp to UTC timestamptotalDifficulty (*hexutil.Big)
Related APIs
eth_getHeaderByNumber
eth_getHeaderByHash
eth_getBlockByNumber
eth_getBlockByHash
Block Body
size
Related APIs
eth_getBlockByNumber
eth_getBlockByHash
Transaction
yParity
Related APIs
eth_getBlockByNumber
eth_getBlockByHash
eth_getTransactionByHash
eth_getTransactionByBlockHashAndIndex
eth_getTransactionByBlockNumberAndIndex
3. Fix internal header queries
Before
transactionRoot
,receiptsRoot
,bloom
,gasUsed
, ...) that needs block body.After
Limitation
RPCStream
does not subscribe cometBlockHeader but ethBlockHeader.4. Fix
txpool
apisNewRPCTransaction
5. Fix outdated blockNumber aliases to be aligned with go-ethereum v1.16.3
Before
After
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
main
branch