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

dkg: sync privkeylock before exiting #2257

Merged
merged 9 commits into from
Jun 1, 2023
3 changes: 2 additions & 1 deletion app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ func Run(ctx context.Context, conf Config) (err error) {
return err
}

life.RegisterStart(lifecycle.AsyncAppCtx, lifecycle.StartPrivkeyLock, lifecycle.HookFunc(lockSvc.Run))
life.RegisterStart(lifecycle.AsyncAppCtx, lifecycle.StartPrivkeyLock, lifecycle.HookFuncErr(lockSvc.Run))
life.RegisterStop(lifecycle.StopPrivkeyLock, lifecycle.HookFuncMin(lockSvc.Close))
}

if err := wireTracing(life, conf); err != nil {
Expand Down
24 changes: 18 additions & 6 deletions app/privkeylock/privkeylock.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
package privkeylock

import (
"context"
"encoding/json"
"os"
"time"
Expand Down Expand Up @@ -50,6 +49,8 @@ func New(path, command string) (Service, error) {
command: command,
path: path,
updatePeriod: updatePeriod,
quit: make(chan struct{}),
done: make(chan struct{}),
}, nil
}

Expand All @@ -58,30 +59,41 @@ type Service struct {
command string
path string
updatePeriod time.Duration
quit chan struct{} // Quit exits the Run goroutine if closed.
done chan struct{} // Done is closed when Run exits, which exits the Close goroutine.
}

// Run runs the service, updating the lock file every second and deleting it on context cancellation.
func (h Service) Run(ctx context.Context) error {
tick := time.NewTicker(h.updatePeriod)
func (s Service) Run() error {
defer close(s.done)

tick := time.NewTicker(s.updatePeriod)
defer tick.Stop()

for {
select {
case <-ctx.Done():
if err := os.Remove(h.path); err != nil {
case <-s.quit:
if err := os.Remove(s.path); err != nil {
return errors.Wrap(err, "deleting private key lock file failed")
}

return nil
case <-tick.C:
// Overwrite lockfile with new metadata
if err := writeFile(h.path, h.command, time.Now()); err != nil {
if err := writeFile(s.path, s.command, time.Now()); err != nil {
return err
}
}
}
}

// Close closes the service, waiting for the Run goroutine to exit.
// Note this will block forever if Run is not called.
func (s Service) Close() {
close(s.quit)
<-s.done
}

// metadata is the metadata stored in the lock file.
type metadata struct {
Command string `json:"command"`
Expand Down
9 changes: 2 additions & 7 deletions app/privkeylock/privkeylock_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
package privkeylock

import (
"context"
"os"
"path/filepath"
"testing"
Expand Down Expand Up @@ -36,16 +35,12 @@ func TestService(t *testing.T) {
// Delete the file so Run will create it again.
require.NoError(t, os.Remove(path))

ctx, cancel := context.WithCancel(context.Background())

var eg errgroup.Group
eg.Go(func() error {
return svc.Run(ctx) // Run will create the file.
})
eg.Go(svc.Run) // Run will create the file.

eg.Go(func() error {
assertFileExists(t, path)
cancel()
svc.Close()

return nil
})
Expand Down
25 changes: 16 additions & 9 deletions dkg/dkg.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,16 +81,23 @@ func Run(ctx context.Context, conf Config) (err error) {

ctx = log.WithTopic(ctx, "dkg")

lockSvc, err := privkeylock.New(p2p.KeyPath(conf.DataDir)+".lock", "charon dkg")
if err != nil {
return err
}

go func(ctx context.Context) {
if err := lockSvc.Run(ctx); err != nil {
log.Error(ctx, "Error locking private key file", err)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

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

// Setup private key locking.
lockSvc, err := privkeylock.New(p2p.KeyPath(conf.DataDir)+".lock", "charon dkg")
if err != nil {
return err
}
}(ctx)

// Start it async
Copy link
Contributor

Choose a reason for hiding this comment

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

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

go func() {
if err := lockSvc.Run(); err != nil {
log.Error(ctx, "Error locking private key file", err)
}
}()

// Stop it on exit.
defer lockSvc.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

version.LogInfo(ctx, "Charon DKG starting")

Expand Down
8 changes: 8 additions & 0 deletions dkg/dkg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,14 @@ func testDKG(t *testing.T, def cluster.Definition, dir string, p2pKeys []*k1.Pri
testutil.SkipIfBindErr(t, err)
require.NoError(t, err)

// check that the privkey lock file has been deleted in all nodes at the end of dkg
for i := 0; i < len(def.Operators); i++ {
lockPath := path.Join(dir, fmt.Sprintf("node%d", i), "charon-enr-private-key.lock")

_, openErr := os.Open(lockPath)
require.ErrorIs(t, openErr, os.ErrNotExist)
}

if keymanager {
// Wait until all keystores are received by the keymanager server
expectedReceives := len(def.Operators)
Expand Down