[DRAFT] Increase MAX_EFFECTIVE_BALANCE minimal spec change#3
[DRAFT] Increase MAX_EFFECTIVE_BALANCE minimal spec change#3michaelneuder wants to merge 100 commits into
MAX_EFFECTIVE_BALANCE minimal spec change#3Conversation
…euder/consensus-specs into maxeb-pyspec-min-viable
JustinDrake
left a comment
There was a problem hiding this comment.
first pass (super minor comments)
JustinDrake
left a comment
There was a problem hiding this comment.
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.
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.
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.
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 == 0The 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.
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] |
| """ | ||
| Check if ``validator`` is partially withdrawable. | ||
| """ | ||
| has_max_effective_balance = validator.effective_balance == MAX_EFFECTIVE_BALANCE |
There was a problem hiding this comment.
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 FalseThere was a problem hiding this comment.
@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: