-
Notifications
You must be signed in to change notification settings - Fork 547
Add transaction size validation to RPC endpoints #1807
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
Open
manuelmauro
wants to merge
7
commits into
polkadot-evm:master
Choose a base branch
from
moonbeam-foundation:manuel/tx-size-validation
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+271
−4
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
4d7f850
feat: :sparkles: add transaction size validation to RPC endpoints
manuelmauro c242d68
style: :art: fmt
manuelmauro 3bc944f
refactor: :rotating_light: lint
manuelmauro fc974a5
docs: :memo: remove links to geth/reth in comments
manuelmauro b5eef63
refactor: :recycle: return error if the tx request can't be converted…
manuelmauro 1b2f4fc
fix(rpc): validate transaction size after message fields are populated
manuelmauro a5c026f
style(rpc): use expect with 'should' convention in tests
manuelmauro File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
validate_size()is still an estimate, not the final signed size.encoded_length()adds a fixed 67-byte signature allowance, but signed transaction RLP is not fixed-width:r/scan shrink when they have leading zeroes, and legacyvis variable. Becauseclient/rpc/src/eth/submit.rsnow uses this as the hard reject foreth_sendTransaction, near-limit requests can be rejected here even though the signed bytes would fit and pass the raw path. Please keep this as a pre-sign guard only, and enforce the definitive 128 KiB limit on the signed transaction bytes.🤖 Prompt for AI Agents
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.
It is meant to be an estimate. Also on the
etherumside the encoded size is just estimated.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.
@manuelmauro, that's a fair point — thank you for the clarification. SinceSIGNATURE_RLP_OVERHEAD = 67is the maximum possible signature size (r and s can only be ≤ 33 bytes each, never more),encoded_length()is a conservative upper-bound estimate. This means:estimated_size > 128 KiB→ reject (the actual signed tx could be slightly smaller, so rare false positives are possible at the very edge)estimated_size ≤ 128 KiB→ the actual signed tx is guaranteed to fit (no false negatives)For DoS protection, this is the right trade-off — you never let an oversized transaction slip through; you might conservatively reject a few borderline ones. The approach is sound.
One minor suggestion: it may be worth updating the
validate_size()doc comment to explicitly note that it uses a conservative upper-bound and that false positives are possible withinSIGNATURE_RLP_OVERHEADbytes of the limit. This would help future reviewers understand the intent without needing to dig into the constants.(´• ω •`) 🐇
✏️ Learnings added