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

Avoid finalization when zero total stake #1107

Closed
wants to merge 2 commits into from

Conversation

Nashatyrev
Copy link
Member

Problem

After applying some number of 'empty slot transitions' to any state all validators are finally exited due to inactivity penalties and the state has zero active validators. From this point the state becomes finalized due to the >= condition (see proposed change).
Of course this is not the case for the actual network but may cause client software issues. E.g. when the client is not synced yet but is trying calculate the state of the current slot (based on system clocks) by applying 'empty slot transitions'. When encountering a finalized epoch it may stick to this zero-balance fork and then just fail to sync correctly

Solution

Change >= to > condition to avoid justifying epoch when the total stake balance is zero

@JustinDrake
Copy link
Collaborator

Side note: That the tests are still passing suggests we're missing an edge-case test for those thresholds.

@djrtwo
Copy link
Contributor

djrtwo commented May 21, 2019

I'm a bit conflicted on this one. All validators exiting the chain is a failure scenario beyond the balance being equal to zero. The chain cannot even be built at this point.

Mathematically, the threshold for finality should be >= so I'm not certain I'm willing compromise that to handle the case where a chain that can no longer be built will prevent finalizing itself.

@JustinDrake The reason this scenario is not tested is that there is no longer a chain to test here.

@JustinDrake
Copy link
Collaborator

JustinDrake commented May 21, 2019

All validators exiting the chain is a failure scenario beyond the balance being equal to zero.

I don't think that's the necessarily the case. The effective balances can be zero without all the validators having exited.

handle the case where a chain that can no longer be built

I think the point is that there are scenarios where the effective balances are all zero and the chain can still be built. (Remember that effective balance is rounded down to the closest EFFECTIVE_BALANCE_INCREMENT, and that there is an exit queue.)

@djrtwo
Copy link
Contributor

djrtwo commented May 21, 2019

Still thinking through such a scenario. In such a case, the remaining zero-balance validators are on a death march unless new capital is deposited into the system, right?

EDIT: -- actually won't they be ejected far before their effective gets to zero in the inactivity leak scenario?

If we need to handle this case, I would prefer the following blocker statement rather than compromise the mathematical integrity finality conditional statements.

if get_total_active_balance(state) == 0:
    return

@Nashatyrev
Copy link
Member Author

My two cents: from my perspective >= and > are almost equivalent for non-zero balance especially taking into account that 2/3 is kind of conceptual ratio.
Adding another couple of ifs looks less intrusive but makes the spec a bit more heavy
Either way I will be pretty fine with any solution

@djrtwo
Copy link
Contributor

djrtwo commented May 24, 2019

I'm pretty sure I'm going to go with the extra if. Taking care of it now with a test on the case.

@JustinDrake
Copy link
Collaborator

I'm pretty sure I'm going to go with the extra if.

It turns out there are other problems with rounding down the balance, namely divisions by zero in a couple places. I would want to avoid putting if statements everywhere. Maybe we can round the balance up instead?

JustinDrake added a commit that referenced this pull request May 26, 2019
Possible fix to avoid four cases of divisions by zero:

* `return state.validator_registry[index].effective_balance // adjusted_quotient // BASE_REWARDS_PER_EPOCH`
* `rewards[index] += get_base_reward(state, index) * attesting_balance // total_balance`
* `validator.effective_balance * min(total_penalties * 3, total_balance) // total_balance`
* `rewards[index] += base_reward * attesting_balance // committee_balance`

See also #1107.
@djrtwo
Copy link
Contributor

djrtwo commented May 28, 2019

Closing in favor of #1123

@djrtwo djrtwo closed this May 28, 2019
djrtwo pushed a commit that referenced this pull request May 30, 2019
* Simplify deposits

* Avoid divisions by zero

Possible fix to avoid four cases of divisions by zero:

* `return state.validator_registry[index].effective_balance // adjusted_quotient // BASE_REWARDS_PER_EPOCH`
* `rewards[index] += get_base_reward(state, index) * attesting_balance // total_balance`
* `validator.effective_balance * min(total_penalties * 3, total_balance) // total_balance`
* `rewards[index] += base_reward * attesting_balance // committee_balance`

See also #1107.

* fix deposit test for new index handling

* tests: deposit with inconsistent withdrawal credentials

* Update README.md

* Update 0_beacon-chain.md

* Fix linter errors

* Update test_process_deposit.py

* fix deposit test

* fix lint
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.

3 participants