Improve performance during CheckAttesterDoubleVotes #8927
Conversation
…kAttesterDoubleVotes
… parallel processing
Codecov Report
@@ Coverage Diff @@
## develop #8927 +/- ##
===========================================
- Coverage 60.90% 60.67% -0.23%
===========================================
Files 529 529
Lines 37372 37367 -5
===========================================
- Hits 22762 22674 -88
- Misses 11344 11434 +90
+ Partials 3266 3259 -7 |
Benchmarks: Old schema vs New shemaBenchmarkStore_CheckDoubleBlockProposals-16 2 504269674 ns/op 208081248 B/op 5010016 allocs/op |
| @@ -9,7 +9,6 @@ package slasherkv | |||
| var ( | |||
| // Slasher buckets. | |||
| attestedEpochsByValidator = []byte("attested-epochs-by-validator") | |||
| attestationRecordsBucket = []byte("attestation-records") | |||
There was a problem hiding this comment.
We should add a database "migration" to delete this bucket to reclaim the space, if it is no longer used.
@rauljordan, what do you think?
There was a problem hiding this comment.
This db is unused at the moment, so we are safe, but we should add a migration in the future for slasher changes
beacon-chain/db/slasherkv/slasher.go
Outdated
| doubleVotesMu := sync.Mutex{} | ||
| eg, egctx := errgroup.WithContext(ctx) | ||
| for _, att := range attestations { | ||
| attToProcess := att // https://golang.org/doc/faq#closures_and_goroutines |
There was a problem hiding this comment.
It is not clear how this comment adds additional context to the reader. Could you please use a full sentence followed by "See: $URL".
beacon-chain/db/slasherkv/slasher.go
Outdated
| eg, egctx := errgroup.WithContext(ctx) | ||
| for _, att := range attestations { | ||
| attToProcess := att // https://golang.org/doc/faq#closures_and_goroutines | ||
| eg.Go(func() error { // process every attestation parallely |
There was a problem hiding this comment.
| eg.Go(func() error { // process every attestation parallely | |
| eg.Go(func() error { // process every attestation parallelly |
beacon-chain/db/slasherkv/slasher.go
Outdated
| attWrapper, err := decodeAttestationRecord(encodedAttRecord) | ||
| if err != nil { | ||
| return err | ||
| attWrapper, err1 := decodeAttestationRecord(encodedAttRecord) | ||
| if err1 != nil { | ||
| return err1 |
There was a problem hiding this comment.
I'm curious about this change. Why rename err to err1? Was it to satisfy some err / shadowing linter, personal preference, or some other standard we should adopt?
There was a problem hiding this comment.
Yes. err is returned by View. By naming like this we can avoid shadowing. We should actually use more legible errName. I have changed this to 'decodeErr'
This reverts commit a958dd2.
What type of PR is this?
Performance Improvement
What does this PR do? Why is it needed?
This PR tries to improves the performance when checking for double votes during attestation.
The code was doing multiple in-directions inside a double loop.
The fix was to store the attestations in multiple places (de-normalize) and process
every attestation in a seperate go routine.
Which issues(s) does this PR fix?
Fixes #8913
Other notes for review
'att.SigningRoot + FastSum64([]ValidatorIndex)' to 'att.SigningRoot + AttestationRecord'