lite2: check header w/ witnesses only when doing bisection (#4929)

* lite2: check header w/ witnesses only when doing bisection

Closes #4872

We don't need to check witnesses if we're doing backwards hash chain
verification. I also think we don't need to do it when sequential
verification is being used.

* lite2: require 1 witness only when verificationMode=skipping

https://github.com/tendermint/tendermint/pull/4929#pullrequestreview-423256477

we don't need witnesses when performing sequential verification (except
when primary fails)

CHANGELOG_PENDING.md

@@ -56,6 +56,7 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi
 - [evidence] [\#4839](https://github.com/tendermint/tendermint/pull/4839) Reject duplicate evidence from being proposed (@cmwaters)
 - [evidence] [\#4892](https://github.com/tendermint/tendermint/pull/4892) Remove redundant header from phantom validator evidence (@cmwaters)
 - [types] [\#4905](https://github.com/tendermint/tendermint/pull/4905) Add ValidateBasic to validator and validator set (@cmwaters)
+- [lite2] [\#4929](https://github.com/tendermint/tendermint/pull/4929) compare header w/ witnesses only when doing bisection (@melekes)
 
 ### BUG FIXES:
 

lite2/client.go

@@ -149,8 +149,9 @@ type Client struct {
 // hash does not match with the one from the header).
 //
 // Witnesses are providers, which will be used for cross-checking the primary
-// provider. At least one witness must be given. A witness can become a primary
-// iff the current primary is unavailable.
+// provider. At least one witness must be given when skipping verification is
+// used (default). A witness can become a primary iff the current primary is
+// unavailable.
 //
 // See all Option(s) for the additional configuration.
 func NewClient(
@@ -219,7 +220,7 @@ func NewClientFromTrustedStore(
 	}
 
 	// Validate the number of witnesses.
-	if len(c.witnesses) < 1 {
+	if len(c.witnesses) < 1 && c.verificationMode == skipping {
 		return nil, errNoWitnesses{}
 	}
 
@@ -605,13 +606,8 @@ func (c *Client) verifyHeader(newHeader *types.SignedHeader, newVals *types.Vali
 		c.logger.Error("Can't verify", "err", err)
 		return err
 	}
-	// 4) Compare header with other witnesses
-	if err := c.compareNewHeaderWithWitnesses(newHeader); err != nil {
-		c.logger.Error("Error when comparing new header with witnesses", "err", err)
-		return err
-	}
 
-	// 5) Once verified, save and return
+	// 4) Once verified, save and return
 	return c.updateTrustedHeaderAndVals(newHeader, newVals)
 }
 
@@ -716,6 +712,15 @@ func (c *Client) bisection(
 		case nil:
 			// Have we verified the last header
 			if depth == 0 {
+				// Compare header with the witnesses to ensure it's not a fork.
+				// More witnesses we have, more chance to notice one.
+				//
+				// CORRECTNESS ASSUMPTION: there's at least 1 correct full node
+				// (primary or one of the witnesses).
+				if err := c.compareNewHeaderWithWitnesses(newHeader); err != nil {
+					c.logger.Error("Failed to compare new header with witnesses", "err", err)
+					return err
+				}
 				return nil
 			}
 			// If not, update the lower bound to the previous upper bound