-
Notifications
You must be signed in to change notification settings - Fork 985
RPC - return null result if requested block not found #9303
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
|
draft bc this has unfortunately broken these hive tests: |
|
In Besu we have an abstract class AbstractBlockParameterOrBlockHashMethod that is used by several RPCs - so any RPC that gets a block by block number or hash will have the same error handling behavior. Including: The spec, and the hive tests, are inconsistent with error handling expectations. return null if block not found |
Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
| } else if (blockParameterOrBlockHash.isNumeric() || blockParameterOrBlockHash.isEarliest()) { | ||
| final OptionalLong blockNumber = blockParameterOrBlockHash.getNumber(); | ||
| if (blockNumber.isEmpty() || blockNumber.getAsLong() < 0) { | ||
| // TODO should this be null result or invalid params? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not clear on this either even after going through #9197. Looks like some clients return an error and some a null response. Think we should just leave this as is returning an error until we know otherwise.
Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: Sally MacFarlane [email protected]
PR description
If block not found, return a Success response with null result, NOT an Error response.
This affects all RPC methods that use
AbstractBlockParameterOrBlockHashMethodincluding
debug_accountAt,debug_setHead,eth_call,eth_getBlockReceipts,eth_getProof,eth_simulateV1,eth_getBalance,eth_getCode,eth_getStorageAt,eth_getTransactionCountI've included this as a breaking change in the changelog since it is a change in behavior. However this is what the hive tests expect and it's moving towards being more consistent with other clients.
Fixed Issue(s)
Fixes #9197
Fixes hive tests:
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew spotlessApply./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests