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

Fix INACTIVITY_PENALTY_QUOTIENT description #1693

Closed
wants to merge 1 commit into from

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Mar 29, 2020

Issue

It's a very old leftover from #949 (comment): when we didn't update the constant number description when we changed INACTIVITY_PENALTY_QUOTIENT from 2**24 to 2**25.

How to fix it

This PR updates corresponding INVERSE_SQRT_E_DROP_TIME to the right number. Alternatively, we can consider removing this paragraph as @JustinDrake suggested.

/cc @benjaminion for the annotated spec project.

@hwwhww hwwhww added the general:presentation Presentation (as opposed to content) label Mar 29, 2020
@djrtwo
Copy link
Contributor

djrtwo commented Apr 6, 2020

Why was it changed in #949? It was listed as "cosmetic" in the PR -- CC @JustinDrake

I've been operating under the assumption of ~2.5 weeks. Not ~3.5 weeks

@JustinDrake
Copy link
Collaborator

Why was it changed in #949? It was listed as "cosmetic" in the PR -- CC @JustinDrake

The previous formula had a // 2 (see INACTIVITY_PENALTY_QUOTIENT // 2 here) which I removed and counter-balanced by changing the constant. The intent was to preserve functionality, i.e. make it a cosmetic change. It is entirely possible I messed up and made a substantive change 😂

@hwwhww
Copy link
Contributor Author

hwwhww commented Apr 7, 2020

@JustinDrake sorry, I didn't notice you removed // 2 in #949 when I read it for this PR!
So if the previous numbers were correct, this section should be:

The INACTIVITY_PENALTY_QUOTIENT // 2 equals INVERSE_SQRT_E_DROP_TIME**2 where INVERSE_SQRT_E_DROP_TIME := 2**12 epochs (about 18 days) is the time it takes the inactivity penalty to reduce the balance of non-participating validators to about 1/sqrt(e) ~= 60.6%. Indeed, the balance retained by offline validators 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).

However, I did some git log trace 🔎

  1. The mysterious // 2 was added in 53dd491 (Changes to inactivity leak #347)
    • Adds a // 2 to the inactivity leak to compensate for it being applied twice
  2. But, in d1d1b73 (Simplify justification and finalization accounting logic #826), changes, the twice penalties became just one.

    Inactivity leak penalty specifically on missing the target, not both the target and the source

Conclusion

Am I correct that we don't even need // 2 after #826, so the true fix should be simply changing INACTIVITY_PENALTY_QUOTIENT from 2**25 to 2**24 if we want INVERSE_SQRT_E_DROP_TIME to be 18 days?

/cc @djrtwo @vbuterin for more 👀 and better memory.

@vbuterin
Copy link
Contributor

vbuterin commented Apr 7, 2020

It does indeed seem like INACTIVITY_PENALTY_QUOTIENT should be changed to 2**24. The description that after n epochs the log-balance decreases by (n**2 / 2) / INACTIVITY_PENALTY_QUOTIENT, and so when n = INVERSE_SQRT_E_DROP_TIME = 2**12 the log-balance drops by 1/2, ie. a sqrt-e factor drop, is correct.

Though it doesn't break security at all if it's kept at 2**25; it would just mean the leak takes 41% longer, so if this ends up being the only change we need to do then not bothering with it is fine too.

@hwwhww
Copy link
Contributor Author

hwwhww commented Apr 17, 2020

closing by #1712

@hwwhww hwwhww closed this Apr 17, 2020
@hwwhww hwwhww deleted the hwwhww/inactivity_description branch April 28, 2020 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:presentation Presentation (as opposed to content)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants