Add EIP-1898 support needed for The Graph compatibility #10815

fridrik01 · 2023-05-03T17:24:55Z

Fixes: #10814
See also: https://github.com/filecoin-project/fvm-pm/issues/389

This PR updates the following RPC methods according to EIP-1898 specs.

The following RPC methods are affected:

eth_getBalance
eth_getStorageAt
eth_getTransactionCount
eth_getCode
eth_call

Note that eth_getBlockByNumber was not included in this list in the spec although it seems it should be affected also?

Currently these methods all accept a blkParam string which can be one of "latest", "earliest", "pending", or a block number (decimal or hex). The spec enables caller to additionally specify a json hash which can include the following fields:

blockNumber EthUint64: A block number (decimal or hex) which is similar to the original use of the blkParam string
blockHash EthHash: The block hash
requireCanonical bool) If true we should make sure the block is in the canonical chain

Since the blkParam needs to support both being a number/string and a json hash then this to properly work we need to introduce a new struct with pointer fields to check if they exist. This is done in the EthBlockParamByNumberOrHash struct which first tries to unmarshal as a json hash (according to eip-1898) and then fallback to unmarshal as string/number.

Test plan

Check that specifying by predefined blocks works:

curl  http://localhost:1234/rpc/v1  -X POST -H "Content-Type: application/json" --data '{"method":"eth_getTransactionCount","params":["0xd4c70007F3F502f212c7e6794b94C06F36173B36", "latest"],"id":1,"jsonrpc":"2.0"}'
{"jsonrpc":"2.0","result":"0x2b","id":1}

Check that specifying by number works:

curl -s http://localhost:1234/rpc/v1  -X POST -H "Content-Type: application/json" --data '{"method":"eth_getTransactionCount","params":["0xd4c70007F3F502f212c7e6794b94C06F36173B36", "0x2B229E"],"id":1,"jsonrpc":"2.0"}'
{"jsonrpc":"2.0","result":"0x2b","id":1}

Check that specifying by blockNumber works:

curl -s http://localhost:1234/rpc/v1  -X POST -H "Content-Type: application/json" --data '{"method":"eth_getTransactionCount","params":["0xd4c70007F3F502f212c7e6794b94C06F36173B36", {"blockNumber": "0x2B229E"}],"id":1,"jsonrpc":"2.0"}'
{"jsonrpc":"2.0","result":"0x2b","id":1}

Check that specifying by blockHash works:

curl -s http://localhost:1234/rpc/v1  -X POST -H "Content-Type: application/json" --data '{"method":"eth_getTransactionCount","params":["0xd4c70007F3F502f212c7e6794b94C06F36173B36", {"blockHash": "0x7f695f396c2782d772a46af38428ef8f27918ba8158b36854d547ff8e3b994b9"}],"id":1,"jsonrpc":"2.0"}'
{"jsonrpc":"2.0","result":"0x2b","id":1}

Check that specifying by blockHash works and when requireCanonical is set:

curl -s http://localhost:1234/rpc/v1  -X POST -H "Content-Type: application/json" --data '{"method":"eth_getTransactionCount","params":["0xd4c70007F3F502f212c7e6794b94C06F36173B36", {"blockHash": "0x7f695f396c2782d772a46af38428ef8f27918ba8158b36854d547ff8e3b994b9", "requireCanonical": true}],"id":1,"jsonrpc":"2.0"}'
{"jsonrpc":"2.0","result":"0x2b","id":1}

Check that specifying both blockNumber and blochHash should fail:

curl -s http://localhost:1234/rpc/v1  -X POST -H "Content-Type: application/json" --data '{"method":"eth_getTransactionCount","params":["0xd4c70007F3F502f212c7e6794b94C06F36173B36", {"blockNumber": "0x2B229E", "blockHash": "0x7f695f396c2782d772a46af38428ef8f27918ba8158b36854d547ff8e3b994b9", "requireCanonical": true}],"id":1,"jsonrpc":"2.0"}'
{"jsonrpc":"2.0","id":1,"error":{"code":-32700,"message":"unmarshaling params for 'eth_getTransactionCount' (param: *ethtypes.EthBlockParamByNumberOrHash): cannot specify both blockNumber and blockHash"}}

Check specifying by blockHash with requireCanonical is set where blockHash is not in the canonical chain:

TODO: Still trying to find a way to get a orphaned blockHash

Stebalien · 2023-05-08T20:46:38Z

api/docgen/docgen.go

@@ -479,6 +479,9 @@ func ExampleValue(method string, t, parent reflect.Type) interface{} {
 			es := exampleStruct(method, t.Elem(), t)
 			ExampleValues[t] = es
 			return es
+		} else if t.Elem().Kind() == reflect.String {


Should probably be a switch statement now.

Stebalien · 2023-05-08T20:56:54Z

chain/types/ethtypes/eth_types.go

+	PredefinedBlock *string
+	Number          *EthUint64


Why do we have two block number fields?

These should both be ignored by json (`json:"-"`). Or private.

@Stebalien I looked better into merging the Number/BlockNumber into one and I think it may not be such a great idea.

For context, the issue is that currently the blkParam can be one of "latest", "earliest", "pending", or a block number (decimal or hex) but with this PR we additionally need to support that blkParam to be able to be a json hash witch a certain named fields (blockNumber,blockHash,requireCanonical). To implement this, I introduced this EthBlockParamByNumberOrHash struct which has fields for both cases (normal as before + support for json hashes) and handles the al the construction/json deserialization.

It would be possible to merge the Number and BlockNumber in EthBlockParamByNumberOrHashinto one, but I would think that makes the code less readable since right now its easy to see from reading the code whether we are talking about a normal block number or a one we need to support for the EIP-1891. Also, it would make error handling cumbersome (like here).

Stebalien · 2023-05-08T21:02:08Z

chain/types/ethtypes/eth_types.go

@@ -815,3 +815,82 @@ func (e EthFeeHistoryParams) MarshalJSON() ([]byte, error) {
 	}
 	return json.Marshal([]interface{}{e.BlkCount, e.NewestBlkNum})
 }
+
+type EthBlockParamByNumberOrHash struct {
+	PredefinedBlock *string


I'd consider using negative constants to represent "latest", etc. Storing them in the BlockNumber field. I.e., we'd have three fields only:

const ( EthBlockLatest EthUint64 = -iota EthBlockPending EthBlockEarliest // ... ) type EthBlockParam struct { BlockNumber *EthUint64 BlockHash *EthHash RequireCanonical bool }

@Stebalien I do recall considering that but EthUint64 is an unsigned type, do you think this would warrant a new type?

Would love feedback from @filecoin-project/lotus-maintainers team on Stebs comment here.

Fixes: #10814 This PR updates the following RPC methods according to EIP-1898 specs. The following RPC methods are affected: - eth_getBalance - eth_getStorageAt - eth_getTransactionCount - eth_getCode - eth_call Note that eth_getBlockByNumber was not included in this list in the spec although it seems it should be affected also? Currently these methods all accept a blkParam string which can be one of "latest", "earliest", "pending", or a block number (decimal or hex). The spec enables caller to additionally specify a json hash which can include the following fields: - blockNumber EthUint64: A block number (decimal or hex) which is similar to the original use of the blkParam string - blockHash EthHash: The block hash - requireCanonical bool) If true we should make sure the block is in the canonical chain Since the blkParam needs to support both being a number/string and a json hash then this to properly work we need to introduce a new struct with pointer fields to check if they exist. This is done in the EthBlockParamByNumberOrHash struct which first tries to unmarshal as a json hash (according to eip-1898) and then fallback to unmarshal as string/number.

Stebalien

Two small changes but otherwise lgtm.

Stebalien · 2023-06-21T17:42:15Z

chain/types/ethtypes/eth_types.go

@@ -839,3 +839,80 @@ func (e EthFeeHistoryParams) MarshalJSON() ([]byte, error) {
 	}
 	return json.Marshal([]interface{}{e.BlkCount, e.NewestBlkNum})
 }
+
+type EthBlockNumberOrHash struct {
+	PredefinedBlock  *string


This field shouldn't be serialized to json.

Stebalien · 2023-06-21T17:48:04Z

node/impl/full/eth.go

+			// walk back the current chain (our head) until we reach targetHeight and validate the block hash
+			currTs := head
+			for {
+				if currTs.Equals(ts) {
+					return ts, nil
+				} else if currTs.Height() < ts.Height() {
+					return nil, fmt.Errorf("could not find block hash %s in canonical chain", blkParam.BlockHash.ToCid())
+				}
+
+				currTs, err = a.Chain.LoadTipSet(ctx, currTs.Parents())
+				if err != nil {
+					return nil, fmt.Errorf("failed to load tipset: %v", err)
+				}
+			}


Call ChainGetTipSetByHeight and let that walk for you. That'll use the index/skip-list.

Then, when you have the tipset at the target height:

Make sure it's actually at the target height (nulls, etc.). Just in case.

Make sure it equals the expected tipset.

Stebalien · 2023-06-21T18:15:41Z

node/impl/full/eth.go

+
+	predefined := blkParam.PredefinedBlock
+	if predefined != nil {
+		if *predefined == "earliest" {


This should probably be a switch, but it's not critical.

Stebalien · 2023-06-21T18:35:30Z

Looks like the final issue is the linter not being happy about some error formatting.

fridrik01 · 2023-06-21T19:29:51Z

Looks like the final issue is the linter not being happy about some error formatting.

I need to look better into it tomorrow, but I think its because of the change of ignoring PredefinedBlock when marshaling json.

Stebalien · 2023-06-21T19:43:48Z

Well, the linter is mad about an Errorf.

fridrik01 · 2023-06-22T12:18:11Z

@Stebalien There were several failed tests that were due to the itest/testkit creating a client and setting the ThroughRPC() option. This causes the struct to always be serialized to JSON and in case of testing the predefined blocks (like here) then when we try to unmarshal the struct, we get an empty hash {} which results in a failure.

To address this we can either:

Serialize this field as json
Disable RPC in our tests
Merge predefined block field with the BlockNumber but that would require creating a new EthInt64 type which would require a big refactor in the codebase as this is used everywhere.

I updated the PR with 1) in mind, but let me know if you would prefer other options

Stebalien · 2023-06-22T16:02:14Z

I believe the correct thing to do is to marshal it into the blockNumber field.

Stebalien · 2023-06-22T16:04:11Z

Well, it's unclear. That's what go-ethereum would do, but it's probably not correct.

IMO, the "most correct" thing to do would be to implement MarshalJSON and:

If a predefined block is specified, marshal to just that as a string.
Otherwise, marshal as an object.

Stebalien · 2023-06-22T16:04:58Z

My concern with the current approach is that we're now exposing predefiendBlock as a field on the API, when it's supposed to just be an implementation detail.

fridrik01 · 2023-06-22T17:14:54Z

Well, it's unclear. That's what go-ethereum would do, but it's probably not correct.

IMO, the "most correct" thing to do would be to implement MarshalJSON and:

If a predefined block is specified, marshal to just that as a string.

Otherwise, marshal as an object.

That should work, just create a tmp struct like in Unmarshal, let me do that!

fridrik01 force-pushed the 10814-eip-1891 branch 6 times, most recently from 32c4244 to 8a64b50 Compare May 3, 2023 21:27

fridrik01 changed the title ~~WIP~~ Add EIP-1898 support to certain RPC methods needed for The Graph compatibility May 4, 2023

fridrik01 requested review from Stebalien, raulk and maciejwitowski May 4, 2023 11:18

fridrik01 force-pushed the 10814-eip-1891 branch from 8a64b50 to 4ebb6a6 Compare May 4, 2023 11:37

fridrik01 changed the title ~~Add EIP-1898 support to certain RPC methods needed for The Graph compatibility~~ Add EIP-1898 support needed for The Graph compatibility May 4, 2023

fridrik01 assigned fridrik01 and unassigned fridrik01 May 4, 2023

fridrik01 force-pushed the 10814-eip-1891 branch 2 times, most recently from 9e18cbf to c9a6734 Compare May 4, 2023 11:54

fridrik01 marked this pull request as ready for review May 4, 2023 12:12

fridrik01 requested a review from a team as a code owner May 4, 2023 12:12

Stebalien reviewed May 8, 2023

View reviewed changes

fridrik01 force-pushed the 10814-eip-1891 branch from c9a6734 to c852491 Compare May 10, 2023 21:03

jennijuju requested a review from snissn May 19, 2023 16:05

fridrik01 requested a review from a team May 27, 2023 11:00

fridrik01 force-pushed the 10814-eip-1891 branch from c852491 to f358160 Compare June 20, 2023 09:38

Fix lint errors

eed38a3

fridrik01 force-pushed the 10814-eip-1891 branch from 189e09e to eed38a3 Compare June 21, 2023 15:34

Stebalien reviewed Jun 21, 2023

View reviewed changes

fridrik01 force-pushed the 10814-eip-1891 branch 2 times, most recently from f6136e0 to 07ac21c Compare June 21, 2023 18:12

Stebalien approved these changes Jun 21, 2023

View reviewed changes

fridrik01 force-pushed the 10814-eip-1891 branch from 07ac21c to 862f623 Compare June 21, 2023 19:02

fridrik01 force-pushed the 10814-eip-1891 branch 4 times, most recently from df2037e to b7d7d4a Compare June 22, 2023 11:43

Refactor EthBlockNumberOrHash and remove the number field

dda2d7e

fridrik01 force-pushed the 10814-eip-1891 branch from b7d7d4a to dda2d7e Compare June 22, 2023 17:18

Stebalien approved these changes Jun 22, 2023

View reviewed changes

fridrik01 merged commit a2431ff into master Jun 23, 2023
93 checks passed

fridrik01 deleted the 10814-eip-1891 branch June 23, 2023 08:29

adlrocha mentioned this pull request Aug 10, 2023

ETH: Implement EIP-1898 support to ETH RPC consensus-shipyard/ipc#230

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EIP-1898 support needed for The Graph compatibility #10815

Add EIP-1898 support needed for The Graph compatibility #10815

fridrik01 commented May 3, 2023 •

edited

Stebalien May 8, 2023

Stebalien May 8, 2023

fridrik01 Jun 21, 2023

Stebalien May 8, 2023

fridrik01 May 9, 2023 •

edited

fridrik01 May 27, 2023

Stebalien left a comment

Stebalien Jun 21, 2023

Stebalien Jun 21, 2023

Stebalien Jun 21, 2023

Stebalien Jun 21, 2023

Stebalien commented Jun 21, 2023

fridrik01 commented Jun 21, 2023

Stebalien commented Jun 21, 2023

fridrik01 commented Jun 22, 2023

Stebalien commented Jun 22, 2023

Stebalien commented Jun 22, 2023

Stebalien commented Jun 22, 2023

fridrik01 commented Jun 22, 2023

Add EIP-1898 support needed for The Graph compatibility #10815

Add EIP-1898 support needed for The Graph compatibility #10815

Conversation

fridrik01 commented May 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fridrik01 May 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Jun 21, 2023

fridrik01 commented Jun 21, 2023

Stebalien commented Jun 21, 2023

fridrik01 commented Jun 22, 2023

Stebalien commented Jun 22, 2023

Stebalien commented Jun 22, 2023

Stebalien commented Jun 22, 2023

fridrik01 commented Jun 22, 2023

fridrik01 commented May 3, 2023 •

edited

fridrik01 May 9, 2023 •

edited