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.

Sign up for GitHub

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

Jump to bottom

RPC methods that lookup block by hash will now return error if no block found #4582

Merged
merged 13 commits into from
Jan 9, 2023
Merged

RPC methods that lookup block by hash will now return error if no block found #4582

merged 13 commits into from
Jan 9, 2023

Conversation

macfarla
Copy link
Contributor

@macfarla macfarla commented Nov 1, 2022 •
edited
Loading

Previously were returning null in this case eg ethGetCode

New behavior matches geth 1.10.25

Do we consider this a breaking change?

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

pinges
pinges approved these changes Nov 1, 2022
View reviewed changes
Copy link
Contributor

@pinges pinges left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@macfarla
added to changelog
b799ad8 
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@macfarla
merge main
39627cf 
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@macfarla
Merge branch 'main' of github.com:hyperledger/besu into besu-odd-rpc
9b2e4e4
@macfarla
merge main post 22.10.0
39d9d1c 
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@macfarla
removed extra error code; fixed tests
2e164ad 
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@macfarla
fixed test
0ba58e9 
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@macfarla
Merge branch 'main' into besu-odd-rpc
6c1e63e
shemnon
shemnon previously requested changes Nov 4, 2022
View reviewed changes
Copy link
Contributor

@shemnon shemnon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I argue this (and geth) are out of spec

https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_getblockbyhash

Returns

Object - A block object, or null when no block was found:
[... description of fields ...]

@shemnon
Copy link
Contributor

shemnon commented Nov 4, 2022

Furthermore this touches on endpoints that use the "default block parameter." Querying by Hash is not a valid option. The data type is "BlockNumberOrTag" - https://github.com/ethereum/execution-apis/blob/main/src/eth/state.yaml#L60

And the type does not accept hashes - https://github.com/ethereum/execution-apis/blob/main/src/schemas/block.yaml#L92-L108

So if there was movement towards the spec conformance direction that step would be to not return answers when looking up by hash even when one is found.

Gabriel-Trintinalia
Gabriel-Trintinalia approved these changes Nov 6, 2022
View reviewed changes
Copy link
Contributor

@Gabriel-Trintinalia Gabriel-Trintinalia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@macfarla
Copy link
Contributor Author

macfarla commented Nov 7, 2022 •
edited
Loading

I argue this (and geth) are out of spec

https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_getblockbyhash

Returns

Object - A block object, or null when no block was found:
[... description of fields ...]

Agree this does not match the spec.

This change was made here #1784
to a subset of methods to support EIP-1898 -
https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1898.md
Status of EIP-1898 is 'stagnant' - does this mean we should revert these changes?

@macfarla
Copy link
Contributor Author

There has been further conversation here https://discord.com/channels/595666850260713488/595701319793377299/1041758135821533266 indicating that it should be part of the spec.

EIP-1898 - changing status from stagnant to draft ethereum/EIPs#5980 and adding relevant parts to the spec ethereum/execution-apis#326

@macfarla
Copy link
Contributor Author

macfarla commented Jan 9, 2023

Update on this

@macfarla
Copy link
Contributor Author

macfarla commented Jan 9, 2023 •
edited
Loading

This execution APIs spec has been updated per ethereum/execution-apis#326 so bumping this for review

@macfarla macfarla dismissed shemnon’s stale review January 9, 2023 22:27

This execution APIs spec has been updated per ethereum/execution-apis#326 to allow block hash

shemnon
shemnon approved these changes Jan 9, 2023
View reviewed changes
Copy link
Contributor

@shemnon shemnon left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The official execution spec is disturbingly void of standard error handing. I'll approve this on the presumption that this is not at odds with how other clients handle such an exception.

@macfarla macfarla mentioned this pull request Jan 9, 2023
Closed
1 task
@macfarla
Copy link
Contributor Author

macfarla commented Jan 9, 2023

The official execution spec is disturbingly void of standard error handing. I'll approve this on the presumption that this is not at odds with how other clients handle such an exception.

Thanks @shemnon - this PR actually arose out of an effort to standardise error handling across clients ethereum/execution-apis#286

@macfarla macfarla merged commit 8e523b0 into hyperledger:main Jan 9, 2023
@macfarla macfarla deleted the besu-odd-rpc branch January 10, 2023 00:13
macfarla added a commit to macfarla/besu that referenced this pull request Jan 10, 2023
@macfarla
RPC methods that lookup block by hash will now return error if no blo…
9b15fdb 
…ck found (hyperledger#4582)

* add specific error for block by hash lookup not found

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
@macfarla @eum602
RPC methods that lookup block by hash will now return error if no blo…
8f35f6a 
…ck found (hyperledger#4582)

* add specific error for block by hash lookup not found

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pinges pinges pinges approved these changes

@Gabriel-Trintinalia Gabriel-Trintinalia Gabriel-Trintinalia approved these changes

@shemnon shemnon shemnon approved these changes

Assignees
No one assigned
Labels
mainnet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@macfarla @shemnon @Gabriel-Trintinalia @pinges