lite: modified bisection to loop (#4400)

refs #4329

As opposed to using recursion to implement the bisection method of verifying a header, which could have problems with memory allocation (especially for smaller devices), the bisection algorithm now uses a for loop.

* modified bisection to loop

* made lint changes

* made lint changes

* move note to VerifyHeader

since it applies both for sequence and bisection

* test bisection jumps to header signed by 1/3+

 of old validator set

* update labels in debug log calls

* copy tc

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

lite2/client.go

@@ -294,8 +294,8 @@ func (c *Client) checkTrustedHeaderUsingOptions(options TrustOptions) error {
 	case options.Height < c.trustedHeader.Height:
 		c.logger.Info("Client initialized with old header (trusted is more recent)",
 			"old", options.Height,
-			"trusted", c.trustedHeader.Height,
-			"trusted-hash", hash2str(c.trustedHeader.Hash()))
+			"trustedHeight", c.trustedHeader.Height,
+			"trustedHash", hash2str(c.trustedHeader.Hash()))
 
 		action := fmt.Sprintf(
 			"Rollback to %d (%X)? Note this will remove newer headers up to %d (%X)",
@@ -373,14 +373,14 @@ func (c *Client) initializeWithTrustOptions(options TrustOptions) error {
 	}
 
 	// 3) Fetch and verify the next vals (verification happens in
-	// updateTrustedHeaderAndVals).
+	// updateTrustedHeaderAndNextVals).
 	nextVals, err := c.validatorSetFromPrimary(options.Height + 1)
 	if err != nil {
 		return err
 	}
 
 	// 4) Persist both of them and continue.
-	return c.updateTrustedHeaderAndVals(h, nextVals)
+	return c.updateTrustedHeaderAndNextVals(h, nextVals)
 }
 
 // Start starts two processes: 1) auto updating 2) removing outdated headers.
@@ -560,6 +560,10 @@ func (c *Client) VerifyHeaderAtHeight(height int64, now time.Time) (*types.Signe
 // If, at any moment, SignedHeader or ValidatorSet are not found by the primary
 // provider, provider.ErrSignedHeaderNotFound /
 // provider.ErrValidatorSetNotFound error is returned.
+//
+// NOTE: although newVals is entered as input, trustedStore will only store the
+// validator set at height newHeader.Height+1 (i.e.
+// newHeader.NextValidatorsHash).
 func (c *Client) VerifyHeader(newHeader *types.SignedHeader, newVals *types.ValidatorSet, now time.Time) error {
 	c.logger.Info("VerifyHeader", "height", newHeader.Height, "hash", hash2str(newHeader.Hash()),
 		"vals", hash2str(newVals.Hash()))
@@ -592,7 +596,7 @@ func (c *Client) VerifyHeader(newHeader *types.SignedHeader, newVals *types.Vali
 	if err != nil {
 		return err
 	}
-	return c.updateTrustedHeaderAndVals(newHeader, nextVals)
+	return c.updateTrustedHeaderAndNextVals(newHeader, nextVals)
 }
 
 // Primary returns the primary provider.
@@ -678,10 +682,10 @@ func (c *Client) sequence(
 		}
 
 		c.logger.Debug("Verify newHeader against trustedHeader",
-			"lastHeight", c.trustedHeader.Height,
-			"lastHash", c.trustedHeader.Hash(),
+			"trustedHeight", c.trustedHeader.Height,
+			"trustedHash", hash2str(c.trustedHeader.Hash()),
 			"newHeight", interimHeader.Height,
-			"newHash", interimHeader.Hash())
+			"newHash", hash2str(interimHeader.Hash()))
 		err = VerifyAdjacent(c.chainID, trustedHeader, interimHeader, trustedNextVals,
 			c.trustingPeriod, now)
 		if err != nil {
@@ -697,7 +701,7 @@ func (c *Client) sequence(
 				return errors.Wrapf(err, "failed to obtain the vals #%d", height+1)
 			}
 		}
-		err = c.updateTrustedHeaderAndVals(interimHeader, interimNextVals)
+		err = c.updateTrustedHeaderAndNextVals(interimHeader, interimNextVals)
 		if err != nil {
 			return errors.Wrapf(err, "failed to update trusted state #%d", height)
 		}
@@ -710,62 +714,55 @@ func (c *Client) sequence(
 
 // see VerifyHeader
 func (c *Client) bisection(
-	lastHeader *types.SignedHeader,
-	lastVals *types.ValidatorSet,
-	newHeader *types.SignedHeader,
-	newVals *types.ValidatorSet,
+	trustedHeader *types.SignedHeader, // height h
+	trustedNextVals *types.ValidatorSet, // height h + 1
+	newHeader *types.SignedHeader, // height g
+	newVals *types.ValidatorSet, // height g
 	now time.Time) error {
 
-	c.logger.Debug("Verify newHeader against lastHeader",
-		"lastHeight", lastHeader.Height,
-		"lastHash", lastHeader.Hash(),
-		"newHeight", newHeader.Height,
-		"newHash", newHeader.Hash())
-	err := Verify(c.chainID, lastHeader, lastVals, newHeader, newVals, c.trustingPeriod, now, c.trustLevel)
-	switch err.(type) {
-	case nil:
-		return nil
-	case ErrNewValSetCantBeTrusted:
-		// continue bisection
-	default:
-		return errors.Wrapf(err, "failed to verify the header #%d", newHeader.Height)
-	}
+	interimVals := newVals
+	interimHeader := newHeader
 
-	pivot := (c.trustedHeader.Height + newHeader.Header.Height) / 2
-	pivotHeader, pivotVals, err := c.fetchHeaderAndValsAtHeight(pivot)
-	if err != nil {
-		return err
-	}
+	for trustedHeader.Height < newHeader.Height {
+		c.logger.Debug("Verify newHeader against trustedHeader",
+			"trustedHeight", trustedHeader.Height,
+			"trustedHash", hash2str(trustedHeader.Hash()),
+			"newHeight", newHeader.Height,
+			"newHash", hash2str(newHeader.Hash()))
+		err := Verify(c.chainID, trustedHeader, trustedNextVals, interimHeader, interimVals, c.trustingPeriod, now,
+			c.trustLevel)
+		switch err.(type) {
+		case nil:
+			// Update the lower bound to the previous upper bound
+			trustedHeader = interimHeader
+			trustedNextVals, err = c.validatorSetFromPrimary(interimHeader.Height + 1)
+			if err != nil {
+				return err
+			}
+			if !bytes.Equal(trustedHeader.NextValidatorsHash, trustedNextVals.Hash()) {
+				return errors.Errorf("expected next validator's hash %X, but got %X (height #%d)",
+					trustedHeader.NextValidatorsHash,
+					trustedNextVals.Hash(),
+					trustedHeader.Height)
+			}
 
-	// left branch
-	{
-		err := c.bisection(lastHeader, lastVals, pivotHeader, pivotVals, now)
-		if err != nil {
-			return errors.Wrapf(err, "bisection of #%d and #%d", lastHeader.Height, pivot)
-		}
-	}
+			err = c.updateTrustedHeaderAndNextVals(trustedHeader, trustedNextVals)
+			if err != nil {
+				return err
+			}
 
-	// right branch
-	{
-		nextVals, err := c.validatorSetFromPrimary(pivot + 1)
-		if err != nil {
-			return errors.Wrapf(err, "failed to obtain the vals #%d", pivot+1)
-		}
-		if !bytes.Equal(pivotHeader.NextValidatorsHash, nextVals.Hash()) {
-			return errors.Errorf("expected next validator's hash %X, but got %X (height #%d)",
-				pivotHeader.NextValidatorsHash,
-				nextVals.Hash(),
-				pivot)
-		}
+			// Update the upper bound to the untrustedHeader
+			interimHeader, interimVals = newHeader, newVals
 
-		err = c.updateTrustedHeaderAndVals(pivotHeader, nextVals)
-		if err != nil {
-			return errors.Wrapf(err, "failed to update trusted state #%d", pivot)
-		}
+		case ErrNewValSetCantBeTrusted:
+			pivotHeight := (interimHeader.Height + trustedHeader.Height) / 2
+			interimHeader, interimVals, err = c.fetchHeaderAndValsAtHeight(pivotHeight)
+			if err != nil {
+				return err
+			}
 
-		err = c.bisection(pivotHeader, nextVals, newHeader, newVals, now)
-		if err != nil {
-			return errors.Wrapf(err, "bisection of #%d and #%d", pivot, newHeader.Height)
+		default:
+			return errors.Wrapf(err, "failed to verify the header #%d", newHeader.Height)
 		}
 	}
 
@@ -773,7 +770,7 @@ func (c *Client) bisection(
 }
 
 // persist header and next validators to trustedStore.
-func (c *Client) updateTrustedHeaderAndVals(h *types.SignedHeader, nextVals *types.ValidatorSet) error {
+func (c *Client) updateTrustedHeaderAndNextVals(h *types.SignedHeader, nextVals *types.ValidatorSet) error {
 	if !bytes.Equal(h.NextValidatorsHash, nextVals.Hash()) {
 		return errors.Errorf("expected next validator's hash %X, but got %X", h.NextValidatorsHash, nextVals.Hash())
 	}

lite2/client_test.go

@@ -27,8 +27,10 @@ var (
 	bTime, _ = time.Parse(time.RFC3339, "2006-01-02T15:04:05Z")
 	h1       = keys.GenSignedHeader(chainID, 1, bTime, nil, vals, vals,
 		[]byte("app_hash"), []byte("cons_hash"), []byte("results_hash"), 0, len(keys))
+	// 3/3 signed
 	h2 = keys.GenSignedHeaderLastBlockID(chainID, 2, bTime.Add(30*time.Minute), nil, vals, vals,
 		[]byte("app_hash"), []byte("cons_hash"), []byte("results_hash"), 0, len(keys), types.BlockID{Hash: h1.Hash()})
+	// 3/3 signed
 	h3 = keys.GenSignedHeaderLastBlockID(chainID, 3, bTime.Add(1*time.Hour), nil, vals, vals,
 		[]byte("app_hash"), []byte("cons_hash"), []byte("results_hash"), 0, len(keys), types.BlockID{Hash: h2.Hash()})
 	trustPeriod  = 4 * time.Hour
@@ -56,13 +58,14 @@ var (
 
 func TestClient_SequentialVerification(t *testing.T) {
 	testCases := []struct {
+		name         string
 		otherHeaders map[int64]*types.SignedHeader // all except ^
 		vals         map[int64]*types.ValidatorSet
 		initErr      bool
 		verifyErr    bool
 	}{
-		// good
 		{
+			"good",
 			map[int64]*types.SignedHeader{
 				// trusted header
 				1: h1,
@@ -80,8 +83,8 @@ func TestClient_SequentialVerification(t *testing.T) {
 			false,
 			false,
 		},
-		// bad: different first header
 		{
+			"bad: different first header",
 			map[int64]*types.SignedHeader{
 				// different header
 				1: keys.GenSignedHeader(chainID, 1, bTime.Add(1*time.Hour), nil, vals, vals,
@@ -93,8 +96,8 @@ func TestClient_SequentialVerification(t *testing.T) {
 			true,
 			false,
 		},
-		// bad: 1/3 signed interim header
 		{
+			"bad: 1/3 signed interim header",
 			map[int64]*types.SignedHeader{
 				// trusted header
 				1: h1,
@@ -114,8 +117,8 @@ func TestClient_SequentialVerification(t *testing.T) {
 			false,
 			true,
 		},
-		// bad: 1/3 signed last header
 		{
+			"bad: 1/3 signed last header",
 			map[int64]*types.SignedHeader{
 				// trusted header
 				1: h1,
@@ -138,39 +141,42 @@ func TestClient_SequentialVerification(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		c, err := NewClient(
-			chainID,
-			trustOptions,
-			mockp.New(
-				chainID,
-				tc.otherHeaders,
-				tc.vals,
-			),
-			[]provider.Provider{mockp.New(
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			c, err := NewClient(
 				chainID,
-				tc.otherHeaders,
-				tc.vals,
-			)},
-			dbs.New(dbm.NewMemDB(), chainID),
-			SequentialVerification(),
-		)
+				trustOptions,
+				mockp.New(
+					chainID,
+					tc.otherHeaders,
+					tc.vals,
+				),
+				[]provider.Provider{mockp.New(
+					chainID,
+					tc.otherHeaders,
+					tc.vals,
+				)},
+				dbs.New(dbm.NewMemDB(), chainID),
+				SequentialVerification(),
+			)
+
+			if tc.initErr {
+				require.Error(t, err)
+				return
+			}
 
-		if tc.initErr {
-			require.Error(t, err)
-			continue
-		} else {
 			require.NoError(t, err)
-		}
-		err = c.Start()
-		require.NoError(t, err)
-		defer c.Stop()
-
-		_, err = c.VerifyHeaderAtHeight(3, bTime.Add(3*time.Hour))
-		if tc.verifyErr {
-			assert.Error(t, err)
-		} else {
-			assert.NoError(t, err)
-		}
+			err = c.Start()
+			require.NoError(t, err)
+			defer c.Stop()
+
+			_, err = c.VerifyHeaderAtHeight(3, bTime.Add(3*time.Hour))
+			if tc.verifyErr {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
 	}
 }
 
@@ -179,12 +185,19 @@ func TestClient_SkippingVerification(t *testing.T) {
 	newKeys := genPrivKeys(4)
 	newVals := newKeys.ToValidators(10, 1)
 
+	// 1/3+ of vals, 2/3- of newVals
+	transitKeys := keys.Extend(3)
+	transitVals := transitKeys.ToValidators(10, 1)
+
 	testCases := []struct {
+		name         string
 		otherHeaders map[int64]*types.SignedHeader // all except ^
 		vals         map[int64]*types.ValidatorSet
+		initErr      bool
+		verifyErr    bool
 	}{
-		// good
 		{
+			"good",
 			map[int64]*types.SignedHeader{
 				// trusted header
 				1: h1,
@@ -197,9 +210,28 @@ func TestClient_SkippingVerification(t *testing.T) {
 				3: vals,
 				4: vals,
 			},
+			false,
+			false,
 		},
-		// good, val set changes 100% at height 2
 		{
+			"good, but val set changes by 2/3 (1/3 of vals is still present)",
+			map[int64]*types.SignedHeader{
+				// trusted header
+				1: h1,
+				3: transitKeys.GenSignedHeader(chainID, 3, bTime.Add(2*time.Hour), nil, transitVals, transitVals,
+					[]byte("app_hash"), []byte("cons_hash"), []byte("results_hash"), 0, len(transitKeys)),
+			},
+			map[int64]*types.ValidatorSet{
+				1: vals,
+				2: vals,
+				3: transitVals,
+				4: transitVals,
+			},
+			false,
+			false,
+		},
+		{
+			"good, but val set changes 100% at height 2",
 			map[int64]*types.SignedHeader{
 				// trusted header
 				1: h1,
@@ -216,33 +248,68 @@ func TestClient_SkippingVerification(t *testing.T) {
 				3: newVals,
 				4: newVals,
 			},
+			false,
+			false,
+		},
+		{
+			"bad: last header signed by newVals, interim header has no signers",
+			map[int64]*types.SignedHeader{
+				// trusted header
+				1: h1,
+				// last header (0/4 of the original val set signed)
+				2: keys.GenSignedHeader(chainID, 2, bTime.Add(1*time.Hour), nil, vals, newVals,
+					[]byte("app_hash"), []byte("cons_hash"), []byte("results_hash"), 0, 0),
+				// last header (0/4 of the original val set signed)
+				3: newKeys.GenSignedHeader(chainID, 3, bTime.Add(2*time.Hour), nil, newVals, newVals,
+					[]byte("app_hash"), []byte("cons_hash"), []byte("results_hash"), 0, len(newKeys)),
+			},
+			map[int64]*types.ValidatorSet{
+				1: vals,
+				2: vals,
+				3: newVals,
+				4: newVals,
+			},
+			false,
+			true,
 		},
 	}
 
 	for _, tc := range testCases {
-		c, err := NewClient(
-			chainID,
-			trustOptions,
-			mockp.New(
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			c, err := NewClient(
 				chainID,
-				tc.otherHeaders,
-				tc.vals,
-			),
-			[]provider.Provider{mockp.New(
-				chainID,
-				tc.otherHeaders,
-				tc.vals,
-			)},
-			dbs.New(dbm.NewMemDB(), chainID),
-			SkippingVerification(DefaultTrustLevel),
-		)
-		require.NoError(t, err)
-		err = c.Start()
-		require.NoError(t, err)
-		defer c.Stop()
+				trustOptions,
+				mockp.New(
+					chainID,
+					tc.otherHeaders,
+					tc.vals,
+				),
+				[]provider.Provider{mockp.New(
+					chainID,
+					tc.otherHeaders,
+					tc.vals,
+				)},
+				dbs.New(dbm.NewMemDB(), chainID),
+				SkippingVerification(DefaultTrustLevel),
+			)
+			if tc.initErr {
+				require.Error(t, err)
+				return
+			}
 
-		_, err = c.VerifyHeaderAtHeight(3, bTime.Add(3*time.Hour))
-		assert.NoError(t, err)
+			require.NoError(t, err)
+			err = c.Start()
+			require.NoError(t, err)
+			defer c.Stop()
+
+			_, err = c.VerifyHeaderAtHeight(3, bTime.Add(3*time.Hour))
+			if tc.verifyErr {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
 	}
 }
 

lite2/test_helpers.go

@@ -36,11 +36,11 @@ func genPrivKeys(n int) privKeys {
 // 	return res
 // }
 
-// // Extend adds n more keys (to remove, just take a slice).
-// func (pkz privKeys) Extend(n int) privKeys {
-// 	extra := genPrivKeys(n)
-// 	return append(pkz, extra...)
-// }
+// Extend adds n more keys (to remove, just take a slice).
+func (pkz privKeys) Extend(n int) privKeys {
+	extra := genPrivKeys(n)
+	return append(pkz, extra...)
+}
 
 // // GenSecpPrivKeys produces an array of secp256k1 private keys to generate commits.
 // func GenSecpPrivKeys(n int) privKeys {