Don't allow duplicates for light client providers (#7781)

* Allow for zero witness providers

* Verify provider duplicates, fix tests

* Add duplicate provider ID to the error

* Return error on attempt to remove last witness

* Verify duplicates when restoring from store

cmd/tendermint/commands/light.go

@@ -83,9 +83,11 @@ will be verified before passing them back to the caller. Other than
 that, it will present the same interface as a full Tendermint node.
 
 Furthermore to the chainID, a fresh instance of a light client will
-need a primary RPC address, a trusted hash and height and witness RPC addresses
-(if not using sequential verification). To restart the node, thereafter
-only the chainID is required.
+need a primary RPC address and a trusted hash and height. It is also highly
+recommended to provide additional witness RPC addresses, especially if
+not using sequential verification.
+
+To restart the node, thereafter only the chainID is required.
 
 When /abci_query is called, the Merkle key path format is:
 
@@ -127,6 +129,10 @@ for applications built w/ Cosmos SDK).
 				}
 			}
 
+			if len(witnessesAddrs) < 1 && !sequential {
+				logger.Info("In skipping verification mode it is highly recommended to provide at least one witness")
+			}
+
 			trustLevel, err := tmmath.ParseFraction(trustLevelStr)
 			if err != nil {
 				return fmt.Errorf("can't parse trust level: %w", err)

light/client.go

@@ -142,14 +142,29 @@ type Client struct {
 	logger log.Logger
 }
 
+func validatePrimaryAndWitnesses(primary provider.Provider, witnesses []provider.Provider) error {
+	witnessMap := make(map[string]struct{})
+	for _, w := range witnesses {
+		if w.ID() == primary.ID() {
+			return fmt.Errorf("primary (%s) cannot be also configured as witness", primary.ID())
+		}
+		if _, duplicate := witnessMap[w.ID()]; duplicate {
+			return fmt.Errorf("witness list must not contain duplicates; duplicate found: %s", w.ID())
+		}
+		witnessMap[w.ID()] = struct{}{}
+	}
+	return nil
+}
+
 // NewClient returns a new light client. It returns an error if it fails to
-// obtain the light block from the primary or they are invalid (e.g. trust
+// obtain the light block from the primary, or they are invalid (e.g. trust
 // hash does not match with the one from the headers).
 //
 // Witnesses are providers, which will be used for cross-checking the primary
-// 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.
+// provider. At least one witness should be given when skipping verification is
+// used (default). A verified header is compared with the headers at same height
+// obtained from the specified witnesses. A witness can become a primary iff the
+// current primary is unavailable.
 //
 // See all Option(s) for the additional configuration.
 func NewClient(
@@ -174,16 +189,16 @@ func NewClient(
 		)
 	}
 
+	// Check that the witness list does not include duplicates or the primary
+	if err := validatePrimaryAndWitnesses(primary, witnesses); err != nil {
+		return nil, err
+	}
+
 	// Validate trust options
 	if err := trustOptions.ValidateBasic(); err != nil {
 		return nil, fmt.Errorf("invalid TrustOptions: %w", err)
 	}
 
-	// Validate the number of witnesses.
-	if len(witnesses) < 1 {
-		return nil, ErrNoWitnesses
-	}
-
 	c := &Client{
 		chainID:          chainID,
 		trustingPeriod:   trustOptions.Period,
@@ -226,6 +241,11 @@ func NewClientFromTrustedStore(
 	trustedStore store.Store,
 	options ...Option) (*Client, error) {
 
+	// Check that the witness list does not include duplicates or the primary
+	if err := validatePrimaryAndWitnesses(primary, witnesses); err != nil {
+		return nil, err
+	}
+
 	c := &Client{
 		chainID:          chainID,
 		trustingPeriod:   trustingPeriod,
@@ -244,11 +264,6 @@ func NewClientFromTrustedStore(
 		o(c)
 	}
 
-	// Validate the number of witnesses.
-	if len(c.witnesses) < 1 {
-		return nil, ErrNoWitnesses
-	}
-
 	// Validate trust level.
 	if err := ValidateTrustLevel(c.trustLevel); err != nil {
 		return nil, err
@@ -960,7 +975,6 @@ func (c *Client) getLightBlock(ctx context.Context, p provider.Provider, height
 
 // NOTE: requires a providerMutex lock
 func (c *Client) removeWitnesses(indexes []int) error {
-	// check that we will still have witnesses remaining
 	if len(c.witnesses) <= len(indexes) {
 		return ErrNoWitnesses
 	}
@@ -1082,7 +1096,7 @@ func (c *Client) compareFirstHeaderWithWitnesses(ctx context.Context, h *types.S
 	defer c.providerMutex.Unlock()
 
 	if len(c.witnesses) < 1 {
-		return ErrNoWitnesses
+		return nil
 	}
 
 	errc := make(chan error, len(c.witnesses))

light/client_benchmark_test.go

@@ -84,7 +84,7 @@ func BenchmarkSequence(b *testing.B) {
 			Hash:   genesisBlock.Hash(),
 		},
 		benchmarkFullNode,
-		[]provider.Provider{benchmarkFullNode},
+		nil,
 		dbs.New(dbm.NewMemDB()),
 		light.Logger(logger),
 		light.SequentialVerification(),
@@ -121,7 +121,7 @@ func BenchmarkBisection(b *testing.B) {
 			Hash:   genesisBlock.Hash(),
 		},
 		benchmarkFullNode,
-		[]provider.Provider{benchmarkFullNode},
+		nil,
 		dbs.New(dbm.NewMemDB()),
 		light.Logger(logger),
 	)
@@ -157,7 +157,7 @@ func BenchmarkBackwards(b *testing.B) {
 			Hash:   trustedBlock.Hash(),
 		},
 		benchmarkFullNode,
-		[]provider.Provider{benchmarkFullNode},
+		nil,
 		dbs.New(dbm.NewMemDB()),
 		light.Logger(logger),
 	)

light/client_test.go

@@ -233,7 +233,7 @@ func TestClient(t *testing.T) {
 					chainID,
 					trustOptions,
 					mockNode,
-					[]provider.Provider{mockNode},
+					nil,
 					dbs.New(dbm.NewMemDB()),
 					light.SequentialVerification(),
 					light.Logger(logger),
@@ -360,7 +360,7 @@ func TestClient(t *testing.T) {
 					chainID,
 					trustOptions,
 					mockNode,
-					[]provider.Provider{mockNode},
+					nil,
 					dbs.New(dbm.NewMemDB()),
 					light.SkippingVerification(light.DefaultTrustLevel),
 					light.Logger(logger),
@@ -416,7 +416,7 @@ func TestClient(t *testing.T) {
 				Hash:   trustedLightBlock.Hash(),
 			},
 			mockNode,
-			[]provider.Provider{mockNode},
+			nil,
 			dbs.New(dbm.NewMemDB()),
 			light.SkippingVerification(light.DefaultTrustLevel),
 		)
@@ -445,7 +445,7 @@ func TestClient(t *testing.T) {
 				Hash:   h1.Hash(),
 			},
 			mockFullNode,
-			[]provider.Provider{mockFullNode},
+			nil,
 			dbs.New(dbm.NewMemDB()),
 			light.SkippingVerification(light.DefaultTrustLevel),
 		)
@@ -475,7 +475,7 @@ func TestClient(t *testing.T) {
 			chainID,
 			trustOptions,
 			mockFullNode,
-			[]provider.Provider{mockFullNode},
+			nil,
 			dbs.New(dbm.NewMemDB()),
 			light.Logger(logger),
 		)
@@ -515,7 +515,7 @@ func TestClient(t *testing.T) {
 				chainID,
 				trustOptions,
 				mockNode,
-				[]provider.Provider{mockNode},
+				nil,
 				trustedStore,
 				light.Logger(logger),
 			)
@@ -554,7 +554,7 @@ func TestClient(t *testing.T) {
 					Hash:   header1.Hash(),
 				},
 				mockNode,
-				[]provider.Provider{mockNode},
+				nil,
 				trustedStore,
 				light.Logger(logger),
 			)
@@ -575,9 +575,14 @@ func TestClient(t *testing.T) {
 		defer cancel()
 
 		mockFullNode := &provider_mocks.Provider{}
+		mockFullNode.On("ID").Return("mockFullNode")
 		mockFullNode.On("LightBlock", mock.Anything, int64(0)).Return(l3, nil)
 		mockFullNode.On("LightBlock", mock.Anything, int64(1)).Return(l1, nil)
-		mockFullNode.On("LightBlock", mock.Anything, int64(3)).Return(l3, nil)
+
+		mockWitnessNode := &provider_mocks.Provider{}
+		mockWitnessNode.On("ID").Return("mockWitnessNode")
+		mockWitnessNode.On("LightBlock", mock.Anything, int64(1)).Return(l1, nil)
+		mockWitnessNode.On("LightBlock", mock.Anything, int64(3)).Return(l3, nil)
 
 		logger := log.NewTestingLogger(t)
 
@@ -586,7 +591,7 @@ func TestClient(t *testing.T) {
 			chainID,
 			trustOptions,
 			mockFullNode,
-			[]provider.Provider{mockFullNode},
+			[]provider.Provider{mockWitnessNode},
 			dbs.New(dbm.NewMemDB()),
 			light.Logger(logger),
 		)
@@ -600,6 +605,7 @@ func TestClient(t *testing.T) {
 			assert.NoError(t, l.ValidateBasic(chainID))
 		}
 		mockFullNode.AssertExpectations(t)
+		mockWitnessNode.AssertExpectations(t)
 	})
 
 	t.Run("Concurrency", func(t *testing.T) {
@@ -615,7 +621,7 @@ func TestClient(t *testing.T) {
 			chainID,
 			trustOptions,
 			mockFullNode,
-			[]provider.Provider{mockFullNode},
+			nil,
 			dbs.New(dbm.NewMemDB()),
 			light.Logger(logger),
 		)
@@ -665,7 +671,7 @@ func TestClient(t *testing.T) {
 			chainID,
 			trustOptions,
 			mockFullNode,
-			[]provider.Provider{mockFullNode},
+			nil,
 			dbs.New(dbm.NewMemDB()),
 			light.Logger(logger),
 		)
@@ -679,9 +685,8 @@ func TestClient(t *testing.T) {
 			close(closeCh)
 		}()
 
-		// NOTE: the light client doesn't check uniqueness of providers
 		c.AddProvider(mockFullNode)
-		require.Len(t, c.Witnesses(), 2)
+		require.Len(t, c.Witnesses(), 1)
 		select {
 		case <-closeCh:
 		case <-time.After(5 * time.Second):
@@ -695,9 +700,10 @@ func TestClient(t *testing.T) {
 
 		mockFullNode := &provider_mocks.Provider{}
 		mockFullNode.On("LightBlock", mock.Anything, mock.Anything).Return(l1, nil)
-
+		mockFullNode.On("ID").Return("mockFullNode")
 		mockDeadNode := &provider_mocks.Provider{}
 		mockDeadNode.On("LightBlock", mock.Anything, mock.Anything).Return(nil, provider.ErrNoResponse)
+		mockDeadNode.On("ID").Return("mockDeadNode")
 
 		logger := log.NewTestingLogger(t)
 
@@ -706,7 +712,7 @@ func TestClient(t *testing.T) {
 			chainID,
 			trustOptions,
 			mockDeadNode,
-			[]provider.Provider{mockDeadNode, mockFullNode},
+			[]provider.Provider{mockFullNode},
 			dbs.New(dbm.NewMemDB()),
 			light.Logger(logger),
 		)
@@ -720,7 +726,7 @@ func TestClient(t *testing.T) {
 
 		// we should still have the dead node as a witness because it
 		// hasn't repeatedly been unresponsive yet
-		assert.Equal(t, 2, len(c.Witnesses()))
+		assert.Equal(t, 1, len(c.Witnesses()))
 		mockDeadNode.AssertExpectations(t)
 		mockFullNode.AssertExpectations(t)
 	})
@@ -730,17 +736,24 @@ func TestClient(t *testing.T) {
 
 		mockFullNode := &provider_mocks.Provider{}
 		mockFullNode.On("LightBlock", mock.Anything, mock.Anything).Return(l1, nil)
+		mockFullNode.On("ID").Return("mockFullNode")
 
 		logger := log.NewTestingLogger(t)
 
-		mockDeadNode := &provider_mocks.Provider{}
-		mockDeadNode.On("LightBlock", mock.Anything, mock.Anything).Return(nil, provider.ErrLightBlockNotFound)
+		mockDeadNode1 := &provider_mocks.Provider{}
+		mockDeadNode1.On("LightBlock", mock.Anything, mock.Anything).Return(nil, provider.ErrLightBlockNotFound)
+		mockDeadNode1.On("ID").Return("mockDeadNode1")
+
+		mockDeadNode2 := &provider_mocks.Provider{}
+		mockDeadNode2.On("LightBlock", mock.Anything, mock.Anything).Return(nil, provider.ErrLightBlockNotFound)
+		mockDeadNode2.On("ID").Return("mockDeadNode2")
+
 		c, err := light.NewClient(
 			ctx,
 			chainID,
 			trustOptions,
-			mockDeadNode,
-			[]provider.Provider{mockDeadNode, mockFullNode},
+			mockDeadNode1,
+			[]provider.Provider{mockFullNode, mockDeadNode2},
 			dbs.New(dbm.NewMemDB()),
 			light.Logger(logger),
 		)
@@ -751,7 +764,7 @@ func TestClient(t *testing.T) {
 		// we should still have the dead node as a witness because it
 		// hasn't repeatedly been unresponsive yet
 		assert.Equal(t, 2, len(c.Witnesses()))
-		mockDeadNode.AssertExpectations(t)
+		mockDeadNode1.AssertExpectations(t)
 		mockFullNode.AssertExpectations(t)
 	})
 	t.Run("BackwardsVerification", func(t *testing.T) {
@@ -777,7 +790,7 @@ func TestClient(t *testing.T) {
 					Hash:   trustHeader.Hash(),
 				},
 				mockLargeFullNode,
-				[]provider.Provider{mockLargeFullNode},
+				nil,
 				dbs.New(dbm.NewMemDB()),
 				light.Logger(logger),
 			)
@@ -836,7 +849,7 @@ func TestClient(t *testing.T) {
 					Hash:   h3.Hash(),
 				},
 				mockNode,
-				[]provider.Provider{mockNode},
+				nil,
 				dbs.New(dbm.NewMemDB()),
 				light.Logger(logger),
 			)
@@ -852,13 +865,15 @@ func TestClient(t *testing.T) {
 		db := dbs.New(dbm.NewMemDB())
 		err := db.SaveLightBlock(l1)
 		require.NoError(t, err)
-		mockNode := &provider_mocks.Provider{}
-
+		mockPrimary := &provider_mocks.Provider{}
+		mockPrimary.On("ID").Return("mockPrimary")
+		mockWitness := &provider_mocks.Provider{}
+		mockWitness.On("ID").Return("mockWitness")
 		c, err := light.NewClientFromTrustedStore(
 			chainID,
 			trustPeriod,
-			mockNode,
-			[]provider.Provider{mockNode},
+			mockPrimary,
+			[]provider.Provider{mockWitness},
 			db,
 		)
 		require.NoError(t, err)
@@ -867,7 +882,8 @@ func TestClient(t *testing.T) {
 		h, err := c.TrustedLightBlock(1)
 		assert.NoError(t, err)
 		assert.EqualValues(t, l1.Height, h.Height)
-		mockNode.AssertExpectations(t)
+		mockPrimary.AssertExpectations(t)
+		mockWitness.AssertExpectations(t)
 	})
 	t.Run("RemovesWitnessIfItSendsUsIncorrectHeader", func(t *testing.T) {
 		logger := log.NewTestingLogger(t)
@@ -885,6 +901,7 @@ func TestClient(t *testing.T) {
 		}
 		mockBadNode1 := mockNodeFromHeadersAndVals(headers1, vals1)
 		mockBadNode1.On("LightBlock", mock.Anything, mock.Anything).Return(nil, provider.ErrLightBlockNotFound)
+		mockBadNode1.On("ID").Return("mockBadNode1")
 
 		// header is empty
 		headers2 := map[int64]*types.SignedHeader{
@@ -897,8 +914,10 @@ func TestClient(t *testing.T) {
 		}
 		mockBadNode2 := mockNodeFromHeadersAndVals(headers2, vals2)
 		mockBadNode2.On("LightBlock", mock.Anything, mock.Anything).Return(nil, provider.ErrLightBlockNotFound)
+		mockBadNode2.On("ID").Return("mockBadNode2")
 
 		mockFullNode := mockNodeFromHeadersAndVals(headerSet, valSet)
+		mockFullNode.On("ID").Return("mockFullNode")
 
 		ctx, cancel := context.WithCancel(context.Background())
 		defer cancel()
@@ -956,6 +975,8 @@ func TestClient(t *testing.T) {
 				1: vals,
 				2: differentVals,
 			})
+		mockBadValSetNode.On("ID").Return("mockBadValSetNode")
+
 		mockFullNode := mockNodeFromHeadersAndVals(
 			map[int64]*types.SignedHeader{
 				1: h1,
@@ -965,13 +986,25 @@ func TestClient(t *testing.T) {
 				1: vals,
 				2: vals,
 			})
+		mockFullNode.On("ID").Return("mockFullNode")
+
+		mockGoodWitness := mockNodeFromHeadersAndVals(
+			map[int64]*types.SignedHeader{
+				1: h1,
+				2: h2,
+			},
+			map[int64]*types.ValidatorSet{
+				1: vals,
+				2: vals,
+			})
+		mockGoodWitness.On("ID").Return("mockGoodWitness")
 
 		c, err := light.NewClient(
 			ctx,
 			chainID,
 			trustOptions,
 			mockFullNode,
-			[]provider.Provider{mockBadValSetNode, mockFullNode},
+			[]provider.Provider{mockBadValSetNode, mockGoodWitness},
 			dbs.New(dbm.NewMemDB()),
 			light.Logger(logger),
 		)
@@ -988,15 +1021,26 @@ func TestClient(t *testing.T) {
 		mockFullNode := mockNodeFromHeadersAndVals(
 			map[int64]*types.SignedHeader{
 				1: h1,
-				3: h3,
 				0: h3,
 			},
 			map[int64]*types.ValidatorSet{
 				1: vals,
-				3: vals,
 				0: vals,
 			})
 
+		mockFullNode.On("ID").Return("mockFullNode")
+
+		mockGoodWitness := mockNodeFromHeadersAndVals(
+			map[int64]*types.SignedHeader{
+				1: h1,
+				3: h3,
+			},
+			map[int64]*types.ValidatorSet{
+				1: vals,
+				3: vals,
+			})
+		mockGoodWitness.On("ID").Return("mockGoodWitness")
+
 		ctx, cancel := context.WithCancel(context.Background())
 		defer cancel()
 		logger := log.NewTestingLogger(t)
@@ -1006,7 +1050,7 @@ func TestClient(t *testing.T) {
 			chainID,
 			trustOptions,
 			mockFullNode,
-			[]provider.Provider{mockFullNode},
+			[]provider.Provider{mockGoodWitness},
 			dbs.New(dbm.NewMemDB()),
 			light.Logger(logger),
 			light.PruningSize(1),
@@ -1022,6 +1066,7 @@ func TestClient(t *testing.T) {
 		_, err = c.TrustedLightBlock(1)
 		assert.Error(t, err)
 		mockFullNode.AssertExpectations(t)
+		mockGoodWitness.AssertExpectations(t)
 	})
 	t.Run("EnsureValidHeadersAndValSets", func(t *testing.T) {
 		emptyValSet := &types.ValidatorSet{
@@ -1098,7 +1143,7 @@ func TestClient(t *testing.T) {
 					chainID,
 					trustOptions,
 					mockBadNode,
-					[]provider.Provider{mockBadNode, mockBadNode},
+					nil,
 					dbs.New(dbm.NewMemDB()),
 				)
 				require.NoError(t, err)

light/detector.go

@@ -26,6 +26,9 @@ import (
 // If there are no conflictinge headers, the light client deems the verified target header
 // trusted and saves it to the trusted store.
 func (c *Client) detectDivergence(ctx context.Context, primaryTrace []*types.LightBlock, now time.Time) error {
+	if len(c.witnesses) < 1 {
+		return nil
+	}
 	if primaryTrace == nil || len(primaryTrace) < 2 {
 		return errors.New("nil or single block primary trace")
 	}
@@ -40,10 +43,6 @@ func (c *Client) detectDivergence(ctx context.Context, primaryTrace []*types.Lig
 	c.providerMutex.Lock()
 	defer c.providerMutex.Unlock()
 
-	if len(c.witnesses) == 0 {
-		return ErrNoWitnesses
-	}
-
 	// launch one goroutine per witness to retrieve the light block of the target height
 	// and compare it with the header from the primary
 	errc := make(chan error, len(c.witnesses))

light/detector_test.go

@@ -56,8 +56,7 @@ func TestLightClientAttackEvidence_Lunatic(t *testing.T) {
 	delete(primaryHeaders, 2)
 
 	mockWitness := mockNodeFromHeadersAndVals(witnessHeaders, witnessValidators)
-	mockPrimary := mockNodeFromHeadersAndVals(primaryHeaders, primaryValidators)
-
+	mockWitness.On("ID").Return("mockWitness")
 	mockWitness.On("ReportEvidence", mock.Anything, mock.MatchedBy(func(evidence types.Evidence) bool {
 		evAgainstPrimary := &types.LightClientAttackEvidence{
 			// after the divergence height the valset doesn't change so we expect the evidence to be for the latest height
@@ -70,6 +69,8 @@ func TestLightClientAttackEvidence_Lunatic(t *testing.T) {
 		return bytes.Equal(evidence.Hash(), evAgainstPrimary.Hash())
 	})).Return(nil)
 
+	mockPrimary := mockNodeFromHeadersAndVals(primaryHeaders, primaryValidators)
+	mockPrimary.On("ID").Return("mockPrimary")
 	mockPrimary.On("ReportEvidence", mock.Anything, mock.MatchedBy(func(evidence types.Evidence) bool {
 		evAgainstWitness := &types.LightClientAttackEvidence{
 			// when forming evidence against witness we learn that the canonical chain continued to change validator sets
@@ -173,10 +174,13 @@ func TestLightClientAttackEvidence_Equivocation(t *testing.T) {
 				delete(witnessHeaders, height)
 			}
 			mockWitness := mockNodeFromHeadersAndVals(witnessHeaders, witnessValidators)
+			mockWitness.On("ID").Return("mockWitness")
+
 			for _, height := range testCase.unusedPrimaryBlockHeights {
 				delete(primaryHeaders, height)
 			}
 			mockPrimary := mockNodeFromHeadersAndVals(primaryHeaders, primaryValidators)
+			mockPrimary.On("ID").Return("mockPrimary")
 
 			// Check evidence was sent to both full nodes.
 			// Common height should be set to the height of the divergent header in the instance
@@ -275,16 +279,13 @@ func TestLightClientAttackEvidence_ForwardLunatic(t *testing.T) {
 		0, len(forgedKeys),
 	)
 	mockPrimary := mockNodeFromHeadersAndVals(primaryHeaders, primaryValidators)
+	mockPrimary.On("ID").Return("mockPrimary")
 	lastBlock, _ := mockPrimary.LightBlock(ctx, forgedHeight)
 	mockPrimary.On("LightBlock", mock.Anything, int64(0)).Return(lastBlock, nil)
 	mockPrimary.On("LightBlock", mock.Anything, mock.Anything).Return(nil, provider.ErrLightBlockNotFound)
 
-	/*
-		for _, unusedHeader := range []int64{3, 5, 6, 8} {
-			delete(witnessHeaders, unusedHeader)
-		}
-	*/
 	mockWitness := mockNodeFromHeadersAndVals(witnessHeaders, witnessValidators)
+	mockWitness.On("ID").Return("mockWitness")
 	lastBlock, _ = mockWitness.LightBlock(ctx, latestHeight)
 	mockWitness.On("LightBlock", mock.Anything, int64(0)).Return(lastBlock, nil).Once()
 	mockWitness.On("LightBlock", mock.Anything, int64(12)).Return(nil, provider.ErrHeightTooHigh)
@@ -303,7 +304,11 @@ func TestLightClientAttackEvidence_ForwardLunatic(t *testing.T) {
 
 	// In order to perform the attack, the primary needs at least one accomplice as a witness to also
 	// send the forged block
-	accomplice := mockPrimary
+	accomplice := mockNodeFromHeadersAndVals(primaryHeaders, primaryValidators)
+	accomplice.On("ID").Return("accomplice")
+	lastBlock, _ = accomplice.LightBlock(ctx, forgedHeight)
+	accomplice.On("LightBlock", mock.Anything, int64(0)).Return(lastBlock, nil)
+	accomplice.On("LightBlock", mock.Anything, mock.Anything).Return(nil, provider.ErrLightBlockNotFound)
 
 	c, err := light.NewClient(
 		ctx,
@@ -362,6 +367,7 @@ func TestLightClientAttackEvidence_ForwardLunatic(t *testing.T) {
 	// Lastly we test the unfortunate case where the light clients supporting witness doesn't update
 	// in enough time
 	mockLaggingWitness := mockNodeFromHeadersAndVals(witnessHeaders, witnessValidators)
+	mockLaggingWitness.On("ID").Return("mockLaggingWitness")
 	mockLaggingWitness.On("LightBlock", mock.Anything, int64(12)).Return(nil, provider.ErrHeightTooHigh)
 	lastBlock, _ = mockLaggingWitness.LightBlock(ctx, latestHeight)
 	mockLaggingWitness.On("LightBlock", mock.Anything, int64(0)).Return(lastBlock, nil)
@@ -397,10 +403,13 @@ func TestClientDivergentTraces1(t *testing.T) {
 
 	headers, vals, _ := genLightBlocksWithKeys(t, chainID, 1, 5, 2, bTime)
 	mockPrimary := mockNodeFromHeadersAndVals(headers, vals)
+	mockPrimary.On("ID").Return("mockPrimary")
+
 	firstBlock, err := mockPrimary.LightBlock(ctx, 1)
 	require.NoError(t, err)
 	headers, vals, _ = genLightBlocksWithKeys(t, chainID, 1, 5, 2, bTime)
 	mockWitness := mockNodeFromHeadersAndVals(headers, vals)
+	mockWitness.On("ID").Return("mockWitness")
 
 	logger := log.NewTestingLogger(t)
 
@@ -432,8 +441,19 @@ func TestClientDivergentTraces2(t *testing.T) {
 
 	headers, vals, _ := genLightBlocksWithKeys(t, chainID, 2, 5, 2, bTime)
 	mockPrimaryNode := mockNodeFromHeadersAndVals(headers, vals)
-	mockDeadNode := &provider_mocks.Provider{}
-	mockDeadNode.On("LightBlock", mock.Anything, mock.Anything).Return(nil, provider.ErrNoResponse)
+	mockPrimaryNode.On("ID").Return("mockPrimaryNode")
+
+	mockGoodWitness := mockNodeFromHeadersAndVals(headers, vals)
+	mockGoodWitness.On("ID").Return("mockGoodWitness")
+
+	mockDeadNode1 := &provider_mocks.Provider{}
+	mockDeadNode1.On("ID").Return("mockDeadNode1")
+	mockDeadNode1.On("LightBlock", mock.Anything, mock.Anything).Return(nil, provider.ErrNoResponse)
+
+	mockDeadNode2 := &provider_mocks.Provider{}
+	mockDeadNode2.On("ID").Return("mockDeadNode2")
+	mockDeadNode2.On("LightBlock", mock.Anything, mock.Anything).Return(nil, provider.ErrNoResponse)
+
 	firstBlock, err := mockPrimaryNode.LightBlock(ctx, 1)
 	require.NoError(t, err)
 	c, err := light.NewClient(
@@ -445,7 +465,7 @@ func TestClientDivergentTraces2(t *testing.T) {
 			Period: 4 * time.Hour,
 		},
 		mockPrimaryNode,
-		[]provider.Provider{mockDeadNode, mockDeadNode, mockPrimaryNode},
+		[]provider.Provider{mockDeadNode1, mockDeadNode2, mockGoodWitness},
 		dbs.New(dbm.NewMemDB()),
 		light.Logger(logger),
 	)
@@ -454,7 +474,7 @@ func TestClientDivergentTraces2(t *testing.T) {
 	_, err = c.VerifyLightBlockAtHeight(ctx, 2, bTime.Add(1*time.Hour))
 	assert.NoError(t, err)
 	assert.Equal(t, 3, len(c.Witnesses()))
-	mockDeadNode.AssertExpectations(t)
+	mockDeadNode1.AssertExpectations(t)
 	mockPrimaryNode.AssertExpectations(t)
 }
 
@@ -467,6 +487,7 @@ func TestClientDivergentTraces3(t *testing.T) {
 	//
 	primaryHeaders, primaryVals, _ := genLightBlocksWithKeys(t, chainID, 2, 5, 2, bTime)
 	mockPrimary := mockNodeFromHeadersAndVals(primaryHeaders, primaryVals)
+	mockPrimary.On("ID").Return("mockPrimary")
 
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
@@ -478,6 +499,7 @@ func TestClientDivergentTraces3(t *testing.T) {
 	mockHeaders[1] = primaryHeaders[1]
 	mockVals[1] = primaryVals[1]
 	mockWitness := mockNodeFromHeadersAndVals(mockHeaders, mockVals)
+	mockWitness.On("ID").Return("mockWitness")
 
 	c, err := light.NewClient(
 		ctx,
@@ -510,6 +532,7 @@ func TestClientDivergentTraces4(t *testing.T) {
 	//
 	primaryHeaders, primaryVals, _ := genLightBlocksWithKeys(t, chainID, 2, 5, 2, bTime)
 	mockPrimary := mockNodeFromHeadersAndVals(primaryHeaders, primaryVals)
+	mockPrimary.On("ID").Return("mockPrimary")
 
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
@@ -521,6 +544,7 @@ func TestClientDivergentTraces4(t *testing.T) {
 	primaryHeaders[2] = witnessHeaders[2]
 	primaryVals[2] = witnessVals[2]
 	mockWitness := mockNodeFromHeadersAndVals(primaryHeaders, primaryVals)
+	mockWitness.On("ID").Return("mockWitness")
 
 	c, err := light.NewClient(
 		ctx,

light/example_test.go

@@ -11,7 +11,6 @@ import (
 	"github.com/tendermint/tendermint/abci/example/kvstore"
 	"github.com/tendermint/tendermint/libs/log"
 	"github.com/tendermint/tendermint/light"
-	"github.com/tendermint/tendermint/light/provider"
 	httpp "github.com/tendermint/tendermint/light/provider/http"
 	dbs "github.com/tendermint/tendermint/light/store/db"
 	rpctest "github.com/tendermint/tendermint/rpc/test"
@@ -74,7 +73,7 @@ func ExampleClient() {
 			Hash:   block.Hash(),
 		},
 		primary,
-		[]provider.Provider{primary}, // NOTE: primary should not be used here
+		nil,
 		dbs.New(db),
 		light.Logger(logger),
 	)

light/light_test.go

@@ -67,7 +67,7 @@ func TestClientIntegration_Update(t *testing.T) {
 			Hash:   block.Hash(),
 		},
 		primary,
-		[]provider.Provider{primary}, // NOTE: primary should not be used here
+		nil,
 		dbs.New(db),
 		light.Logger(logger),
 	)
@@ -127,7 +127,7 @@ func TestClientIntegration_VerifyLightBlockAtHeight(t *testing.T) {
 			Hash:   block.Hash(),
 		},
 		primary,
-		[]provider.Provider{primary}, // NOTE: primary should not be used here
+		nil,
 		dbs.New(db),
 		light.Logger(logger),
 	)
@@ -197,10 +197,9 @@ func TestClientStatusRPC(t *testing.T) {
 	db, err := dbm.NewGoLevelDB("light-client-db", dbDir)
 	require.NoError(t, err)
 
-	// In order to not create a full testnet to verify whether we get the correct IPs
-	// if we have more than one witness, we add the primary multiple times
-	// TODO This should be buggy behavior, we should not be allowed to add the same nodes as witnesses
-	witnesses := []provider.Provider{primary, primary, primary}
+	// In order to not create a full testnet we create the light client with no witnesses
+	// and only verify the primary IP address.
+	witnesses := []provider.Provider{}
 
 	c, err := light.NewClient(ctx,
 		chainID,
@@ -223,9 +222,6 @@ func TestClientStatusRPC(t *testing.T) {
 	// Verify primary IP
 	require.True(t, lightStatus.PrimaryID == primary.ID())
 
-	// Verify IPs of witnesses
-	require.ElementsMatch(t, mapProviderArrayToIP(witnesses), lightStatus.WitnessesID)
-
 	// Verify that number of peers is equal to number of witnesses  (+ 1 if the primary is not a witness)
 	require.Equal(t, len(witnesses)+1*primaryNotInWitnessList(witnesses, primary), lightStatus.NumPeers)
 
@@ -238,15 +234,6 @@ func TestClientStatusRPC(t *testing.T) {
 
 }
 
-// Extract the IP address of all the providers within an array
-func mapProviderArrayToIP(el []provider.Provider) []string {
-	ips := make([]string, len(el))
-	for i, v := range el {
-		ips[i] = v.ID()
-	}
-	return ips
-}
-
 // If the primary is not in the witness list, we will return 1
 // Otherwise, return 0
 func primaryNotInWitnessList(witnesses []provider.Provider, primary provider.Provider) int {

light/setup.go

@@ -2,7 +2,6 @@ package light
 
 import (
 	"context"
-	"time"
 
 	"github.com/tendermint/tendermint/light/provider"
 	"github.com/tendermint/tendermint/light/provider/http"
@@ -39,34 +38,6 @@ func NewHTTPClient(
 		options...)
 }
 
-// NewHTTPClientFromTrustedStore initiates an instance of a light client using
-// HTTP addresses for both the primary provider and witnesses and uses a
-// trusted store as the root of trust.
-//
-// See all Option(s) for the additional configuration.
-// See NewClientFromTrustedStore.
-func NewHTTPClientFromTrustedStore(
-	chainID string,
-	trustingPeriod time.Duration,
-	primaryAddress string,
-	witnessesAddresses []string,
-	trustedStore store.Store,
-	options ...Option) (*Client, error) {
-
-	providers, err := providersFromAddresses(append(witnessesAddresses, primaryAddress), chainID)
-	if err != nil {
-		return nil, err
-	}
-
-	return NewClientFromTrustedStore(
-		chainID,
-		trustingPeriod,
-		providers[len(providers)-1],
-		providers[:len(providers)-1],
-		trustedStore,
-		options...)
-}
-
 func providersFromAddresses(addrs []string, chainID string) ([]provider.Provider, error) {
 	providers := make([]provider.Provider, len(addrs))
 	for idx, address := range addrs {