-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
…euder/consensus-specs into maxeb-pyspec-min-viable
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.
first pass (super minor comments)
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.
minor comments round 2 :)
…ion-penalty Revert "Constant penalty for proposer equivocations"
Co-authored-by: fradamt <104826920+fradamt@users.noreply.github.com>
Co-authored-by: fradamt <104826920+fradamt@users.noreply.github.com>
consolidation by moving balance upon exit
`index` was only set in the case of a top-up and not of a deposit
off by one index and need keyword arguments
Make initial slashing penalty negligible
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 |
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 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.
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 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)
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 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 :)
consolidation = signed_consolidation.message | ||
target_validator = state.validators[consolidation.target_index] | ||
source_validator = state.validators[consolidation.source_index] |
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 guess it's worth making sure that these are two different validators
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] |
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.
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 |
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.
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
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.
@avsetsin Please can you port your review comments to ethereum#3618 ?
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: