Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Finalize deposit before applying it to a state #3689

Closed
wants to merge 3 commits into from
Closed

Finalize deposit before applying it to a state #3689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9aed70b
Finalize deposit before applying it to a state
mkalinin Apr 17, 2024
d772386
Apply fixes by @fradamt
mkalinin Apr 18, 2024
3d4fe06
Get rid of is_eligible_for_activation_queue
mkalinin Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
138 changes: 61 additions & 77 deletions specs/electra/beacon-chain.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
- [Containers](#containers)
- [New containers](#new-containers)
- [`DepositReceipt`](#depositreceipt)
- [`PendingBalanceDeposit`](#pendingbalancedeposit)
- [`PendingDeposit`](#pendingdeposit)
- [`PendingPartialWithdrawal`](#pendingpartialwithdrawal)
- [`ExecutionLayerWithdrawalRequest`](#executionlayerwithdrawalrequest)
- [`Consolidation`](#consolidation)
Expand All @@ -41,7 +41,7 @@
- [`BeaconState`](#beaconstate)
- [Helper functions](#helper-functions)
- [Predicates](#predicates)
- [Updated `is_eligible_for_activation_queue`](#updated-is_eligible_for_activation_queue)
- [Updated `is_eligible_for_activation`](#updated-is_eligible_for_activation)
- [New `is_compounding_withdrawal_credential`](#new-is_compounding_withdrawal_credential)
- [New `has_compounding_withdrawal_credential`](#new-has_compounding_withdrawal_credential)
- [New `has_execution_withdrawal_credential`](#new-has_execution_withdrawal_credential)
Expand Down Expand Up @@ -69,7 +69,7 @@
- [Epoch processing](#epoch-processing)
- [Updated `process_epoch`](#updated-process_epoch)
- [Updated `process_registry_updates`](#updated--process_registry_updates)
- [New `process_pending_balance_deposits`](#new-process_pending_balance_deposits)
- [New `process_pending_deposits`](#new-process_pending_deposits)
- [New `process_pending_consolidations`](#new-process_pending_consolidations)
- [Updated `process_effective_balance_updates`](#updated-process_effective_balance_updates)
- [Block processing](#block-processing)
Expand All @@ -85,8 +85,6 @@
- [Deposits](#deposits)
- [Updated `apply_deposit`](#updated--apply_deposit)
- [New `is_valid_deposit_signature`](#new-is_valid_deposit_signature)
- [Modified `add_validator_to_registry`](#modified-add_validator_to_registry)
- [Updated `get_validator_from_deposit`](#updated-get_validator_from_deposit)
- [Voluntary exits](#voluntary-exits)
- [Updated `process_voluntary_exit`](#updated-process_voluntary_exit)
- [Execution layer withdrawal requests](#execution-layer-withdrawal-requests)
Expand Down Expand Up @@ -155,7 +153,7 @@ The following values are (non-configurable) constants used throughout the specif

| Name | Value | Unit |
| - | - | :-: |
| `PENDING_BALANCE_DEPOSITS_LIMIT` | `uint64(2**27)` (= 134,217,728) | pending balance deposits |
| `PENDING_DEPOSITS_LIMIT` | `uint64(2**27)` (= 134,217,728) | pending deposits |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2^32 would be the theoretical limit with infinite gas fee and a single block that fills the entire deposit tree

| `PENDING_PARTIAL_WITHDRAWALS_LIMIT` | `uint64(2**27)` (= 134,217,728) | pending partial withdrawals |
| `PENDING_CONSOLIDATIONS_LIMIT` | `uint64(2**18)` (= 262,144) | pending consolidations |

Expand Down Expand Up @@ -200,14 +198,17 @@ class DepositReceipt(Container):
index: uint64
```

#### `PendingBalanceDeposit`
#### `PendingDeposit`

*Note*: The container is new in EIP7251.

```python
class PendingBalanceDeposit(Container):
index: ValidatorIndex
amount: Gwei
class PendingDeposit(Container):
epoch: Epoch
pubkey: BLSPubkey
withdrawal_credentials: Bytes32
amount: uint64
signature: BLSSignature
```

#### `PendingPartialWithdrawal`
Expand Down Expand Up @@ -421,7 +422,7 @@ class BeaconState(Container):
earliest_exit_epoch: Epoch # [New in Electra:EIP7251]
consolidation_balance_to_consume: Gwei # [New in Electra:EIP7251]
earliest_consolidation_epoch: Epoch # [New in Electra:EIP7251]
pending_balance_deposits: List[PendingBalanceDeposit, PENDING_BALANCE_DEPOSITS_LIMIT] # [New in Electra:EIP7251]
pending_deposits: List[PendingDeposit, PENDING_DEPOSITS_LIMIT] # [New in Electra:EIP7251]
# [New in Electra:EIP7251]
pending_partial_withdrawals: List[PendingPartialWithdrawal, PENDING_PARTIAL_WITHDRAWALS_LIMIT]
pending_consolidations: List[PendingConsolidation, PENDING_CONSOLIDATIONS_LIMIT] # [New in Electra:EIP7251]
Expand All @@ -431,16 +432,18 @@ class BeaconState(Container):

### Predicates

#### Updated `is_eligible_for_activation_queue`
#### Updated `is_eligible_for_activation`

```python
def is_eligible_for_activation_queue(validator: Validator) -> bool:
def is_eligible_for_activation(state: BeaconState, validator: Validator) -> bool:
"""
Check if ``validator`` is eligible to be placed into the activation queue.
Check if ``validator`` is eligible for activation.
"""
return (
validator.activation_eligibility_epoch == FAR_FUTURE_EPOCH
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct to assume that validators can never have an activation_eligibility_epoch > ELECTRA_FORK_EPOCH? e.g., by depositing during the last slot before electra activates.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those validators are added to the pending deposits queue in upgrade_to_electra

and validator.effective_balance >= MIN_ACTIVATION_BALANCE # [Modified in Electra:EIP7251]
# Has sufficient effective balance
validator.effective_balance >= MIN_ACTIVATION_BALANCE # [Modified in Electra:EIP7251]
# Has not yet been activated
and validator.activation_epoch == FAR_FUTURE_EPOCH
)
```

Expand Down Expand Up @@ -631,12 +634,19 @@ def switch_to_compounding_validator(state: BeaconState, index: ValidatorIndex) -

```python
def queue_excess_active_balance(state: BeaconState, index: ValidatorIndex) -> None:
validator = state.validators[index]
balance = state.balances[index]
if balance > MIN_ACTIVATION_BALANCE:
excess_balance = balance - MIN_ACTIVATION_BALANCE
state.balances[index] = MIN_ACTIVATION_BALANCE
state.pending_balance_deposits.append(
PendingBalanceDeposit(index=index, amount=excess_balance)
state.pending_deposits.append(
PendingDeposit(
epoch=get_current_epoch(state),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an off-by-one here?

When epoch N finalizes, everything up through and including compute_start_slot_at_epoch(N) is finalized.

  1. If activations are processed for deposit.epoch <= finalized_checkpoint.epoch, then that would also activate deposits on the non-finalized slots within N (only the initial slot of N is finalized).
  2. If activations are processed for deposit.epoch < finalized_checkpoint.epoch, then deposits within the very first slot of N are not processed for another ~6 minutes despite already being finalized.

Currently, it seems we are following the second strategy (deposit.epoch >= state.finalized_checkpoint.epoch). Probably fine to have the extra ~6 minutes wait for deposits within the first slot, but could be avoided, so still keeping the comment for awareness.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally in favour of strategy (2) because of its simplicity

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this and I changed my mind. The issue is that clients may (and probably will) filter deposits by blocks or db invariants like "finalized", the presence of this off by one creates edge cases that need to be always checked and tested. While it's not likely to create a consensus bug I feel it will be a source of bugs nonetheless

Copy link
Collaborator Author

@mkalinin mkalinin Jun 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we set deposit.epoch in the queue to get_current_epoch(state) + 1, will that prevent issues that you have in mind? It can also be renamed to deposit.activation_epoch to better reflect the semantics

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess storing slot and processing deposits up through deposit_receipt.slot <= compute_start_slot_at_epoch(state.finalized_checkpoint.epoch) is the most straight forward.

pubkey=validator.pubkey,
withdrawal_credentials=validator.withdrawal_credentials,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are withdrawal_credentials necessary to be saved here?

  • If yes, there could be a race where a BLSToExecutionChange gets processed while the PendingDeposit is in the queue, changing the withdrawal credentials of the validator while the PendingDeposit retains the stale entry.
  • If no, we could just set this to a zero value (or to None with EIP-7688 optionals).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn’t necessary so I think that setting it to zero value should be fine

amount=balance,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excess_balance? Because MIN_ACTIVATION_BALANCE was already credited.

signature=BLSSignature(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For SyncAggregate, G2_POINT_AT_INFINITY is used instead of a zero value when noone signed.

Or, None with EIP-7688 optionals (but I guess zero value or infinity is also fine, as it is not checked anyway).

)
)
```

Expand All @@ -648,8 +658,14 @@ def queue_entire_balance_and_reset_validator(state: BeaconState, index: Validato
validator = state.validators[index]
validator.effective_balance = 0
validator.activation_eligibility_epoch = FAR_FUTURE_EPOCH
state.pending_balance_deposits.append(
PendingBalanceDeposit(index=index, amount=balance)
state.pending_deposits.append(
PendingDeposit(
epoch=get_current_epoch(state),
pubkey=validator.pubkey,
withdrawal_credentials=validator.withdrawal_credentials,
amount=balance,
signature=BLSSignature(),
)
)
```

Expand Down Expand Up @@ -744,7 +760,7 @@ def process_epoch(state: BeaconState) -> None:
process_registry_updates(state) # [Modified in Electra:EIP7251]
process_slashings(state)
process_eth1_data_reset(state)
process_pending_balance_deposits(state) # [New in Electra:EIP7251]
process_pending_deposits(state) # [New in Electra:EIP7251]
process_pending_consolidations(state) # [New in Electra:EIP7251]
process_effective_balance_updates(state) # [Modified in Electra:EIP7251]
process_slashings_reset(state)
Expand All @@ -763,9 +779,6 @@ and changes how the activation epochs are computed for eligible validators.
def process_registry_updates(state: BeaconState) -> None:
# Process activation eligibility and ejections
for index, validator in enumerate(state.validators):
if is_eligible_for_activation_queue(validator):
validator.activation_eligibility_epoch = get_current_epoch(state) + 1

if (
is_active_validator(validator, get_current_epoch(state))
and validator.effective_balance <= EJECTION_BALANCE
Expand All @@ -779,24 +792,32 @@ def process_registry_updates(state: BeaconState) -> None:
validator.activation_epoch = activation_epoch
```

#### New `process_pending_balance_deposits`
#### New `process_pending_deposits`

```python
def process_pending_balance_deposits(state: BeaconState) -> None:
def process_pending_deposits(state: BeaconState) -> None:
available_for_processing = state.deposit_balance_to_consume + get_activation_exit_churn_limit(state)
processed_amount = 0
next_deposit_index = 0

for deposit in state.pending_balance_deposits:
for deposit in state.pending_deposits:
if processed_amount + deposit.amount > available_for_processing:
break
increase_balance(state, deposit.index, deposit.amount)
if deposit.epoch >= state.finalized_checkpoint.epoch:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also check for the legacy style deposits import to have caught up with the queue.

Otherwise, deposits from the old style (via eth1_data) and from the new queue can get reordered, possibly messing with decentralized staking pools that rely on a linear deposit order.

break
apply_deposit(
state=state,
pubkey=deposit.pubkey,
withdrawal_credentials=deposit.withdrawal_credentials,
amount=deposit.amount,
signature=deposit.signature,
)
processed_amount += deposit.amount
next_deposit_index += 1

state.pending_balance_deposits = state.pending_balance_deposits[next_deposit_index:]
state.pending_deposits = state.pending_deposits[next_deposit_index:]

if len(state.pending_balance_deposits) == 0:
if len(state.pending_deposits) == 0:
state.deposit_balance_to_consume = Gwei(0)
else:
state.deposit_balance_to_consume = available_for_processing - processed_amount
Expand Down Expand Up @@ -1103,10 +1124,8 @@ def apply_deposit(state: BeaconState,
else:
# Increase balance by deposit amount
index = ValidatorIndex(validator_pubkeys.index(pubkey))
mkalinin marked this conversation as resolved.
Show resolved Hide resolved
state.pending_balance_deposits.append(
PendingBalanceDeposit(index=index, amount=amount)
) # [Modified in Electra:EIP-7251]
# Check if valid deposit switch to compounding credentials
increase_balance(state, index, amount)
# Check if valid deposit switch to compounding credentials [New in Electra:EIP-7251]
if (
is_compounding_withdrawal_credential(withdrawal_credentials)
and has_eth1_withdrawal_credential(state.validators[index])
Expand All @@ -1133,38 +1152,6 @@ def is_valid_deposit_signature(pubkey: BLSPubkey,
return bls.Verify(pubkey, signing_root, signature)
```

###### Modified `add_validator_to_registry`

```python
def add_validator_to_registry(state: BeaconState,
pubkey: BLSPubkey,
withdrawal_credentials: Bytes32,
amount: uint64) -> None:
index = get_index_for_new_validator(state)
validator = get_validator_from_deposit(pubkey, withdrawal_credentials)
set_or_append_list(state.validators, index, validator)
set_or_append_list(state.balances, index, 0) # [Modified in Electra:EIP7251]
set_or_append_list(state.previous_epoch_participation, index, ParticipationFlags(0b0000_0000))
set_or_append_list(state.current_epoch_participation, index, ParticipationFlags(0b0000_0000))
set_or_append_list(state.inactivity_scores, index, uint64(0))
state.pending_balance_deposits.append(PendingBalanceDeposit(index=index, amount=amount)) # [New in Electra:EIP7251]
```

###### Updated `get_validator_from_deposit`

```python
def get_validator_from_deposit(pubkey: BLSPubkey, withdrawal_credentials: Bytes32) -> Validator:
return Validator(
pubkey=pubkey,
withdrawal_credentials=withdrawal_credentials,
activation_eligibility_epoch=FAR_FUTURE_EPOCH,
activation_epoch=FAR_FUTURE_EPOCH,
exit_epoch=FAR_FUTURE_EPOCH,
withdrawable_epoch=FAR_FUTURE_EPOCH,
effective_balance=0, # [Modified in Electra:EIP7251]
)
```

##### Voluntary exits
###### Updated `process_voluntary_exit`

Expand Down Expand Up @@ -1271,12 +1258,14 @@ def process_deposit_receipt(state: BeaconState, deposit_receipt: DepositReceipt)
if state.deposit_receipts_start_index == UNSET_DEPOSIT_RECEIPTS_START_INDEX:
state.deposit_receipts_start_index = deposit_receipt.index

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about checking is_valid_deposit_signature here? Unless we specifically want to avoid burning a top-up with incorrect signature

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two things why do signature check as is:

  1. Some infra might already have top-ups sent with zero signatures (worth checking tho)
  2. Rate limiting a number of deposit signature checks per epoch is really nice, it alleviates the risk of delaying block processing if someone manages to pack a thousand deposits into it

apply_deposit(
state=state,
pubkey=deposit_receipt.pubkey,
withdrawal_credentials=deposit_receipt.withdrawal_credentials,
amount=deposit_receipt.amount,
signature=deposit_receipt.signature,
state.pending_deposits.append(
PendingDeposit(
epoch=get_current_epoch(state),
pubkey=deposit_receipt.pubkey,
withdrawal_credentials=deposit_receipt.withdrawal_credentials,
amount=deposit_receipt.amount,
signature=deposit_receipt.signature,
)
)
```

Expand Down Expand Up @@ -1367,11 +1356,6 @@ def initialize_beacon_state_from_eth1(eth1_block_hash: Hash32,
state.eth1_data.deposit_root = hash_tree_root(deposit_data_list)
process_deposit(state, deposit)

# Process deposit balance updates
for deposit in state.pending_balance_deposits:
increase_balance(state, deposit.index, deposit.amount)
state.pending_balance_deposits = []

# Process activations
for index, validator in enumerate(state.validators):
balance = state.balances[index]
Expand Down
2 changes: 1 addition & 1 deletion specs/electra/fork.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ def upgrade_to_electra(pre: deneb.BeaconState) -> BeaconState:
earliest_exit_epoch=earliest_exit_epoch,
consolidation_balance_to_consume=get_consolidation_churn_limit(pre),
earliest_consolidation_epoch=compute_activation_exit_epoch(get_current_epoch(pre)),
pending_balance_deposits=[],
pending_deposits=[],
pending_partial_withdrawals=[],
pending_consolidations=[],
)
Expand Down
Loading