-
Notifications
You must be signed in to change notification settings - Fork 7
feat: limit pool tx size #317
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
Signed-off-by: Gregory Edison <[email protected]>
Signed-off-by: Gregory Edison <[email protected]>
CodSpeed Performance ReportMerging #317 will not alter performanceComparing Summary
|
Signed-off-by: Gregory Edison <[email protected]>
Signed-off-by: Gregory Edison <[email protected]>
Signed-off-by: Gregory Edison <[email protected]>
Signed-off-by: Gregory Edison <[email protected]>
| .with_local_transactions_config( | ||
| pool_config_overrides.clone().apply(ctx.pool_config()).local_transactions_config, | ||
| ) | ||
| .with_max_tx_input_bytes(ctx.chain_spec().chain_config().max_tx_payload_bytes_per_block) |
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.
Just a few notes:
- Here we only check
tx.input, but the overall transaction could be much larger (e.g. by including a long authorization list). - Also, if
tx.input == max_payload_size, then we'll admin it into the txpool, but we must not include it in the block (the full serialized block must respect the same limit).
That said, this is not very critical, and the current impl seems idential to l2geth. The critical part is ensuring these limits during block building.
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.
Was not aware of the second point, can always add this to the block builder, it's not much. For the first point, I think we check tx.rlp_encoding_length so this should cover the authorization list.
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 check is here: https://github.com/scroll-tech/go-ethereum/blob/develop/miner/scroll_worker.go#L836. Yes we should add it to the block builder.
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.
After a check, this was implemented by Morty here.
* feat: limit rollup fee in tx pool Signed-off-by: Gregory Edison <[email protected]> * fix: check rollup fee Signed-off-by: Gregory Edison <[email protected]> * feat: limit pool tx size (#317) * feat: add maxTxPayloadBytesPerBlock to ScrollChainConfig Signed-off-by: Gregory Edison <[email protected]> * feat: limit pool max tx input bytes Signed-off-by: Gregory Edison <[email protected]> * test: set max_tx_payload_bytes_per_block Signed-off-by: Gregory Edison <[email protected]> * feat: make MockEthProvider more generic Signed-off-by: Gregory Edison <[email protected]> * test: add pool limit tests Signed-off-by: Gregory Edison <[email protected]> * fix: lints Signed-off-by: Gregory Edison <[email protected]> --------- Signed-off-by: Gregory Edison <[email protected]> * test: update Signed-off-by: Gregory Edison <[email protected]> --------- Signed-off-by: Gregory Edison <[email protected]>
* feat: limit rollup fee in tx pool Signed-off-by: Gregory Edison <[email protected]> * fix: check rollup fee Signed-off-by: Gregory Edison <[email protected]> * feat: limit pool tx size (#317) * feat: add maxTxPayloadBytesPerBlock to ScrollChainConfig Signed-off-by: Gregory Edison <[email protected]> * feat: limit pool max tx input bytes Signed-off-by: Gregory Edison <[email protected]> * test: set max_tx_payload_bytes_per_block Signed-off-by: Gregory Edison <[email protected]> * feat: make MockEthProvider more generic Signed-off-by: Gregory Edison <[email protected]> * test: add pool limit tests Signed-off-by: Gregory Edison <[email protected]> * fix: lints Signed-off-by: Gregory Edison <[email protected]> --------- Signed-off-by: Gregory Edison <[email protected]> * test: update Signed-off-by: Gregory Edison <[email protected]> * feat: disallow L1 messages in tx pool Signed-off-by: Gregory Edison <[email protected]> * test: disallow L1 messages in tx pool Signed-off-by: Gregory Edison <[email protected]> --------- Signed-off-by: Gregory Edison <[email protected]> Co-authored-by: frisitano <[email protected]>
Resolves #310