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.

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

Fix eth_getBlockByNumber with empty params returns #8134

Merged

Conversation

iryoung
Copy link
Contributor

@iryoung iryoung commented Jan 17, 2025

PR description

This PR addresses a bug in the eth_getBlockByNumber method, where invoking it with empty parameters results in the error message "Invalid block, unable to parse RLP".

There're some consideration while I investigate & fix the bug

  • I would have preferred to use the message proposed in the issue; however, a pre-defined error message better fits this situation, so I opted to use it.

  • Adding data would require modifications to org.hyperledger.besu.ethereum.api.jsonrpc.execution.BaseJsonRpcProcessor.process(), which is outside the scope of this fix.

  • The message for RpcErrorType.INVALID_BLOCK_PARAMS might seem slightly inconsistent, but adjusting it is also beyond the scope of this PR.

  • Request

POST http://localhost:8545
Content-Type: application/json

{"jsonrpc":"2.0","id":"53","method":"eth_getBlockByNumber","params":[]}
  • Before
{
  "jsonrpc": "2.0",
  "id": "53",
  "error": {
    "code": -32602,
    "message": "Invalid block, unable to parse RLP"
  }
}
  • After
{
  "jsonrpc": "2.0",
  "id": "53",
  "error": {
    "code": -32602,
    "message": "Invalid block number params"
  }
}

Fixed Issue(s)

fixes #7918

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

Signed-off-by: Iryoung Jeong <[email protected]>
@iryoung iryoung force-pushed the bug-getblockbynumber-empty-params branch from 8507822 to 5d160fd Compare January 20, 2025 07:49
Copy link
Contributor

@Matilda-Clerke Matilda-Clerke left a comment

Choose a reason for hiding this comment

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

LGTM, appreciate the limited scope for this PR

@iryoung
Copy link
Contributor Author

iryoung commented Jan 21, 2025

I realized I missed updating one test case, so I’ve added a commit to address it and ensure the test passes.
Please re-run the workflows.

Copy link
Contributor

@Matilda-Clerke Matilda-Clerke left a comment

Choose a reason for hiding this comment

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

Additional changes LGTM

@iryoung
Copy link
Contributor Author

iryoung commented Jan 21, 2025

Finally, all checks have passed, but it says, This branch is out-of-date with the base branch. Should I update the branch to merge? I'm concerned that doing so might trigger another round of workflows, which are quite challenging to pass.

@Matilda-Clerke
Copy link
Contributor

I'll handle getting it merged from here. Thanks again for your contribution!

@Matilda-Clerke Matilda-Clerke enabled auto-merge (squash) January 21, 2025 21:37
@Matilda-Clerke Matilda-Clerke merged commit 519323f into hyperledger:main Jan 21, 2025
43 checks passed
@iryoung
Copy link
Contributor Author

iryoung commented Jan 22, 2025

Thank you for your help! I’ll explore other issues to find something suitable and continue contributing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants