Skip to content
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

Hex number as parameter for start/count on engine_getPayloadBodiesByRange results in error #26623

Closed
marioevz opened this issue Feb 6, 2023 · 6 comments · Fixed by #26624
Closed
Labels

Comments

@marioevz
Copy link
Member

marioevz commented Feb 6, 2023

System information

Geth version: Geth/v1.11.0-unstable-3a5aceed/linux-amd64/go1.19.5
CL client & version: hive ethereum/hive@97d9b7b
OS & Version: Linux
Commit hash : 3a5acee

Expected behaviour

Sending QUANTITY value (see https://github.com/ethereum/execution-apis/blob/main/src/engine/common.md#encoding) as start and/or count parameters for engine_getPayloadBodiesByRangeV1 (see https://github.com/ethereum/execution-apis/blob/main/src/engine/shanghai.md#request-4) should return a valid response.

Actual behaviour

Sending

{"jsonrpc":"2.0","id":9,"method":"engine_getPayloadBodiesByRangeV1","params":["0x1","0x1"]}

Results in:

{"jsonrpc":"2.0","id":9,"error":{"code":-32602,"message":"invalid argument 0: json: cannot unmarshal string into Go value of type uint64"}}

See logs: https://hivetests2.ethdevops.io/suite.html?suiteid=1675704876-3f3a1bd514cc40ba5dfbe08c7a775cb8.json&suitename=engine-withdrawals#test-213

Steps to reproduce the behaviour

Run hive engine-withdrawals test suite on latest branch using:

./hive --client go-ethereum --sim ethereum/engine --sim.limit "engine-withdrawals/GetPayloadBodiesByRange"
@jwasinger jwasinger self-assigned this Feb 7, 2023
@jwasinger
Copy link
Contributor

I've made #26624 to address this. The hive test still fails, but now the failure is in the rpc method logic (or the hive test? I haven't looked further).

log excerpt from running the test now:

>> (2d62df26) {"jsonrpc":"2.0","id":9,"method":"engine_getPayloadBodiesByRangeV1","params":["0x1","0x1"]}
<< (2d62df26) {"jsonrpc":"2.0","id":9,"result":[{"transactions":[]}]}
>> (2d62df26) {"jsonrpc":"2.0","id":10,"method":"engine_getPayloadBodiesByRangeV1","params":["0x2","0x1"]}
<< (2d62df26) {"jsonrpc":"2.0","id":10,"result":null}

@holiman holiman linked a pull request Feb 7, 2023 that will close this issue
@marioevz
Copy link
Member Author

marioevz commented Feb 7, 2023

Hey @jwasinger, I have not rerun the test myself yet but this looks incorrect because, if we are requesting beyond the latest known block number, it should respond with empty array, instead of null.
If we are requesting a known block, it should respond with the payload body, but either way I don't think it should respond with null at any point.

@marioevz
Copy link
Member Author

marioevz commented Feb 7, 2023

I ran the test using the fix branch and there is something definitely going on:
Latest block is B2, which was correctly set as head using engine_forkchoiceUpdated, and then we request it by:

{
  "jsonrpc": "2.0",
  "id": 8,
  "method": "eth_getBlockByNumber",
  "params": [
    "0x2",
    false
  ]
}

And we get a block:

{
  "jsonrpc": "2.0",
  "id": 8,
  "result": {
    "baseFeePerGas": "0x2da282a8",
    "difficulty": "0x0",
    "extraData": "0xd883010b00846765746888676f312e31392e35856c696e7578",
    "gasLimit": "0x3007cf",
    "gasUsed": "0x0",
    "hash": "0x44d438a288837cc9b79b6d3f1c3617f67476de43f9b12f24d1ce3d0a286a3a23",
    "logsBloom": "0x00...",
    "miner": "0x00...",
    "mixHash": "0xd7fd5146a2cc3df501b34ceb50a92e44ad7604fc93f227d261c46ab3c8172020",
    "nonce": "0x0000000000000000",
    "number": "0x2",
    "parentHash": "0x759c2d610a33755817382e8f9e1caa0a899522a6850e7549b2a560c1acfa75de",
    "receiptsRoot": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421",
    "sha3Uncles": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347",
    "size": "0x23a",
    "stateRoot": "0x0a1d0145d9c5a1da81dd9a48dd9758d43366a076c3c1d0da18126fc4e81cb104",
    "timestamp": "0x1236",
    "totalDifficulty": "0x0",
    "transactions": [],
    "transactionsRoot": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421",
    "uncles": [],
    "withdrawals": [],
    "withdrawalsRoot": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421"
  }
}

Then we request:

{
  "jsonrpc": "2.0",
  "id": 10,
  "method": "engine_getPayloadBodiesByRangeV1",
  "params": [
    "0x2",
    "0x1"
  ]
}

Which should return:

{
  "jsonrpc": "2.0",
  "id": 10,
  "result": [
    {
      "transactions": [],
      "withdrawals": []
    }
  ]
}

@jwasinger jwasinger removed their assignment Feb 8, 2023
@jwasinger
Copy link
Contributor

Unassigned myself. If this is easy for anyone else, feel free to pick it up. I will continue looking into this but I have to do some digging/learning to figure out how to properly debug in Hive.

Afaict, the issue is improper serialization of the result from GetPayloadBodiesByRangeV1.

@holiman
Copy link
Contributor

holiman commented Feb 8, 2023

Which should return:

... well, what does it return?

@holiman
Copy link
Contributor

holiman commented Feb 8, 2023

doh!

<< (2d62df26) {"jsonrpc":"2.0","id":10,"result":null}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants