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.

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

EIP6914 - Reuse indexes with full sweep #3307

Merged
merged 10 commits into from
Apr 19, 2023
Merged

Conversation

dapplion
Copy link
Collaborator

@dapplion dapplion commented Mar 28, 2023

Re-use beacon chain indexes

Assign new deposits to existing validator records that have withdrawn long ago

Motivation

Withdrawn indexes are dead weight that adds cost to the protocol:

  • Beacon state size is roughly proportional to validator count
  • Some epoch transition steps require a full sweep over all validators
  • Higher tree-hashing cost / complexity

Safe to reuse epochs constant

Name Value Unit Duration
SAFE_EPOCHS_TO_REUSE_INDEX uint64(2**16) (= 65,536) epochs ~0.8 year

From @djrtwo : Tentatively ~3x WS period, so 1 year

Refer to ethereum/EIPs#6914 for security considerations and attacks.

Strategy A: Full-sweep

def assign_index_to_deposit(state: BeaconState) -> uint64:
    for index, validator in enumerate(state.validators):
        if is_reusable_validator(...):
            return index
    return len(state.validators)
  • Most efficient, always allocate re-usable indexes to validators
  • Requires a cache to be efficient, see full sweep cache

Strategy B: Bounded sweep

def compute_reuse_index(state: BeaconState) -> uint64:
    index = state.next_reuse_sweep_index
    bound = min(len(state.validators), MAX_VALIDATORS_PER_REUSE_SWEEP)
    for _ in range(bound):
        if is_reusable_validator(...):
            state.next_reuse_sweep_index = index + 1
            return index
        index = ValidatorIndex((index + 1) % len(state.validators))
    state.next_reuse_sweep_index = index
    return len(state.validators)
  • Similar to withdrawals sweep, boundness should prevent the need a cache.
  • Will not allocate all potentially re-usable indexes to validators, see bounded sweep reuse efficiency

Full sweep cache

Define reusable_indexes(state) as the set of all validator indexes in state that satisfy predicate is_reusable_validator.

Reusable indexes must satisfy conditions:

  1. validator.withdrawable_epoch < epoch - REUSE_VALIDATOR_INDEX_DELAY: epoch is constant through an epoch (duh) and validator.withdrawable_epoch can only change if this validator is reused. Re-using is an irreversible action during an epoch. So the set reusable_indexes(state_0) includes all possible sets reusable_indexes(state_child) where state_child is a descendant of state_0 in the same epoch.
  2. balance == 0. Balance may not be zero if there's remaining balance from active duties (in extreme configurations). Balance can only be increased with a deposit topup. Balance can only be decreased to zero with a full withdrawal (if EPOCHS_PER_SLASHINGS_VECTOR < REUSE_VALIDATOR_INDEX_DELAY)

Cache strategy

  • Once per dependent root after applying an epoch transition, compute a list potential_reusable_indexes of all validator indexes with validator.withdrawable_epoch < epoch. That set includes all possible reusable indexes on that epoch. Store that list as immutable referenced by all child states in epoch. Instantiate an empty set reused_indexes
  • On each fork share the immutable reference to potential_reusable_indexes and clone reused_indexes
  • On apply_deposit, iterate sequentially over potential_reusable_indexes and check index not in reused_indexes and balance == 0. If so, add the index to reused_indexes
  • (extra) If last finalized < MIN_VALIDATOR_WITHDRAWABILITY_DELAY the cached list is guaranteed to be the same for all states at epoch N.
def on_epoch_process(state: BeaconState, cache):
    # Immutable shared cache for all child states in same epoch
    cache.potential_reusable_indexes = []
    # Per state set of indexes reused
    cache.reused_indexes = set()
    for index, validator in enumerate(state.validators):
        if validator.withdrawable_epoch < get_current_epoch(state) - REUSE_VALIDATOR_INDEX_DELAY:
            cache.potential_reusable_indexes.push(index)

def compute_reuse_index(state: BeaconState, cache) -> int:
    for index in cache.potential_reusable_indexes:
        if state.balances[index] == 0:
            cache.reused_indexes.add(index)
            return index
    return len(state.validators)

Bounded sweep reuse efficiency

Define efficiency as the ratio of re-used indexes with partial sweep by re-used indexes with full sweep (theoretical maximum). We want to maximize efficiency.

We also want to bound computational cost on block processing, such that avg deposits per block * MAX_VALIDATORS_PER_REUSE_SWEEP << len(state.validators). Note that after EIP6110, deposits per block is bounded only by gas, and could be much higher than current MAX_DEPOSITS = 16.

Note that this two goals oppose each other. Also note that missing the opportunity to re-use an index results on a significant long term cost (until new index withdraws) of an extra validator record.

Toy examples

Consider the following patterns of validator records:

  • Pattern ------YY means 6 non-reusable indexes, 2 reusable indexes
  • Assume MAX_VALIDATORS_PER_REUSE_SWEEP = 2
Reuse eff. Computational eff.
Concentrated ------YY Very low some wasted sweeps
Sparse -Y-Y-Y-Y ok ok
None -------- ok all wasted sweeps
All YYYYYYYY ok ok

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

nice! this is generally what I've had in mind. Nice to see it come out so simple (assuming we don't bound the sweep, but even then, won't be too bad)

specs/_features/reuse_indexes/beacon-chain.md Outdated Show resolved Hide resolved
specs/altair/beacon-chain.md Outdated Show resolved Hide resolved
specs/altair/beacon-chain.md Outdated Show resolved Hide resolved
@djrtwo djrtwo added the general:enhancement New feature or request label Mar 28, 2023
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

great work! 🚀

Just some minor suggestions here. I think this approach works.

specs/_features/reuse_indexes/beacon-chain.md Outdated Show resolved Hide resolved
specs/altair/beacon-chain.md Outdated Show resolved Hide resolved
specs/_features/reuse_indexes/beacon-chain.md Outdated Show resolved Hide resolved
specs/altair/beacon-chain.md Outdated Show resolved Hide resolved
specs/_features/reuse_indexes/beacon-chain.md Outdated Show resolved Hide resolved
@@ -511,16 +511,29 @@ def apply_deposit(state: BeaconState,
signing_root = compute_signing_root(deposit_message, domain)
# Initialize validator if the deposit signature is valid
if bls.Verify(pubkey, signing_root, signature):
state.validators.append(get_validator_from_deposit(pubkey, withdrawal_credentials, amount))
state.balances.append(amount)
index = get_index_for_new_validator(state)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggesting to define add_validator_to_registry (there could be a better name) and doing the following refactor:

Phase0

def add_validator_to_registry(state: BeaconState, validator: Validator) -> None:
    # Add validator and balance entries
    state.validators.append(get_validator_from_deposit(pubkey, withdrawal_credentials, amount))
    state.balances.append(amount)

Replace the two lines with a call to this method

Altair

Modify add_validator_to_registry to be aligned with Altair updates. Which also makes Altair spec leaner as a side effect (no need to redefine entire apply_deposit, just add_validator_to_registry).

Reuse indexes

Modify add_validator_to_registry respectively, and encapsulate get_index_for_new_validator and update or append logic in this feature only.

Copy link
Collaborator Author

@dapplion dapplion Mar 30, 2023

Choose a reason for hiding this comment

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

The intent of update_or_append_to_list was to prevent having to duplicate initialization statements, which will keep growing with every fork. And to keep this doc agnostic of altair initialization itself. No strong opinion on either way. I'll PR the add_validator_to_registry separately since it's mostly orthogonal to the change

state.balances.append(amount)
index = get_index_for_new_validator(state)
validator = get_validator_from_deposit(pubkey, withdrawal_credentials, amount)
update_or_append_to_list(state.validators, index, validator)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Slight preference define the update or append logic explicitly i.e:

if index < len(state.validators):
    # Update
else:
    # Append

@dapplion
Copy link
Collaborator Author

dapplion commented Mar 30, 2023

There's another instance of dead validator records not covered by this PR: incomplete deposits. If there's a valid deposit for a validator for 1ETH, that index will stay taken forever with current design. Should we tackle that case too? This feels more like an attack to bloat space than a by-product of honest usage, so not sure if it's relevant given how un-economic it is.

@ajsutton
Copy link
Contributor

There's another instance of dead validator records not covered by this PR: incomplete deposits. If there's a valid deposit for a validator for 1ETH, that index will stay taken forever with current design. Should we tackle that case too? This feels more like an attack to bloat space than a by-product of honest usage, so not sure if it's relevant given how un-economic it is.

I'd say that's very definitely a different discussion to have. You'd have to burn the deposited funds to be able to reuse those indices and that seems unreasonable (at minimum it will be controversial).

@potuz
Copy link
Contributor

potuz commented Mar 30, 2023

I'd say that's very definitely a different discussion to have. You'd have to burn the deposited funds to be able to reuse those indices and that seems unreasonable (at minimum it will be controversial).

One way of not punching holes in the validator registry (but still keeping the same storage space used) is to not assign indices to validators until they are on the activation queue. This is equivalent to having a cache. There's no real reason to have an index assigned until the validator is ready to be included

@mkalinin
Copy link
Collaborator

Another way of employing the same attack is to deposit 1 ETH to the balance of already withdrawn validator. This is a dead end for this ETH but even in this case overwriting this validator record would be controversial as it is still burning one's ETH.

@mcdee
Copy link
Contributor

mcdee commented Mar 30, 2023

Another way of employing the same attack is to deposit 1 ETH to the balance of already withdrawn validator. This is a dead end for this ETH but even in this case overwriting this validator record would be controversial as it is still burning one's ETH.

Wouldn't this result in a withdrawal, so the attack window would be from the time of the deposit until the withdrawals clock ticked around to reduce the validator's index back to 0?

@ppopth
Copy link
Member

ppopth commented Apr 3, 2023

Another way of employing the same attack is to deposit 1 ETH to the balance of already withdrawn validator. This is a dead end for this ETH but even in this case overwriting this validator record would be controversial as it is still burning one's ETH.

Wouldn't this result in a withdrawal, so the attack window would be from the time of the deposit until the withdrawals clock ticked around to reduce the validator's index back to 0?

I think this attack is impossible at all. The overwriting will happen only to the validators with no balance. So if there is 1 ETH in the balance of some already withdrawn validator, that withdrawn validator will never be overwritten.

So no attack window at all.

def is_reusable_validator(validator: Validator, balance: Gwei, epoch: Epoch) -> bool:
    """
    Check if ``validator`` index can be re-assigned to a new deposit.
    """
    return (
        epoch > validator.withdrawable_epoch + REUSE_VALIDATOR_INDEX_DELAY
        and balance == 0
    )

@dapplion dapplion changed the title Reuse indexes with full sweep EIP6914 - Reuse indexes with full sweep Apr 19, 2023
@djrtwo djrtwo mentioned this pull request Apr 19, 2023
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

nice!

@djrtwo djrtwo merged commit 5e11533 into ethereum:dev Apr 19, 2023
@djrtwo
Copy link
Contributor

djrtwo commented Apr 19, 2023

Merging the core here, please use #3335 for follow-up discussions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants