diff --git a/internal/p2p/peermanager.go b/internal/p2p/peermanager.go index 4ce0c3011..a32749526 100644 --- a/internal/p2p/peermanager.go +++ b/internal/p2p/peermanager.go @@ -338,7 +338,7 @@ func NewPeerManager(_ctx context.Context, selfID types.NodeID, peerDB dbm.DB, op options.optimize() - store, err := newPeerStore(peerDB) + store, err := newPeerStore(peerDB, log.NewNopLogger().With("module", "peerstore")) if err != nil { return nil, err } @@ -379,6 +379,9 @@ func NewPeerManager(_ctx context.Context, selfID types.NodeID, peerDB dbm.DB, op // SetLogger sets a logger for the PeerManager func (m *PeerManager) SetLogger(logger log.Logger) { m.logger = logger + if m.store != nil { + m.store.logger = logger.With("module", "peerstore") + } } // Close closes peer manager and frees up all resources @@ -1371,15 +1374,22 @@ type peerStore struct { peers map[types.NodeID]*peerInfo index map[NodeAddress]types.NodeID ranked []*peerInfo // cache for Ranked(), nil invalidates cache + logger log.Logger } // newPeerStore creates a new peer store, loading all persisted peers from the // database into memory. -func newPeerStore(db dbm.DB) (*peerStore, error) { +func newPeerStore(db dbm.DB, logger log.Logger) (*peerStore, error) { if db == nil { return nil, errors.New("no database provided") } - store := &peerStore{db: db} + if logger == nil { + logger = log.NewNopLogger() + } + store := &peerStore{ + db: db, + logger: logger, + } if err := store.loadPeers(); err != nil { return nil, err } @@ -1398,15 +1408,15 @@ func (s *peerStore) loadPeers() error { } defer iter.Close() for ; iter.Valid(); iter.Next() { - // FIXME: We may want to tolerate failures here, by simply logging - // the errors and ignoring the faulty peer entries. msg := new(p2pproto.PeerInfo) if err := proto.Unmarshal(iter.Value(), msg); err != nil { - return fmt.Errorf("invalid peer Protobuf data: %w", err) + s.logger.Error("invalid peer Protobuf data, skipping", "key", fmt.Sprintf("%x", iter.Key()), "error", err) + continue } peer, err := peerInfoFromProto(msg) if err != nil { - return fmt.Errorf("invalid peer data: %w", err) + s.logger.Error("invalid peer data, skipping", "key", fmt.Sprintf("%x", iter.Key()), "error", err) + continue } peers[peer.ID] = peer for addr := range peer.AddressInfo { diff --git a/internal/p2p/peermanager_scoring_test.go b/internal/p2p/peermanager_scoring_test.go index 50d7ff6d6..c0469ccf9 100644 --- a/internal/p2p/peermanager_scoring_test.go +++ b/internal/p2p/peermanager_scoring_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/require" "github.com/dashpay/tenderdash/crypto/ed25519" + "github.com/dashpay/tenderdash/libs/log" "github.com/dashpay/tenderdash/types" ) @@ -100,7 +101,7 @@ func TestPeerScoring(t *testing.T) { func makeMockPeerStore(t *testing.T, peers ...peerInfo) *peerStore { t.Helper() - s, err := newPeerStore(dbm.NewMemDB()) + s, err := newPeerStore(dbm.NewMemDB(), log.NewNopLogger()) if err != nil { t.Fatal(err) } diff --git a/internal/p2p/peermanager_test.go b/internal/p2p/peermanager_test.go index 32474a5c4..a8e7e3c65 100644 --- a/internal/p2p/peermanager_test.go +++ b/internal/p2p/peermanager_test.go @@ -10,11 +10,13 @@ import ( dbm "github.com/cometbft/cometbft-db" "github.com/fortytw2/leaktest" + "github.com/google/orderedcode" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/dashpay/tenderdash/internal/p2p" "github.com/dashpay/tenderdash/libs/log" + p2pproto "github.com/dashpay/tenderdash/proto/tendermint/p2p" "github.com/dashpay/tenderdash/types" ) @@ -111,6 +113,56 @@ func TestPeerManagerOptions_Validate(t *testing.T) { } } +func TestPeerStoreLoadPeersSkipsInvalidAddress(t *testing.T) { + db := dbm.NewMemDB() + logger, err := log.NewDefaultLogger(log.LogFormatText, log.LogLevelInfo) + require.NoError(t, err) + + invalidID := types.NodeID(strings.Repeat("a", 40)) + invalidPeer := &p2pproto.PeerInfo{ + ID: string(invalidID), + AddressInfo: []*p2pproto.PeerAddressInfo{{ + Address: "mconn://" + string(invalidID) + "@[173.199.71.83:26656]:26656", + }}, + } + invalidBz, err := invalidPeer.Marshal() + require.NoError(t, err) + require.NoError(t, db.Set(peerInfoKey(t, invalidID), invalidBz)) + + validID := types.NodeID(strings.Repeat("b", 40)) + validAddr := p2p.NodeAddress{ + Protocol: "mconn", + NodeID: validID, + Hostname: "127.0.0.1", + Port: 26656, + } + validPeer := &p2pproto.PeerInfo{ + ID: string(validID), + AddressInfo: []*p2pproto.PeerAddressInfo{{ + Address: validAddr.String(), + }}, + } + validBz, err := validPeer.Marshal() + require.NoError(t, err) + require.NoError(t, db.Set(peerInfoKey(t, validID), validBz)) + + selfID := types.NodeID(strings.Repeat("c", 40)) + peerManager, err := p2p.NewPeerManager(context.Background(), selfID, db, p2p.PeerManagerOptions{}) + require.NoError(t, err) + peerManager.SetLogger(logger) + + peers := peerManager.Peers() + require.Contains(t, peers, validID) + require.NotContains(t, peers, invalidID) +} + +func peerInfoKey(t *testing.T, id types.NodeID) []byte { + t.Helper() + key, err := orderedcode.Append(nil, int64(1), string(id)) + require.NoError(t, err) + return key +} + func TestNewPeerManager(t *testing.T) { ctx := context.TODO() // Zero options should be valid.