-
Notifications
You must be signed in to change notification settings - Fork 155
chore(tests|forks): add max blobs per tx limit #1884
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
I tried to fix the coverage for this line below, in the following commit:
|
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.
Nothing obvious stands out to me here! 🚀
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.
Looks great! Just a couple of comments, thanks!
# This allows us to keep the creation of single transactions for Cancun/Prague where the | ||
# MAX_BLOBS_PER_TX is not enforced, hitting coverage for block level blob gas validation | ||
# when parent_blobs > MAX_BLOBS_PER_BLOCK. | ||
max_blobs_per_tx = fork.max_blobs_per_tx() if fork >= Osaka else parent_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.
We could check whether max per block != max per tx instead of checking explicitly for Osaka.
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.
Yeah I think I had this initially but changed it to this to see if it would fix the coverage! I will revert back.
# when parent_blobs > MAX_BLOBS_PER_BLOCK. | ||
max_blobs_per_tx = fork.max_blobs_per_tx() if fork >= Osaka else parent_blobs | ||
blob_chunks = [ | ||
range(i, min(i + max_blobs_per_tx, parent_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.
Nice.
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py
Outdated
Show resolved
Hide resolved
3571174
to
193ea5e
Compare
🗒️ Description
Following the changes to EIP-7594: ethereum/EIPs#9981
Adds some more
execute_blobs
tests for PeerDAS, including tests for the new max blobs per tx limit.Updates fork logic and EIP-4844 tests to use a new
max_blobs_per_tx
function.Adds specific tests for the new change (including transition tests) within the PeerDAS EIP testing folder.
Currently filling with this
eels_resolution.json
:The magic EELS commit is: ethereum/execution-specs@0769917
Requires
🔗 Related Issues or PRs
#1798
✅ Checklist
tox
checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
type(scope):
.