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

VC: Use scoring function to select best attestation data when using multiple BNs. #5101

Merged
merged 12 commits into from
Jul 6, 2023

Conversation

cheatfate
Copy link
Contributor

@cheatfate cheatfate commented Jun 20, 2023

With this PR our validator client (VC) will be able to select the best scored attestation data when using configurations with multiple beacon nodes.
To achieve maximum performance you should enable VC's block monitoring feature using --block-monitor-type=poll (polling BN's for latest head 3 times per slot) or --block-monitor-type=event (using BN's /eth/v1/events interface).

When block monitoring is enabled - VC will be able to obtain "perfect" score and continue attesting without waiting for responses from all the beacon nodes. Without this feature VC will wait for all BN responses and selects the best response using scoring algorithm.

"perfect score" - is score when attestation data slot equal to block's slot and target epoch = source epoch + 1.

@github-actions
Copy link

github-actions bot commented Jun 20, 2023

Unit Test Results

         9 files  ±0    1 071 suites  ±0   42m 2s ⏱️ + 4m 30s
  3 711 tests +1    3 432 ✔️ +1  279 💤 ±0  0 ±0 
15 823 runs  +3  15 518 ✔️ +3  305 💤 ±0  0 ±0 

Results for commit 422350b. ± Comparison against base commit 66febf2.

♻️ This comment has been updated with latest results.

var resultReady = false
let onlineNodes =
try:
if iterations == 0:
Copy link
Member

Choose a reason for hiding this comment

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

what's the idea with multiple iterations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a period of time which is regulated by timeFut value, so if we not received any successful responses in one iteration we are trying to request again with same/new set of BNs until timeFut will not expire.

Choose a reason for hiding this comment

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

ok

@etan-status
Copy link
Contributor

This needs to merge from unstable to take account of the new CI job - as of #5140 we build both the pinned nim version as well as the latest upstream 1.6 version to catch regressions in nim more easily.

beacon_chain/validator_client/common.nim has conflict so not touching this PR

BestNodeResponse.init(node, handlerResponse, score))
if perfectScore(score):
perfectScoreFound = true
break
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
break
break mainLoop

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There some cleanup should be done in inner loop before exiting to mainLoop.

Copy link
Member

Choose a reason for hiding this comment

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

break innerLoop then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no innerLoop.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, yep there is, but as i said there should be some cleanup made right after pendingRequests loop, so it breaks out of pendingRequests loop and after cleanup it breaks to innerLoop.

if not(future.finished()):
pendingCancel.add(future.cancelAndWait())
# Awaiting cancellations.
await allFutures(pendingCancel)
Copy link
Member

Choose a reason for hiding this comment

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

when calling cancelAndWait, is it possible that some of these futures are completed instead of cancelled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its only possible to get into this situation
So in this case:
<future not finished> -> <future not finished> -> <future finished> result future will be finished, not cancelled, but it sill requires number of async steps to get into this.
And in this case
<future not finished> -> <future not finished> -> <future cancelled> result future will be cancelled, but only if some of the intermediate futures will not decide to swallow cancellation or replace it with an error.

Fix registerBlock post-rebase issues.
@arnetheduck arnetheduck merged commit ac1b026 into unstable Jul 6, 2023
@arnetheduck arnetheduck deleted the best-attestation branch July 6, 2023 09:14
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.

4 participants