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

core/tracker: update tracker for DutyAggregator #1164

Merged
merged 4 commits into from
Sep 26, 2022
Merged

Conversation

dB2510
Copy link
Contributor

@dB2510 dB2510 commented Sep 25, 2022

Adds support for DutyAggregator and DutyPrepareAggregator failures to tracker.

category: feature
ticket: #1154

@codecov
Copy link

codecov bot commented Sep 25, 2022

Codecov Report

Base: 53.28% // Head: 53.27% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (b9a57ce) compared to base (08a0255).
Patch coverage: 70.58% of modified lines in pull request are covered.

❗ Current head b9a57ce differs from pull request most recent head 99d25e2. Consider uploading reports for the commit 99d25e2 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1164      +/-   ##
==========================================
- Coverage   53.28%   53.27%   -0.01%     
==========================================
  Files         131      131              
  Lines       15341    15385      +44     
==========================================
+ Hits         8174     8196      +22     
- Misses       5983     6000      +17     
- Partials     1184     1189       +5     
Impacted Files Coverage Δ
core/fetcher/fetcher.go 59.75% <0.00%> (-0.74%) ⬇️
core/tracker/component_string.go 40.00% <ø> (ø)
core/tracker/tracker.go 71.59% <73.46%> (-0.28%) ⬇️
app/metrics.go 84.21% <0.00%> (-15.79%) ⬇️
app/vmock.go 69.33% <0.00%> (-4.67%) ⬇️
core/scheduler/scheduler.go 73.31% <0.00%> (-0.54%) ⬇️
app/app.go 56.85% <0.00%> (+0.09%) ⬆️
core/leadercast/transport.go 76.33% <0.00%> (+1.18%) ⬆️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -77,6 +77,8 @@ func (f *Fetcher) Fetch(ctx context.Context, duty core.Duty, defSet core.DutyDef
unsignedSet, err = f.fetchAggregatorData(ctx, duty.Slot, defSet)
if err != nil {
return errors.Wrap(err, "fetch aggregator data")
} else if len(unsignedSet) == 0 { // No aggregators found in this slot
Copy link
Contributor

Choose a reason for hiding this comment

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

should add a test for this

@@ -193,6 +196,30 @@ func analyseDutyFailed(duty core.Duty, allEvents map[core.Duty][]event) (bool, c
}
}
}

if duty.Type == core.DutyAggregator {
Copy link
Contributor

Choose a reason for hiding this comment

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

When DuyAggregator fails in fetcher, it is impossible to distringuish between "no aggregators" and "couldn't fetch duty data from beacon node". We will need to emit some other kind of event for this. We could emit a "timedout" event from retryer maybe? Or just assume that it is "no aggregators" for now.

@@ -77,6 +77,8 @@ func (f *Fetcher) Fetch(ctx context.Context, duty core.Duty, defSet core.DutyDef
unsignedSet, err = f.fetchAggregatorData(ctx, duty.Slot, defSet)
if err != nil {
return errors.Wrap(err, "fetch aggregator data")
} else if len(unsignedSet) == 0 { // No aggregators found in this slot
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to add tests for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

Comment on lines 50 to 62
fetcherMsg = "couldn't fetch duty data from the beacon node"
fetcherAggregatorNoAttDataMsg = "couldn't aggregate attestation due to failed attester duty"
fetcherAggregatorFewPreparesMsg = "couldn't aggregate attestation due to insufficient partial committee subscriptions"
fetcherAggregatorZeroPreparesMsg = "couldn't aggregate attestation due to zero partial committee subscriptions"
fetcherAggregatorFailedPrepareMsg = "couldn't aggregate attestation due to failed prepare aggregator duty "
fetcherProposerFewRandaosMsg = "couldn't propose block due to insufficient partial randao signatures"
fetcherProposerZeroRandaosMsg = "couldn't propose block due to zero partial randao signatures"
fetcherProposerFailedRandaoMsg = "couldn't propose block due to failed randao duty"
consensusMsg = "consensus algorithm didn't complete"
validatorAPIMsg = "signed duty not submitted by local validator client"
parSigDBInternalMsg = "partial signature database didn't trigger partial signature exchange"
parSigExMsg = "no partial signatures received from peers"
parSigDBThresholdMsg = "insufficient partial signatures received, minimum required threshold not reached"
Copy link
Contributor

Choose a reason for hiding this comment

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

@dB2510 @thomasheremans we should consider a collapsed section of the FAQ on the doc site that explains each of these errors in two sentences or so. We should aim that if people google these errors exactly our docs show up in the indexed google results.

Copy link
Contributor

@thomasheremans thomasheremans Sep 25, 2022

Choose a reason for hiding this comment

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

Sure @dB2510 can you provide me with the two sentences or so for each and I will add them to the docs, thanks 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

@corverroos corverroos added the merge when ready Indicates bulldozer bot may merge when all checks pass label Sep 26, 2022
@obol-bulldozer obol-bulldozer bot merged commit c281d8d into main Sep 26, 2022
@obol-bulldozer obol-bulldozer bot deleted the dhruv/trackeragg branch September 26, 2022 07:51
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.

None yet

4 participants