Post-PR8 follow-up: host lifecycle state enum, signing/verification tests, disconnect test realism

pkg/p2p/host/host.go

@@ -38,6 +38,15 @@ const (
 	validationTimeout = 20 * time.Second
 )
 
+// hostState represents the lifecycle state of the Host.
+type hostState int
+
+const (
+	hostStopped  hostState = iota // initial state; never started or fully shut down
+	hostRunning                   // host is up and processing messages
+	hostStopping                  // Stop has been called; shutdown is in progress
+)
+
 // Host manages the P2P network functionality
 type Host struct {
 	cfg       *config.P2PConfig
@@ -48,7 +57,7 @@ type Host struct {
 	peerStore *PeerStore
 	validator *security.Validator
 	logger    *zap.Logger
-	running   bool
+	state     hostState
 
 	// Channels for coordination
 	shutdown   chan struct{}
@@ -164,10 +173,16 @@ func NewHost(ctx context.Context, cfg *config.Config, logger *zap.Logger, repo d
 // Start begins P2P network operations
 func (h *Host) Start(ctx context.Context) error {
 	h.mu.Lock()
-	if h.running {
+	if h.state != hostStopped {
 		h.mu.Unlock()
-		return fmt.Errorf("host is already running")
+		if h.state == hostRunning {
+			return fmt.Errorf("host is already running")
+		}
+		return fmt.Errorf("host is shutting down, cannot start")
 	}
+	// Mark as running while the lock is still held so that concurrent Start
+	// calls are rejected before any startup work begins.
+	h.state = hostRunning
 	h.mu.Unlock()
 
 	h.logger.Info("Starting P2P host",
@@ -189,21 +204,20 @@ func (h *Host) Start(ctx context.Context) error {
 		h.logger.Warn("Failed to connect to some bootstrap peers", zap.Error(err))
 	}
 	h.status.UpdateStatus(true, false, nil)
-	h.mu.Lock()
-	h.running = true
-	h.mu.Unlock()
 
 	return nil
 }
 
 // Stop gracefully shuts down the P2P host
 func (h *Host) Stop() error {
 	h.mu.Lock()
-	if !h.running {
+	if h.state != hostRunning {
 		h.mu.Unlock()
 		return nil
 	}
-	h.running = false
+	// Transition to stopping so that IsRunning() immediately reflects the
+	// in-progress shutdown while work below is still executing.
+	h.state = hostStopping
 	h.mu.Unlock()
 
 	h.logger.Info("Stopping P2P host")
@@ -230,9 +244,17 @@ func (h *Host) Stop() error {
 		}
 	}
 	if err := h.host.Close(); err != nil {
+		// Even on error, record that the host is no longer running.
+		h.mu.Lock()
+		h.state = hostStopped
+		h.mu.Unlock()
 		return fmt.Errorf("failed to close libp2p host: %w", err)
 	}
 
+	h.mu.Lock()
+	h.state = hostStopped
+	h.mu.Unlock()
+
 	h.logger.Info("P2P host stopped")
 	return nil
 }
@@ -599,11 +621,13 @@ func (h *Host) StringToPeerID(peerIDStr string) (libp2pPeer.ID, error) {
 	return libp2pPeer.Decode(peerIDStr)
 }
 
-// IsRunning returns the current running state of the host
+// IsRunning returns true only when the host is fully started and not yet
+// stopping. It returns false both before Start is called and once Stop has
+// been called (even while shutdown is still in progress).
 func (h *Host) IsRunning() bool {
 	h.mu.RLock()
 	defer h.mu.RUnlock()
-	return h.running
+	return h.state == hostRunning
 }
 
 func (h *Host) RequestData(ctx context.Context, peerID string, request data.DataRequest) error {

pkg/p2p/host/host_test.go

@@ -0,0 +1,39 @@
+package host
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// TestHostState_InitialStateIsNotRunning verifies that a zero-value Host
+// reports IsRunning() == false.  This guards against accidental initialisation
+// of the state field to a truthy value.
+func TestHostState_InitialStateIsNotRunning(t *testing.T) {
+	h := &Host{}
+	assert.False(t, h.IsRunning(), "a freshly-created host must not report itself as running")
+}
+
+// TestHostState_RunningStateIsDetected verifies that setting state to
+// hostRunning causes IsRunning to return true.
+func TestHostState_RunningStateIsDetected(t *testing.T) {
+	h := &Host{state: hostRunning}
+	assert.True(t, h.IsRunning(), "IsRunning must return true when state == hostRunning")
+}
+
+// TestHostState_StoppingIsNotRunning verifies that IsRunning returns false
+// while the host is in the stopping state.  This is the key semantic
+// improvement: callers that observe IsRunning()==false cannot distinguish
+// "never started" from "shutting down", but they correctly know the host is
+// not accepting new work.
+func TestHostState_StoppingIsNotRunning(t *testing.T) {
+	h := &Host{state: hostStopping}
+	assert.False(t, h.IsRunning(), "IsRunning must return false while the host is stopping")
+}
+
+// TestHostState_StoppedIsNotRunning verifies that IsRunning returns false
+// after the host has fully stopped.
+func TestHostState_StoppedIsNotRunning(t *testing.T) {
+	h := &Host{state: hostStopped}
+	assert.False(t, h.IsRunning(), "IsRunning must return false after the host has stopped")
+}

pkg/p2p/message/message_test.go

@@ -3,9 +3,11 @@ package message
 import (
 	"bytes"
 	"encoding/json"
+	"math/rand"
 	"testing"
 	"time"
 
+	libp2pCrypto "github.com/libp2p/go-libp2p/core/crypto"
 	"github.com/libp2p/go-libp2p/core/peer"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -147,3 +149,72 @@ func TestMarshalUnmarshal_RoundTrip(t *testing.T) {
 	assert.Equal(t, msg.ID, parsed["id"])
 	assert.NotNil(t, parsed["data"])
 }
+
+// TestSignVerify_RoundTrip is an end-to-end signing/verification test that
+// uses real libp2p Ed25519 crypto primitives.  It verifies that the bytes
+// produced by MarshalWithoutSignature can be signed and that the resulting
+// signature verifies against the same payload.
+func TestSignVerify_RoundTrip(t *testing.T) {
+	privKey, pubKey, err := libp2pCrypto.GenerateEd25519Key(rand.New(rand.NewSource(42)))
+	require.NoError(t, err)
+
+	msg := makeTestMessage()
+	payload, err := msg.MarshalWithoutSignature()
+	require.NoError(t, err)
+
+	sig, err := privKey.Sign(payload)
+	require.NoError(t, err)
+	msg.Signature = sig
+
+	// Re-derive the payload from the now-signed message; the signature field
+	// must not be included so the payload bytes are identical to what was signed.
+	payload2, err := msg.MarshalWithoutSignature()
+	require.NoError(t, err)
+
+	ok, err := pubKey.Verify(payload2, msg.Signature)
+	require.NoError(t, err)
+	assert.True(t, ok, "signature must verify successfully for an unmodified message")
+}
+
+// TestSignVerify_TamperedMessage verifies that modifying any data field after
+// signing causes signature verification to fail.
+func TestSignVerify_TamperedMessage(t *testing.T) {
+	privKey, pubKey, err := libp2pCrypto.GenerateEd25519Key(rand.New(rand.NewSource(99)))
+	require.NoError(t, err)
+
+	msg := makeTestMessage()
+	payload, err := msg.MarshalWithoutSignature()
+	require.NoError(t, err)
+
+	sig, err := privKey.Sign(payload)
+	require.NoError(t, err)
+	msg.Signature = sig
+
+	// Tamper with the data field after signing.
+	msg.Data = map[string]interface{}{"symbol": "EVIL", "price": 999999.0}
+
+	tamperedPayload, err := msg.MarshalWithoutSignature()
+	require.NoError(t, err)
+
+	ok, _ := pubKey.Verify(tamperedPayload, msg.Signature)
+	assert.False(t, ok, "signature must not verify after the message data has been tampered with")
+}
+
+// TestSignVerify_WrongKey verifies that a signature produced with one key
+// does not verify against a different public key.
+func TestSignVerify_WrongKey(t *testing.T) {
+	privKey, _, err := libp2pCrypto.GenerateEd25519Key(rand.New(rand.NewSource(1)))
+	require.NoError(t, err)
+	_, otherPubKey, err := libp2pCrypto.GenerateEd25519Key(rand.New(rand.NewSource(2)))
+	require.NoError(t, err)
+
+	msg := makeTestMessage()
+	payload, err := msg.MarshalWithoutSignature()
+	require.NoError(t, err)
+
+	sig, err := privKey.Sign(payload)
+	require.NoError(t, err)
+
+	ok, _ := otherPubKey.Verify(payload, sig)
+	assert.False(t, ok, "signature must not verify against a different public key")
+}

pkg/peer/connection_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/libp2p/go-libp2p/core/peer"
 	ma "github.com/multiformats/go-multiaddr"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"go.uber.org/zap/zaptest"
 )
 
@@ -168,3 +169,42 @@ func TestConnectionAccounting_GetConnectedPeers(t *testing.T) {
 	assert.Len(t, got, 1)
 	assert.Equal(t, peerB, got[0])
 }
+
+// TestDisconnectPeer_UnknownPeer verifies that calling DisconnectPeer for a
+// peer that has never connected returns an error without touching the host
+// network.  This exercises the guard in DisconnectPeer without requiring a
+// real libp2p host.
+func TestDisconnectPeer_UnknownPeer(t *testing.T) {
+	cm := newTestConnectionManager(t)
+	unknown := peer.ID("never-seen-peer")
+
+	err := cm.DisconnectPeer(unknown)
+	require.Error(t, err, "disconnecting an untracked peer must return an error")
+	assert.Contains(t, err.Error(), "not connected",
+		"error message should indicate the peer is not connected")
+
+	// State must remain clean.
+	assert.Equal(t, 0, cm.ConnectionCount())
+	assert.False(t, cm.IsConnected(unknown))
+}
+
+// TestDisconnectPeer_AfterHandlerDisconnect verifies that once the network
+// notifier has already fired handleDisconnected, calling DisconnectPeer for
+// the same peer also returns an error (the peer is no longer in activeConns).
+func TestDisconnectPeer_AfterHandlerDisconnect(t *testing.T) {
+	cm := newTestConnectionManager(t)
+	peerA := peer.ID("peerA")
+	conn := &stubConn{remote: peerA}
+
+	cm.handleConnected(nil, conn)
+	assert.Equal(t, 1, cm.ConnectionCount())
+
+	// Simulate the network notifier firing (e.g. remote end closed).
+	cm.handleDisconnected(nil, conn)
+	assert.Equal(t, 0, cm.ConnectionCount())
+
+	// DisconnectPeer should now report the peer as not connected.
+	err := cm.DisconnectPeer(peerA)
+	require.Error(t, err, "DisconnectPeer must return an error for a peer that already disconnected")
+	assert.Contains(t, err.Error(), "not connected")
+}