-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Check gas limit consistency with the target #5236
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
Changes from all commits
3f80200
60e24b4
0df6375
dd31277
2dcc10c
b50daf5
964919b
e92f06f
ae110b1
9b480f3
54d1068
1bb549b
5357718
d66e57d
3b61385
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 |
|---|---|---|
|
|
@@ -90,7 +90,7 @@ class ProposerPreferences(Container): | |
| proposal_slot: Slot | ||
| validator_index: ValidatorIndex | ||
| fee_recipient: ExecutionAddress | ||
| gas_limit: uint64 | ||
| target_gas_limit: uint64 | ||
| ``` | ||
|
|
||
| #### New `SignedProposerPreferences` | ||
|
|
@@ -347,7 +347,6 @@ where `parent_state` is the post-state of `bid.parent_block_root`, and the alias | |
| `is_active_builder(state, bid.builder_index)` returns `True`. | ||
| - _[REJECT]_ `bid.execution_payment == 0`. | ||
| - _[REJECT]_ `bid.fee_recipient == proposer_preferences.fee_recipient`. | ||
| - _[REJECT]_ `bid.gas_limit == proposer_preferences.gas_limit`. | ||
| - _[REJECT]_ The length of KZG commitments is less than or equal to the | ||
| limitation defined in the consensus layer -- i.e. validate that | ||
| `len(bid.blob_kzg_commitments) <= get_blob_parameters(compute_epoch_at_slot(bid.slot)).max_blobs_per_block`. | ||
|
|
@@ -358,12 +357,34 @@ where `parent_state` is the post-state of `bid.parent_block_root`, and the alias | |
| - _[IGNORE]_ `bid.value` is less or equal than the builder's excess balance -- | ||
| i.e. `can_builder_cover_bid(state, builder_index, amount)` returns `True`. | ||
| - _[IGNORE]_ `bid.parent_block_hash` is the block hash of a known execution | ||
| payload in fork choice. | ||
| payload in fork choice and | ||
| `is_gas_limit_target_compatible(parent_gas_limit, bid.gas_limit, proposer_preferences.target_gas_limit)` | ||
| is `True` where `parent_gas_limit` is the `gas_limit` of that execution | ||
| payload. | ||
| - _[IGNORE]_ `bid.parent_block_root` is the hash tree root of a known beacon | ||
| block in fork choice. | ||
| - _[REJECT]_ `signed_execution_payload_bid.signature` is valid with respect to | ||
| the `bid.builder_index`. | ||
|
|
||
| ```python | ||
| def is_gas_limit_target_compatible( | ||
| parent_gas_limit: uint64, gas_limit: uint64, target_gas_limit: uint64 | ||
| ) -> bool: | ||
| """ | ||
| Check if ``gas_limit`` is compatible with ``target_gas_limit`` under the | ||
| EIP-1559 transition rule from ``parent_gas_limit``. | ||
| """ | ||
| max_gas_limit_difference = max(parent_gas_limit // 1024, 1) - 1 | ||
| min_gas_limit = parent_gas_limit - max_gas_limit_difference | ||
|
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. Don't we have to worry about overflow here? What if
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. Actually nvm, it's fine because of how |
||
| max_gas_limit = parent_gas_limit + max_gas_limit_difference | ||
|
|
||
| if target_gas_limit >= min_gas_limit and target_gas_limit <= max_gas_limit: | ||
| return gas_limit == target_gas_limit | ||
| if target_gas_limit > max_gas_limit: | ||
| return gas_limit == max_gas_limit | ||
| return gas_limit == min_gas_limit | ||
|
jtraglia marked this conversation as resolved.
|
||
| ``` | ||
|
|
||
| *Note*: Implementations SHOULD include DoS prevention measures to mitigate spam | ||
| from malicious builders submitting numerous bids with minimal value increments. | ||
| Possible strategies include: (1) only forwarding bids that exceed the current | ||
|
|
@@ -376,7 +397,7 @@ bid at regular time intervals. | |
|
|
||
| This topic is used to propagate signed proposer preferences as | ||
| `SignedProposerPreferences`. These messages allow validators to communicate | ||
| their preferred `fee_recipient` and `gas_limit` to builders. | ||
| their preferred `fee_recipient` and `target_gas_limit` to builders. | ||
|
|
||
| The following validations MUST pass before forwarding the | ||
| `signed_proposer_preferences` on the network, assuming the alias | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| from eth_consensus_specs.test.context import ( | ||
| single_phase, | ||
| spec_test, | ||
| with_gloas_and_later, | ||
| ) | ||
|
|
||
|
|
||
| @with_gloas_and_later | ||
| @spec_test | ||
| @single_phase | ||
| def test_increase_within_limit(spec): | ||
| assert spec.is_gas_limit_target_compatible(60_000_000, 60_000_100, 60_000_100) | ||
|
|
||
|
|
||
| @with_gloas_and_later | ||
| @spec_test | ||
| @single_phase | ||
| def test_increase_exceeding_limit(spec): | ||
| # max_gas_limit_difference = 60_000_000 // 1024 - 1 = 58_592 | ||
| assert spec.is_gas_limit_target_compatible(60_000_000, 60_058_592, 100_000_000) | ||
|
|
||
|
|
||
| @with_gloas_and_later | ||
| @spec_test | ||
| @single_phase | ||
| def test_increase_exceeding_limit_off_by_one_fails(spec): | ||
| # gas_limit one above max_gas_limit (= 60_058_592) must fail (off by one) | ||
| assert not spec.is_gas_limit_target_compatible(60_000_000, 60_058_593, 100_000_000) | ||
|
|
||
|
|
||
| @with_gloas_and_later | ||
| @spec_test | ||
| @single_phase | ||
| def test_decrease_within_limit(spec): | ||
| assert spec.is_gas_limit_target_compatible(60_000_000, 59_999_990, 59_999_990) | ||
|
|
||
|
|
||
| @with_gloas_and_later | ||
| @spec_test | ||
| @single_phase | ||
| def test_decrease_exceeding_limit(spec): | ||
| # max_gas_limit_difference = 60_000_000 // 1024 - 1 = 58_592 | ||
| assert spec.is_gas_limit_target_compatible(60_000_000, 59_941_408, 30_000_000) | ||
|
|
||
|
|
||
| @with_gloas_and_later | ||
| @spec_test | ||
| @single_phase | ||
| def test_target_equals_parent(spec): | ||
| assert spec.is_gas_limit_target_compatible(60_000_000, 60_000_000, 60_000_000) | ||
|
|
||
|
|
||
| @with_gloas_and_later | ||
| @spec_test | ||
| @single_phase | ||
| def test_parent_gas_limit_underflows(spec): | ||
| # parent_gas_limit // 1024 = 0; guard clamps to max(0, 1) - 1 = 0 (no underflow) | ||
| assert spec.is_gas_limit_target_compatible(1023, 1023, 60_000_000) |
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.
just realized this but shouldn't this be a
REJECTifis_gas_limit_target_compatible = False? we do the same if fee recipient is incorrect, wondering if we usedIGNOREjust because it's combined with "known execution payload in fork choice" conditionThere 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.
Yes, I think it should. Can you open a PR for this?
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.