graphql: use a decimal representation for gas limit and gas used by atoulme · Pull Request #21883 · ethereum/go-ethereum

atoulme · 2020-11-21T02:26:27Z

No description provided.

renaynay · 2020-11-23T16:11:37Z

Here's the diff from Hive of the control (graphql tested on master) vs. on this branch:

diff graphql_control.log graphql_use_longs.log| colordiff
348,349c348,349
<       "gasLimit": "0x2fefd8",
<       "gasUsed": "0x5c21",
---
>       "gasLimit": 3141592,
>       "gasUsed": 23585,
407,484d406
< Test failed.
< HTTP response code: 200 OK
< expected: {
<   "data": {
<     "block": {
<       "account": {
<         "balance": "0x12c"
<       },
<       "difficulty": "0x20740",
<       "extraData": "0x",
<       "gasLimit": 3141592,
<       "gasUsed": 23585,
<       "hash": "0xc8df1f061abb4d0c107b2b1a794ade8780b3120e681f723fe55a7be586d95ba6",
<       "logsBloom": "0x00000000000000000000000800000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000040000000000000080000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000400000000000000000200000000000000000002000000000000000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000800000000040000000000000000000000000000000000000000010000000000000000000000000",
<       "miner": {
<         "address": "0x8888f1f195afa192cfee860698584c030f4c9db1"
<       },
<       "mixHash": "0x6ce1c4afb4f85fefd1b0ed966b20cd248f08d9a5b0df773f75c6c2f5cc237b7c",
<       "nonce": "0x5c321bd9e9f040f1",
<       "ommerAt": null,
<       "ommerCount": 0,
<       "ommerHash": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347",
<       "ommers": [],
<       "parent": {
<         "hash": "0xf8cfa377bd766cdf22edb388dd08cc149e85d24f2796678c835f3c54ab930803"
<       },
<       "receiptsRoot": "0x88b3b304b058b39791c26fdb94a05cc16ce67cf8f84f7348cb3c60c0ff342d0d",
<       "stateRoot": "0xdb46d6bb168130fe2cb60b4b24346137b5741f11283e0d7edace65c5f5466b2e",
<       "timestamp": "0x561bc336",
<       "totalDifficulty": "0x3e6cc0",
<       "transactionCount": 1,
<       "transactions": [
<         {
<           "hash": "0x9cc6c7e602c56aa30c554bb691377f8703d778cec8845f4b88c0f72516b304f4"
<         }
<       ],
<       "transactionsRoot": "0x5a8d5d966b48e1331ae19eb459eb28882cdc7654e615d37774b79204e875dc01"
<     }
<   }
< }
< got: {
<   "data": {
<     "block": {
<       "account": {
<         "balance": "0x12c"
<       },
<       "difficulty": "0x20740",
<       "extraData": "0x",
<       "gasLimit": "0x2fefd8",
<       "gasUsed": "0x5c21",
<       "hash": "0xc8df1f061abb4d0c107b2b1a794ade8780b3120e681f723fe55a7be586d95ba6",
<       "logsBloom": "0x00000000000000000000000800000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000040000000000000080000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000400000000000000000200000000000000000002000000000000000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000800000000040000000000000000000000000000000000000000010000000000000000000000000",
<       "miner": {
<         "address": "0x8888f1f195afa192cfee860698584c030f4c9db1"
<       },
<       "mixHash": "0x6ce1c4afb4f85fefd1b0ed966b20cd248f08d9a5b0df773f75c6c2f5cc237b7c",
<       "nonce": "0x5c321bd9e9f040f1",
<       "ommerAt": null,
<       "ommerCount": 0,
<       "ommerHash": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347",
<       "ommers": [],
<       "parent": {
<         "hash": "0xf8cfa377bd766cdf22edb388dd08cc149e85d24f2796678c835f3c54ab930803"
<       },
<       "receiptsRoot": "0x88b3b304b058b39791c26fdb94a05cc16ce67cf8f84f7348cb3c60c0ff342d0d",
<       "stateRoot": "0xdb46d6bb168130fe2cb60b4b24346137b5741f11283e0d7edace65c5f5466b2e",
<       "timestamp": "0x561bc336",
<       "totalDifficulty": "0x3e6cc0",
<       "transactionCount": 1,
<       "transactions": [
<         {
<           "hash": "0x9cc6c7e602c56aa30c554bb691377f8703d778cec8845f4b88c0f72516b304f4"
<         }
<       ],
<       "transactionsRoot": "0x5a8d5d966b48e1331ae19eb459eb28882cdc7654e615d37774b79204e875dc01"
<     }
<   }
< }
550,605d471
<     "block": {
<       "difficulty": "0x20740",
<       "extraData": "0x",
<       "gasLimit": 3141592,
<       "gasUsed": 23585,
<       "hash": "0xc8df1f061abb4d0c107b2b1a794ade8780b3120e681f723fe55a7be586d95ba6",
<       "logsBloom": "0x00000000000000000000000800000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000040000000000000080000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000400000000000000000200000000000000000002000000000000000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000800000000040000000000000000000000000000000000000000010000000000000000000000000",
<       "mixHash": "0x6ce1c4afb4f85fefd1b0ed966b20cd248f08d9a5b0df773f75c6c2f5cc237b7c",
<       "nonce": "0x5c321bd9e9f040f1",
<       "ommerCount": 0,
<       "ommerHash": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347",
<       "receiptsRoot": "0x88b3b304b058b39791c26fdb94a05cc16ce67cf8f84f7348cb3c60c0ff342d0d",
<       "stateRoot": "0xdb46d6bb168130fe2cb60b4b24346137b5741f11283e0d7edace65c5f5466b2e",
<       "timestamp": "0x561bc336",
<       "totalDifficulty": "0x3e6cc0",
<       "transactionCount": 1,
<       "transactions": [
<         {
<           "hash": "0x9cc6c7e602c56aa30c554bb691377f8703d778cec8845f4b88c0f72516b304f4"
<         }
<       ],
<       "transactionsRoot": "0x5a8d5d966b48e1331ae19eb459eb28882cdc7654e615d37774b79204e875dc01"
<     }
<   }
< }
< got: {
<   "data": {
<     "block": {
<       "difficulty": "0x20740",
<       "extraData": "0x",
<       "gasLimit": "0x2fefd8",
<       "gasUsed": "0x5c21",
<       "hash": "0xc8df1f061abb4d0c107b2b1a794ade8780b3120e681f723fe55a7be586d95ba6",
<       "logsBloom": "0x00000000000000000000000800000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000040000000000000080000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000400000000000000000200000000000000000002000000000000000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000800000000040000000000000000000000000000000000000000010000000000000000000000000",
<       "mixHash": "0x6ce1c4afb4f85fefd1b0ed966b20cd248f08d9a5b0df773f75c6c2f5cc237b7c",
<       "nonce": "0x5c321bd9e9f040f1",
<       "ommerCount": 0,
<       "ommerHash": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347",
<       "receiptsRoot": "0x88b3b304b058b39791c26fdb94a05cc16ce67cf8f84f7348cb3c60c0ff342d0d",
<       "stateRoot": "0xdb46d6bb168130fe2cb60b4b24346137b5741f11283e0d7edace65c5f5466b2e",
<       "timestamp": "0x561bc336",
<       "totalDifficulty": "0x3e6cc0",
<       "transactionCount": 1,
<       "transactions": [
<         {
<           "hash": "0x9cc6c7e602c56aa30c554bb691377f8703d778cec8845f4b88c0f72516b304f4"
<         }
<       ],
<       "transactionsRoot": "0x5a8d5d966b48e1331ae19eb459eb28882cdc7654e615d37774b79204e875dc01"
<     }
<   }
< }
< Test failed.
< HTTP response code: 200 OK
< expected: {
<   "data": {
1096,1097c962,963
<         "gasLimit": "0x2fefd8",
<         "gasUsed": "0x5c21",
---
>         "gasLimit": 3141592,
>         "gasUsed": 23585,
1106,1107c972,973
<         "gasLimit": "0x2fefd8",
<         "gasUsed": "0x5eef",
---
>         "gasLimit": 3141592,
>         "gasUsed": 24303,
1116,1117c982,983
<         "gasLimit": "0x2fefd8",
<         "gasUsed": "0x5c99",
---
>         "gasLimit": 3141592,
>         "gasUsed": 23705,

holiman · 2020-11-25T13:49:43Z

This now needs a rebase

renaynay

LGTM -- this change would increase geth's conformance to the graphql spec

atoulme · 2020-11-27T06:02:57Z

Rebased.

holiman

Some comments + questions

holiman · 2020-11-27T10:32:40Z

+	case string:
+		value, err := strconv.ParseUint(input, 10, 64)
+		*b = Long(value)
+		return err


This feels a bit breaking. Generally, when interacting with geth, you can feed it strings representing integers. In all those cases, the string is 0x-prefixed hex encodings. Now, in this particular instance, you have to feed it "10" instead of "0x0a".

So basically, some options are :

Allow 0x-prefixed hex-encoded strings on input, and non-0x-prefixed decimal encoded strings,

Only allow non-0x-prefixed decimal encoded strings (the way it is in this PR)

Reject string-encoded values

I'm not sure which is best, just wanted to raise the topic.

Hm, actually, I don't quite get this.
If I modify the testcase like so:

body := strings.NewReader(`{"query": "{block(number:\"0\"){number,gasUsed,gasLimit}}","variables": null}`)

Then I get

expected: "{\"data\":{\"block\":{\"number\":0,\"gasUsed\":0,\"gasLimit\":11500000}}}" actual : "{\"errors\":[{\"message\":\"hex string without 0x prefix\"}],\"data\":{}}"

So it appears that block number selectors on input are still required to be 0x-prefixed (if strings).

That's a bug. I'll fix it now.

The schema of the EIP is clear: decimals only. You can probably support strings for hex values but that's beyond what the spec recommends and won't be in line with other clients.

👍 ambiguity == bad, strictness == good

holiman · 2020-11-27T10:34:58Z

 	header, err := b.resolveHeader(ctx)
 	if err != nil {
-		return 0, err
+		return Long(0), err


explicit cast not needed, you can leave this as it was

Suggested change

return Long(0), err

return 0, err

holiman · 2020-11-27T21:54:08Z

I pushed some changes, mainly to make the tests table-driven. I also added a couple of tests, one which shows that a common graphql query to get a specific block now fails, due to some dissonance about whether the number is int32 or int64.

I don't really know why one is to be preferred over another (although geth typically uses a third option normally, uint64). So there are some TODO's still left here.

atoulme · 2020-11-28T04:34:47Z

OK, I fixed the tests. Take a look?

holiman

Almost done :)

holiman · 2020-11-28T08:59:20Z

+		code int
+	}{
+		{
+			name: "get_number",


IMO, making tests table-driven makes it easier to just dump in new test-vectors without having to do a lot of work. Having a name on each vector may look "nice", but also makes "dump in X new test vectors" more laborious.

Not a blocker though, leave them in if you feel like it

I'm old - I can't count. I get interrupted by kids when I count to 7.
I'll find a compromise.

holiman · 2020-11-28T09:00:33Z

+		},
+		// uncomment to test hex string support
+		//{
+		//	name: "string_0x0",


Yeah let's either uncomment or remove these. The tests run blazing fast now that they reuse the same stack, so IMO just uncomment them (and add whatever the response is) for more coverage.

The code enabling this behavior is commented out too.

My view on this: We should get the EIP updated before this gets uncommented.

I'll push a change to show what I mean. If we don't support it, the tests should be there anyway, to 'cement' the behaviour of not supporting it. If we decide to support it, then we change the test. Tests are good to prevent accidental changes.

holiman

LGTM. Notes for other reviewers, or release-notes. This PR contains some backwards-incompatible change, in that

hex-strings are no longer allowed in graphql selectors/filters, and
the number -1 can no longer be used to select latest.

fjl · 2020-12-08T09:42:25Z

Note to self: need to figure out if this breaks getting the latest block

holiman · 2020-12-12T09:19:58Z

Note to self: need to figure out if this breaks getting the latest block

Or perhaps @atoulme can answer: What is the 'correct' way to write a query that returns you the gas limit for the latest block?

Historically, the string latest or the number -1 have been used for this purpose, which IIUC is not supported with this PR.

atoulme · 2020-12-12T16:45:52Z

Spec says:

# Block fetches an Ethereum block by number or by hash. If neither is
    # supplied, the most recent known block is returned.

atoulme · 2020-12-12T16:46:37Z

And iirc that’s what geth code does. No number or hash passed in? Apply -1 for latest.

holiman · 2020-12-13T15:35:12Z

We should change the tests so that it actually contains some blocks, and not just a genesis.

holiman · 2020-12-14T07:58:44Z

I have now modified the test so that it imports 10 blocks, and indeed, unless a block number is given, it does return info about the latest block.
@fjl PTAL

atoulme requested a review from gballet as a code owner November 21, 2020 02:26

fjl changed the title ~~Use a decimal representation for gas limit and gas used~~ graphql: use a decimal representation for gas limit and gas used Nov 23, 2020

renaynay mentioned this pull request Nov 23, 2020

[WIP ] graphql: use Long for blockNumber, gas, estimateGas, and cumulativeGasUsed instead of hex string #21895

Closed

renaynay approved these changes Nov 25, 2020

View reviewed changes

renaynay approved these changes Nov 26, 2020

View reviewed changes

atoulme added 2 commits November 26, 2020 21:42

Use a decimal representation for gas limit and gas used

a99b602

Use Long for block number

6ebf10d

atoulme force-pushed the use_longs branch from f1fe93c to 6ebf10d Compare November 27, 2020 05:59

holiman reviewed Nov 27, 2020

View reviewed changes

atoulme and others added 3 commits November 27, 2020 08:36

Don't accept strings for block numbers

90f9e5f

graphql: remove unnecessary cast

0b69389

graphql: make tests table-based, simplify code

2939d6a

Code review changes

e35dbe6

Appease ci

eb9ad97

holiman reviewed Nov 28, 2020

View reviewed changes

atoulme and others added 2 commits November 28, 2020 10:24

Remove test names, print test body instead

fd6f7de

graphql: more test coverage

4186ada

holiman approved these changes Nov 29, 2020

View reviewed changes

holiman added the status:triage label Dec 1, 2020

fjl removed the status:triage label Dec 8, 2020

fjl added this to the 1.10.0 milestone Dec 8, 2020

holiman assigned fjl Dec 11, 2020

graphql: use a non-empty chain for tests

b8c15e6

renaynay added the status:triage label Jan 4, 2021

fjl removed the status:triage label Jan 5, 2021

fjl approved these changes Jan 5, 2021

View reviewed changes

fjl merged commit eb2a1df into ethereum:master Jan 5, 2021

renaynay mentioned this pull request Jan 10, 2021

simulators/ethereum/graphql: replace #42 test query with decimal instead of hex string ethereum/hive#419

Merged

renaynay mentioned this pull request Jan 18, 2021

graphql: Return Long for gas, gasUsed, cumulativeGasUsed, estimateGas, and status #20040

Closed

renaynay mentioned this pull request Feb 24, 2021

simulators/ethereum/graphql/testcases: change block number input to number instead of hexadecimal ethereum/hive#445

Merged

quorumbot mentioned this pull request Sep 3, 2021

[Upgrade] Go-Ethereum release v1.10.0 Consensys/quorum#1249

Merged

9 tasks

baptiste-b-pegasys mentioned this pull request Sep 3, 2021

[Upgrade] Go-Ethereum release v1.10.0 baptiste-b-pegasys/quorum#16

Closed

9 tasks

s1na mentioned this pull request Nov 9, 2022

graphql: fix return types to be compatible with schema definition #26118

Closed

s1na mentioned this pull request Mar 15, 2023

graphql: encode Long values as hex #26894

Merged

Conversation

atoulme commented Nov 21, 2020

Uh oh!

renaynay commented Nov 23, 2020

Uh oh!

holiman commented Nov 25, 2020

Uh oh!

renaynay left a comment

Choose a reason for hiding this comment

Uh oh!

atoulme commented Nov 27, 2020

Uh oh!

holiman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holiman commented Nov 27, 2020

Uh oh!

atoulme commented Nov 28, 2020

Uh oh!

holiman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holiman left a comment

Choose a reason for hiding this comment

Uh oh!

fjl commented Dec 8, 2020

Uh oh!

holiman commented Dec 12, 2020

Uh oh!

atoulme commented Dec 12, 2020

Uh oh!

atoulme commented Dec 12, 2020

Uh oh!

holiman commented Dec 13, 2020

Uh oh!

holiman commented Dec 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants