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

Track effectiveBalance as small number #2981

Closed
twoeths opened this issue Aug 20, 2021 · 3 comments · Fixed by #3044
Closed

Track effectiveBalance as small number #2981

twoeths opened this issue Aug 20, 2021 · 3 comments · Fixed by #3044
Assignees
Labels
scope-performance Performance issue and ideas to improve performance.

Comments

@twoeths
Copy link
Contributor

twoeths commented Aug 20, 2021

Is your feature request related to a problem? Please describe.

We should use number whenever possible as BigInt calculation is about x250 times slower as analysed by @dapplion

Describe the solution you'd like

  1. effectiveBalance is always a multiple of 1e9 and it's <= 32*1e9 so should be able to track 30, 31, 32 instead of BigInt(30 * 1e9), BigInt(31 * 1e9), BigInt(32 * 1e9). This would resolve issue Improve forkchoice updateHead #2747
  2. In epochProcess.ts, totalActiveStake, prevEpochUnslashedStake and currEpochUnslashedTargetStake could also be tracked as number since they are sum of effectiveBalances
    • May need to tweak a little bit for some functions like computeBaseRewardPerIncrement(), processEffectiveBalanceUpdates()
    • Need to do an inventory for the usage of all of these variables but as far as I see it's all multiply/divide by EFFECTIVE_BALANCE_INCREMENT and this constant (1e9) is not likely to be changed

This should speed up beforeProcessEpoch() and some other calculations a lot since we don't have to deal with BigInt() there

@twoeths twoeths added the scope-performance Performance issue and ideas to improve performance. label Aug 21, 2021
@dapplion
Copy link
Contributor

dapplion commented Aug 22, 2021

NOTE! ⚠️ We must add a check in the Lodestar constructor to check

if (EFFECTIVE_BALANCE_INCREMENT !== 1e9) {
  throw Error(`Lodestar assumes EFFECTIVE_BALANCE_INCREMENT = 1e9`)
}

Otherwise if it's not too complicated we could compute effectiveBalances as the multiple of EFFECTIVE_BALANCE_INCREMENT, regardless of what it is. Most of the logic in state transition function computes the ratio of effectiveBalance to the effectiveBalance sum so EFFECTIVE_BALANCE_INCREMENT is factored out.

In that case we should ensure that (safetyBuffer =~ 1.5 for example)

validatorCount * safetyBuffer * MAX_EFFECTIVE_BALANCE / EFFECTIVE_BALANCE_INCREMENT < Number.MAX_SAFE_INTEGER

@twoeths
Copy link
Contributor Author

twoeths commented Aug 23, 2021

if EFFECTIVE_BALANCE_INCREMENT is not 1e9, it's not a problem to track effectiveBalance as 30, 31, 32 because it's always a multiple of EFFECTIVE_BALANCE_INCREMENT. We only have a problem if the multiple of EFFECTIVE_BALANCE_INCREMENT logic gets changed which is not likely to happen.

Anyway in node, when we want effectiveBalance as number, it means double, not integer. So let's think about it as a number < 32, not an integer < 32.

I also want to track totalActiveStake, prevEpochUnslashedStake and currEpochUnslashedTargetStake inside EpochProcess as number to avoid BigInt calculation there, that could reduce the time for beforeEpochProcess a lot. It's not possible to do that if we use effectiveBalance as multiple of EFFECTIVE_BALANCE_INCREMENT which is too big since they are the sum of effectiveBalances

@dapplion
Copy link
Contributor

Sounds good then, let's compute at ETH, regardless of EFFECTIVE_BALANCE_INCREMENT value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope-performance Performance issue and ideas to improve performance.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants