From f368b91caf7270c37744233609b1ebad93783a83 Mon Sep 17 00:00:00 2001 From: Callum Waters Date: Tue, 1 Dec 2020 13:53:53 +0100 Subject: [PATCH] light: minor fixes / standardising errors (#5716) ## Description I'm just doing a self audit of the light client. There's a few things I've changed - Validate trust level in `VerifyNonAdjacent` function - Make errNoWitnesses public (it's something people running software on top of a light client should be able to parse) - Remove `ChainID` check of witnesses on start up. We do this already when we compare the first header with witnesses - Remove `ChainID()` from provider interface Closes: #4538 --- light/client.go | 26 +++++++++----------------- light/detector.go | 2 +- light/errors.go | 28 +++++++++++++--------------- light/provider/http/http.go | 5 ----- light/provider/mock/deadmock.go | 15 ++++++++------- light/provider/mock/mock.go | 13 ++++--------- light/provider/provider.go | 3 --- light/store/db/db.go | 2 +- light/verifier.go | 15 +++++++++++---- 9 files changed, 47 insertions(+), 62 deletions(-) diff --git a/light/client.go b/light/client.go index 17c665b2b..3b54c48a9 100644 --- a/light/client.go +++ b/light/client.go @@ -219,15 +219,7 @@ func NewClientFromTrustedStore( // Validate the number of witnesses. if len(c.witnesses) < 1 { - return nil, errNoWitnesses{} - } - - // Verify witnesses are all on the same chain. - for i, w := range witnesses { - if w.ChainID() != chainID { - return nil, fmt.Errorf("witness #%d: %v is on another chain %s, expected %s", - i, w, w.ChainID(), chainID) - } + return nil, ErrNoWitnesses } // Validate trust level. @@ -455,7 +447,7 @@ func (c *Client) VerifyLightBlockAtHeight(ctx context.Context, height int64, now return nil, errors.New("negative or zero height") } - // Check if the light block already verified. + // Check if the light block is already verified. h, err := c.TrustedLightBlock(height) if err == nil { c.logger.Info("Header has already been verified", "height", height, "hash", hash2str(h.Hash())) @@ -634,7 +626,7 @@ func (c *Client) verifySequential( // If some intermediate header is invalid, replace the primary and try // again. - c.logger.Error("primary sent invalid header -> replacing", "err", err) + c.logger.Error("primary sent invalid header -> replacing", "err", err, "primary", c.primary) replaceErr := c.replacePrimaryProvider() if replaceErr != nil { c.logger.Error("Can't replace primary", "err", replaceErr) @@ -768,7 +760,7 @@ func (c *Client) verifySkippingAgainstPrimary( // If some intermediate header is invalid, replace the primary and try // again. - c.logger.Error("primary sent invalid header -> replacing", "err", err) + c.logger.Error("primary sent invalid header -> replacing", "err", err, "primary", c.primary) replaceErr := c.replacePrimaryProvider() if replaceErr != nil { c.logger.Error("Can't replace primary", "err", replaceErr) @@ -853,7 +845,7 @@ func (c *Client) Witnesses() []provider.Provider { // Cleanup removes all the data (headers and validator sets) stored. Note: the // client must be stopped at this point. func (c *Client) Cleanup() error { - c.logger.Info("Removing all the data") + c.logger.Info("Removing all light blocks") c.latestTrustedBlock = nil return c.trustedStore.Prune(0) } @@ -932,7 +924,7 @@ func (c *Client) backwards( "newHeight", interimHeader.Height, "newHash", hash2str(interimHeader.Hash())) if err := VerifyBackwards(interimHeader, verifiedHeader); err != nil { - c.logger.Error("primary sent invalid header -> replacing", "err", err) + c.logger.Error("primary sent invalid header -> replacing", "err", err, "primary", c.primary) if replaceErr := c.replacePrimaryProvider(); replaceErr != nil { c.logger.Error("Can't replace primary", "err", replaceErr) // return original error @@ -968,7 +960,7 @@ func (c *Client) replacePrimaryProvider() error { defer c.providerMutex.Unlock() if len(c.witnesses) <= 1 { - return errNoWitnesses{} + return ErrNoWitnesses } c.primary = c.witnesses[0] c.witnesses = c.witnesses[1:] @@ -985,7 +977,7 @@ func (c *Client) lightBlockFromPrimary(ctx context.Context, height int64) (*type l, err := c.primary.LightBlock(ctx, height) c.providerMutex.Unlock() if err != nil { - c.logger.Debug("Error on light block request from primary", "error", err) + c.logger.Debug("Error on light block request from primary", "error", err, "primary", c.primary) replaceErr := c.replacePrimaryProvider() if replaceErr != nil { return nil, fmt.Errorf("%v. Tried to replace primary but: %w", err.Error(), replaceErr) @@ -1003,7 +995,7 @@ func (c *Client) compareFirstHeaderWithWitnesses(ctx context.Context, h *types.S defer cancel() if len(c.witnesses) < 1 { - return errNoWitnesses{} + return ErrNoWitnesses } errc := make(chan error, len(c.witnesses)) diff --git a/light/detector.go b/light/detector.go index 6c8e5f8d1..743658f9c 100644 --- a/light/detector.go +++ b/light/detector.go @@ -41,7 +41,7 @@ func (c *Client) detectDivergence(ctx context.Context, primaryTrace []*types.Lig defer c.providerMutex.Unlock() if len(c.witnesses) == 0 { - return errNoWitnesses{} + return ErrNoWitnesses } // launch one goroutine per witness to retrieve the light block of the target height diff --git a/light/errors.go b/light/errors.go index ae54436d0..3669e6fcb 100644 --- a/light/errors.go +++ b/light/errors.go @@ -42,9 +42,10 @@ func (e ErrInvalidHeader) Error() string { // ErrFailedHeaderCrossReferencing is returned when the detector was not able to cross reference the header // with any of the connected witnesses. -var ErrFailedHeaderCrossReferencing = errors.New("all witnesses have either not responded, don't have the " + - " blocks or sent invalid blocks. You should look to change your witnesses" + - " or review the light client's logs for more information") +var ErrFailedHeaderCrossReferencing = errors.New( + `all witnesses have either not responded, don't have the blocks or sent invalid blocks. + You should look to change your witnesses or review the light client's logs for more information`, +) // ErrVerificationFailed means either sequential or skipping verification has // failed to verify from header #1 to header #2 due to some reason. @@ -65,10 +66,15 @@ func (e ErrVerificationFailed) Error() string { // ErrLightClientAttack is returned when the light client has detected an attempt // to verify a false header and has sent the evidence to either a witness or primary. -var ErrLightClientAttack = errors.New("attempted attack detected." + - " Light client received valid conflicting header from witness." + - " Unable to verify header. Evidence has been sent to both providers." + - " Check logs for full evidence and trace") +var ErrLightClientAttack = errors.New(`attempted attack detected. + Light client received valid conflicting header from witness. + Unable to verify header. Evidence has been sent to both providers. + Check logs for full evidence and trace`, +) + +// ErrNoWitnesses means that there are not enough witnesses connected to +// continue running the light client. +var ErrNoWitnesses = errors.New("no witnesses connected. please reset light client") // ----------------------------- INTERNAL ERRORS --------------------------------- @@ -84,14 +90,6 @@ func (e errConflictingHeaders) Error() string { e.Block.Hash(), e.WitnessIndex) } -// errNoWitnesses means that there are not enough witnesses connected to -// continue running the light client. -type errNoWitnesses struct{} - -func (e errNoWitnesses) Error() string { - return "no witnesses connected. please reset light client" -} - // errBadWitness is returned when the witness either does not respond or // responds with an invalid header. type errBadWitness struct { diff --git a/light/provider/http/http.go b/light/provider/http/http.go index fd42d75d7..f6b616cdd 100644 --- a/light/provider/http/http.go +++ b/light/provider/http/http.go @@ -51,11 +51,6 @@ func NewWithClient(chainID string, client rpcclient.RemoteClient) provider.Provi } } -// ChainID returns a chainID this provider was configured with. -func (p *http) ChainID() string { - return p.chainID -} - func (p *http) String() string { return fmt.Sprintf("http{%s}", p.client.Remote()) } diff --git a/light/provider/mock/deadmock.go b/light/provider/mock/deadmock.go index e32e6372a..76ea9e9d0 100644 --- a/light/provider/mock/deadmock.go +++ b/light/provider/mock/deadmock.go @@ -3,6 +3,7 @@ package mock import ( "context" "errors" + "fmt" "github.com/tendermint/tendermint/light/provider" "github.com/tendermint/tendermint/types" @@ -11,17 +12,17 @@ import ( var errNoResp = errors.New("no response from provider") type deadMock struct { - chainID string + id string } -// NewDeadMock creates a mock provider that always errors. -func NewDeadMock(chainID string) provider.Provider { - return &deadMock{chainID: chainID} +// NewDeadMock creates a mock provider that always errors. id is used in case of multiple providers. +func NewDeadMock(id string) provider.Provider { + return &deadMock{id: id} } -func (p *deadMock) ChainID() string { return p.chainID } - -func (p *deadMock) String() string { return "deadMock" } +func (p *deadMock) String() string { + return fmt.Sprintf("DeadMock-%s", p.id) +} func (p *deadMock) LightBlock(_ context.Context, height int64) (*types.LightBlock, error) { return nil, errNoResp diff --git a/light/provider/mock/mock.go b/light/provider/mock/mock.go index cf28846ef..310646de0 100644 --- a/light/provider/mock/mock.go +++ b/light/provider/mock/mock.go @@ -11,7 +11,7 @@ import ( ) type Mock struct { - chainID string + id string headers map[int64]*types.SignedHeader vals map[int64]*types.ValidatorSet evidenceToReport map[string]types.Evidence // hash => evidence @@ -21,20 +21,15 @@ var _ provider.Provider = (*Mock)(nil) // New creates a mock provider with the given set of headers and validator // sets. -func New(chainID string, headers map[int64]*types.SignedHeader, vals map[int64]*types.ValidatorSet) *Mock { +func New(id string, headers map[int64]*types.SignedHeader, vals map[int64]*types.ValidatorSet) *Mock { return &Mock{ - chainID: chainID, + id: id, headers: headers, vals: vals, evidenceToReport: make(map[string]types.Evidence), } } -// ChainID returns the blockchain ID. -func (p *Mock) ChainID() string { - return p.chainID -} - func (p *Mock) String() string { var headers strings.Builder for _, h := range p.headers { @@ -46,7 +41,7 @@ func (p *Mock) String() string { fmt.Fprintf(&vals, " %X", v.Hash()) } - return fmt.Sprintf("Mock{headers: %s, vals: %v}", headers.String(), vals.String()) + return fmt.Sprintf("Mock{id: %s, headers: %s, vals: %v}", p.id, headers.String(), vals.String()) } func (p *Mock) LightBlock(_ context.Context, height int64) (*types.LightBlock, error) { diff --git a/light/provider/provider.go b/light/provider/provider.go index 80e8dbc15..d12b611d0 100644 --- a/light/provider/provider.go +++ b/light/provider/provider.go @@ -9,9 +9,6 @@ import ( // Provider provides information for the light client to sync (verification // happens in the client). type Provider interface { - // ChainID returns the blockchain ID. - ChainID() string - // LightBlock returns the LightBlock that corresponds to the given // height. // diff --git a/light/store/db/db.go b/light/store/db/db.go index adbb33871..db87608e7 100644 --- a/light/store/db/db.go +++ b/light/store/db/db.go @@ -238,7 +238,7 @@ func (s *dbs) Prune(size uint16) error { append(s.lbKey(1<<63-1), byte(0x00)), ) if err != nil { - return err + panic(err) } defer itr.Close() diff --git a/light/verifier.go b/light/verifier.go index 0ea4c7332..a8a7ee56c 100644 --- a/light/verifier.go +++ b/light/verifier.go @@ -46,6 +46,10 @@ func VerifyNonAdjacent( return errors.New("headers must be non adjacent in height") } + if err := ValidateTrustLevel(trustLevel); err != nil { + return err + } + if HeaderExpired(trustedHeader, trustingPeriod, now) { return ErrOldHeaderExpired{trustedHeader.Time.Add(trustingPeriod), now} } @@ -57,14 +61,15 @@ func VerifyNonAdjacent( return ErrInvalidHeader{err} } - // Ensure that +`trustLevel` (default 1/3) or more of last trusted validators signed correctly. + // Ensure that +`trustLevel` (default 1/3) or more in voting power of the last trusted validator + // set signed correctly. err := trustedVals.VerifyCommitLightTrusting(trustedHeader.ChainID, untrustedHeader.Commit, trustLevel) if err != nil { switch e := err.(type) { case types.ErrNotEnoughVotingPowerSigned: return ErrNewValSetCantBeTrusted{e} default: - return e + return ErrInvalidHeader{e} } } @@ -186,14 +191,16 @@ func HeaderExpired(h *types.SignedHeader, trustingPeriod time.Duration, now time // c) that the LastBlockID hash of the trusted header is the same as the hash // of the trusted header // -// For any of these cases ErrInvalidHeader is returned. +// For any of these cases ErrInvalidHeader is returned. +// NOTE: This does not check whether the trusted header has expired or not. func VerifyBackwards(untrustedHeader, trustedHeader *types.Header) error { if err := untrustedHeader.ValidateBasic(); err != nil { return ErrInvalidHeader{err} } if untrustedHeader.ChainID != trustedHeader.ChainID { - return ErrInvalidHeader{errors.New("header belongs to another chain")} + return ErrInvalidHeader{fmt.Errorf("new header belongs to a different chain (%s != %s)", + untrustedHeader.ChainID, trustedHeader.ChainID)} } if !untrustedHeader.Time.Before(trustedHeader.Time) {