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.

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

fix: api: Length check the array sent to RPC. #11696

Merged
merged 9 commits into from
Mar 11, 2024
Merged

fix: api: Length check the array sent to RPC. #11696

merged 9 commits into from
Mar 11, 2024

Conversation

parthshah1
Copy link
Contributor

@parthshah1 parthshah1 commented Mar 8, 2024

Related Issues

When calling eth_feeHistory, it takes input a rewardPercentile array. Currently, there isn't any restriction on the length of the array, can result in high computation and potentially leading to OOM issues in RPC nodes. The fix is to set a hard limit of the array to 100.

Proposed Changes

Changing the length of RewardPercentiles Array to upto 100.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

node/impl/full/eth.go Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Mar 8, 2024

Seems reasonable, except that I don't know what a reasonable limit on this would be; geth doesn't appear to have one, at least when you get this deep: https://github.com/ethereum/go-ethereum/blob/c41105ce80f12f60ec4bf6c65c4c59c6bf4a86e7/eth/gasprice/feehistory.go#L200 and it event does an 8*len allocation at one point. Maybe there's a total params limit size, which I guess would be appropriate for us too if we don't have one.

Co-authored-by: Rod Vagg <rod@vagg.org>
@parthshah1
Copy link
Contributor Author

@rvagg. I had a discussion about the limits with @Stebalien, that maybe 100 would be reasonable number since one would not need more than that to calculate the eth_feehistory. It also aims to solve computation issues this might cause with huge arrays.

it event does an 8*len allocation at one point. Maybe there's a total params limit size, which I guess would be appropriate for us too if we don't have one.

I was not aware of such a case in geth. I am not too expert in this area and how often, how much data it is expected to compute. I will agree to what you think should be right. Though, imo keeping the hard fix on the length won't harm.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

this seems fine to me, we can bump it up if there's a genuine request for more

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Nits, but this looks correct.

node/impl/full/eth.go Outdated Show resolved Hide resolved
node/impl/full/eth.go Outdated Show resolved Hide resolved
@Stebalien
Copy link
Member

Thank you! Sorry for all the nits.

One final thing: it looks like some of the lints are unhappy. I think you just need to run go fmt and fix some spelling.

node/impl/full/eth.go Outdated Show resolved Hide resolved
@rvagg rvagg merged commit 7516aff into filecoin-project:master Mar 11, 2024
87 of 88 checks passed
Stebalien added a commit that referenced this pull request Mar 13, 2024
Co-authored-by: Rod Vagg <rod@vagg.org>
Co-authored-by: Steven Allen <steven@stebalien.com>
Stebalien added a commit that referenced this pull request Mar 13, 2024
Co-authored-by: Rod Vagg <rod@vagg.org>
Co-authored-by: Steven Allen <steven@stebalien.com>
rjan90 pushed a commit that referenced this pull request Mar 22, 2024
Co-authored-by: Rod Vagg <rod@vagg.org>
Co-authored-by: Steven Allen <steven@stebalien.com>
@rvagg rvagg mentioned this pull request May 1, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

4 participants