reth

reth copied to clipboard

Describe the bug

The debug_getRawTransaction/get-invalid-hash test fails with

>>  {"jsonrpc":"2.0","id":1,"method":"debug_getRawTransaction","params":["1000000000000000000000000000000000000000000000000000000000000001"]}
<<  {"jsonrpc":"2.0","result":"0x","id":1}
response differs from expected:
 {
   "id": 1,
   "jsonrpc": "2.0",
-  "result": "0x"
+  "error": {
+    "code": -32602,
+    "message": "invalid argument 0: json: cannot unmarshal hex string without 0x prefix into Go value of type common.Hash"
+  }
 }

This is because our H256 type allows non-0x-prefixed strings. This means it is not QUANTITY:

Unformatted data

When encoding unformatted data (byte arrays, account addresses, hashes, bytecode arrays): encode as hex, prefix with "0x", two hex digits per byte.

Here are some examples:

• 0x41 (size 1, "A")
• 0x004200 (size 3, "\0B\0")
• 0x (size 0, "")
• WRONG: 0xf0f0f (must be even number of digits)
• WRONG: 004200 (must be prefixed 0x)

From: https://ethereum.org/en/developers/docs/apis/json-rpc/#unformatted-data-encoding

The deserialization logic comes here comes from ruint, so this rule could be either enforced there or in a new type that we maintain and use for all hash types in RPC.

Steps to reproduce

Run hive:

./hive --client reth --sim ethereum/rpc-compat --sim.limit "/debug_getRawTransaction/get-invalid-hash"

Node logs

No response

Platform(s)

No response

What version/commit are you on?

No response

Code of Conduct

• [X] I agree to follow the Code of Conduct

this should be fixed in ruint directly.

It looks like this needs to be fixed in revm_primitives instead!

Ok, I'm questioning whether or not we should actually fix this, or if we should just exclude this test. While we can require 0x prefixes for 256-bit hashes, but genesis.json files often have:

   "alloc": {
     "658bdf435d810c91414ec09147daa6db62406379": {
       "balance": "0x487a9a304539440000"
     },

Here, the key 658bdf435d810c91414ec09147daa6db62406379 is a H160, and does not have a 0x prefix. So we won't be able to enforce this for H160, or else genesis file parsing will break. If we can't enforce this for H160, why enforce it for H256? The other option is introducing a separate type which MUST have the 0x prefix.

Any updated thoughts on how we should move forward with this?

Bump on this

After discussion, closing this. Reasoning:

• This doesn't really make sense to add for ruint, so we would need to add a wrapper type
• This is really bad DX
• It's a debug namespace invalid-input test in Hive, which is low priority, and the discrepancy is not fatal

We can revisit later, perhaps?