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.

Sign up for GitHub

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

Jump to bottom

EpochProcess: Prepare activeValidatorIndices for the next epoch #2871

Merged
merged 1 commit into from Aug 4, 2021
Merged

EpochProcess: Prepare activeValidatorIndices for the next epoch #2871

merged 1 commit into from Aug 4, 2021

Conversation

twoeths
Copy link
Contributor

@twoeths twoeths commented Jul 22, 2021

Motivation

  • On an altair syncing node, it takes 3.5% of the time to get active validator indices on rotateEpoch

Description

  • Calculate it in IEpochProcess at the end of the previous epoch to save 1 validators loop

Closes #2866

Steps to test or reproduce

  • Sync altair-devnet-1
  • was able to sync to head with this branch
Jul-22 11:44:30.003 []                 info: Synced - finalized: 1551 0x5b68…4f6f - head: 49721 0xf9bd…6132 - clockSlot: 49722 - peers: 17

@codeclimate
Copy link

codeclimate bot commented Jul 22, 2021 •
edited

Code Climate has analyzed commit 273c211 and detected 0 issues on this pull request.

View more on Code Climate.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 22, 2021 •
edited

Performance Report

✔️ no performance regression detected

Full benchmark results
Benchmark suite Current: dc3fc9a Previous: 40358ff Ratio
getCommitteeAssignments - req 1000 vs - 250000 vc 9.5931 ms/op 9.0318 ms/op 1.06
epoch altair - 250000 vs - 7PWei - processJustificationAndFinalization 194.07 us/op 327.04 us/op 0.59
epoch altair - 250000 vs - 7PWei - processInactivityUpdates 2.2702 s/op 2.5108 s/op 0.90
epoch altair - 250000 vs - 7PWei - processRewardsAndPenalties 996.19 ms/op 1.1491 s/op 0.87
epoch altair - 250000 vs - 7PWei - processRegistryUpdates 18.140 us/op 21.338 us/op 0.85
epoch altair - 250000 vs - 7PWei - processSlashings 61.441 us/op 115.39 us/op 0.53
epoch altair - 250000 vs - 7PWei - processEffectiveBalanceUpdates 55.252 ms/op 59.556 ms/op 0.93
epoch altair - 250000 vs - 7PWei - processParticipationFlagUpdates 270.96 ms/op 303.56 ms/op 0.89
epoch altair - 250000 vs - 7PWei - prepareEpochProcessState 475.85 ms/op 661.29 ms/op 0.72
Process block - 250000 vs - 7PWei - with 0 validator exit 418.42 us/op 478.02 us/op 0.88
Process block - 250000 vs - 7PWei - with 1 validator exit 23.325 ms/op 28.601 ms/op 0.82
Process block - 250000 vs - 7PWei - with 16 validator exits 30.791 ms/op 37.345 ms/op 0.82
epoch phase0 - 250000 vs - 7PWei - processJustificationAndFinalization 76.508 us/op 116.17 us/op 0.66
epoch phase0 - 250000 vs - 7PWei - processRewardsAndPenalties 632.69 ms/op 556.83 ms/op 1.14
epoch phase0 - 250000 vs - 7PWei - processRegistryUpdates 25.049 us/op 25.996 us/op 0.96
epoch phase0 - 250000 vs - 7PWei - processSlashings 86.037 us/op 105.78 us/op 0.81
epoch phase0 - 250000 vs - 7PWei - processFinalUpdates 106.43 ms/op 58.114 ms/op 1.83
epoch phase0 - 250000 vs - 7PWei - prepareEpochProcessState 770.28 ms/op 1.2676 s/op 0.61
getAttestationDeltas - 250000 vs - 7PWei 84.806 ms/op 116.38 ms/op 0.73
processSlots - 250000 vs - 7PWei - 32 empty slots 5.0557 s/op 7.2261 s/op 0.70
shuffle list - 16384 els 2.0641 ms/op 1.9988 ms/op 1.03
shuffle list - 250000 els 29.311 ms/op 26.156 ms/op 1.12
getPubkeys - persistent - req 1000 vs - 250000 vc 19.674 us/op 19.955 us/op 0.99
BLS verify - blst-native 2.3150 ms/op 2.1009 ms/op 1.10
BLS verifyMultipleSignatures 3 - blst-native 4.5556 ms/op 4.3661 ms/op 1.04
BLS verifyMultipleSignatures 8 - blst-native 9.8126 ms/op 9.5036 ms/op 1.03
BLS verifyMultipleSignatures 32 - blst-native 35.318 ms/op 34.674 ms/op 1.02
BLS aggregatePubkeys 32 - blst-native 46.355 us/op 47.872 us/op 0.97
BLS aggregatePubkeys 128 - blst-native 182.11 us/op 176.81 us/op 1.03
getAttestationsForBlock 106.94 ms/op 140.65 ms/op 0.76
validate gossip signedAggregateAndProof - struct 7.8318 ms/op 5.6994 ms/op 1.37
validate gossip signedAggregateAndProof - treeBacked 5.5132 ms/op 5.5172 ms/op 1.00
validate gossip attestation - struct 2.6137 ms/op 2.5409 ms/op 1.03
validate gossip attestation - treeBacked 2.5615 ms/op 2.5342 ms/op 1.01

by benchmarkbot/action

@twoeths twoeths changed the title EpochProces: Prepare activeValidatorIndices for the next epoch EpochProcess: Prepare activeValidatorIndices for the next epoch Jul 22, 2021
wemeetagain
wemeetagain reviewed Jul 26, 2021
View reviewed changes
packages/beacon-state-transition/src/allForks/util/epochProcess.ts Outdated
@@ -145,6 +148,11 @@ export function prepareEpochProcessState<T extends allForks.BeaconState>(state:

out.statuses.push(status);
out.indicesBounded.push([i, v.activationEpoch, v.exitEpoch]);
if (nextEpoch >= config.ALTAIR_FORK_EPOCH) {
Copy link
Member

Choose a reason for hiding this comment

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

This can be precalculated above this loop

@twoeths twoeths marked this pull request as draft August 3, 2021 08:28
@github-actions github-actions bot added the Api label Aug 4, 2021
@twoeths twoeths marked this pull request as ready for review August 4, 2021 03:37
@twoeths
Copy link
Contributor Author

twoeths commented Aug 4, 2021

Since we have activeIndices cached in epochShuffling, this PR is changed to making sure there is exactly 1 validators loop during epoch transition

Aug-04 12:14:18.012 []                 info: Synced - finalized: 1284 0x314c…667d - head: 41171 0x0da7…ae2c - clockSlot: 41171 - peers: 15
Aug-04 12:14:30.002 []                 info: Synced - finalized: 1284 0x314c…667d - head: 41172 0x62e7…77b5 - clockSlot: 41172 - peers: 15

@dapplion
Copy link
Contributor

dapplion commented Aug 4, 2021 •
edited

👍 Nice approach! This PR relies on MAX_SEED_LOOKAHEAD >= 1, otherwise it can't guarantee that the return value of isActiveValidator(nextEpoch) will be the same for the next epoch. You should add a check in somewhere so the application exists in a panic if that happens. MAX_SEED_LOOKAHEAD is a customizable variable, someone could run lodestar with --params.MAX_SEED_LOOKAHEAD 0

@tuyennhv please if possible tackle in a new PR

@dapplion dapplion merged commit 196e5df into master Aug 4, 2021
@dapplion dapplion deleted the tuyen/altair-rotate-epoch branch August 4, 2021 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wemeetagain wemeetagain wemeetagain left review comments

@dapplion dapplion dapplion approved these changes

@mpetrunic mpetrunic Awaiting requested review from mpetrunic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve altair rotateEpoch
3 participants
@twoeths @dapplion @wemeetagain