-
Notifications
You must be signed in to change notification settings - Fork 840
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
Improve blob size transaction selector #7312
Improve blob size transaction selector #7312
Conversation
53f13b9
to
b8eec9b
Compare
return true; | ||
} | ||
|
||
return transaction.getGasLimit() + blobGasUsed |
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.
adding blobGasUsed
is incorrect here, since here we only want to check the gas used and not the data gas used
Can we use the term |
from reading the description this PR is doing 4 very excellent things, but would be easier to review as several smaller PRs |
b8eec9b
to
1c9d469
Compare
Fair point, splitting in 3 PRs: first one Stop transaction selection on TX_EVALUATION_TOO_LONG the fix to blob gas is related to this refactor so make sense to keep in this PR |
I also like more blob gas because is more common, and I will rename to it, but in many other places we have data gas like the |
2de28d5
to
b17a880
Compare
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
b17a880
to
16b455b
Compare
PR description
The main goal of this PR is to make clearer the reason a blob tx is not selected during block creation, with the introduction of 2 specifics not selected results:
and to achieve that it was clearer to extract the blob tx selection login in its own class
BlobSizeTransactionSelector
, while doing that a small bug was fixed, since data gas used was added to gas used when computing if a block was full, while these 2 gas types are independent, see here for details.Fixed Issue(s)
Thanks for sending a pull request! Have you done the following?
doc-change-required
label to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew build
./gradlew acceptanceTest
./gradlew integrationTest
./gradlew ethereum:referenceTests:referenceTests