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

[DRAFT] Increase MAX_EFFECTIVE_BALANCE minimal spec change #3

Closed
wants to merge 100 commits into from

Conversation

michaelneuder
Copy link
Owner

@michaelneuder michaelneuder commented May 22, 2023

Spec changes associated with https://notes.ethereum.org/@mikeneuder/increase-maxeb

For analysis on the changes see https://notes.ethereum.org/@fradamt/meb-increase-security

Changes:

  1. exit queue rate limiting by stake weight
  2. activation queue rate limiting by stake weight
  3. is aggregator changes
  4. limit top ups to 32 eth to avoid by passing activation queue
  5. partial withdrawals depending on 0x02 credential

Copy link
Collaborator

@JustinDrake JustinDrake left a comment

Choose a reason for hiding this comment

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

first pass (super minor comments)

specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@JustinDrake JustinDrake left a comment

Choose a reason for hiding this comment

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

minor comments round 2 :)

specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
specs/_features/maxeb_increase/capella.py Outdated Show resolved Hide resolved
mkalinin and others added 2 commits January 29, 2024 14:27
Co-authored-by: fradamt <104826920+fradamt@users.noreply.github.com>
min_balance_increments = validator.effective_balance // MIN_ACTIVATION_BALANCE
committee_balance_increments = get_total_balance(state, set(committee)) // MIN_ACTIVATION_BALANCE
denominator = committee_balance_increments ** min_balance_increments
numerator = denominator - (committee_balance_increments - TARGET_AGGREGATORS_PER_COMMITTEE) ** min_balance_increments
Copy link
Collaborator

@dapplion dapplion Feb 18, 2024

Choose a reason for hiding this comment

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

This intermediary product can overflow u64: a committee with 1M active indexes is 488 in size. If all members are consolidated committee_balance_increments = 31232, and min_balance_increments max value is 64. So the intermediate exponentiation is (488 * 64) ** 64 = 4e+287. Even if a single committee member is consolidated, (488 * 1) ** 64 = 1e172

EDIT: 2nd issue: If validator. effective_balance < 32 this code attempt to divide by zero.

Copy link
Collaborator

@dapplion dapplion Feb 18, 2024

Choose a reason for hiding this comment

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

I think (have not double checked) that this code achieves the same probabilities, without dividing by zero or having large intermediary values

factor = total_committee_balance // validator_balance
modulo = max(1, factor // TARGET_AGGREGATORS_PER_COMMITTEE)

It's conceptually equivalent to the code before where:

validator_indexes = 1
total_committee_indexes = len(get_beacon_committee(state, slot, index))
factor = total_committee_indexes / validator_indexes
modulo = max(1, factor // TARGET_AGGREGATORS_PER_COMMITTEE)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I had been thinking to simplify it to this, just explicitly looping through each "virtual validator" (corresponding to 32 eth) and checking if it is an aggregator roughly the same way we do today:

def is_aggregator(state: BeaconState, slot: Slot, index: CommitteeIndex, validator_index: ValidatorIndex, slot_signature: BLSSignature) -> bool:
    validator = state.validators[validator_index]
    committee = get_beacon_committee(state, slot, index)
    virtual_validators = max(1, state.balances[validator] // MIN_ACTIVATION_BALANCE)
    modulo = max(1, len(committee) // TARGET_AGGREGATORS_PER_COMMITTEE)
return any(is_aggregator_virtual_validator(slot_signature, i, modulo) for i in range(virtual_validators))

def is_aggregator_virtual_validator(slot_signature: BLSSignature, virtual_validator_index: ValidatorIndex, modulo: uint64) -> bool:
    vrf_bytes = hash(uint_to_bytes(bytes_to_uint64(slot_signature) + virtual_validator_index))
    return bytes_to_uint64(vrf_bytes[0:8]) % modulo == 0

The difference between this approach (which is equivalent to the weird one that's currently in this spec) and what you're proposing is that your approach still targets 16 validators as aggregators, whereas this approach targets 16 "virtual validators" as aggregators, which on average would correspond to less than 16 aggregators if there is consolidation, because multiple virtual validators might map to the same validator. Originally I had chosen this approach with virtual validators because it seems to map exactly to what we have today, but I don't really see a reason why your approach wouldn't be fine as well. At the end of the day what's important is that:

  • there's not too many aggregators
  • there's high probability of at least an honest aggregator

Will give it more thought, but I think I would go with your approach :)

Comment on lines +2017 to +2019
consolidation = signed_consolidation.message
target_validator = state.validators[consolidation.target_index]
source_validator = state.validators[consolidation.source_index]

Choose a reason for hiding this comment

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

I guess it's worth making sure that these are two different validators

Suggested change
consolidation = signed_consolidation.message
target_validator = state.validators[consolidation.target_index]
source_validator = state.validators[consolidation.source_index]
consolidation = signed_consolidation.message
assert(consolidation.target_index != consolidation.source_index)
target_validator = state.validators[consolidation.target_index]
source_validator = state.validators[consolidation.source_index]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Checked in the canonical PR ethereum#3618


def is_partially_withdrawable_validator(validator: Validator, balance: Gwei) -> bool:
"""
Check if ``validator`` is partially withdrawable.
"""
has_max_effective_balance = validator.effective_balance == MAX_EFFECTIVE_BALANCE
Copy link

@avsetsin avsetsin Mar 25, 2024

Choose a reason for hiding this comment

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

It looks like the check has_max_effective_balance was removed by mistake.

There may be a situation where a validator has lost 0.25 eth below MAX_EFFECTIVE_BALANCE resulting in its effective balance decreasing by 1 eth. The validator could then bring the balance up to MAX_EFFECTIVE_BALANCE or higher, but less than MAX_EFFECTIVE_BALANCE + 0.25 eth (the value needed to return the effective balance to MAX_EFFECTIVE_BALANCE).

The current code allows partial withdrawals of the validator balance when effective balance < MAX_EFFECTIVE_BALANCE, because of which it is very likely that the validator balance will not reach the necessary MAX_EFFECTIVE_BALANCE + 0.25 eth to return its effective balance to equal MAX_EFFECTIVE_BALANCE.

The same is true for 0x01 type and MIN_ACTIVATION_BALANCE

def is_partially_withdrawable_validator(validator: Validator, balance: Gwei) -> bool:
    """
    Check if ``validator`` is partially withdrawable.
    """
    if has_eth1_withdrawal_credential(validator) and validator.effective_balance == MIN_ACTIVATION_BALANCE:
        return get_validator_excess_balance(validator, balance) > 0
    if has_compounding_withdrawal_credential(validator) and validator.effective_balance == MAX_EFFECTIVE_BALANCE:
        return get_validator_excess_balance(validator, balance) > 0
    return False

Copy link
Collaborator

Choose a reason for hiding this comment

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

@avsetsin Please can you port your review comments to ethereum#3618 ?

@dapplion
Copy link
Collaborator

@dapplion dapplion closed this Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants