From a08316f16a0f9b5991c60fa329bd04a328f2bdef Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Wed, 15 Jul 2020 15:44:30 +0400 Subject: [PATCH] =?UTF-8?q?light:=20use=20bisection=20(not=20VerifyCommitT?= =?UTF-8?q?rusting)=20when=20verifying=20a=20head=E2=80=A6=20(#5119)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #4934 * light: do not compare trusted header w/ witnesses we don't have trusted state to bisect from * check header before checking height otherwise you can get nil panic --- light/client.go | 219 ++++++++++++++++++++++++++++--------------- light/client_test.go | 29 +----- light/errors.go | 5 +- 3 files changed, 153 insertions(+), 100 deletions(-) diff --git a/light/client.go b/light/client.go index 92593d153..f741f1bee 100644 --- a/light/client.go +++ b/light/client.go @@ -348,8 +348,7 @@ func (c *Client) checkTrustedHeaderUsingOptions(options TrustOptions) error { } // initializeWithTrustOptions fetches the weakly-trusted header and vals from -// primary provider. The header is cross-checked with witnesses for additional -// security. +// primary provider. func (c *Client) initializeWithTrustOptions(options TrustOptions) error { // 1) Fetch and verify the header. h, err := c.signedHeaderFromPrimary(options.Height) @@ -368,11 +367,6 @@ func (c *Client) initializeWithTrustOptions(options TrustOptions) error { return fmt.Errorf("expected header's hash %X, but got %X", options.Hash, h.Hash()) } - err = c.compareNewHeaderWithWitnesses(h) - if err != nil { - return err - } - // 2) Fetch and verify the vals. vals, err := c.validatorSetFromPrimary(options.Height) if err != nil { @@ -493,7 +487,7 @@ func (c *Client) VerifyHeaderAtHeight(height int64, now time.Time) (*types.Signe } // Request the header and the vals. - newHeader, newVals, err := c.fetchHeaderAndValsAtHeight(height) + newHeader, newVals, err := c.signedHeaderAndValSetFromPrimary(height) if err != nil { return nil, err } @@ -561,7 +555,7 @@ func (c *Client) verifyHeader(newHeader *types.SignedHeader, newVals *types.Vali case sequential: err = c.sequence(c.latestTrustedHeader, newHeader, newVals, now) case skipping: - err = c.bisection(c.latestTrustedHeader, c.latestTrustedVals, newHeader, newVals, now) + err = c.bisectionAgainstPrimary(c.latestTrustedHeader, c.latestTrustedVals, newHeader, newVals, now) default: panic(fmt.Sprintf("Unknown verification mode: %b", c.verificationMode)) } @@ -600,7 +594,7 @@ func (c *Client) verifyHeader(newHeader *types.SignedHeader, newVals *types.Vali if err != nil { return fmt.Errorf("can't get validator set at height %d: %w", closestHeader.Height, err) } - err = c.bisection(closestHeader, closestValidatorSet, newHeader, newVals, now) + err = c.bisectionAgainstPrimary(closestHeader, closestValidatorSet, newHeader, newVals, now) } } } @@ -634,7 +628,7 @@ func (c *Client) sequence( if height == newHeader.Height { // last header interimHeader, interimVals = newHeader, newVals } else { // intermediate headers - interimHeader, interimVals, err = c.fetchHeaderAndValsAtHeight(height) + interimHeader, interimVals, err = c.signedHeaderAndValSetFromPrimary(height) if err != nil { return err } @@ -659,7 +653,8 @@ func (c *Client) sequence( replaceErr := c.replacePrimaryProvider() if replaceErr != nil { c.logger.Error("Can't replace primary", "err", replaceErr) - return fmt.Errorf("%v. Tried to replace primary but: %w", err.Error(), replaceErr) + // return original error + return err } // attempt to verify header again height-- @@ -677,11 +672,14 @@ func (c *Client) sequence( } // see VerifyHeader -// Bisection finds the middle header between a trusted and new header, reiterating the action until it -// verifies a header. A cache of headers requested by the primary is kept such that when a -// verification is made, and the light client tries again to verify the new header in the middle, -// the light client does not need to ask for all the same headers again. +// +// Bisection finds the middle header between a trusted and new header, +// reiterating the action until it verifies a header. A cache of headers +// requested from source is kept such that when a verification is made, and the +// light client tries again to verify the new header in the middle, the light +// client does not need to ask for all the same headers again. func (c *Client) bisection( + source provider.Provider, initiallyTrustedHeader *types.SignedHeader, initiallyTrustedVals *types.ValidatorSet, newHeader *types.SignedHeader, @@ -714,15 +712,6 @@ 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 @@ -737,7 +726,7 @@ func (c *Client) bisection( if depth == len(headerCache)-1 { pivotHeight := trustedHeader.Height + (headerCache[depth].sh.Height-trustedHeader. Height)*bisectionNumerator/bisectionDenominator - interimHeader, interimVals, err := c.fetchHeaderAndValsAtHeight(pivotHeight) + interimHeader, interimVals, err := c.signedHeaderAndValSetFrom(pivotHeight, source) if err != nil { return err } @@ -745,28 +734,6 @@ func (c *Client) bisection( } depth++ - case ErrInvalidHeader: - c.logger.Error("primary sent invalid header -> replacing", "err", err) - replaceErr := c.replacePrimaryProvider() - if replaceErr != nil { - c.logger.Error("Can't replace primary", "err", replaceErr) - // return original error - return fmt.Errorf("verify non adjacent from #%d to #%d failed: %w", - trustedHeader.Height, headerCache[depth].sh.Height, err) - } - newProviderHeader, newProviderVals, err := c.fetchHeaderAndValsAtHeight(newHeader.Height) - if err != nil { - return err - } - if !bytes.Equal(newProviderHeader.Hash(), newHeader.Hash()) || !bytes.Equal(newProviderVals.Hash(), newVals.Hash()) { - err := fmt.Errorf("replacement provider has a different header: %X and/or vals: %X at height: %d"+ - "to the one being verified", newProviderHeader.Hash(), newProviderVals.Hash(), newHeader.Height) - return fmt.Errorf("verify non adjacent from #%d to #%d failed: %w", - trustedHeader.Height, headerCache[depth].sh.Height, err) - } - // attempt to verify the header again - return c.bisection(initiallyTrustedHeader, initiallyTrustedVals, newHeader, newVals, now) - default: return fmt.Errorf("verify non adjacent from #%d to #%d failed: %w", trustedHeader.Height, headerCache[depth].sh.Height, err) @@ -774,6 +741,70 @@ func (c *Client) bisection( } } +// bisectionAgainstPrimary does bisection plus it compares new header with +// witnesses and replaces primary if it does not respond after +// MaxRetryAttempts. +func (c *Client) bisectionAgainstPrimary( + initiallyTrustedHeader *types.SignedHeader, + initiallyTrustedVals *types.ValidatorSet, + newHeader *types.SignedHeader, + newVals *types.ValidatorSet, + now time.Time) error { + + err := c.bisection(c.primary, initiallyTrustedHeader, initiallyTrustedVals, newHeader, newVals, now) + + switch errors.Unwrap(err).(type) { + case ErrInvalidHeader: + c.logger.Error("primary sent invalid header -> replacing", "err", err) + replaceErr := c.replacePrimaryProvider() + if replaceErr != nil { + c.logger.Error("Can't replace primary", "err", replaceErr) + // return original error + return err + } + + replacementHeader, replacementVals, fErr := c.signedHeaderAndValSetFromPrimary(newHeader.Height) + if fErr != nil { + c.logger.Error("Can't fetch header/vals from primary", "err", fErr) + // return original error + return err + } + + if !bytes.Equal(replacementHeader.Hash(), newHeader.Hash()) || + !bytes.Equal(replacementVals.Hash(), newVals.Hash()) { + c.logger.Error("Replacement provider has a different header/vals", + "newHash", newHeader.Hash(), + "newVals", newVals.Hash(), + "replHash", replacementHeader.Hash(), + "replVals", replacementVals.Hash()) + // return original error + return err + } + + // attempt to verify the header again + return c.bisectionAgainstPrimary( + initiallyTrustedHeader, + initiallyTrustedVals, + replacementHeader, + replacementVals, + now, + ) + case nil: + // 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 cmpErr := c.compareNewHeaderWithWitnesses(newHeader, now); cmpErr != nil { + return cmpErr + } + default: + return err + } + + return nil +} + // LastTrustedHeight returns a last trusted height. -1 and nil are returned if // there are no trusted headers. // @@ -879,20 +910,60 @@ func (c *Client) updateTrustedHeaderAndVals(h *types.SignedHeader, vals *types.V return nil } -// fetch header and validators for the given height (0 - latest) from primary -// provider. -func (c *Client) fetchHeaderAndValsAtHeight(height int64) (*types.SignedHeader, *types.ValidatorSet, error) { +// 0 - latest header +// Note it swaps the primary with a witness if primary is not responding after +// MaxRetryAttempts. +func (c *Client) signedHeaderAndValSetFromPrimary(height int64) (*types.SignedHeader, *types.ValidatorSet, error) { h, err := c.signedHeaderFromPrimary(height) if err != nil { - return nil, nil, fmt.Errorf("failed to obtain the header #%d: %w", height, err) + return nil, nil, fmt.Errorf("can't fetch header: %w", err) } vals, err := c.validatorSetFromPrimary(height) if err != nil { - return nil, nil, fmt.Errorf("failed to obtain the vals #%d: %w", height, err) + return nil, nil, fmt.Errorf("can't fetch vals: %w", err) } return h, vals, nil } +// 0 - latest header +// Note it does not do retries nor swapping. +func (c *Client) signedHeaderAndValSetFromWitness(height int64, + witness provider.Provider) (*types.SignedHeader, *types.ValidatorSet, *errBadWitness) { + + h, err := witness.SignedHeader(height) + if err != nil { + return nil, nil, &errBadWitness{err, noResponse, -1} + } + err = c.validateHeader(h, height) + if err != nil { + return nil, nil, &errBadWitness{err, invalidHeader, -1} + } + + vals, err := witness.ValidatorSet(height) + if err != nil { + return nil, nil, &errBadWitness{err, noResponse, -1} + } + err = c.validateValidatorSet(vals) + if err != nil { + return nil, nil, &errBadWitness{err, invalidValidatorSet, -1} + } + + return h, vals, nil +} + +func (c *Client) signedHeaderAndValSetFrom(height int64, + source provider.Provider) (*types.SignedHeader, *types.ValidatorSet, error) { + + c.providerMutex.Lock() + sourceIsPrimary := (c.primary == source) + c.providerMutex.Unlock() + + if sourceIsPrimary { + return c.signedHeaderAndValSetFromPrimary(height) + } + return c.signedHeaderAndValSetFromWitness(height, source) +} + // backwards verification (see VerifyHeaderBackwards func in the spec) verifies // headers before a trusted header. If a sent header is invalid the primary is // replaced with another provider and the operation is repeated. @@ -944,7 +1015,7 @@ func (c *Client) backwards( } // compare header with all witnesses provided. -func (c *Client) compareNewHeaderWithWitnesses(h *types.SignedHeader) error { +func (c *Client) compareNewHeaderWithWitnesses(h *types.SignedHeader, now time.Time) error { c.providerMutex.Lock() defer c.providerMutex.Unlock() @@ -961,7 +1032,7 @@ func (c *Client) compareNewHeaderWithWitnesses(h *types.SignedHeader) error { // launch one goroutine per witness errc := make(chan error, len(c.witnesses)) for i, witness := range c.witnesses { - go c.compareNewHeaderWithWitness(errc, h, witness, i) + go c.compareNewHeaderWithWitness(errc, h, witness, i, now) } witnessesToRemove := make([]int, 0) @@ -973,14 +1044,15 @@ func (c *Client) compareNewHeaderWithWitnesses(h *types.SignedHeader) error { switch e := err.(type) { case nil: // at least one header matched headerMatched = true - case ErrConflictingHeaders: // potential fork - c.logger.Error(err.Error(), "witness", e.Witness) + case ErrConflictingHeaders: // fork detected + c.logger.Info("FORK DETECTED", "witness", e.Witness, "err", err) c.sendConflictingHeadersEvidence(&types.ConflictingHeadersEvidence{H1: h, H2: e.H2}) lastErrConfHeaders = e case errBadWitness: - c.logger.Error(err.Error(), "witness", c.witnesses[e.WitnessIndex]) - // if witness sent us invalid header, remove it - if e.Code == invalidHeader { + c.logger.Info("Bad witness", "witness", c.witnesses[e.WitnessIndex], "err", err) + // if witness sent us invalid header / vals, remove it + if e.Code == invalidHeader || e.Code == invalidValidatorSet { + c.logger.Info("Witness sent us invalid header / vals -> removing it", "witness", c.witnesses[e.WitnessIndex]) witnessesToRemove = append(witnessesToRemove, e.WitnessIndex) } } @@ -1006,27 +1078,20 @@ func (c *Client) compareNewHeaderWithWitnesses(h *types.SignedHeader) error { } func (c *Client) compareNewHeaderWithWitness(errc chan error, h *types.SignedHeader, - witness provider.Provider, witnessIndex int) { + witness provider.Provider, witnessIndex int, now time.Time) { - altH, err := witness.SignedHeader(h.Height) + altH, altVals, err := c.signedHeaderAndValSetFromWitness(h.Height, witness) if err != nil { - errc <- errBadWitness{err, noResponse, witnessIndex} - return - } - - if err = altH.ValidateBasic(c.chainID); err != nil { - errc <- errBadWitness{err, invalidHeader, witnessIndex} + err.WitnessIndex = witnessIndex + errc <- err return } if !bytes.Equal(h.Hash(), altH.Hash()) { - // FIXME: call bisection instead of VerifyCommitLightTrusting - // https://github.com/tendermint/tendermint/issues/4934 - if err = c.latestTrustedVals.VerifyCommitLightTrusting(c.chainID, altH.Commit, c.trustLevel); err != nil { - errc <- errBadWitness{err, invalidHeader, witnessIndex} + if bsErr := c.bisection(witness, c.latestTrustedHeader, c.latestTrustedVals, altH, altVals, now); bsErr != nil { + errc <- errBadWitness{bsErr, invalidHeader, witnessIndex} return } - errc <- ErrConflictingHeaders{H1: h, Primary: c.primary, H2: altH, Witness: witness} } @@ -1060,7 +1125,7 @@ func (c *Client) Update(now time.Time) (*types.SignedHeader, error) { return nil, nil } - latestHeader, latestVals, err := c.fetchHeaderAndValsAtHeight(0) + latestHeader, latestVals, err := c.signedHeaderAndValSetFromPrimary(0) if err != nil { return nil, err } @@ -1134,10 +1199,14 @@ func (c *Client) validateHeader(h *types.SignedHeader, expectedHeight int64) err if h == nil { return errors.New("nil header") } + err := h.ValidateBasic(c.chainID) + if err != nil { + return err + } if expectedHeight > 0 && h.Height != expectedHeight { return errors.New("height mismatch") } - return h.ValidateBasic(c.chainID) + return nil } // validatorSetFromPrimary retrieves the ValidatorSet from the primary provider diff --git a/light/client_test.go b/light/client_test.go index 8b39ee819..a97281a6e 100644 --- a/light/client_test.go +++ b/light/client_test.go @@ -202,6 +202,7 @@ func TestClient_SequentialVerification(t *testing.T) { )}, dbs.New(dbm.NewMemDB(), chainID), light.SequentialVerification(), + light.Logger(log.TestingLogger()), ) if tc.initErr { @@ -325,6 +326,7 @@ func TestClient_SkippingVerification(t *testing.T) { )}, dbs.New(dbm.NewMemDB(), chainID), light.SkippingVerification(light.DefaultTrustLevel), + light.Logger(log.TestingLogger()), ) if tc.initErr { require.Error(t, err) @@ -395,7 +397,6 @@ func TestClientBisectionBetweenTrustedHeaders(t *testing.T) { // verify using bisection the header between the two trusted headers _, err = c.VerifyHeaderAtHeight(2, bTime.Add(1*time.Hour)) assert.NoError(t, err) - } func TestClient_Cleanup(t *testing.T) { @@ -923,21 +924,6 @@ func TestClient_NewClientFromTrustedStore(t *testing.T) { } } -func TestNewClientErrorsIfAllWitnessesUnavailable(t *testing.T) { - _, err := light.NewClient( - chainID, - trustOptions, - fullNode, - []provider.Provider{deadNode, deadNode}, - dbs.New(dbm.NewMemDB(), chainID), - light.Logger(log.TestingLogger()), - light.MaxRetryAttempts(1), - ) - if assert.Error(t, err) { - assert.Contains(t, err.Error(), "awaiting response from all witnesses exceeded dropout time") - } -} - func TestClientRemovesWitnessIfItSendsUsIncorrectHeader(t *testing.T) { // different headers hash then primary plus less than 1/3 signed (no fork) badProvider1 := mockp.New( @@ -987,18 +973,13 @@ func TestClientRemovesWitnessIfItSendsUsIncorrectHeader(t *testing.T) { // header should still be verified assert.EqualValues(t, 2, h.Height) - // remaining withness doesn't have header -> error + // remaining witnesses doesn't have header -> error _, err = c.VerifyHeaderAtHeight(3, bTime.Add(2*time.Hour)) if assert.Error(t, err) { assert.Equal(t, "awaiting response from all witnesses exceeded dropout time", err.Error()) } - assert.EqualValues(t, 0, len(c.Witnesses())) - - // no witnesses left, will not be allowed to verify a header - _, err = c.VerifyHeaderAtHeight(3, bTime.Add(2*time.Hour)) - if assert.Error(t, err) { - assert.Equal(t, "no witnesses connected. please reset light client", err.Error()) - } + // witness does not have a header -> left in the list + assert.EqualValues(t, 1, len(c.Witnesses())) } func TestClientTrustedValidatorSet(t *testing.T) { diff --git a/light/errors.go b/light/errors.go index d40200110..a592d1c60 100644 --- a/light/errors.go +++ b/light/errors.go @@ -69,6 +69,7 @@ type badWitnessCode int const ( noResponse badWitnessCode = iota + 1 invalidHeader + invalidValidatorSet ) // errBadWitness is returned when the witness either does not respond or @@ -82,9 +83,11 @@ type errBadWitness struct { func (e errBadWitness) Error() string { switch e.Code { case noResponse: - return fmt.Sprintf("failed to get a header from witness: %v", e.Reason) + return fmt.Sprintf("failed to get a header/vals from witness: %v", e.Reason) case invalidHeader: return fmt.Sprintf("witness sent us invalid header: %v", e.Reason) + case invalidValidatorSet: + return fmt.Sprintf("witness sent us invalid validator set: %v", e.Reason) default: return fmt.Sprintf("unknown code: %d", e.Code) }