Guard discv4/discv5 protocol pointers on the generic node

nodes/node.go

@@ -165,27 +165,38 @@ func (n *Node) SetAddr(addr *net.UDPAddr) {
 
 // V4 returns the discv4 node if available.
 func (n *Node) V4() *node.Node {
+	n.mu.RLock()
+	defer n.mu.RUnlock()
 	return n.v4Node
 }
 
 // V5 returns the discv5 node if available.
 func (n *Node) V5() *discv5node.Node {
+	n.mu.RLock()
+	defer n.mu.RUnlock()
 	return n.v5Node
 }
 
 // HasV4 returns true if this node supports discv4.
 func (n *Node) HasV4() bool {
+	n.mu.RLock()
+	defer n.mu.RUnlock()
 	return n.v4Node != nil
 }
 
 // HasV5 returns true if this node supports discv5.
 func (n *Node) HasV5() bool {
+	n.mu.RLock()
+	defer n.mu.RUnlock()
 	return n.v5Node != nil
 }
 
 // SetV4 sets the discv4 node and marks protocol support dirty.
 func (n *Node) SetV4(v4 *node.Node) {
+	n.mu.Lock()
 	n.v4Node = v4
+	n.mu.Unlock()
+
 	if v4 != nil && n.nodeStats != nil {
 		// Ensure callback is set up (in case stats were created elsewhere)
 		n.setupSharedStatsCallback()
@@ -197,7 +208,10 @@ func (n *Node) SetV4(v4 *node.Node) {
 
 // SetV5 sets the discv5 node and marks protocol support dirty.
 func (n *Node) SetV5(v5 *discv5node.Node) {
+	n.mu.Lock()
 	n.v5Node = v5
+	n.mu.Unlock()
+
 	if v5 != nil && n.nodeStats != nil {
 		// Ensure callback is set up (in case stats were created elsewhere)
 		n.setupSharedStatsCallback()
@@ -209,8 +223,12 @@ func (n *Node) SetV5(v5 *discv5node.Node) {
 
 // Enode returns the node's enode:// URL representation.
 func (n *Node) Enode() *enode.Enode {
-	if n.v4Node != nil {
-		return n.v4Node.Enode()
+	n.mu.RLock()
+	v4 := n.v4Node
+	n.mu.RUnlock()
+
+	if v4 != nil {
+		return v4.Enode()
 	}
 
 	// Build from generic node info
@@ -388,8 +406,12 @@ func (n *Node) Record() *enr.Record {
 // PeerID returns the libp2p peer ID for this node.
 // Delegates to the v5 node if available, otherwise builds it from the public key.
 func (n *Node) PeerID() string {
-	if n.v5Node != nil {
-		return n.v5Node.PeerID()
+	n.mu.RLock()
+	v5 := n.v5Node
+	n.mu.RUnlock()
+
+	if v5 != nil {
+		return v5.PeerID()
 	}
 	// Fallback: build peer ID from public key
 	if n.pubKey != nil {
@@ -410,8 +432,11 @@ func (n *Node) UpdateENR(newRecord *enr.Record) bool {
 		n.enr = newRecord
 
 		// Update v5 node if available
-		if n.v5Node != nil {
-			n.v5Node.UpdateENR(newRecord)
+		n.mu.RLock()
+		v5 := n.v5Node
+		n.mu.RUnlock()
+		if v5 != nil {
+			v5.UpdateENR(newRecord)
 		}
 
 		return true
@@ -434,7 +459,11 @@ func (n *Node) IsAlive(maxAge time.Duration, maxFailures int) bool {
 // CalculateScore computes a quality score for the node.
 // Delegates to the v5 node if available.
 func (n *Node) CalculateScore(forkInfo *ForkScoringInfo) float64 {
-	if n.v5Node != nil {
+	n.mu.RLock()
+	v5 := n.v5Node
+	n.mu.RUnlock()
+
+	if v5 != nil {
 		// Cast forkInfo to the v5 node's ForkScoringInfo type
 		if forkInfo != nil {
 			// Convert table.ForkScoringInfo to discv5/node.ForkScoringInfo
@@ -444,10 +473,10 @@ func (n *Node) CalculateScore(forkInfo *ForkScoringInfo) float64 {
 				GenesisForkDigest:  forkInfo.GenesisForkDigest,
 				GracePeriodEnd:     forkInfo.GracePeriodEnd,
 			}
-			return n.v5Node.CalculateScore(v5ForkInfo)
+			return v5.CalculateScore(v5ForkInfo)
 		}
 		// If forkInfo is nil or wrong type, call with nil
-		return n.v5Node.CalculateScore(nil)
+		return v5.CalculateScore(nil)
 	}
 	// Basic fallback score based on success rate
 	successCount := n.SuccessCount()

nodes/node_test.go

@@ -0,0 +1,176 @@
+package nodes
+
+import (
+	"net"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/ethereum/go-ethereum/crypto"
+	discv4node "github.com/ethpandaops/bootnodoor/discv4/node"
+	discv5node "github.com/ethpandaops/bootnodoor/discv5/node"
+	"github.com/ethpandaops/bootnodoor/enr"
+)
+
+// newTestNode builds a node for exercising the protocol-pointer accessors.
+// nodeStats is left nil so SetV4/SetV5 only touch the v4Node/v5Node pointers,
+// keeping these tests focused on their synchronization.
+func newTestNode(t *testing.T) *Node {
+	t.Helper()
+	key, err := crypto.GenerateKey()
+	if err != nil {
+		t.Fatalf("generate key: %v", err)
+	}
+	return &Node{
+		pubKey: &key.PublicKey,
+		addr:   &net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 30303},
+	}
+}
+
+func makeV4(t *testing.T) *discv4node.Node {
+	t.Helper()
+	key, err := crypto.GenerateKey()
+	if err != nil {
+		t.Fatalf("generate key: %v", err)
+	}
+	return discv4node.New(&key.PublicKey, &net.UDPAddr{IP: net.IPv4(2, 2, 2, 2), Port: 30303})
+}
+
+func makeV5(t *testing.T) *discv5node.Node {
+	t.Helper()
+	key, err := crypto.GenerateKey()
+	if err != nil {
+		t.Fatalf("generate key: %v", err)
+	}
+	rec := enr.New()
+	if err := rec.Set("ip", net.IPv4(3, 3, 3, 3)); err != nil {
+		t.Fatalf("set ip: %v", err)
+	}
+	if err := rec.Set("udp", uint16(30303)); err != nil {
+		t.Fatalf("set udp: %v", err)
+	}
+	if err := rec.Sign(key); err != nil {
+		t.Fatalf("sign: %v", err)
+	}
+	v5, err := discv5node.New(rec)
+	if err != nil {
+		t.Fatalf("new v5 node: %v", err)
+	}
+	return v5
+}
+
+// TestNodeConcurrentProtocolPointerAccess exercises the readers and writers of
+// the v4Node/v5Node pointers concurrently. Under the race detector it fails if
+// any access to those pointers is unsynchronized.
+func TestNodeConcurrentProtocolPointerAccess(t *testing.T) {
+	n := newTestNode(t)
+	v4 := makeV4(t)
+	v5 := makeV5(t)
+
+	var wg sync.WaitGroup
+	stop := make(chan struct{})
+
+	writer := func(set func(i int)) {
+		defer wg.Done()
+		for i := 0; ; i++ {
+			select {
+			case <-stop:
+				return
+			default:
+				set(i)
+			}
+		}
+	}
+
+	wg.Add(2)
+	go writer(func(i int) {
+		if i%2 == 0 {
+			n.SetV4(v4)
+		} else {
+			n.SetV4(nil)
+		}
+	})
+	go writer(func(i int) {
+		if i%2 == 0 {
+			n.SetV5(v5)
+		} else {
+			n.SetV5(nil)
+		}
+	})
+
+	for r := 0; r < 4; r++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			for {
+				select {
+				case <-stop:
+					return
+				default:
+					_ = n.V4()
+					_ = n.V5()
+					_ = n.HasV4()
+					_ = n.HasV5()
+					_ = n.PeerID()
+					_ = n.Enode()
+					_ = n.String()
+				}
+			}
+		}()
+	}
+
+	time.Sleep(200 * time.Millisecond)
+	close(stop)
+	wg.Wait()
+}
+
+// TestNodeNoNilDerefDuringProtocolSwap targets the check-then-deref reader
+// PeerID: while a node repeatedly gains and loses its v5 support, PeerID must
+// never dereference a pointer that was cleared between the nil check and the
+// call. CalculateScore reads the same pointer through the identical locked
+// path.
+func TestNodeNoNilDerefDuringProtocolSwap(t *testing.T) {
+	n := newTestNode(t)
+	v5 := makeV5(t)
+
+	var wg sync.WaitGroup
+	stop := make(chan struct{})
+
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		for i := 0; ; i++ {
+			select {
+			case <-stop:
+				return
+			default:
+				if i%2 == 0 {
+					n.SetV5(v5)
+				} else {
+					n.SetV5(nil)
+				}
+			}
+		}
+	}()
+
+	for r := 0; r < 4; r++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			for {
+				select {
+				case <-stop:
+					return
+				default:
+					_ = n.PeerID()
+					_ = n.V5()
+					_ = n.HasV5()
+				}
+			}
+		}()
+	}
+
+	time.Sleep(200 * time.Millisecond)
+	close(stop)
+	wg.Wait()
+}