Skip to content

Conversation

hlongvu
Copy link

@hlongvu hlongvu commented Aug 1, 2025

Add cli configurable limit for the number of addresses allowed in eth_getLogs filter criteria: #32264

Key changes:

  • Added --rpc.getlogmaxaddrs CLI flag (default: 1000) to configure the maximum number of addresses
  • Updated ethconfig.Config with FilterMaxAddresses field for configuration management
  • Modified filter system to use the configurable limit instead of the hardcoded maxAddresses constant
  • Enhanced test coverage with new test cases for address limit validation
  • Removed hardcoded validation from JSON unmarshaling, moving it to runtime validation

Please notice that I remove the check at FilterCriteria UnmarshalJSON because the runtime config can not pass into this validation.

Please help review this change!

@hlongvu hlongvu requested a review from rjl493456442 as a code owner August 1, 2025 07:20
Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

Very solid work, thanks for your contribution!

Just one nitpick, please fix it.

EthGetLogMaxAddressFlag = &cli.Uint64Flag{
Name: "rpc.getlogmaxaddrs",
Usage: "Maximum number of addresses allowed in eth_getLogs filter criteria",
Value: 1000,
Copy link
Member

Choose a reason for hiding this comment

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

nitpick, please use ethconfig.Defaults.FilterMaxAddresses

@rjl493456442 rjl493456442 changed the title eth_getLogs configurable address limit eth/filters, cmd: add config of eth_getLogs address limit Aug 1, 2025
@hlongvu
Copy link
Author

hlongvu commented Aug 1, 2025

thanks, I've updated the PR

@rjl493456442 rjl493456442 self-assigned this Aug 1, 2025
@zsfelfoldi
Copy link
Contributor

It is not logical to limit the number of addresses but not limit the number of topic alternatives at each topic position (topics is a 2D array and maxTopics only limits one dimension). They can cause similar extra load to the search algorithm so I'd just introduce a single limit that applies to both the address field and each of the topic positions. I have no good idea though on how to name this flag.
Otherwise this PR could be good, should be fairly easy to add checks for the topics too. Please do not merge though until we figure out how these constraints should exactly look like and what we should name the flag.

@s1na
Copy link
Contributor

s1na commented Aug 28, 2025

I have no good idea though on how to name this flag.

@zsfelfoldi what do you think about maxCriteria or maxQueryOptions? We can explain it more in the flag description. Another is queryBudget.

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

Successfully merging this pull request may close these issues.

4 participants