-
Notifications
You must be signed in to change notification settings - Fork 113
feat: add limit for batch request and response size #249
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm, thank you for the contribution.
Similar to other PRs, could you add a more thorough description here and in the issue of the problem and your solution? The reference was helpful for context, but the PR you referenced is different from the changes you made here.
rpc/backend/call_tx.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| length := len(res.Ret) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you removed this by mistake in the latest commits :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert the space, no change for this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I see, the restriction is now happening on the geth side. Lgtm!
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add back the code here
|
@mmsqe we have resolved this in the audit and the change will be integrated once published. Will close this PR once integrated |
|
@mmsqe this is fixed in 0.3 and will be added back onto |
|
@aljo242 just saw missing config for toml from main, not sure if it's intended |
|
Makes sense to have in the config - thank you! this was an oversight |
* feat: add limit for eth_call return value * align batch limit * add back space * add back space * resolve --------- Co-authored-by: Alex | Interchain Labs <alex@interchainlabs.io>
Description
batch-request-limitconfig to enforces the maximum number of requests per batch and prevents abuse by oversized batch requestsbatch-response-max-sizeconfig to allow server should reject the batch or truncate the response, returning an error or partial result indicating the response size limit was exceeded.Closes: #241, for test info
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
mainbranchReviewers Checklist
All items are required.
Please add a note if the item is not applicable
and please add your handle next to the items reviewed
if you only reviewed selected items.
I have...
Unreleasedsection inCHANGELOG.md