feat: Include missing block id in error responses #7416

ryanschneider · 2024-04-01T19:51:49Z

Fixes #7368 which explicitly mentions eth_call, but also updates:

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.

mattsse

this is great, ty!

one pedantic style nit

do we always want to include the unknown block id prefix?
I don't think this matches geth?

mattsse · 2024-04-02T12:44:05Z

crates/rpc/rpc/src/eth/api/mod.rs

+            ProviderError::HeaderNotFound(_) => EthApiError::UnknownBlockId(at),
+            ProviderError::BlockHashNotFound(_) => EthApiError::UnknownBlockId(at),


style: these can be combined into one arm: ProviderError::HeaderNotFound(_) | ProviderError::BlockHashNotFound(_)

Good catch @mattsse, fixed in 3a91c22, also rebased off latest main and retested locally.

Fixes paradigmxyz#7368.

ryanschneider · 2024-04-02T15:31:18Z

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.

ryanschneider · 2024-04-02T16:06:50Z

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.

mattsse · 2024-04-08T13:50:17Z

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

onbjerg · 2024-04-24T11:12:09Z

Hey @ryanschneider, any blocker here? Let me know so we can get this merged 😄

ryanschneider · 2024-05-31T15:23:28Z

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 #8045 which definitely complicates this PR, seems like changing the error response might break. The comment in main mentions ethereum-optimism/optimism#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?

emhane · 2024-07-30T16:05:41Z

header not found: number 0x131088f or header for hash not found: hash 0x3d9046bfd79e0d10404c01cf236a09806400233a8ca4761aba4c61cb3d888c11 is that still your opinion?

latter can be simplified: header not found: hash <hash>

emhane

lgtm, would you like help fixing merge conflicts?

mattsse · 2024-07-30T17:35:24Z

oh yeah, sorry we let this go stale...

we still want this, please take over @emhane

emhane · 2024-08-03T13:59:30Z

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 · 2024-08-03T14:00:00Z

re-request review @mattsse @onbjerg

crates/rpc/rpc-eth-types/src/error.rs

emhane · 2024-08-31T18:19:33Z

finally fixed all conflicts here!

re-request review also @mattsse

onbjerg

overall lgtm but please remove the unrelated changes

crates/rpc/rpc-types-compat/src/block.rs

crates/rpc/rpc-types-compat/src/transaction/mod.rs

mattsse

a few nits, and I want to see full error messages in tests, otherwise it is now non trivial to reason about what we're actually returning as the error.

mattsse · 2024-09-03T10:27:20Z

crates/rpc/rpc-api/src/otterscan.rs

@@ -24,7 +24,7 @@ pub trait Otterscan {

    /// Check if a certain address contains a deployed code.
    #[method(name = "hasCode")]
-    async fn has_code(&self, address: Address, block_number: Option<u64>) -> RpcResult<bool>;
+    async fn has_code(&self, address: Address, block_id: Option<BlockId>) -> RpcResult<bool>;


this is incorrect, this makes it incompatible with the otterscan API this must be u64 not hex

okay this is actually no longer the case, so BlockId is correct now

https://otterscan.github.io/execution-apis/api-documentation/

crates/rpc/rpc-server-types/src/result.rs

crates/rpc/rpc-types-compat/src/block.rs

crates/rpc/rpc-types-compat/src/transaction/mod.rs

mattsse · 2024-09-03T10:39:23Z

crates/rpc/rpc-eth-types/src/error.rs

-    /// Thrown when querying for `finalized` or `safe` block before the merge transition is
-    /// finalized, <https://github.com/ethereum/execution-apis/blob/6d17705a875e52c26826124c2a8a15ed542aeca2/src/schemas/block.yaml#L109>
-    ///
-    /// op-node now checks for either `Unknown block` OR `unknown block`:
-    /// <https://github.com/ethereum-optimism/optimism/blob/3b374c292e2b05cc51b52212ba68dd88ffce2a3b/op-service/sources/l2_client.go#L105>
-    ///
-    /// TODO(#8045): Temporary, until a version of <https://github.com/ethereum-optimism/optimism/pull/10071> is pushed through that doesn't require this to figure out the EL sync status.


we're changing the error now, does this affect op logic?

input pls @joshie as author of ethereum-optimism/optimism#10071

https://github.com/ethereum-optimism/optimism/pull/11759/files

emhane · 2024-09-05T14:23:11Z

blocked by ethereum-optimism/optimism#11759

mattsse

tysm @emhane 🙏

I think we shouldn't block on the op pr

mattsse · 2024-09-05T17:05:29Z

crates/rpc/rpc-api/src/otterscan.rs

@@ -24,7 +24,7 @@ pub trait Otterscan {

    /// Check if a certain address contains a deployed code.
    #[method(name = "hasCode")]
-    async fn has_code(&self, address: Address, block_number: Option<u64>) -> RpcResult<bool>;
+    async fn has_code(&self, address: Address, block_id: Option<BlockId>) -> RpcResult<bool>;


okay this is actually no longer the case, so BlockId is correct now

https://otterscan.github.io/execution-apis/api-documentation/

ryanschneider · 2024-09-06T15:38:16Z

Yup thanks @emhane for getting this over the finish line!

completed

ryanschneider mentioned this pull request Apr 1, 2024

eth_call targeting an unknown block hash returns "unknown block number" #7368

Closed

1 task

ryanschneider force-pushed the fix-7368-error-response branch 3 times, most recently from a2ecd95 to e8a3335 Compare April 1, 2024 23:27

ryanschneider marked this pull request as ready for review April 1, 2024 23:41

ryanschneider requested review from mattsse and Rjected as code owners April 1, 2024 23:41

onbjerg added C-debt Refactor of code section that is hard to understand or maintain A-rpc Related to the RPC implementation labels Apr 2, 2024

mattsse requested changes Apr 2, 2024

View reviewed changes

ryanschneider added 3 commits April 2, 2024 08:21

feat: Include missing block id in error responses

a4b73ea

Fixes paradigmxyz#7368.

fix: Update all block_number RPC args to block_id.

a0183d6

chore: style fix to remove redundant match arms

3a91c22

ryanschneider force-pushed the fix-7368-error-response branch from cab8251 to 3a91c22 Compare April 2, 2024 15:24

ryanschneider requested a review from mattsse April 2, 2024 15:25

gakonst requested a review from emhane as a code owner July 30, 2024 00:45

emhane approved these changes Jul 30, 2024

View reviewed changes

Merge branch 'main' into fix-7368-error-response

acd3a4b

Update EthApiError::UnknownBlockId string to 'header not found'

f9486f6

onbjerg requested changes Aug 5, 2024

View reviewed changes

crates/rpc/rpc-eth-types/src/error.rs Show resolved Hide resolved

emhane mentioned this pull request Aug 5, 2024

Add conversion from BlockHashOrNumber to BlockId alloy-rs/alloy#1127

Merged

3 tasks

emhane requested review from emhane, harunfox72s and onbjerg August 31, 2024 13:23

Merge branch 'main' into fix-7368-error-response

f3284c8

onbjerg previously requested changes Sep 2, 2024

View reviewed changes

crates/rpc/rpc-types-compat/src/block.rs Outdated Show resolved Hide resolved

crates/rpc/rpc-types-compat/src/transaction/mod.rs Outdated Show resolved Hide resolved

mattsse requested changes Sep 3, 2024

View reviewed changes

emhane added 4 commits September 5, 2024 14:43

Revert changes to reth-rpc-types-compat in favour of changes in 3efe22e

7753285

Merge branch 'main' into fix-7368-error-response

4307649

Add test for EthApiError::HeaderNotFound

9ca4680

Remove todo

3be86c8

emhane mentioned this pull request Sep 5, 2024

Catch 'header not found' string in L2BlockRefByLabel ethereum-optimism/optimism#11759

Merged

emhane added the S-blocked This cannot more forward until something else changes label Sep 5, 2024

Add test for display EthApiError::HeaderNotFound for canonical hash

d2a040a

emhane requested review from onbjerg and mattsse September 5, 2024 15:05

Fix display for EthApiError::HeaderNotFound, block number as hex

42ef3c4

mattsse approved these changes Sep 5, 2024

View reviewed changes

emhane added this pull request to the merge queue Sep 6, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 6, 2024

mattsse added 2 commits September 7, 2024 12:35

Merge branch 'main' into fix-7368-error-response

c46c44a

update lock

18c1a68

mattsse enabled auto-merge September 7, 2024 10:43

mattsse added this pull request to the merge queue Sep 7, 2024

Merged via the queue into paradigmxyz:main with commit 7a20b41 Sep 7, 2024
35 checks passed

emhane mentioned this pull request Sep 12, 2024

eth_call doesn't support integer for block number on reth client, only block hash #10869

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Include missing block id in error responses #7416

feat: Include missing block id in error responses #7416

ryanschneider commented Apr 1, 2024 •

edited by emhane

Loading

mattsse left a comment •

edited

Loading

mattsse Apr 2, 2024

ryanschneider Apr 2, 2024

ryanschneider commented Apr 2, 2024

ryanschneider commented Apr 2, 2024

mattsse commented Apr 8, 2024 •

edited

Loading

onbjerg commented Apr 24, 2024

ryanschneider commented May 31, 2024

emhane commented Jul 30, 2024

emhane left a comment

mattsse commented Jul 30, 2024

emhane commented Aug 3, 2024

emhane commented Aug 3, 2024

emhane commented Aug 31, 2024

onbjerg left a comment

mattsse left a comment

mattsse Sep 3, 2024

mattsse Sep 5, 2024

mattsse Sep 3, 2024

emhane Sep 4, 2024

emhane Sep 5, 2024

emhane commented Sep 5, 2024

mattsse left a comment •

edited

Loading

mattsse Sep 5, 2024

ryanschneider commented Sep 6, 2024

		ProviderError::HeaderNotFound(_) => EthApiError::UnknownBlockId(at),
		ProviderError::BlockHashNotFound(_) => EthApiError::UnknownBlockId(at),

feat: Include missing block id in error responses #7416

feat: Include missing block id in error responses #7416

Conversation

ryanschneider commented Apr 1, 2024 • edited by emhane Loading

mattsse left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanschneider commented Apr 2, 2024

ryanschneider commented Apr 2, 2024

mattsse commented Apr 8, 2024 • edited Loading

onbjerg commented Apr 24, 2024

ryanschneider commented May 31, 2024

emhane commented Jul 30, 2024

emhane left a comment

Choose a reason for hiding this comment

mattsse commented Jul 30, 2024

emhane commented Aug 3, 2024

emhane commented Aug 3, 2024

emhane commented Aug 31, 2024

onbjerg left a comment

Choose a reason for hiding this comment

mattsse 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

Choose a reason for hiding this comment

emhane commented Sep 5, 2024

mattsse left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanschneider commented Sep 6, 2024

ryanschneider commented Apr 1, 2024 •

edited by emhane

Loading

mattsse left a comment •

edited

Loading

mattsse commented Apr 8, 2024 •

edited

Loading

mattsse left a comment •

edited

Loading