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

Enable limit on range Of JSON-RPC API trace_filter method (#5971) #6446

Merged
merged 10 commits into from
Jan 25, 2024
Merged

Enable limit on range Of JSON-RPC API trace_filter method (#5971) #6446

merged 10 commits into from
Jan 25, 2024

Conversation

alyokaz
Copy link
Contributor

@alyokaz alyokaz commented Jan 22, 2024

Enable a limit on the range of blocks that can be supplied to the JSON-RPC trace_filter method.

The limit has a default value and can be overridden with a command line option at start up.

fixes #5971

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 22, 2024 •
edited by Gabriel-Trintinalia
Loading

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

@alyokaz alyokaz changed the title Enable Limit On Range Of JSON-RPC API trace_filter Method (#5971) Enable limit on range Of JSON-RPC API trace_filter method (#5971) Jan 22, 2024
@alyokaz alyokaz force-pushed the enable_trace_filter_range_limit branch from 9d2c922 to d743aaf Compare January 22, 2024 22:44
@alyokaz
Enable limit on range of JSON-RPC API trace_filter method (hyperledge…
8aba27f 
…r#5971)

Enable a limit on the range of blocks that can be supplied to the
JSON-RPC trace_filter method.

The limit has a default value and can be overridden with a command
line option at start up.

Signed-off-by: alyokaz <[email protected]>
@alyokaz alyokaz force-pushed the enable_trace_filter_range_limit branch from d743aaf to 8aba27f Compare January 22, 2024 23:04
Gabriel-Trintinalia
Gabriel-Trintinalia reviewed Jan 23, 2024
View reviewed changes
Copy link
Contributor

@Gabriel-Trintinalia Gabriel-Trintinalia 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.

Thank you for the PR! I have added some comments in the code.

  • This is a breaking change. Could you please add a entry to the changelog?
  • Also, a unit test similar to shouldFailIfParamsExceedMaxRange would be nice

...pi/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/TraceFilter.java Outdated Show resolved Hide resolved
besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java Outdated Show resolved Hide resolved
besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java Outdated Show resolved Hide resolved
besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java Outdated Show resolved Hide resolved
...pi/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/TraceFilter.java Outdated Show resolved Hide resolved
besu/src/test/resources/everything_config.toml Outdated Show resolved Hide resolved
@alyokaz
Copy link
Contributor Author

alyokaz commented Jan 23, 2024

Thank you for the PR! I have added some comments in the code.

  • This is a breaking change. Could you please add a entry to the changelog?
  • Also, a unit test similar to shouldFailIfParamsExceedMaxRange would be nice

Changelog entry added.

I've added a unit test for TraceFilter covering the new option. I wanted to add a test for the 0 parameter, but without mocking traceFilterWithPipeline using something like Powermock, it would require extensive refactoring and I felt it might run orthogonal to the "big function" style of the code. I think maybe it might be better dealt with in an integration test?

Gabriel-Trintinalia
Gabriel-Trintinalia reviewed Jan 24, 2024
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.

Looking good, just a text suggestion and the the right copyright for the test class

CHANGELOG.md Outdated Show resolved Hide resolved
...rc/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/TraceFilterTest.java Outdated Show resolved Hide resolved
macfarla
macfarla reviewed Jan 25, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
macfarla
macfarla approved these changes Jan 25, 2024
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.

approved with a couple of minor nits

...rc/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/TraceFilterTest.java Outdated Show resolved Hide resolved
@macfarla macfarla added doc-change-required Indicates an issue or PR that requires doc to be updated RPC labels Jan 25, 2024
@macfarla macfarla enabled auto-merge (squash) January 25, 2024 00:56
@macfarla macfarla merged commit a1a5b20 into hyperledger:main Jan 25, 2024
18 checks passed
@bgravenorst bgravenorst mentioned this pull request Jan 29, 2024
Closed
@bgravenorst bgravenorst removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gabriel-Trintinalia Gabriel-Trintinalia Gabriel-Trintinalia left review comments

@macfarla macfarla macfarla approved these changes

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

Successfully merging this pull request may close these issues.

The JSON-RPC trace_filter method can cause a Thread block
4 participants
@alyokaz @macfarla @Gabriel-Trintinalia @bgravenorst