core/tracker: refactor inclusion checker #2120
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2120 +/- ##
==========================================
- Coverage 53.52% 53.36% -0.17%
==========================================
Files 173 173
Lines 22457 22618 +161
==========================================
+ Hits 12021 12069 +48
- Misses 8981 9095 +114
+ Partials 1455 1454 -1
... and 9 files with indirect coverage changes 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. |
corverroos
changed the title
| } | ||
| } | ||
|
|
||
| func reportAttInclusion(ctx context.Context, sub submission, block block) { |
There was a problem hiding this comment.
no need to pass full block, just pass blockSlot. AttSlot can be taken from submission.
There was a problem hiding this comment.
Aggregated logging field requires more info
core/tracker/inclusion.go
Outdated
| } | ||
|
|
||
| // CheckBlock checks whether the block includes any of the submitted duties. | ||
| func (i *inclusion) CheckBlock(ctx context.Context, block block) { |
There was a problem hiding this comment.
why this is a public method?
| } | ||
|
|
||
| // InclusionChecker checks whether duties have been included in blocks. | ||
| type InclusionChecker struct { |
There was a problem hiding this comment.
why you needed to wrap inclusion with InclusionChecker? Also some methods on inclusion struct are public which can be private as they only called by InclusionChecker methods which are exposed to other packages.
There was a problem hiding this comment.
renamed it to inclusionChecker which wrap a inclusionCore. The core is decoupled and optimised for easy testing.
| continue // This duty is for another committee | ||
| } | ||
|
|
||
| if !att.AggregationBits.BitAt(duty.ValidatorCommitteeIndex) { |
There was a problem hiding this comment.
@corverroos you need to add this check in new inclusion checker. To check if our unaggregated attestation is included as part of an aggregated attestation.
There was a problem hiding this comment.
I think you haven't pushed that commit. It might be in local commit?
There was a problem hiding this comment.
Cool, should be here now: https://github.com/ObolNetwork/charon/pull/2120/files#r1174812448
| z.I64("inclusion_delay", inclDelay), | ||
| z.Any("broadcast_delay", sub.Delay), | ||
| z.Int("aggregate_len", len(aggIndices)), | ||
| z.Bool("aggregated", len(aggIndices) > 1), |
There was a problem hiding this comment.
@dB2510 the aggregated logging field is added here
There was a problem hiding this comment.
Just to clarify, this aggregated logging will tell us that attestation is an aggregate attestation and NOT an unaggregated one. Well here aggregated: true only when we are aggregator of that slot AND it is our aggregate attestation.
| // TODO(corver): We should probably check if validator included in AggregationBits | ||
| // for attester duty by caching scheduler duty data to get validator committee index. |
There was a problem hiding this comment.
cool, i was looking for this. Will push in next PR probably?
| continue | ||
| } | ||
|
|
||
| // TODO(corver): We should probably check if validator included in AggregationBits |
There was a problem hiding this comment.
Yeah. If we wish, we can also understand if the attestation was aggregated or not by checking
if NO_OF_SET_BITS(attestation.aggregation_bits) == 1 {
"attestation was NOT aggregated by any aggregator"
}
corverroos
added
the
merge when ready
Refactors the inclusion delay tracker to also detect missed inclusions and to log broadcast delay of included and missed duties. The aim is to find a correlation between broadcast performance and inclusion delay.
category: feature
ticket: #1538