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

app/eth2wrap: implement and wire validator cache #2121

Merged
merged 6 commits into from
Apr 24, 2023
Merged

Conversation

corverroos
Copy link
Contributor

@corverroos corverroos commented Apr 23, 2023

This is a second attempt at caching active validators. Instead of caching the eth2client.ValidatorProvider endpoints which return full Validator objects that are not actually immutable by epoch, rather implement a new set of client endpoint specifically for cached immutable active validators by current epoch that only return the validator index and pubkey which are immutable. The only race condition is the fact that we query this by "head", so could be for another slot that what we think is current, although this was already the case previously, so no new races are introdcued.

This should significantly decrease BN load, especially if using the validatormock.

category: feature
ticket: #1396

@codecov
Copy link

codecov bot commented Apr 23, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@9b11d06). Click here to learn what that means.
Patch coverage: 45.66% of modified lines in pull request are covered.

❗ Current head 7393ae4 differs from pull request most recent head bd93590. Consider uploading reports for the commit bd93590 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2121   +/-   ##
=======================================
  Coverage        ?   53.56%           
=======================================
  Files           ?      174           
  Lines           ?    22547           
  Branches        ?        0           
=======================================
  Hits            ?    12078           
  Misses          ?     9014           
  Partials        ?     1455           
Impacted Files Coverage Δ
app/eth2wrap/eth2wrap_gen.go 12.53% <ø> (ø)
app/eth2wrap/lazy.go 44.44% <0.00%> (ø)
app/vmock.go 0.00% <0.00%> (ø)
core/scheduler/scheduler.go 75.55% <ø> (ø)
testutil/validatormock/synccomm.go 3.17% <0.00%> (ø)
app/eth2wrap/eth2wrap.go 57.20% <20.83%> (ø)
app/eth2wrap/httpwrap.go 23.12% <25.00%> (ø)
app/app.go 6.74% <30.00%> (ø)
testutil/beaconmock/beaconmock.go 54.03% <50.00%> (ø)
core/validatorapi/validatorapi.go 56.34% <62.96%> (ø)
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@xenowits xenowits left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -36,6 +37,34 @@ type NodePeerCountProvider interface {
NodePeerCount(ctx context.Context) (int, error)
}

type ActiveValidators map[eth2p0.ValidatorIndex]eth2p0.BLSPubKey
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: missing godoc

Copy link
Collaborator

Choose a reason for hiding this comment

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

another nit: maybe this type should live in valcache.go?

@corverroos corverroos added the merge when ready Indicates bulldozer bot may merge when all checks pass label Apr 24, 2023
resp[val.Index] = val.Validator.PublicKey
}

c.active = resp
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to lock mutex around this (when c.active is updated) and not above as we are calling eth2Cl after locking mutex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no the point here is to lock while only one call to populate the cache is done

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it 👍

@obol-bulldozer obol-bulldozer bot merged commit 96af658 into main Apr 24, 2023
@obol-bulldozer obol-bulldozer bot deleted the corver/valcache2 branch April 24, 2023 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants