dkg: sync privkeylock before exiting

[gsora]

Block dkg exit until privkeylock.Run() enclosing goroutine exits, ensuring that the private key lock file is always deleted.

Run() is supposed to be executed in a goroutine, and since we can't control the scheduler it might be scheduled after the main one exits, leading to the ctx.Done() code path to sometimes never be executed.

category: bug
ticket: #2258

[codecov]

Codecov Report

Patch coverage: 62.50% and project coverage change: +0.03 🎉

Comparison is base (f59769f) 53.83% compared to head (8fb0237) 53.86%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2257      +/-   ##
==========================================
+ Coverage   53.83%   53.86%   +0.03%     
==========================================
  Files         188      188              
  Lines       25513    25524      +11     
==========================================
+ Hits        13734    13749      +15     
  Misses      10092    10092              
+ Partials     1687     1683       -4     

Impacted Files	Coverage Δ
app/app.go	6.96% <0.00%> (-0.02%)	⬇️
dkg/dkg.go	57.60% <50.00%> (+0.17%)	⬆️
app/privkeylock/privkeylock.go	60.34% <83.33%> (+5.44%)	⬆️

... and 11 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.

[xenowits]

@gsora hear me out on this:

• Instead of using a sync.WaitGroup, let's just add a quit channel:

// Service is a private key locking service.
type Service struct {
         ...
	quit            chan struct{}   // New
}

• In the Stop() method, we simply close the quit chan:

func (s *Service) Done() {
	close(s.quit)
}

• We add another case in the select block:

case <-ctx.Done():
	cleanup = true
case <-h.quit:
	cleanup = true

• Break out of the loop if cleanup == true
• Finally, do the delete file stuff

It essentially does the same thing as the waitgroup but the quit chan pattern is very common in the charon codebase (see scheduler, aggsigdb etc). The DKG process also doesn't have to wait for privkeylock deletion to complete, since it will eventually happen.

Wdyt?

[gsora]

@xenowits

The DKG process also doesn't have to wait for privkeylock deletion to complete, since it will eventually happen.

That is not correct: since the deletion happens in a different goroutine you are not sure if it'll be scheduled.

I just implemented your solution as a test, and while it does work more often than just relying on defer cancel(), it sometimes doesn't work.

The idea here is having the main goroutine waiting until the child one has actually deleted the file.

[gsora]

I did implement a channel-based solution in a call with @dB2510: it worked fine.

I fell back on sync.WorkGroup because I wanted to not think about side-effects and rely on a piece of stdlib that has been build to do exactly this: synchronize different goroutines.

A thing we could do to make it more obvious is spawning the background goroutine inside privkeylock.Run() instead of leaving the caller to do that.

[gsora]

Actually thinking about loud, maybe this fix shouldn't be in privkeylock, rather it must just concern dkg.

Implementing a different solution as we speak.

[xenowits]

LGTM! I think this is a more simpler approach 👍

[dB2510]

nit: move this line before go func, after we initialise lockSvc

[dB2510]

@corverroos why do we need a separate block for this?

[dB2510]

Suggested change

	// Start it async
	// Start private key lock service async.