Skip to content

rpc: validate log query limit when creating filters - #23598

Open
yperbasis wants to merge 2 commits into
mainfrom
yperbasis/log-filter-review-followup
Open

rpc: validate log query limit when creating filters#23598
yperbasis wants to merge 2 commits into
mainfrom
yperbasis/log-filter-review-followup

Conversation

@yperbasis

@yperbasis yperbasis commented Aug 26, 2026

Copy link
Copy Markdown
Member

Follow-up to #23296.

Summary

  • reject eth_newFilter criteria that exceed rpc.logs.querylimit before creating the filter
  • share query-limit validation between eth_newFilter and eth_getLogs
  • reject invalid eth_getLogs query-limit criteria before opening a database transaction
  • restore the concurrent subscribe/unsubscribe regression test with valid four-position criteria and explicit success checks
  • document the creation-time behavior, including the live-subscription and unlimited alternatives

TDD

Red: TestNewFilterAppliesLogQueryLimitAtCreation failed because both address and topic-alternative cases returned a filter ID without an error.

Green: eth_newFilter now performs the same static topic-position and query-limit validation before subscribing, and the regression test passes.

Red: TestGetLogsAppliesLogQueryLimitBeforeOpeningTransaction panicked at BeginTemporalRo because query-limit validation happened after database access.

Green: eth_getLogs now validates the query limit before opening the transaction, and the regression test passes without a database.

The concurrent subscribe/unsubscribe test correction is mechanical: #23296 made its ten-position criteria invalid, so the test stopped creating subscriptions. No production behavior change was needed for that item.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds early rpc.logs.querylimit validation to eth_newFilter, aligning it with eth_getLogs.

Changes:

  • Shares log-query limit validation across RPC methods.
  • Adds focused validation tests and repairs the concurrency regression test.
  • Updates CLI documentation and changelog.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
rpc/jsonrpc/eth_receipts.go Extracts shared query-limit validation.
rpc/jsonrpc/eth_filters.go Validates criteria before filter creation.
rpc/jsonrpc/eth_filters_test.go Tests rejection and restores valid concurrency coverage.
cmd/utils/flags.go Updates flag help text.
docs/site/docs/fundamentals/configuring-erigon.mdx Documents eth_newFilter coverage.
docs/site/static/llms-full.txt Updates generated documentation.
llms-full.txt Updates consolidated documentation.
ChangeLog.md Records the breaking behavior change.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@yperbasis
yperbasis marked this pull request as ready for review August 26, 2026 12:04
@yperbasis
yperbasis requested review from taratorio and a balanced review from Copilot August 26, 2026 12:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

@taratorio taratorio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this change is not necessary

@taratorio

Copy link
Copy Markdown
Member

eth_newFilter's main use is with eth_getFilterChanges and that performs no historical scan - it reads accumulated changes in the filter

rpc.logs.querylimit is used when querying the DB in methods like eth_getLogs and eth_getFilterLogs, not for filter changes

the reason I didn't make this change in my PR is because AI is wrong to suggest that

@taratorio

Copy link
Copy Markdown
Member

furthermore for subscriptions (i.e. eth_newFilter + eth_getFilterChanges and web socket based equivalent eth_subscribe("logs")) we already have a different flag that covers the same thing - rpc.subscription.filters.maxtopics (it defaulted to 0 before my PR)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants