test: add tests (#2372)

<!--
Please read and fill out this form before submitting your PR.

Please make sure you have reviewed our contributors guide before
submitting your
first PR.

NOTE: PR titles should follow semantic commits:
https://www.conventionalcommits.org/en/v1.0.0/
-->

## Overview

This pr adds test cases and fixes an issue or two 


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **New Features**
	- Added a method to indicate if the node is running.
- **Bug Fixes**
- Improved peer listing to ensure a client does not include itself among
its connected or discovered peers.
- **Tests**
- Introduced comprehensive tests for node lifecycle management and
client peer/network info retrieval.
- **Refactor**
- Unified node lifecycle management into a single method with enhanced
error handling and logging.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: tac0turtle <you@example.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Author: 3 people

node/light.go

@@ -31,6 +31,8 @@ type LightNode struct {
 	Store        store.Store
 	rpcServer    *http.Server
 	nodeConfig   config.Config
+
+	running bool
 }
 
 func newLightNode(
@@ -59,8 +61,21 @@ func newLightNode(
 	return node, nil
 }
 
-// OnStart starts the P2P and HeaderSync services
-func (ln *LightNode) OnStart(ctx context.Context) error {
+// IsRunning returns true if the node is running.
+func (ln *LightNode) IsRunning() bool {
+	return ln.running
+}
+
+// Run implements the Service interface.
+// It starts all subservices and manages the node's lifecycle.
+func (ln *LightNode) Run(parentCtx context.Context) error {
+	ctx, cancelNode := context.WithCancel(parentCtx)
+	defer func() {
+		ln.running = false
+		cancelNode()
+	}()
+
+	ln.running = true
 	// Start RPC server
 	handler, err := rpcserver.NewServiceHandler(ln.Store, ln.P2P)
 	if err != nil {
@@ -76,44 +91,77 @@ func (ln *LightNode) OnStart(ctx context.Context) error {
 	}
 
 	go func() {
-		if err := ln.rpcServer.ListenAndServe(); err != http.ErrServerClosed {
+		ln.Logger.Info("started RPC server", "addr", ln.nodeConfig.RPC.Address)
+		if err := ln.rpcServer.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
 			ln.Logger.Error("RPC server error", "err", err)
 		}
 	}()
 
-	ln.Logger.Info("Started RPC server", "addr", ln.nodeConfig.RPC.Address)
-
 	if err := ln.P2P.Start(ctx); err != nil {
-		return err
+		return fmt.Errorf("error while starting P2P client: %w", err)
 	}
 
 	if err := ln.hSyncService.Start(ctx); err != nil {
 		return fmt.Errorf("error while starting header sync service: %w", err)
 	}
 
-	return nil
-}
+	<-parentCtx.Done()
+	ln.Logger.Info("context canceled, stopping node")
+	cancelNode()
 
-// OnStop stops the light node
-func (ln *LightNode) OnStop(ctx context.Context) {
-	ln.Logger.Info("halting light node...")
+	ln.Logger.Info("halting light node and its sub services...")
 
-	// Use a timeout context to ensure shutdown doesn't hang
 	shutdownCtx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
 	defer cancel()
 
-	err := ln.P2P.Close()
-	err = errors.Join(err, ln.hSyncService.Stop(shutdownCtx))
+	var multiErr error
+
+	// Stop Header Sync Service
+	err = ln.hSyncService.Stop(shutdownCtx)
+	if err != nil {
+		if !errors.Is(err, context.Canceled) && !errors.Is(err, context.DeadlineExceeded) {
+			multiErr = errors.Join(multiErr, fmt.Errorf("stopping header sync service: %w", err))
+		} else {
+			ln.Logger.Debug("header sync service stop context ended", "reason", err)
+		}
+	}
 
+	// Shutdown RPC Server
 	if ln.rpcServer != nil {
-		err = errors.Join(err, ln.rpcServer.Shutdown(shutdownCtx))
+		err = ln.rpcServer.Shutdown(shutdownCtx)
+		if err != nil && !errors.Is(err, http.ErrServerClosed) {
+			multiErr = errors.Join(multiErr, fmt.Errorf("shutting down RPC server: %w", err))
+		} else {
+			ln.Logger.Debug("RPC server shutdown context ended", "reason", err)
+		}
 	}
 
-	err = errors.Join(err, ln.Store.Close())
-	ln.Logger.Error("errors while stopping node:", "errors", err)
-}
+	// Stop P2P Client
+	err = ln.P2P.Close()
+	if err != nil {
+		multiErr = errors.Join(multiErr, fmt.Errorf("closing P2P client: %w", err))
+	}
 
-// IsRunning returns true if the node is running.
-func (ln *LightNode) IsRunning() bool {
-	return ln.P2P != nil && ln.hSyncService != nil
+	if err = ln.Store.Close(); err != nil {
+		multiErr = errors.Join(multiErr, fmt.Errorf("closing store: %w", err))
+	} else {
+		ln.Logger.Debug("store closed")
+	}
+
+	if multiErr != nil {
+		if unwrapper, ok := multiErr.(interface{ Unwrap() []error }); ok {
+			for _, err := range unwrapper.Unwrap() {
+				ln.Logger.Error("error during shutdown", "error", err)
+			}
+		} else {
+			ln.Logger.Error("error during shutdown", "error", multiErr)
+		}
+		ln.Logger.Error("error during shutdown", "error", err)
+	}
+
+	if ctx.Err() != nil {
+		return ctx.Err()
+	}
+
+	return multiErr
 }

node/light_test.go

@@ -0,0 +1,71 @@
+package node
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"cosmossdk.io/log"
+	ds "github.com/ipfs/go-datastore"
+	ds_sync "github.com/ipfs/go-datastore/sync"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/rollkit/rollkit/pkg/config"
+	"github.com/rollkit/rollkit/pkg/genesis"
+	"github.com/rollkit/rollkit/pkg/p2p"
+	p2p_key "github.com/rollkit/rollkit/pkg/p2p/key"
+)
+
+// TestLightNodeLifecycle tests the light node's lifecycle.
+func TestLightNodeLifecycle(t *testing.T) {
+	assert := assert.New(t)
+	require := require.New(t)
+
+	conf := config.Config{
+		RootDir: t.TempDir(),
+		P2P: config.P2PConfig{
+			ListenAddress: "/ip4/127.0.0.1/tcp/0",
+		},
+		RPC: config.RPCConfig{
+			Address: "127.0.0.1:0",
+		},
+	}
+	gen := genesis.Genesis{
+		ChainID: "test-chain",
+	}
+	p2pKey, err := p2p_key.GenerateNodeKey()
+	require.NoError(err)
+
+	logger := log.NewNopLogger()
+	p2pMetrics := p2p.NopMetrics()
+
+	db := ds_sync.MutexWrap(ds.NewMapDatastore())
+
+	p2pClient, err := p2p.NewClient(conf, p2pKey, db, logger, p2pMetrics)
+	require.NoError(err)
+
+	ln, err := newLightNode(conf, gen, p2pClient, db, logger)
+	require.NoError(err)
+	require.NotNil(ln)
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	runResult := make(chan error, 1)
+	go func() {
+		runResult <- ln.Run(ctx)
+	}()
+
+	time.Sleep(200 * time.Millisecond)
+
+	cancel()
+
+	// Wait for the Run method to return.
+	select {
+	case err := <-runResult:
+		assert.ErrorIs(err, context.Canceled, "Expected context.Canceled error")
+	case <-time.After(5 * time.Second):
+		t.Fatal("Node did not stop gracefully within the timeout")
+	}
+}

pkg/p2p/client.go

@@ -219,7 +219,9 @@ func (c *Client) Info() (string, string, string, error) {
 func (c *Client) PeerIDs() []peer.ID {
 	peerIDs := make([]peer.ID, 0)
 	for _, conn := range c.host.Network().Conns() {
-		peerIDs = append(peerIDs, conn.RemotePeer())
+		if conn.RemotePeer() != c.host.ID() {
+			peerIDs = append(peerIDs, conn.RemotePeer())
+		}
 	}
 	return peerIDs
 }
@@ -403,8 +405,11 @@ func (c *Client) GetPeers() ([]peer.AddrInfo, error) {
 	}
 
 	var peers []peer.AddrInfo
-	for peer := range peerCh {
-		peers = append(peers, peer)
+	for p := range peerCh {
+		if p.ID == c.host.ID() {
+			continue
+		}
+		peers = append(peers, p)
 	}
 	return peers, nil
 }

pkg/p2p/client_test.go

@@ -2,6 +2,7 @@ package p2p
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"path/filepath"
 	"testing"
@@ -11,6 +12,7 @@ import (
 	"github.com/ipfs/go-datastore"
 	dssync "github.com/ipfs/go-datastore/sync"
 	libp2p "github.com/libp2p/go-libp2p"
+	"github.com/libp2p/go-libp2p/core/host"
 	"github.com/libp2p/go-libp2p/core/peer"
 	"github.com/multiformats/go-multiaddr"
 	"github.com/stretchr/testify/assert"
@@ -249,3 +251,138 @@ func ClientInitFiles(t *testing.T, tempDir string) {
 	}
 
 }
+
+// waitForCondition waits for a condition to be true or a timeout to occur.
+// It checks the condition immediately, then polls at a regular interval.
+func waitForCondition(timeout time.Duration, conditionFunc func() bool) error {
+	// Check immediately
+	if conditionFunc() {
+		return nil
+	}
+
+	// Poll
+	ticker := time.NewTicker(100 * time.Millisecond) // Adjust poll interval as needed
+	defer ticker.Stop()
+
+	timer := time.NewTimer(timeout)
+	defer timer.Stop()
+
+	for {
+		select {
+		case <-ticker.C:
+			if conditionFunc() {
+				return nil
+			}
+		case <-timer.C:
+			return fmt.Errorf("timed out after %v waiting for condition", timeout)
+		}
+	}
+}
+
+func TestClientInfoMethods(t *testing.T) {
+	require := require.New(t)
+	assert := assert.New(t)
+	logger := log.NewTestLogger(t)
+
+	tempDir := t.TempDir()
+	ClientInitFiles(t, tempDir)
+	conf := config.DefaultConfig
+	conf.RootDir = tempDir
+	conf.ChainID = "test-chain"
+
+	mn := mocknet.New()
+	defer mn.Close()
+
+	var clients []*Client
+	var hosts []host.Host
+	var err error
+
+	for i := 0; i < 3; i++ {
+		nodeKey, e := key.GenerateNodeKey()
+		require.NoError(e)
+		h, e := mn.AddPeer(nodeKey.PrivKey, multiaddr.StringCast("/ip4/127.0.0.1/tcp/0"))
+		require.NoError(e)
+		c, e := NewClientWithHost(conf, nodeKey, dssync.MutexWrap(datastore.NewMapDatastore()), logger, NopMetrics(), h)
+		require.NoError(e)
+		clients = append(clients, c)
+		hosts = append(hosts, h)
+		defer c.Close()
+	}
+
+	client0 := clients[0]
+	client1 := clients[1]
+	client2 := clients[2]
+
+	// Link all peers in the mocknet
+	err = mn.LinkAll()
+	require.NoError(err)
+	err = mn.ConnectAllButSelf()
+	require.NoError(err)
+
+	// Start clients
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	for _, c := range clients {
+		err = c.Start(ctx)
+		require.NoError(err)
+	}
+
+	// Wait for connections to establish and DHT to settle
+	err = waitForCondition(5*time.Second, func() bool {
+		connectedPeers := client0.Peers()
+		return len(connectedPeers) == 2
+	})
+
+	require.NoError(err)
+	t.Run("GetNetworkInfo", func(t *testing.T) {
+		netInfo, err := client0.GetNetworkInfo()
+		assert.NoError(err)
+		assert.Equal(client0.host.ID().String(), netInfo.ID)
+		assert.Contains(netInfo.ListenAddress[0], hosts[0].Addrs()[0].String()) // Use h0.Addrs()[0].String()
+		assert.ElementsMatch([]peer.ID{client1.host.ID(), client2.host.ID()}, netInfo.ConnectedPeers)
+	})
+
+	t.Run("GetPeers", func(t *testing.T) {
+		peers, err := client0.GetPeers()
+		assert.NoError(err)
+		expectedPeerIDs := []peer.ID{client1.host.ID(), client2.host.ID()}
+		actualPeerIDs := make([]peer.ID, len(peers))
+		for i, p := range peers {
+			actualPeerIDs[i] = p.ID
+		}
+		assert.ElementsMatch(expectedPeerIDs, actualPeerIDs)
+	})
+
+	t.Run("Peers", func(t *testing.T) {
+		connectedPeers := client0.Peers()
+		assert.Len(connectedPeers, 2)
+
+		foundClient1 := false
+		foundClient2 := false
+		for _, p := range connectedPeers {
+			if p.NodeInfo.NodeID == client1.host.ID().String() {
+				foundClient1 = true
+			} else if p.NodeInfo.NodeID == client2.host.ID().String() {
+				foundClient2 = true
+			}
+			assert.Equal(client0.conf.ListenAddress, p.NodeInfo.ListenAddr)
+			assert.Equal(client0.chainID, p.NodeInfo.Network)
+			assert.NotEmpty(p.RemoteIP)
+		}
+		assert.True(foundClient1, "client1 not found in connected peers")
+		assert.True(foundClient2, "client2 not found in connected peers")
+	})
+
+	t.Run("PeerIDs", func(t *testing.T) {
+		peerIDs := client0.PeerIDs()
+		assert.ElementsMatch([]peer.ID{client1.host.ID(), client2.host.ID()}, peerIDs)
+	})
+
+	t.Run("Info", func(t *testing.T) {
+		nodeID, listenAddr, chainID, err := client0.Info()
+		assert.NoError(err)
+		assert.NotEmpty(nodeID)
+		assert.Equal(client0.conf.ListenAddress, listenAddr)
+		assert.Equal(client0.chainID, chainID)
+	})
+}