Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

fix: OOM when eth_getLogs response too large #860

Merged
merged 10 commits into from
Dec 29, 2021

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Dec 28, 2021

Closes: #858, #861

Description

  • add limit to number of logs of filter response
  • make block limit and log limit configurable
  • fix empty response when specifying blockHash but without address/topics filter critiera.

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@yihuang yihuang changed the title add response limit to eth_getLogs API limit the number of logs returned by eth_getLogs Dec 28, 2021
@codecov
Copy link

codecov bot commented Dec 28, 2021

Codecov Report

Merging #860 (ec183af) into main (eb17366) will decrease coverage by 0.06%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #860      +/-   ##
==========================================
- Coverage   56.61%   56.55%   -0.07%     
==========================================
  Files          72       72              
  Lines        6066     6076      +10     
==========================================
+ Hits         3434     3436       +2     
- Misses       2433     2441       +8     
  Partials      199      199              
Impacted Files Coverage Δ
server/config/config.go 21.05% <20.00%> (-0.08%) ⬇️

@yihuang yihuang changed the title limit the number of logs returned by eth_getLogs fix: OOM when eth_getLogs response too large Dec 28, 2021
Closes: evmos#858

- add limit to number of logs of filter response
- make block limit and log limit configurable
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Can you add a changelog entry too?

server/config/config.go Show resolved Hide resolved
@yihuang
Copy link
Contributor Author

yihuang commented Dec 29, 2021

Can you add a changelog entry too?

done

@fedekunze fedekunze enabled auto-merge (squash) December 29, 2021 14:53
@fedekunze fedekunze disabled auto-merge December 29, 2021 21:46
@fedekunze fedekunze merged commit ced5280 into evmos:main Dec 29, 2021
@yihuang yihuang deleted the optim-getlogs branch December 30, 2021 01:45
Copy link
Contributor

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @yihuang. I've added a comment about a potential overflow that could bypass this change and trigger the OOM.

filtered := FilterLogs(ethLog, f.criteria.FromBlock, f.criteria.ToBlock, f.criteria.Addresses, f.criteria.Topics)
logs = append(logs, filtered...)
// check logs limit
if len(logs)+len(filtered) > logLimit {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I still want to trigger an OOM, I can ensure that len(logs) > maxint/2 as well as len(filtered) > maxint/2 which will bypass this check as they'll overflow and become negative in the addition len(logs)+len(filtered). To properly fix this, you mos def want to check arithmetic by subtraction and comparisons individually against len(logs) then len(filtered)

Copy link
Contributor Author

@yihuang yihuang Dec 30, 2021

Choose a reason for hiding this comment

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

Since len(logs) <= logLimit and len(filtered) <= block gas limit / gas per log, minimal gas per log is 375, a typical block gas limit is tens of millions, so it's pretty safe even for 32bit int, basically impossible for 64bit int.

yihuang added a commit to yihuang/ethermint that referenced this pull request Dec 31, 2021
* fix: OOM when eth_getLogs response too large

Closes: evmos#858

- add limit to number of logs of filter response
- make block limit and log limit configurable

* return error if exceeds log limit

* Apply suggestions from code review

* parse from config

* read cli flags

* add to config template

* fix bloomFilter

* changelog

* add validation

Co-authored-by: Federico Kunze Küllmer <[email protected]>
yihuang added a commit to crypto-org-chain/ethermint that referenced this pull request Feb 15, 2022
* fix: OOM when eth_getLogs response too large

Closes: evmos#858

- add limit to number of logs of filter response
- make block limit and log limit configurable

* return error if exceeds log limit

* Apply suggestions from code review

* parse from config

* read cli flags

* add to config template

* fix bloomFilter

* changelog

* add validation

Co-authored-by: Federico Kunze Küllmer <[email protected]>

fix lint

Update PR url in CHANGELOG
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem: eth_getLogs with large block range is slow and cause OOM
3 participants