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

Poll node in batch to get validator indices #2730

Merged
merged 5 commits into from
Jun 28, 2021

Conversation

twoeths
Copy link
Contributor

@twoeths twoeths commented Jun 22, 2021

Motivation

  • Imported 160 oononba validators into contabo-1 and it failed to request validator indices due to the query max length

Description

  • Query in batches, max batch=5
  • Each batch contains at most 5 http requests to the node
  • Each http request could work on 40 validators at most
  • Work on at most 1000 validator pubkeys per poll

Let me know if we want to tweak the above numbers.

Closes #2727

Tested in contabo-1

Jun-22 09:24:26.394 []                 info: Decrypted 160 validator keystores
Jun-22 09:24:26.459 []                 info: Genesis available
Jun-22 09:24:26.477 []                 info: Verified node and validator have same config
Jun-22 09:24:26.727 []                 info: Discovered new validators count=160
Jun-22 09:24:36.614 []                 info: Published SyncCommitteeSignature slot=145718, count=160
Jun-22 09:24:36.832 []                 info: Published attestations slot=145718, index=0, count=5
Jun-22 09:24:40.068 []                 info: Published SyncCommitteeContribution slot=145718, index=1, count=1

@codeclimate
Copy link

codeclimate bot commented Jun 22, 2021

Code Climate has analyzed commit 906d49c and detected 0 issues on this pull request.

View more on Code Climate.

@twoeths twoeths marked this pull request as ready for review June 22, 2021 13:50
@dapplion
Copy link
Contributor

Looks good! What numbers do Teku use? We could base ours on them

@twoeths
Copy link
Contributor Author

twoeths commented Jun 23, 2021

Looks good! What numbers do Teku use? We could base ours on them

@dapplion it's 10 per batch https://github.com/ConsenSys/teku/blob/79e49c48e0a6a78eac5b7760860d680a046e78af/validator/remote/src/main/java/tech/pegasys/teku/validator/remote/RemoteValidatorApiHandler.java#L75

I think it's quite conservative there, I used to be able to query 80 validator indices successfully, the issue only happened with 160 validators per batch. So

  • 10 as Teku
  • or 40 as in this PR
  • or 20 which is in the middle

@wemeetagain
Copy link
Member

Teku may be going off of this: https://stackoverflow.com/questions/3091485/what-is-the-limit-on-querystring-get-url-parameters#answer-56527673 I've seen this answer copy/pasted around, tho I can't find the original

Also related, I just posted this, maybe this can be more definitely resolved at the standards level: ethereum/beacon-APIs#153

@twoeths twoeths force-pushed the tuyen/pollValidatorIndices branch from 0ac8f8c to 08dfc71 Compare June 24, 2021 00:56
export function pubkeysToBatches(
pubkeysHex: string[],
maxPubkeysPerRequest: number = PUBKEYS_PER_REQUEST,
maxRequesPerBatch: number = REQUESTS_PER_BATCH
Copy link
Contributor

Choose a reason for hiding this comment

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

typo maxRequestsPerBatch


const pubkeysPerBatch = maxPubkeysPerRequest * maxRequesPerBatch;
let batch: Batch = [];
let pubkeysPerRequest: string[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can find a similar approach without mutating scoped variables? You could have two for loops and scope the arrays inside them only with const

@github-actions
Copy link
Contributor

Performance Report

✔️ no performance regression detected

Full benchmark results
Benchmark suite Current: 6c3780c Previous: 15766f4 Ratio
Process block - 250000 vs - with 0 validator exit 6.2893 ms/op 5.5543 ms/op 1.13
Process block - 250000 vs - with 1 validator exit 24.931 ms/op 22.065 ms/op 1.13
Process block - 250000 vs - with 16 validator exits 25.894 ms/op 22.641 ms/op 1.14
epoch - 250000 vs - processJustificationAndFinalization 101.72 us/op 89.589 us/op 1.14
epoch - 250000 vs - processRewardsAndPenalties 373.38 ms/op 329.08 ms/op 1.13
epoch - 250000 vs - processRegistryUpdates 9.9070 us/op 8.7800 us/op 1.13
epoch - 250000 vs - processSlashings 3.8138 ms/op 3.2909 ms/op 1.16
epoch - 250000 vs - processFinalUpdates 43.563 ms/op 37.574 ms/op 1.16
epoch - 250000 vs - prepareEpochProcessState 1.0065 s/op 876.69 ms/op 1.15
getAttestationDeltas - 250000 vs 168.65 ms/op 157.14 ms/op 1.07
processSlots - 250000 vs - 32 empty slots 1.9861 s/op 1.7430 s/op 1.14
shuffle list - 512 els 1.5292 ms/op 1.3357 ms/op 1.14
shuffle list - 16384 els 17.974 ms/op 15.492 ms/op 1.16
shuffle list - 250000 els 260.80 ms/op 226.70 ms/op 1.15
aggregationBits - 2048 els - readonlyValues 267.36 us/op 220.88 us/op 1.21
aggregationBits - 2048 els - zipIndexesInBitList 52.675 us/op 46.689 us/op 1.13
ssz.Root.equals 1.4670 us/op 1.1940 us/op 1.23
ssz.Root.equals with valueOf() 1.8490 us/op 1.6820 us/op 1.10
byteArrayEquals with valueOf() 1.7400 us/op 1.6680 us/op 1.04

by benchmarkbot/action

@dapplion dapplion merged commit 866da32 into master Jun 28, 2021
@dapplion dapplion deleted the tuyen/pollValidatorIndices branch June 28, 2021 17:19
@dapplion dapplion mentioned this pull request Nov 30, 2021
2 tasks
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.

Validator: attestation and sync committee services stop working
4 participants