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: track inclusion events #2299

Merged
merged 3 commits into from
Jun 12, 2023
Merged

core/tracker: track inclusion events #2299

merged 3 commits into from
Jun 12, 2023

Conversation

corverroos
Copy link
Contributor

Extends tracker duty failure reasons to include "chain inclusion" results. Duties will subsequently only be marked as success if included onchain, and accordinlgy marked as failed if not included onchain.

Note that chain inclusion is only checked if the duty was broadcasted, so this doesn't actually "make duty failures objective", since if the local node failed to broadcast, it will not check inclusion and will mark the duty as failed regardless of actual results.

category: feature
ticket: #2200

inclCheckLag = 16
// inclTrimLag is the number of slots after which we delete cached submissions.
// This matches scheduler trimEpochOffset.
inclTrimLag = 32 * 3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

decrease from 3 to 1 epoch since that is max at this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

just wondering, what if we are running charon in a network with slots_per_epoch != 32, for example, gnosis? Shouldn't we read this kind of information from beacon node?

If we are taking slot as smallest unit in any network then its fine to hardcode it to 32.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah not so worried about this, 32 slots is fine, even for gnosis chain.

@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Patch coverage: 58.82% and project coverage change: +0.12 🎉

Comparison is base (c937ee6) 53.75% compared to head (3ccfca7) 53.88%.

❗ Current head 3ccfca7 differs from pull request most recent head 84d2043. Consider uploading reports for the commit 84d2043 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2299      +/-   ##
==========================================
+ Coverage   53.75%   53.88%   +0.12%     
==========================================
  Files         191      191              
  Lines       25767    25792      +25     
==========================================
+ Hits        13852    13897      +45     
+ Misses      10227    10203      -24     
- Partials     1688     1692       +4     
Impacted Files Coverage Δ
app/app.go 6.81% <0.00%> (ø)
core/interfaces.go 0.00% <ø> (ø)
core/tracker/tracker.go 79.59% <63.63%> (-0.41%) ⬇️
core/tracker/inclusion.go 41.31% <66.66%> (+1.15%) ⬆️

... and 9 files with indirect coverage changes

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

Copy link
Contributor

@dB2510 dB2510 left a comment

Choose a reason for hiding this comment

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

LGTM!
Let's test this.

core/tracker/tracker.go Outdated Show resolved Hide resolved
inclCheckLag = 16
// inclTrimLag is the number of slots after which we delete cached submissions.
// This matches scheduler trimEpochOffset.
inclTrimLag = 32 * 3
Copy link
Contributor

Choose a reason for hiding this comment

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

just wondering, what if we are running charon in a network with slots_per_epoch != 32, for example, gnosis? Shouldn't we read this kind of information from beacon node?

If we are taking slot as smallest unit in any network then its fine to hardcode it to 32.

@@ -17,6 +17,10 @@ maintain system performance.
- *Summary*: failed to broadcast duty to beacon node
- *Details*: Reason `bcast` indicates that beacon node returned an error while submitting aggregated duty signature to beacon node.

### Failure Reason: `chain_inclusion`
- *Summary*: duty not included on-chain
- *Details*: Reason `chain_inclusion` indicates that even though charon broadcasted the duty successfully, it wasn`t included in the beacon chain. This is expected for up to 20% of attestations. It may however indicate problematic charon broadcast delays or beacon node network problems.
Copy link
Contributor

@xenowits xenowits Jun 12, 2023

Choose a reason for hiding this comment

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

nit: do we have data that only 80% broadcasted attestations end up on-chain?

Copy link
Contributor Author

@corverroos corverroos Jun 12, 2023

Choose a reason for hiding this comment

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

ppromrated network effectiveness:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although that might not be accurate, here is better data. Seems like 5% is more realistic for goerli at least. Well see how mainnet does after releasing v0.16.

image

@corverroos corverroos added the merge when ready Indicates bulldozer bot may merge when all checks pass label Jun 12, 2023
@obol-bulldozer obol-bulldozer bot merged commit 4f8787b into main Jun 12, 2023
@obol-bulldozer obol-bulldozer bot deleted the corver/trackincl branch June 12, 2023 12:57
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.

3 participants