-
Notifications
You must be signed in to change notification settings - Fork 994
add --max-blobs cli flag and only check maxBlobsPerBlock >= maxBlobsPerTransaction if it's a public Ethereum network #9531
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
base: main
Are you sure you want to change the base?
Conversation
85c4c4f to
024c892
Compare
7e64f37 to
799f7cb
Compare
…erTransaction if it's a public Ethereum network Signed-off-by: daniellehrner <daniel.lehrner@consensys.net>
Signed-off-by: daniellehrner <daniel.lehrner@consensys.net>
Signed-off-by: daniellehrner <daniel.lehrner@consensys.net>
Signed-off-by: daniellehrner <daniel.lehrner@consensys.net>
Signed-off-by: daniellehrner <daniel.lehrner@consensys.net>
799f7cb to
7c9b059
Compare
| // These forks use OsakaTargetingGasLimitCalculator which requires maxBlobsPerBlock >= | ||
| // maxBlobsPerTransaction | ||
| for (MainnetHardforkId hardfork : MainnetHardforkId.values()) { | ||
| if (hardfork.ordinal() >= MainnetHardforkId.OSAKA.ordinal()) { |
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 works, but using enum ordinals seems fragile, why not leverage something derived from MilestoneDefinitions::createMainnetMilestoneDefinitions where we manually enforce the orders?
| hardfork == MainnetHardforkId.OSAKA | ||
| ? Optional.of(BlobSchedule.PRAGUE_DEFAULT) | ||
| : Optional.empty()); |
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.
why this special case for Osaka, what will happen for future hardforks?
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.
is it correct to say that if hardfork == MainnetHardforkId.OSAKA then opts.getBlobSchedule(hardfork) is not empty?
If so the code in or is not needed
| bs -> | ||
| checkArgument( | ||
| bs.getMax() >= maxBlobsPerTransaction, | ||
| "maxBlobsPerTransaction (%s) must not be greater than maxBlobsPerBlock (%s) for %s. " |
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.
| "maxBlobsPerTransaction (%s) must not be greater than maxBlobsPerBlock (%s) for %s. " | |
| "maxBlobsPerTransaction (%d) must not be greater than maxBlobsPerBlock (%d) for %s. " |
| DEFAULT_PLUGIN_BLOCK_TXS_SELECTION_MAX_TIME; | ||
|
|
||
| @Option( | ||
| names = {"--max-blobs"}, |
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.
is the option name fixed, or it can be extended to tell that the number if per transactions, to avoid ambiguity with per block, etc...
| "--max-blobs", | ||
| "0"); | ||
| } | ||
|
|
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.
while be nice to have a test about the failure, when max block per tx is greater than max blol per block
| public int getMaxBlobsPerTransaction() { | ||
| return getMutableInitValues().getMaxBlobsPerTransaction(); | ||
| } |
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 seems this value cannot be changed at runtime, so it should not be part of mutable values
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Gabriel-Trintinalia
left a comment
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 really don’t think DEFAULT_MAX_BLOBS_PER_TRANSACTION should live in MiningConfiguration. It really belongs in OsakaTargetingGasLimitCalculator. While the same constant exists there today, it’s only used by tests in this PR. That means if a future fork changes the default, we’d have to change it through the mining configuration, which isn’t ideal. The default should exist in a single place: the TargetingGasLimitCalculator implementations.
A cleaner approach is to make maxBlobsPerTransaction in MiningConfiguration an OptionalInt that defaults to empty. If the user doesn’t explicitly set --max-blobs, we pass OptionalInt.empty() and let the TargetingGasLimitCalculator apply its own constant depending on the fork. This makes the code more resilient to future protocol changes.
Regarding the maxBlobsPerBlock >= maxBlobsPerTransaction check, I’m fine either removing it entirely or not treating L1s and L2s differently. The protocol doesn’t define that behaviour, so Besu shouldn’t enforce it. If we keep the check, L2s can bypass it by setting the blob schedule to zero and --max-blobs=0, so the current logic doesn’t actually enforce anything meaningful.
PR description
Adding the
--max-blobsflag to restrict how many blobs a block builder will include independently of how many are avaialable in the tx poool.Fix an issue with the blob schedule where Ethereum network blob restrictions where also imposed on Linea and other networks with a custom genesis file.
Fixed Issue(s)
fixes #7683
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew spotlessApply./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests