reth
reth copied to clipboard
paradigmxyz
feat: Include missing block id in error responses
Fixes #7368 which explicitly mentions eth_call, but also updates:
- [x]
eth_call - [x]
eth_getStorageAt - [ ]
eth_getProof(see below) - [x]
eth_getTransactionCount - [x]
eth_getCode - [x]
eth_getBalance - [x]
eth_createAccessList - [x]
debug_traceCall - [x]
trace_rawTransaction - [x]
trace_callMany - [x]
trace_call
In each case, the error response is of the form:
{"jsonrpc":"2.0","error":{"code":-32001,"message":"unknown block id: number 0x230d842afd1face"},"id":"test"}
or
{"jsonrpc":"2.0","error":{"code":-32001,"message":"unknown block id: hash 0x230d842afd1faceaa3c5c0dacd24228747565c441ed5e1e3a9306c2c6b55d619"},"id":"test"}
Testing performed for the above w/ cast and curl here: https://gist.github.com/ryanschneider/20e8f78e46666d8249d266abec118f6a
For eth_getProof I opted not to interfere with the current situation where only block_id's that resolve to "latest" are supported.
I'm 100% open to hearing about better ways to implement this, one thing I don't like is that EthApiError::UnknownBlockNumber responses can still get through, if there's a good way to leverage Rust types to prevent that I'm all ears.
do we always want to include the unknown block id prefix? I don't think this matches geth?
Personally I'm not convinced we should try to sync up exact error messages, just codes (and as you pointed out EIP-1474 clearly shows we should return -32001), and try to provide the same semantic value the messages, but I can definitely be convinced otherwise.
Actually geth returns:
{"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"header not found"}}
{"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"header for hash not found"}}
I suppose we could make the error message "header not found: {block_id}" e.g. "header not found: number 0x230d842afd1face", for substring-matching compatibility but that feels wrong to me.
hmm, yeah rpc error inconsistencies are very annoying, I'm not sure what to do here, but I'd lean towards just matching geth's error messages, or make it possible to match substrings.
I think we should mimic geth's messages but can include the number or hash
Hey @ryanschneider, any blocker here? Let me know so we can get this merged 😄
Sorry @onbjerg I've been away for a bit but am hoping to get back to this soon, however in rebasing off main I saw https://github.com/paradigmxyz/reth/issues/8045 which definitely complicates this PR, seems like changing the error response might break. The comment in main mentions https://github.com/ethereum-optimism/optimism/pull/10071 but that PR was closed w/ a mention of the issue being fixed in another PR, but I've lost the thread after that.
@mattsse I think we settled on the error text being:
header not found: number 0x131088f or header for hash not found: hash 0x3d9046bfd79e0d10404c01cf236a09806400233a8ca4761aba4c61cb3d888c11 is that still your opinion?
header not found: number 0x131088forheader for hash not found: hash 0x3d9046bfd79e0d10404c01cf236a09806400233a8ca4761aba4c61cb3d888c11is that still your opinion?
latter can be simplified: header not found: hash <hash>
oh yeah, sorry we let this go stale...
we still want this, please take over @emhane
do we always want to include the unknown block id prefix? I don't think this matches geth?
Personally I'm not convinced we should try to sync up exact error messages, just codes (and as you pointed out EIP-1474 clearly shows we should return
-32001), and try to provide the same semantic value the messages, but I can definitely be convinced otherwise.
seems like a bug in geth or alloy then? Resource not found is correct here, not invalid input
https://github.com/alloy-rs/alloy/blob/3bf3618b111df609bafedbd648664e2d82122c47/crates/rpc-types-eth/src/error.rs#L27-L28
@emhane tests are failing and I think your merge revived a bunch of old files in the PR
am aware @onbjerg , doesn't compile since blocked by https://github.com/paradigmxyz/reth/pull/7416#issuecomment-2269745597