-
Notifications
You must be signed in to change notification settings - Fork 419
feat(specs): add max bal item check #2109
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: forks/amsterdam
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,9 +17,11 @@ | |
| from typing import TYPE_CHECKING, Dict, List, Set | ||
|
|
||
| from ethereum_types.bytes import Bytes | ||
| from ethereum_types.numeric import U64, U256 | ||
| from ethereum_types.numeric import U64, U256, Uint | ||
|
|
||
| from ..exceptions import BlockAccessListGasLimitExceededError | ||
| from ..fork_types import Address | ||
| from ..transactions import TX_ACCESS_LIST_STORAGE_KEY_COST, TX_BASE_COST | ||
| from .rlp_types import ( | ||
| AccountChanges, | ||
| BalanceChange, | ||
|
|
@@ -517,3 +519,56 @@ def build_block_access_list( | |
| add_code_change(builder, address, block_access_index, new_code) | ||
|
|
||
| return _build_from_builder(builder) | ||
|
|
||
|
|
||
| def validate_block_access_list_gas_limit( | ||
| block_access_list: BlockAccessList, | ||
| block_gas_limit: Uint, | ||
| tx_count: int, | ||
| ) -> None: | ||
| """ | ||
| Validate that the block access list does not exceed the gas limit. | ||
|
|
||
| The constraint is: | ||
| ``bal_items * ITEM_COST <= available_gas + system_allowance`` | ||
|
|
||
| Parameters | ||
| ---------- | ||
| block_access_list : | ||
| The block access list to validate. | ||
| block_gas_limit : | ||
| The gas limit of the block. | ||
| tx_count : | ||
| The number of transactions in the block. | ||
|
|
||
| Raises | ||
| ------ | ||
| BlockAccessListGasLimitExceededError : | ||
| If the block access list gas cost exceeds the available gas. | ||
|
|
||
| """ | ||
| from ..fork import ( | ||
| MAX_CONSOLIDATION_REQUESTS_PER_BLOCK, | ||
| MAX_WITHDRAWAL_REQUESTS_PER_BLOCK, | ||
| ) | ||
| from ..vm.gas import GAS_WARM_ACCESS | ||
|
|
||
| total_storage_reads = sum( | ||
| len(account.storage_reads) for account in block_access_list | ||
| ) | ||
| total_addresses = len(block_access_list) | ||
| bal_items = Uint(total_storage_reads + total_addresses) | ||
|
|
||
| item_cost = GAS_WARM_ACCESS + TX_ACCESS_LIST_STORAGE_KEY_COST | ||
| available_gas = block_gas_limit - Uint(tx_count) * TX_BASE_COST | ||
| system_allowance = ( | ||
| Uint(15) | ||
| + Uint(3) | ||
| * ( | ||
| MAX_WITHDRAWAL_REQUESTS_PER_BLOCK | ||
| + MAX_CONSOLIDATION_REQUESTS_PER_BLOCK | ||
| ) | ||
| ) * item_cost | ||
|
|
||
| if bal_items * item_cost > available_gas + system_allowance: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assumes that all
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah good point!
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. |
||
| raise BlockAccessListGasLimitExceededError | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,7 +29,10 @@ | |
| ) | ||
|
|
||
| from . import vm | ||
| from .block_access_lists.builder import build_block_access_list | ||
| from .block_access_lists.builder import ( | ||
| build_block_access_list, | ||
| validate_block_access_list_gas_limit, | ||
| ) | ||
| from .block_access_lists.rlp_utils import compute_block_access_list_hash | ||
| from .blocks import Block, Header, Log, Receipt, Withdrawal, encode_receipt | ||
| from .bloom import logs_bloom | ||
|
|
@@ -125,6 +128,8 @@ | |
| HISTORY_STORAGE_ADDRESS = hex_to_address( | ||
| "0x0000F90827F1C53a10cb7A02335B175320002935" | ||
| ) | ||
| MAX_WITHDRAWAL_REQUESTS_PER_BLOCK = Uint(16) | ||
| MAX_CONSOLIDATION_REQUESTS_PER_BLOCK = Uint(2) | ||
| MAX_BLOCK_SIZE = 10_485_760 | ||
| SAFETY_MARGIN = 2_097_152 | ||
| MAX_RLP_BLOCK_SIZE = MAX_BLOCK_SIZE - SAFETY_MARGIN | ||
|
|
@@ -834,6 +839,13 @@ def apply_body( | |
| block_env.state_changes | ||
| ) | ||
|
|
||
| # Validate block access list gas limit constraint (EIP-7928) | ||
| validate_block_access_list_gas_limit( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we likely need to add this here so we can test this appropriately. |
||
| block_access_list=block_output.block_access_list, | ||
| block_gas_limit=block_env.block_gas_limit, | ||
| tx_count=len(transactions), | ||
| ) | ||
|
|
||
| return block_output | ||
|
|
||
|
|
||
|
|
||
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.
total_storage_reads cost
TX_ACCESS_LIST_STORAGE_KEY_COSTper 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.