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

Corrected eth_getLogs default fromBlock value. #4513

Merged
merged 8 commits into from
Oct 26, 2022
Merged

Corrected eth_getLogs default fromBlock value. #4513

merged 8 commits into from
Oct 26, 2022

Conversation

mark-terry
Copy link
Contributor

@mark-terry mark-terry commented Oct 10, 2022 •
edited
Loading

Signed-off-by: mark-terry mark.terry@consensys.net

PR description

Changes the default fromBlock value in eth_getLogs from earliest to latest per the spec.

Added tests to verify BlockParameter value interpretation.

Fixed Issue(s)

Fixes #4123

Documentation

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

Changelog

@mark-terry mark-terry added TeamRevenant GH issues worked on by Revenant Team RPC labels Oct 10, 2022
@mark-terry mark-terry self-assigned this Oct 10, 2022
@mark-terry mark-terry marked this pull request as ready for review October 11, 2022 01:16
macfarla
macfarla reviewed Oct 11, 2022
View reviewed changes
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

for completeness can you add a test for the invalid case - what happens if I supply a non-parseable string

...t/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/parameters/BlockParameterTest.java Outdated Show resolved Hide resolved
...t/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/parameters/BlockParameterTest.java Outdated Show resolved Hide resolved
fab-10
fab-10 requested changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

I think we need to first define which is our goal, because I see 2 possibility here:

  1. Follow the specs, then we need to reject everything that is not an uint, and return an error to the caller
  2. Extend the functionality, accepting with block names "lastest", "earlier" etc... then we need to interpret them correctly

@mark-terry
Copy link
Contributor Author

I think we need to first define which is our goal, because I see 2 possibility here:

  1. Follow the specs, then we need to reject everything that is not an uint, and return an error to the caller
  2. Extend the functionality, accepting with block names "lastest", "earlier" etc... then we need to interpret them correctly

I think option 2 is preferable; maintaining API consistency by using the filterOptions parameter type. "earliest", "latest" and numeric values are already covered; will investigate the others...

@fab-10
Copy link
Contributor

fab-10 commented Oct 11, 2022

I think we need to first define which is our goal, because I see 2 possibility here:

  1. Follow the specs, then we need to reject everything that is not an uint, and return an error to the caller
  2. Extend the functionality, accepting with block names "lastest", "earlier" etc... then we need to interpret them correctly

I think option 2 is preferable; maintaining API consistency by using the filterOptions parameter type. "earliest", "latest" and numeric values are already covered; will investigate the others...

Ok then take a look at AbstractBlockParameterMethod class, probably it can help to how block parameters are handled in other methods

@mark-terry
Fixed Fixed default fromBlock value and improved parameter interpetat…
8f6a803 
…ion in eth_getLogs RPC handler. Added tests.

Signed-off-by: mark-terry <mark.terry@consensys.net>
@mark-terry mark-terry requested a review from fab-10 October 13, 2022 06:42
mark-terry and others added 2 commits October 13, 2022 21:00
fab-10
fab-10 reviewed Oct 14, 2022
View reviewed changes
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

ok the parameter interpretation, it will be nice to have an abstract class for FilterParameters similar to what AbstractBlockParameterMethod does for BlockParameter to unify all methods that use the filter, but this is probably a refactor that can be done separately.
Have you tested this PR with a Consensus client? since eth_getLogs is critial after the merge

...src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGetLogsTest.java Outdated Show resolved Hide resolved
mark-terry and others added 4 commits October 18, 2022 23:11
@mark-terry
Refactoring. Tests.
0478195 
Signed-off-by: mark-terry <mark.terry@consensys.net>
@mark-terry
Refactor.
64a3295 
Signed-off-by: mark-terry <mark.terry@consensys.net>
@mark-terry
Merge branch 'eth_getLogs_fix' of github.com:mark-terry/besu into eth…
ed2efdd 
…_getLogs_fix
@mark-terry
Merge branch 'main' into eth_getLogs_fix
c30cf2d
@mark-terry
Copy link
Contributor Author

Have you tested this PR with a Consensus client? since eth_getLogs is critial after the merge

It's currently running with Teku on Sepolia without any obvious issues. 👍

@mark-terry mark-terry requested a review from fab-10 October 20, 2022 12:02
@mark-terry
Merge branch 'main' into eth_getLogs_fix
3b29797 
Signed-off-by: mark-terry <36909937+mark-terry@users.noreply.github.com>
@mark-terry mark-terry merged commit 26c4bcf into hyperledger:main Oct 26, 2022
@mark-terry mark-terry deleted the eth_getLogs_fix branch October 26, 2022 04:18
fab-10
fab-10 reviewed Oct 26, 2022
View reviewed changes
CHANGELOG.md
Comment on lines -39 to -40
- Initiate connection to maintained peers soon after startup. [#4469](https://github.com/hyperledger/besu/pull/4469)
- Update apache-commons-text to 1.10.0 to address CVE-2022-42889 [#4542](https://github.com/hyperledger/besu/pull/4542)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please readd these 2 entries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#4560

@mark-terry mark-terry restored the eth_getLogs_fix branch October 26, 2022 12:04
@mark-terry mark-terry deleted the eth_getLogs_fix branch October 26, 2022 12:09
@mark-terry mark-terry mentioned this pull request Oct 26, 2022
Open
macfarla pushed a commit to jflo/besu that referenced this pull request Jan 10, 2023
@mark-terry @macfarla
Corrected eth_getLogs default fromBlock value. (hyperledger#4513)
5f6b49e 
* Fixed default fromBlock value and improved parameter interpretation in eth_getLogs RPC handler. Improved test coverage.

Signed-off-by: mark-terry <mark.terry@consensys.net>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
@mark-terry @eum602
Corrected eth_getLogs default fromBlock value. (hyperledger#4513)
b5001f7 
* Fixed default fromBlock value and improved parameter interpretation in eth_getLogs RPC handler. Improved test coverage.

Signed-off-by: mark-terry <mark.terry@consensys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fab-10 fab-10 fab-10 approved these changes

@macfarla macfarla macfarla left review comments

Assignees

@mark-terry mark-terry

Labels
mainnet RPC TeamRevenant GH issues worked on by Revenant Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JSON-RPC method eth_getLogs improperly interprets block arguments
3 participants
@mark-terry @fab-10 @macfarla