core/scheduler: add duty query method #384

leolara · 2022-04-07T13:29:27Z

Add a method to the scheduler to query a duty arguments once it has been resolved

category: feature
ticket: #352

codecov · 2022-04-07T13:31:21Z

Codecov Report

Merging #384 (333f922) into main (39c2aa8) will decrease coverage by 0.12%.
The diff coverage is 80.64%.

❗ Current head 333f922 differs from pull request most recent head c1af5d0. Consider uploading reports for the commit c1af5d0 to get more accurate results

@@            Coverage Diff             @@
##             main     #384      +/-   ##
==========================================
- Coverage   56.93%   56.81%   -0.13%     
==========================================
  Files          63       63              
  Lines        5605     5506      -99     
==========================================
- Hits         3191     3128      -63     
+ Misses       1993     1965      -28     
+ Partials      421      413       -8

Impacted Files	Coverage Δ
core/scheduler/scheduler.go	`73.92% <80.64%> (-0.11%)`	⬇️
core/validatorapi/router.go	`61.06% <0.00%> (-3.65%)`	⬇️
core/validatorapi/validatorapi.go	`47.85% <0.00%> (-3.58%)`	⬇️
app/app.go	`65.07% <0.00%> (+0.95%)`	⬆️
testutil/beaconmock/beaconmock.go	`68.49% <0.00%> (+1.82%)`	⬆️
core/types.go	`44.77% <0.00%> (+3.10%)`	⬆️
core/encode.go	`64.00% <0.00%> (+5.91%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39c2aa8...c1af5d0. Read the comment docs.

corverroos · 2022-04-07T15:06:11Z

core/scheduler/scheduler.go

@@ -290,6 +327,32 @@ type slot struct {
 	SlotDuration  time.Duration
 }

+func newSlot(ctx context.Context, eth2Cl eth2Provider, height uint64) (slot, error) {


resolveDuties doesn't need the full internal slot type, it only uses slot and epoch int64s. I feel it would be simpler to refactor resolveDuties rather than adding this which adds some code duplication (from newSlotTicker).

corverroos · 2022-04-07T15:06:29Z

core/scheduler/scheduler.go

@@ -173,6 +209,7 @@ func (s *Scheduler) scheduleSlot(ctx context.Context, slot slot) error {
 }

 // resolveDuties resolves the duties for the slot's epoch, caching the results.
+// Do not call if you do not hold the dutiesMutex.


you do not hold the mutex when call this?

The caller holds the mutex

this comment not accurate anymore, can remove

corverroos · 2022-04-07T15:08:48Z

core/scheduler/scheduler.go

+	s.dutiesMutex.Lock()
+	defer s.dutiesMutex.Unlock()


I always decrease the time locks held to a minimum. I would strongly discourage doing network calls while a lock is held for example. Beacon API calls often take multiple seconds, during which you will be locking up validatorapi in this case (or whatever other components use GetDuty

Suggest adding getter/setter methods for mutex protected data. Then the logic can be lock free.

func (s *Scheduler) getFetchArgSet(duty core.Duty) core.FetchArgSet, bool { s.dutiesMutex.Lock() defer s.dutiesMutex.Unlock() return s.duties[duty] } func (s *Scheduler) setFetchArg(duty core.Duty, pubkey core.Pubkey, core.FetchArg) bool { s.dutiesMutex.Lock() defer s.dutiesMutex.Unlock() argSet, ok := s.duties[duty] if !ok { argSet = make(core.FetchArgSet) } if _, ok := argSet[pubkey]; ok { return false } s.duties[duty] = set return true }

@corverroos it is not just the duties map that we are sharing, it is also the resolvedEpoch, that is why I put mutex around everything. Otherwise, we will need another mutex and another extra private methods for resolvedEpoch, that perhaps that is what you want.

I recommend a redesign, where these duties are on a external component that stores them, and also the resolved epochs

you can update the resolveDuties map in setFetchArg and also add a getter resolvedEpoch with similar pattern.

Another mutex isn't required, nor a is refactor I think.

if we add a map, I am confused why we keep resolved epochs instead of resolved slots?

Also, I don't think I can update the resolvedEpochs or resolveSlot as an setFetchArg does not mean the whole slot is resolved.

If we move to a resolvedSlot map to bool then is ok, but I think it would be easier then to just put an empty value when a slot is resolved but there are no duties.

corverroos · 2022-04-07T15:12:10Z

core/scheduler/scheduler.go

@@ -123,8 +125,42 @@ func (s *Scheduler) Run() error {
 	}
 }

+func (s *Scheduler) GetDuty(ctx context.Context, duty core.Duty) (core.FetchArgSet, error) {


please add a godoc

Yes, sorry about that

corverroos · 2022-04-07T15:21:22Z

core/scheduler/scheduler.go

+	return s.duties[duty], nil
+}
+
+func (s *Scheduler) isEpochResolved(epoch uint64) bool {


I agree with your concern that this will result in skipping a bunch of duties if a VC calls some far future slot. We should refactor resolvedEpoch into a map or something

I propose to refactor with an external component that has all that logic, perhaps ResolvedDutiesDB ?

Perhaps we should define:

Duty

ResolvedDuty

SolvedDuty

or some separated concepts and include them in the type system for clarity.

I do not think that is necessary, we are almost there.

leolara · 2022-04-07T18:05:26Z

Let's think about this for a moment, all this is because the validatorAPI and the scheduler need to know the public key of a proposer for a given slot.

Instead of making the validtorAPI communicate with the scheduler and access some data structure that was thought to be internal of scheduler in the first time.

Lets create a new component that provides this information (and caches it if the BN cliend does not cache it) to both the scheduler and the validatorAPI.

leolara · 2022-04-08T09:45:36Z

@corverroos please check it before I add tests

corverroos · 2022-04-08T12:13:10Z

core/scheduler/scheduler.go

+	return argSet, ok
+}
+
+func (s *Scheduler) setFetchArg(duty core.Duty, pubkey core.PubKey, set core.FetchArg) bool {


leolara · 2022-04-08T12:16:54Z

@corverroos I forgot a few places to call the private methods, I think now they are all

corverroos · 2022-04-08T12:30:03Z

core/scheduler/scheduler.go

@@ -206,7 +230,7 @@ func (s *Scheduler) resolveDuties(ctx context.Context, slot slot) error {

 			duty := core.Duty{Slot: int64(attDuty.Slot), Type: core.DutyAttester}

-			argSet, ok := s.duties[duty]
+			argSet, ok := s.getFetchArgSet(duty)


no need to call getFetchArgSet first. Just call setFetchArgSet below.

if !s.setFetchArg(duty, pubkey, arg){ log.Debug(ctx, "Ignoring previously resolved duty", z.Any("duty", duty)) continue }

corverroos · 2022-04-08T12:48:09Z

core/scheduler/scheduler.go

@@ -159,7 +182,7 @@ func (s *Scheduler) scheduleSlot(ctx context.Context, slot slot) error {
 		}

 		span.End()
-		delete(s.duties, duty)
+		s.deleteDuty(duty)


Here we delete duties after scheduling, so calls to GetDuty will error after it has been scheduled.

Suggest removing the deleteDuty function and adding a TODO:

// TODO(leo): Trim duties after some time.

If we had the shared component this would not be necessary

Well, we will indeed not need to do this here . But we will still need to do it in DutyResolver though. So same logic is required somewhere.

corverroos · 2022-04-08T13:11:31Z

core/scheduler/scheduler.go

+func (s *Scheduler) isEpochResolved(epoch uint64) bool {
+	return s.getResolvedEpoch() >= epoch
+}


Note that this will return true for default s.resolvedEpoch=math.MaxUint64 before the first epoch is resolved which is technically not correct, but maybe that is not a big issue.

@corverroos ok, so we need to take this into account

corverroos

LGTM

leolara · 2022-04-08T14:04:09Z

Ok, I am going to add tests

corverroos

LGTM

core/scheduler: add duty query method

0a77f33

leolara requested a review from corverroos April 7, 2022 13:29

corverroos reviewed Apr 7, 2022

View reviewed changes

fix concurrency issues

b947d41

corverroos reviewed Apr 8, 2022

View reviewed changes

Fix call to private mutexed methods everywhere

3bae951

corverroos reviewed Apr 8, 2022

View reviewed changes

Remove comment

937add0

leolara force-pushed the leolara/duty-query-scheduler branch from 612e695 to 937add0 Compare April 8, 2022 12:35

corverroos reviewed Apr 8, 2022

View reviewed changes

Add comment about deleting duties

26bbf1a

corverroos reviewed Apr 8, 2022

View reviewed changes

corverroos approved these changes Apr 8, 2022

View reviewed changes

leolara added 3 commits April 8, 2022 16:23

Consider in isEpochResolved if we have not started resolving yet

fb6185f

Merge branch 'main' into leolara/duty-query-scheduler

b7d10fe

Test GetDuty

333f922

leolara marked this pull request as ready for review April 11, 2022 12:24

corverroos approved these changes Apr 11, 2022

View reviewed changes

leolara added 2 commits April 12, 2022 10:25

Merge branch 'main' into leolara/duty-query-scheduler

b8809a0

linting issues

c1af5d0

leolara added the merge when ready Indicates bulldozer bot may merge when all checks pass label Apr 12, 2022

obol-bulldozer bot merged commit 46ffd09 into main Apr 12, 2022

obol-bulldozer bot deleted the leolara/duty-query-scheduler branch April 12, 2022 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/scheduler: add duty query method #384

core/scheduler: add duty query method #384

leolara commented Apr 7, 2022 •

edited

Loading

codecov bot commented Apr 7, 2022 •

edited

Loading

corverroos Apr 7, 2022

corverroos Apr 7, 2022

leolara Apr 7, 2022

corverroos Apr 8, 2022

corverroos Apr 7, 2022

corverroos Apr 7, 2022

leolara Apr 7, 2022

corverroos Apr 7, 2022 •

edited

Loading

leolara Apr 7, 2022

leolara Apr 7, 2022

corverroos Apr 7, 2022

leolara Apr 7, 2022

corverroos Apr 7, 2022

leolara Apr 7, 2022

corverroos Apr 7, 2022

leolara commented Apr 7, 2022

leolara commented Apr 8, 2022

corverroos Apr 8, 2022

leolara commented Apr 8, 2022

corverroos Apr 8, 2022

leolara Apr 8, 2022

corverroos Apr 8, 2022

leolara Apr 8, 2022

corverroos Apr 8, 2022

corverroos Apr 8, 2022

leolara Apr 8, 2022

leolara Apr 8, 2022

corverroos Apr 8, 2022

corverroos left a comment

leolara commented Apr 8, 2022

corverroos left a comment

core/scheduler: add duty query method #384

core/scheduler: add duty query method #384

Conversation

leolara commented Apr 7, 2022 • edited Loading

codecov bot commented Apr 7, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corverroos Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leolara commented Apr 7, 2022

leolara commented Apr 8, 2022

Choose a reason for hiding this comment

leolara commented Apr 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corverroos left a comment

Choose a reason for hiding this comment

leolara commented Apr 8, 2022

corverroos left a comment

Choose a reason for hiding this comment

leolara commented Apr 7, 2022 •

edited

Loading

codecov bot commented Apr 7, 2022 •

edited

Loading

corverroos Apr 7, 2022 •

edited

Loading