-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Update EIP-7732 #4438
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: master
Are you sure you want to change the base?
Update EIP-7732 #4438
Conversation
- Adds a new constant '0x03' builder withdrawal prefix. - Fixes `is_compounding_withdrawal_credentials` to also allow builders to compound. - Adds helper predicates to check if a validator is a builder. - When processing a an execution payload header, checks if the builder has builders' withdrawal credentials or if the proposer is self-building with zero value. - Only propagate bids from builders.
- Removes rewards for PTC attestations and for the proposer for including them. - Kees PTC members in the beacon committee.
- Adds a slot to the `ExecutionPayloadEnvelope` structure. - Checks that the slot is compatible with the block when processing the envelope and on p2p when gossiping payloads.
- Remove payload status enumeration and use a boolean `payload_present` instead. - Make payload attestations a maximum of 2 per block. - Remove instances of payload status in forkchoice (defer forkchoice changes until later commit) - Remove instances of payload status in the p2p spec.
- Move some presets to constants - Add a queue of builder pending payments and a queue of builder pending withdrawals to the beacon state - Add a fee recipient to the builder's bid - Modify process epoch to either delete the builder's pending payment if the block was not voted enough, or queue a withdrawal if the block was voted enough. Every epoch the pending payments from the previous epoch are all processed (unless the payload was already included in which case the payment was already processed) and the current epoch payments are moved to the beginning of the queue. - Modify the withdrawal handling by first processing all withdrawals from the builder's pending queue, before any other withdrawal. These withdrawals are processed as follows. If the builder is slashed then they are kept in the queue until the builder is withdrawable, in which case the payments will be fullfilled with the remaining balance. - Modify how to process the bid in the beacon block. Instead of deducting the builder immediately, we set a pending payment in the queue, to be deducted later with the churn when processing the withdrawal. - Modify process attestations to keep track of those attestations that voted for the beacon block when no payload was included.
- Adds a bitvector to track payload availability on the beacon state. - Check for payload compatibility to reward for timely head. - Check that the index is less than 2 and it equals 0 when attesting for the current slot. - Add fork upgrading code to initiate the builder pending payments for the pre-fork epoch. - Add fork upgrading code to make all previous payloads available.
This comment has been minimized.
This comment has been minimized.
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.
I didn't fully review everything, but I felt that this was enough for now. I'm liking your changes here! Overall very clean and understandable.
# floorlog2(get_generalized_index(BeaconBlockBody, 'blob_kzg_commitments')) + 1 + ceillog2(MAX_BLOB_COMMITMENTS_PER_BLOCK) (= 8 + 1 + 5 = 14) | ||
KZG_COMMITMENT_INCLUSION_PROOF_DEPTH_EIP7732: 14 | ||
KZG_COMMITMENT_INCLUSION_PROOF_DEPTH_EIP7732: 15 |
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.
Need to update the comment, it still says 14.
- [`bit_floor`](#bit_floor) | ||
- [Misc](#misc-1) | ||
- [Misc](#misc-2) | ||
- [`remove_flag`](#remove_flag) | ||
- [Predicates](#predicates) | ||
- [New `is_attestation_same_slot`](#new-is_attestation_same_slot) | ||
- [Modified `is_compounding_withdrawal_credential`](#modified-is_compounding_withdrawal_credential) | ||
- [New `is_builder_withdrawal_credential`](#new-is_builder_withdrawal_credential) | ||
- [New `is_builder`](#new-is_builder) | ||
- [`is_valid_indexed_payload_attestation`](#is_valid_indexed_payload_attestation) | ||
- [`is_parent_block_full`](#is_parent_block_full) | ||
- [Beacon State accessors](#beacon-state-accessors) | ||
- [`get_attestation_participation_flag_indices`](#get_attestation_participation_flag_indices) | ||
- [`get_ptc`](#get_ptc) | ||
- [Modified `get_attesting_indices`](#modified-get_attesting_indices) | ||
- [`get_payload_attesting_indices`](#get_payload_attesting_indices) | ||
- [`get_indexed_payload_attestation`](#get_indexed_payload_attestation) |
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.
A reminder for later, we should add New
or Modified
to all of these.
|
||
| Name | Value | Unit | | ||
| ----------------------------------- | ----------------------------- | --------------------------- | | ||
| `BUILDER_PENDING_WITHDRAWALS_LIMIT` | `uint64(2**20)` (= 1,048,576) | builder pending withdrawals | |
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.
Should capitalize the unit sentence.
| `BUILDER_PENDING_WITHDRAWALS_LIMIT` | `uint64(2**20)` (= 1,048,576) | builder pending withdrawals | | |
| `BUILDER_PENDING_WITHDRAWALS_LIMIT` | `uint64(2**20)` (= 1,048,576) | Builder pending withdrawals | |
|
||
| Name | Value | | ||
| ----------------------- | ---------------------------------------------- | | ||
| `DOMAIN_BEACON_BUILDER` | `DomainType('0x1B000000')` # (New in EIP-7732) | |
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.
Is there a reason this started with 1? I believe this should be:
| `DOMAIN_BEACON_BUILDER` | `DomainType('0x1B000000')` # (New in EIP-7732) | | |
| `DOMAIN_BEACON_BUILDER` | `DomainType('0x0B000000')` # (New in EIP-7732) | |
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.
I'm carrying this since forever because I didn't know which fork it would collide with, I feel it's better to keep a random number until it's time to ship (if that ever happens)
| `DOMAIN_PTC_ATTESTER` | `DomainType('0x0C000000')` # (New in EIP-7732) | | ||
| Name | Value | | ||
| -------------------------- | ----------------------- | | ||
| `MAX_PAYLOAD_ATTESTATIONS` | `2` # (New in EIP-7732) | |
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.
You don't have to make this change here, but I want to remove these comments. We don't do this elsewhere.
| `MAX_PAYLOAD_ATTESTATIONS` | `2` # (New in EIP-7732) | | |
| `MAX_PAYLOAD_ATTESTATIONS` | `2` | |
##### New `should_process_builder_withdrawal` | ||
|
||
```python | ||
def should_process_builder_withdrawal( |
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.
Not a fan of "should" here. I think I would prefer is_builder_payment_withdrawable
.
assert data.target.epoch == compute_epoch_at_slot(data.slot) | ||
assert data.slot + MIN_ATTESTATION_INCLUSION_DELAY <= state.slot | ||
|
||
# [Modified in Electra:EIP7549] |
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.
There are three comments like this (search Electra:
) which should be removed.
# Verify the withdrawals root | ||
assert hash_tree_root(payload.withdrawals) == state.latest_withdrawals_root | ||
|
||
# Verify the gas_limit | ||
assert committed_header.gas_limit == payload.gas_limit | ||
|
||
assert committed_header.block_hash == payload.block_hash | ||
# Verify consistency of the parent hash with respect to the previous execution payload |
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.
Nit, can we delete the blank lines & add a comment above line 1142?
def for_ops(operations: Sequence[Any], fn: Callable[[BeaconState, Any], None]) -> None: | ||
for operation in operations: | ||
fn(state, operation) | ||
# Process Electra operations |
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.
Let's remove this comment. I don't want to mention other forks.
the bid is accepted. The builder **MUST** have balance enough to fulfill | ||
this bid. |
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.
This is actually an old change, but it should be:
The builder **MUST** have enough excess balance to fulfill this bid and all pending payments.
10. Set `header.fee_recipient` to be an execution address to receive the | ||
payment. This address can be obtained from the proposer directly via a | ||
request or can be set from the withdrawal credentials of the proposer. |
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.
Maybe worth mentioning that the fallback is the zero-addr / burn-addr.
*withholding boost* to the builder, which would increase the forkchoice weight | ||
of the parent block, favoring it and preventing the builder from being charged | ||
for the bid by not revealing. | ||
should simply act as if no block was ever produced and simply not broadcast the |
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.
Can simplify a bit 😄
should simply act as if no block was ever produced and simply not broadcast the | |
should act as if no block was produced and not broadcast the |
- _[REJECT]_ `aggregate.data.index < 2` | ||
- _[REJECT]_ `aggregate.data.index == 0` if `block.slot == aggregate.data.slot` | ||
|
||
The following validations are removed: | ||
|
||
- _[REJECT]_ `aggregate.data.index == 0` |
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.
Nit, there should be punctuation here.
- _[REJECT]_ `aggregate.data.index < 2` | |
- _[REJECT]_ `aggregate.data.index == 0` if `block.slot == aggregate.data.slot` | |
The following validations are removed: | |
- _[REJECT]_ `aggregate.data.index == 0` | |
- _[REJECT]_ `aggregate.data.index < 2`. | |
- _[REJECT]_ `aggregate.data.index == 0` if `block.slot == aggregate.data.slot`. | |
The following validations are removed: | |
- _[REJECT]_ `aggregate.data.index == 0`. |
The following validations are added: | ||
|
||
- _[REJECT]_ `attestation.data.index < 2` | ||
- _[REJECT]_ `attestation.data.index == 0` if | ||
`block.slot == attestation.data.slot` | ||
|
||
The following validations are removed: | ||
|
||
- _[REJECT]_ `attestation.data.index == 0` |
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.
Punctuation for these items too.
for_ops(requests.withdrawals, process_withdrawal_request) | ||
for_ops(requests.consolidations, process_consolidation_request) | ||
|
||
# Queue the proposer payment |
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.
For consistency, we refer to these as "builder payments" right?
# Queue the proposer payment | |
# Queue the builder payment |
increase_balance(state, get_beacon_proposer_index(state), proposer_reward) | ||
# update builder payment weight | ||
|
||
if current_epoch_target: |
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.
increase_balance(state, get_beacon_proposer_index(state), proposer_reward) | |
# update builder payment weight | |
if current_epoch_target: | |
increase_balance(state, get_beacon_proposer_index(state), proposer_reward) | |
# Update builder payment weight | |
if current_epoch_target: |
|
||
proposer_reward_numerator = 0 | ||
for index in get_attesting_indices(state, attestation): | ||
for flag_index, weight in enumerate(PARTICIPATION_FLAG_WEIGHTS): | ||
if flag_index in participation_flag_indices and not has_flag( | ||
epoch_participation[index], flag_index | ||
): | ||
epoch_participation[index] = add_flag(epoch_participation[index], flag_index) | ||
proposer_reward_numerator += get_base_reward(state, index) * weight | ||
if flag_index == TIMELY_HEAD_FLAG_INDEX and is_attestation_same_slot(state, data): | ||
payment.weight += state.validators[index].effective_balance |
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.
There's new code in this section, but it's not clearly marked.
# Sweep for builder payments | ||
for withdrawal in state.builder_pending_withdrawals: | ||
if ( | ||
withdrawal.withdrawable_epoch > epoch | ||
or len(withdrawals) + 1 == MAX_WITHDRAWALS_PER_PAYLOAD | ||
): | ||
break | ||
if should_process_builder_withdrawal(state, withdrawal): | ||
total_withdrawn = sum( | ||
w.amount for w in withdrawals if w.validator_index == withdrawal.builder_index | ||
) | ||
balance = state.balances[withdrawal.builder_index] - total_withdrawn | ||
builder = state.validators[withdrawal.builder_index] | ||
if builder.slashed: | ||
withdrawable_balance = min(balance, withdrawal.amount) | ||
elif balance > MIN_ACTIVATION_BALANCE: | ||
withdrawable_balance = min(balance - MIN_ACTIVATION_BALANCE, withdrawal.amount) | ||
else: | ||
withdrawable_balance = 0 | ||
|
||
withdrawals.append( | ||
Withdrawal( | ||
index=withdrawal_index, | ||
validator_index=withdrawal.builder_index, | ||
address=withdrawal.fee_recipient, | ||
amount=withdrawable_balance, | ||
) | ||
) | ||
withdrawal_index += WithdrawalIndex(1) | ||
processed_builder_withdrawals_count += 1 |
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.
What happens if between the builder's bid being accepted & the builder payment, the builder misses an attestation and no longer has enough funds to fully cover the payment? Based on the code here, it appears that the proposer would get all of the builder's excess balance. The possible difference here would be practically zero, but still sort of annoying for the proposer to not get exactly what was promised.
I can think of two solutions to this:
- Add a small buffer. Like min activation balance + 1 ETH + the bid value.
- Pull from the builder's min activation balance. You could remove the three cases and simply use
withdrawable_balance = min(balance, withdrawal.amount)
. This would behave as if there were a validator with exactly 32 ETH that missed an attestation; left with a sub-32 ETH balance.
I would prefer option 2, as it's the simplest/cleanest.
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.
This was a design decision of erring in the side of chain security vs proposer payment safety, instead of an attestation the biggest risk is the builder slashing itself. Notice that even if you delay or look at the min activation balance etc. Even after processing the bid, only a withdrawal is churned, and during that time the builder can still lose balance enough to fall below the payment line.
This PR updates EIP-7732 in several places. It should be read commit by commit. The list of commits is as follows
0x03
which is a compounding validator, allow any validator to self-build for a zero value, but only 0x03 validators to build for others.This PR does not update the forkchoice logic, that will come on a separate PR perhaps from @fradamt