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

Revamp balances and incentivisation #949

Merged
merged 31 commits into from
Apr 24, 2019
Merged

Revamp balances and incentivisation #949

merged 31 commits into from
Apr 24, 2019

Conversation

JustinDrake
Copy link
Collaborator

@JustinDrake JustinDrake commented Apr 17, 2019

Substantive changes:

  • Use capped (at 32 ETH) and rounded balance (with hysteresis) as the "effective balance" (stored in validator.effective_balance) throughout beacon chain consensus
    • Consistency with fork choice rule and light clients 🎉
  • Only update validator.effective_balance once per epoch
    • Ability to compute the proposer for the current epoch 🎉
    • Cleaner modularity and parallel processing of rewards
    • Restore per-epoch (as opposed to per-slot) proposer reward processing
  • Introduce PendingAttestation.proposer_index for attestation proposer rewards

Cosmetic changes (mostly related to get_attestation_deltas):

  • Significantly improve readability and remove 82 lines 🎉
  • Remove and merge helper functions where appropriate.
  • Improve naming of helper functions
  • Abstract away common logic and rework for readability
  • Add MIN_EPOCHS_TO_INACTIVITY_PENALTY and BASE_REWARDS_PER_EPOCH constants
  • Rescale INACTIVITY_PENALTY_QUOTIENT

Cosmetic changes related to `get_justification_and_finalization_deltas`:

* Review naming of misc helper functions and variables
* Abstract away common logic and rework for readability
* Add `MAX_FINALITY_LOOKBACK` and `BASE_REWARDS_PER_EPOCH` constants
* Rescale `INACTIVITY_PENALTY_QUOTIENT`

Substantive changes:

* Make logic relative to `previous_epoch` throughout (as opposed to mixing `current_epoch` and `previous_epoch`)
* Replace inclusion delay bonus by an inclusion delay penalty
specs/core/0_beacon-chain.md Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
@JustinDrake JustinDrake changed the title Simplify get_justification_and_finalization_deltas Revamp balances and incentivisation Apr 20, 2019
@JustinDrake JustinDrake added the general:enhancement New feature or request label Apr 20, 2019
@dankrad
Copy link
Contributor

dankrad commented Apr 21, 2019

@JustinDrake could you give a summary of how exactly the rewards/penalty change through this? I would like to read it with this info side by side :)

@JustinDrake
Copy link
Collaborator Author

See the "substantive changes" section (which I fleshed out a bit) in the PR description.

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.

Did a pass.

The main concern is the use of get_beacon_proposer_index outside of the current epoch during get_attestation_deltas. get_beacon_proposer_index is not guaranteed to return the correct value outside of the current epoch.

specs/core/0_beacon-chain.md Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Show resolved Hide resolved
specs/core/0_beacon-chain.md Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
test_libs/pyspec/tests/test_sanity.py Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Show resolved Hide resolved
test_libs/pyspec/tests/test_sanity.py Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
| `INACTIVITY_PENALTY_QUOTIENT` | `2**24` (= 16,777,216) |
| `MIN_PENALTY_QUOTIENT` | `2**5` (= 32) |
| `INACTIVITY_PENALTY_QUOTIENT` | `2**25` (= 33,554,432) |
| `MIN_SLASHING_PENALTY_QUOTIENT` | `2**5` (= 32) |

* **The `BASE_REWARD_QUOTIENT` is NOT final. Once all other protocol details are finalized it will be adjusted, to target a theoretical maximum total issuance of `2**21` ETH per year if `2**27` ETH is validating (and therefore `2**20` per year if `2**25` ETH is validating, etc etc)**
* The `INACTIVITY_PENALTY_QUOTIENT` equals `INVERSE_SQRT_E_DROP_TIME**2` where `INVERSE_SQRT_E_DROP_TIME := 2**12 epochs` (~18 days) is the time it takes the inactivity penalty to reduce the balance of non-participating [validators](#dfn-validator) to about `1/sqrt(e) ~= 60.6%`. Indeed, the balance retained by offline [validators](#dfn-validator) after `n` epochs is about `(1 - 1/INACTIVITY_PENALTY_QUOTIENT)**(n**2/2)` so after `INVERSE_SQRT_E_DROP_TIME` epochs it is roughly `(1 - 1/INACTIVITY_PENALTY_QUOTIENT)**(INACTIVITY_PENALTY_QUOTIENT/2) ~= 1/sqrt(e)`.
Copy link
Contributor

Choose a reason for hiding this comment

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

If INACTIVITY_PENALTY_QUOTIENT changes from 2**25 to 2**24, then INACTIVITY_PENALTY_QUOTIENT != INVERSE_SQRT_E_DROP_TIME**2 now. This sentence needs to be updated or we change INACTIVITY_PENALTY_QUOTIENT back to 2**24 with INACTIVITY_PENALTY_QUOTIENT // 2 in formula.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this sentence should go away. We should have a separate document explaining design decisions/rationales. (This will be included in the Transparent Paper, whenever I get round to writing it.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's also mentioned in @vbuterin's https://notes.ethereum.org/s/rkhCgQteN#Slashing-and-anti-correlation-penalties. We could (i) update rationales doc (ii) remove this sentence from the spec and link to rationale doc for now.

@djrtwo djrtwo merged commit ade74f0 into dev Apr 24, 2019
@djrtwo djrtwo deleted the JustinDrake-patch-14 branch April 24, 2019 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants