ethereum / consensus-specs

Ethereum Proof-of-Stake Consensus Specifications
Creative Commons Zero v1.0 Universal
3.55k stars 969 forks source link

Update of state missing in process_deposit? #1541

Open franck44 opened 4 years ago

franck44 commented 4 years ago

The process_deposit code below seems to work as follows.

If the deposit originates from a pubkey that is not in state.validators, a new entry is created and appended at the end of state.validators and initialised with the deposit. The corresponding state.balances entry is also updated. However, if the pubkey of the deposit is in state.validators, say at index i, only the state.balances[i] is updated, not the state.validators[i].effective_balance.

Is it desirable that the following invariant holds: for all (Beacon) state s, forall 0 <= i < s.validators.length, s.validators[i].effective_balance == s.balances[i]?

If yes it seems to be broken when a deposit is made by a validator that is already in state.validators. The last else statement updates the balances but not the corresponding BeaconState state.validators[i].effective_balance. increase_balance does not update the state. Is it a bug or a feature?

Note: even when a new validator is created the balances is updated with the amount but the state is updated with amount - amount % EFFECTIVE_BALANCE_INCREMENT, MAX_EFFECTIVE_BALANCE), which are two different values. Is this also a bug or a feature?

def process_deposit(state: BeaconState, deposit: Deposit) -> None:
    # Verify the Merkle branch
    assert is_valid_merkle_branch(
        leaf=hash_tree_root(deposit.data),
        branch=deposit.proof,
        depth=DEPOSIT_CONTRACT_TREE_DEPTH + 1,  # Add 1 for the `List` length mix-in
        index=state.eth1_deposit_index,
        root=state.eth1_data.deposit_root,
    )

    # Deposits must be processed in order
    state.eth1_deposit_index += 1

    pubkey = deposit.data.pubkey
    amount = deposit.data.amount
    validator_pubkeys = [v.pubkey for v in state.validators]
    if pubkey not in validator_pubkeys:
        # Verify the deposit signature (proof of possession) for new validators.
        # Note: The deposit contract does not check signatures.
        # Note: Deposits are valid across forks, thus the deposit domain is retrieved directly from `compute_domain`.
        domain = compute_domain(DOMAIN_DEPOSIT)
        deposit_message = DepositMessage(
            pubkey=deposit.data.pubkey,
            withdrawal_credentials=deposit.data.withdrawal_credentials,
            amount=deposit.data.amount)
        if not bls_verify(pubkey, hash_tree_root(deposit_message), deposit.data.signature, domain):
            return

        # Add validator and balance entries
        state.validators.append(Validator(
            pubkey=pubkey,
            withdrawal_credentials=deposit.data.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=min(amount - amount % EFFECTIVE_BALANCE_INCREMENT, MAX_EFFECTIVE_BALANCE),
        ))
        state.balances.append(amount)
    else:
        # Increase balance by deposit amount
        index = ValidatorIndex(validator_pubkeys.index(pubkey))
        increase_balance(state, index, amount)
protolambda commented 4 years ago

The effective balance is purposefully lagging, and will be updated by the subsequent epoch transition. This is for different reasons, but generally: at the end of the epoch the effective balance is used for different calculations, and we do not want to consider it as full-epoch stake when it was only deposited near the end of the epoch. Edit: and to clarify, initial deposits add effective balance to non-active validators, so it does not matter much there. (except that we need the effective balance to be there at the start of epoch transitioning, for quicker activation eligibility than if we only update it at the end of the epoch transition with the others)

franck44 commented 4 years ago

Thanks @protolambda. So if I understand well, the invariant should be that at the end of each epoch for a (Beacon) state s, forall 0 <= i < s.validators.length, s.validastors[i] is active implies (s.validators[i].effective_balance == s.balances[i])? Or is it that, for each epoch, effective_balance of an active validator is frozen, and updated only at the end when transitioning to a new epoch?

There was another part in my question (for non-active validators) related to the fact that one update uses amount and the other one min(amount - amount EFFECTIVE_BALANCE_INCREMENT, MAX_EFFECTIVE_BALANCE) )) Are these values adjusted to match at the end of the epoch or are they different on purpose?

protolambda commented 4 years ago

So if I understand well, the invariant should be that at the end of each epoch for a (Beacon) state s, forall 0 <= i < s.validators.length, s.validastors[i] is active implies (s.validators[i].effective_balance == s.balances[i])?

Almost, the effective balance changes stepwise, using hysteresis to avoid stake information from having to update too often, and to incentivize to keep it stable (it will take more efforty to recover a balance to its original after bad behavior). At the end of the epoch is the adjustment yes.

Or is it that, for each epoch, effective_balance of an active validator is frozen, and updated only at the end when transitioning to a new epoch?

Yes. We have validator.slashed to disable any bad validator during the epoch. It will still be considered as part of the committee, as we can't just reshuffle in the middle of work whenever someone gets slashed.

There was another part in my question (for non-active validators) related to the fact that one update uses amount and the other one min(amount - amount EFFECTIVE_BALANCE_INCREMENT, MAX_EFFECTIVE_BALANCE) )) Are these values adjusted to match at the end of the epoch or are they different on purpose?

The initial effective balance vs the updated one? Can you highlight the line? It should just be hysteresis everywhere. Maybe it's already a maxed out effective balance, hence no adjustment necessary?

protolambda commented 4 years ago

Is this question still open? Or can I close this? I am trying to clean up some stale issues, let me know if I or others can help.

franck44 commented 4 years ago

We have not verified that invariant yet. So we may leave it one for now?