feat(specs): add max bal item check#2109
feat(specs): add max bal item check#2109nerolation wants to merge 3 commits intoethereum:forks/amsterdamfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #2109 +/- ##
===================================================
- Coverage 86.07% 85.47% -0.61%
===================================================
Files 599 599
Lines 39472 39489 +17
Branches 3780 3781 +1
===================================================
- Hits 33977 33754 -223
- Misses 4862 5102 +240
Partials 633 633
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Note that the two new constants are from EIP-7002 and EIP-7251. |
| ) | ||
| ) * item_cost | ||
|
|
||
| if bal_items * item_cost > available_gas + system_allowance: |
There was a problem hiding this comment.
This assumes that all bal_items have a cost, but there are also free items, like the sender and receiver address. This will flag BALs which are valid as "too full"
There was a problem hiding this comment.
We should also add tests with BALs near max-size (I think we already have this) to check if clients don't early-reject those because they are too large. We cant create a BAL which has the max size as the hard cap calculation makes some assumptions which are not true in practice (SSTORE/SLOAD without pushing items to stack for instance)
There was a problem hiding this comment.
yeah good point!
I wouldn't say you get the sender and the receiver for free though - there's still the 21k gas costs which is above the 2000. The only truly "free" items are those from system contracts, if I'm not forgetting something here.
We should also add tests with BALs near max-size (I think we already have this) to check if clients don't early-reject those because they are too large. We cant create a BAL which has the max size as the hard cap calculation makes some assumptions which are not true in practice (SSTORE/SLOAD without pushing items to stack for instance)
The goal here was to not create any new practical constrain for block building. This means, as long as there is gas available during builders, we must ensure that the BAL item size isn't hit yet -> thus the very conservate 2000 gas per item + not having the system contract items impact it.
Re tests, yeah, we should definitely have test for this, e.g. create a block that is valid (thus it must obey the max item limit too) but as well have tests with invalid BALs that are slightly over the max-items to make sure those are rejected early on.
| len(account.storage_reads) for account in block_access_list | ||
| ) | ||
| total_addresses = len(block_access_list) | ||
| bal_items = Uint(total_storage_reads + total_addresses) |
There was a problem hiding this comment.
total_storage_reads cost TX_ACCESS_LIST_STORAGE_KEY_COST per item, and for address the param respectively to that for addresses (which is higher). The cost is thus underestimated here (which is fine, because overestimating cost will invalidate blocks faster)
There was a problem hiding this comment.
yeah right - it's a trade-off between simplicity and accuracy. Since this is mainly needed for early rejections of BALs that have way too many items (e.g. 2x what is realistic; those would cause 2x the work which we aren't accounting for when setting the gas limit), to counter DoS, I tend leaning towards simplicity first.
| ) | ||
|
|
||
| # Validate block access list gas limit constraint (EIP-7928) | ||
| validate_block_access_list_gas_limit( |
There was a problem hiding this comment.
Should the t8n tool be updated to reflect this as well?
There was a problem hiding this comment.
Yeah, we likely need to add this here so we can test this appropriately.
4 participants
🗒️ Description
Adds a gas limit constraint validation for EIP-7928 block access lists.
It bounds the block access list size based on available gas after accounting for transaction base costs.
Changes:
🔗 Related Issues or PRs
ethereum/EIPs#11223