Fix bug on reshare process (#898)

* fix bug on reshare process
* take beacon id from group file on resharing command
* add an extra check on reshare cmd on CLI
* add one extra check to deny use of period flag on reshare cmd

cmd/drand-cli/control.go

@@ -223,6 +223,10 @@ func reshareCmd(c *cli.Context) error {
 		return err
 	}
 
+	if c.IsSet(periodFlag.Name) {
+		return fmt.Errorf("%s flag is not allowed on resharing", periodFlag.Name)
+	}
+
 	if !c.IsSet(connectFlag.Name) {
 		return fmt.Errorf("need to the address of the coordinator to create the group file")
 	}
@@ -234,6 +238,8 @@ func reshareCmd(c *cli.Context) error {
 		return fmt.Errorf("could not create client: %v", err)
 	}
 
+	beaconID := c.String(beaconIDFlag.Name)
+
 	// resharing case needs the previous group
 	var oldPath string
 	if c.IsSet(transitionFlag.Name) {
@@ -244,10 +250,15 @@ func reshareCmd(c *cli.Context) error {
 		if err := key.Load(c.String(oldGroupFlag.Name), oldGroup); err != nil {
 			return fmt.Errorf("could not load drand from path: %s", err)
 		}
+
 		oldPath = c.String(oldGroupFlag.Name)
-	}
 
-	beaconID := c.String(beaconIDFlag.Name)
+		if beaconID != "" {
+			return fmt.Errorf("beacon id flag is not required when using --%s", oldGroupFlag.Name)
+		}
+
+		beaconID = oldGroup.ID
+	}
 
 	fmt.Fprintf(output, "Participating to the resharing. Beacon ID: [%s] \n", beaconID)
 
@@ -268,6 +279,10 @@ func leadReshareCmd(c *cli.Context) error {
 		return err
 	}
 
+	if c.IsSet(periodFlag.Name) {
+		return fmt.Errorf("%s flag is not allowed on resharing", periodFlag.Name)
+	}
+
 	if !c.IsSet(thresholdFlag.Name) || !c.IsSet(shareNodeFlag.Name) {
 		return fmt.Errorf("leader needs to specify --nodes and --threshold for sharing")
 	}
@@ -279,6 +294,8 @@ func leadReshareCmd(c *cli.Context) error {
 		return fmt.Errorf("could not create client: %v", err)
 	}
 
+	beaconID := c.String(beaconIDFlag.Name)
+
 	// resharing case needs the previous group
 	var oldPath string
 	if c.IsSet(transitionFlag.Name) {
@@ -290,6 +307,12 @@ func leadReshareCmd(c *cli.Context) error {
 			return fmt.Errorf("could not load drand from path: %s", err)
 		}
 		oldPath = c.String(oldGroupFlag.Name)
+
+		if beaconID != "" {
+			return fmt.Errorf("beacon id flag is not required when using --%s", oldGroupFlag.Name)
+		}
+
+		beaconID = oldGroup.ID
 	}
 
 	offset := int(core.DefaultResharingOffset.Seconds())
@@ -304,8 +327,6 @@ func leadReshareCmd(c *cli.Context) error {
 		}
 	}
 
-	beaconID := c.String(beaconIDFlag.Name)
-
 	fmt.Fprintf(output, "Initiating the resharing as a leader. Beacon ID: [%s] \n", beaconID)
 	groupP, shareErr := ctrlClient.InitReshareLeader(nodes, args.threshold, args.timeout,
 		catchupPeriod, args.secret, oldPath, offset, beaconID)

common/beacon.go

@@ -27,3 +27,17 @@ func GetBeaconIDFromEnv() string {
 func IsDefaultBeaconID(beaconID string) bool {
 	return beaconID == DefaultBeaconID || beaconID == ""
 }
+
+// CompareBeaconIDs indicates if two different beacon ids are equivalent or not.
+// It handles default values too.
+func CompareBeaconIDs(id1, id2 string) bool {
+	if IsDefaultBeaconID(id1) && IsDefaultBeaconID(id2) {
+		return true
+	}
+
+	if id1 != id2 {
+		return false
+	}
+
+	return true
+}

common/beacon_test.go

@@ -0,0 +1,18 @@
+package common
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestCompareBeaconIDs(t *testing.T) {
+	require.True(t, CompareBeaconIDs("", ""))
+	require.True(t, CompareBeaconIDs("", DefaultBeaconID))
+	require.True(t, CompareBeaconIDs(DefaultBeaconID, ""))
+	require.True(t, CompareBeaconIDs(DefaultBeaconID, DefaultBeaconID))
+	require.False(t, CompareBeaconIDs("beacon_5s", DefaultBeaconID))
+	require.False(t, CompareBeaconIDs("beacon_5s", ""))
+	require.False(t, CompareBeaconIDs(DefaultBeaconID, "beacon_5s"))
+	require.False(t, CompareBeaconIDs("", "beacon_5s"))
+}

core/drand_beacon_control.go

@@ -104,8 +104,13 @@ func (bp *BeaconProcess) InitReshare(c context.Context, in *drand.InitResharePac
 		return nil, err
 	}
 
+	rcvBeaconID := in.GetMetadata().GetBeaconID()
 	beaconID := oldGroup.ID
 
+	if !commonutils.CompareBeaconIDs(rcvBeaconID, beaconID) {
+		return nil, fmt.Errorf("drand: invalid setup configuration: beacon id on flag is different to beacon id on group file")
+	}
+
 	if !in.GetInfo().GetLeader() {
 		bp.log.Infow("", "beacon_id", beaconID, "init_reshare", "begin", "leader", false)
 		return bp.setupAutomaticResharing(c, oldGroup, in)

core/drand_daemon_helper.go

@@ -19,10 +19,15 @@ func (dd *DrandDaemon) readBeaconID(metadata *protoCommon.Metadata) (string, err
 		dd.state.Unlock()
 
 		if isChainHashFound {
+			// check if rcv beacon id on request points to a different id obtained from chain hash
 			if rcvBeaconID != "" && rcvBeaconID != beaconIDByHash {
 				return "", fmt.Errorf("invalid chain hash")
 			}
+
 			rcvBeaconID = beaconIDByHash
+
+			// set beacon id found from chain hash on message to make it available for everyone
+			metadata.BeaconID = beaconIDByHash
 		}
 	}
 

test/docker/utils/reshareBeacon.sh

@@ -8,7 +8,7 @@
 
 ## Default beacon
 # Start leader
-nohup docker exec -u drand drand_0 /bin/sh -c 'drand share --transition --leader --nodes 5 --threshold 4 --period "5s"' &
+nohup docker exec -u drand drand_0 /bin/sh -c 'drand share --transition --leader --nodes 5 --threshold 4' &
 
 sleep 3s
 
@@ -25,12 +25,12 @@ sleep 10s
 nohup docker exec -u drand drand_4 /bin/sh -c 'drand generate-keypair --tls-disable --id test_beacon "drand_4:8480"' &
 
 # Start leader
-nohup docker exec -u drand drand_0 /bin/sh -c 'drand share --transition --leader --nodes 5 --threshold 4 --period "60s" --id test_beacon' &
+nohup docker exec -u drand drand_0 /bin/sh -c 'drand share --transition --leader --nodes 5 --threshold 4 --id test_beacon' &
 
 sleep 3s
 
 # Start the rest of the nodes
 nohup docker exec -u drand drand_2 /bin/sh -c 'drand share --transition  --connect drand_0:8080 --tls-disable --id test_beacon' &
 nohup docker exec -u drand drand_1 /bin/sh -c 'drand share --transition  --connect drand_0:8080 --tls-disable --id test_beacon' &
 nohup docker exec -u drand drand_3 /bin/sh -c 'drand share --transition  --connect drand_0:8080 --tls-disable --id test_beacon' &
-nohup docker exec -u drand drand_4 /bin/sh -c 'drand share --connect drand_0:8080 --from ./data/drand/.drand/multibeacon/test_beacon/groups/drand_group.toml --tls-disable --id test_beacon' &
+nohup docker exec -u drand drand_4 /bin/sh -c 'drand share --connect drand_0:8080 --from ./data/drand/.drand/multibeacon/test_beacon/groups/drand_group.toml --tls-disable' &