Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat!: limit the max tx size to 2 MiB #3909
feat!: limit the max tx size to 2 MiB #3909
Changes from 9 commits
fb9066f
dacf9e8
334e789
f7b7d59
89a4e69
a31c0b8
d906239
941f55b
4c7f063
567d832
7d34dfd
172e29b
156dc32
6dd6a1f
c73e687
12dfb01
54b2b4d
a873099
70b4174
f600227
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Correct the transaction size comparison operator.
The current condition rejects transactions that are exactly the maximum allowed size:
If the intention is to allow transactions up to and including the maximum size, consider changing the comparison operator to
>
:This change ensures that transactions equal to the maximum size are accepted, and only those exceeding it are rejected.
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.
this is good to note, whichever is used, we simply need to document it
using strictly greater than makes sense to me, as we can just use that constant everywhere instead of subtracting 1 everywhere as well
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 second this that it would be better if it were strictly greater than 2MB
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.
optional, but useful
we can add a bool to determine if this is checkTx here, then add that bool to the test sdk.Context as will with
sdkCtx.WithIsCheckTx(true)
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.
both addressed 54b2b4d and a873099
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.
[not blocking] we should have considered naming it something else to avoid conflicting with the mempool parameter
MaxTxBytes
. For exampleMaxTxSize
would've workedThere 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 can change it in a follow-up since i need to update the ante handler anyway :)