discv4/discv5 remainders on develop: NEIGHBORS, NODES cap, node-map flood eviction

discv4/protocol/handler.go

@@ -1058,8 +1058,11 @@ func (h *Handler) sendPong(to *node.Node, addr *net.UDPAddr, localAddr *net.UDPA
 
 // sendNeighbors sends NEIGHBORS response(s).
 func (h *Handler) sendNeighbors(to *node.Node, addr *net.UDPAddr, localAddr *net.UDPAddr, nodes []*node.Node) error {
-	// Split nodes into packets of MaxNeighbors
-	for i := 0; i < len(nodes); i += MaxNeighbors {
+	// Split nodes into packets of MaxNeighbors. Always send at least one packet,
+	// even when we have no nodes to offer: go-ethereum's querier waits for a
+	// NEIGHBORS reply and only stops early once at least one arrives, so a silent
+	// (zero-packet) response makes it wait out the full request timeout.
+	for i := 0; i == 0 || i < len(nodes); i += MaxNeighbors {
 		end := i + MaxNeighbors
 		if end > len(nodes) {
 			end = len(nodes)
@@ -1069,10 +1072,18 @@ func (h *Handler) sendNeighbors(to *node.Node, addr *net.UDPAddr, localAddr *net
 		nodeRecords := make([]NodeRecord, len(batch))
 
 		for j, n := range batch {
+			// Advertise the node's real TCP port from its ENR when known;
+			// only fall back to the UDP port if no ENR tcp entry is available.
+			tcpPort := uint16(n.Addr().Port)
+			if rec := n.ENR(); rec != nil {
+				if t := rec.TCP(); t != 0 {
+					tcpPort = t
+				}
+			}
 			nodeRecords[j] = NodeRecord{
 				IP:  n.Addr().IP,
 				UDP: uint16(n.Addr().Port),
-				TCP: uint16(n.Addr().Port),
+				TCP: tcpPort,
 				ID:  EncodePubkey(n.PublicKey()),
 			}
 		}
@@ -1157,12 +1168,26 @@ func (h *Handler) lookupOrCreateNode(id node.ID, pubkey *ecdsa.PublicKey, addr *
 	n = node.New(pubkey, addr)
 	n.UpdateLastSeen()
 
-	// Bound the map so an unauthenticated flood of distinct node IDs (for
-	// example fabricated NEIGHBORS records) cannot grow it without limit. Stale
-	// unbonded entries are reclaimed by cleanup; until a slot frees up we still
-	// return the node so the packet is handled, but we do not retain it.
+	// Bound the map so an unauthenticated flood of distinct node IDs (for example
+	// fabricated NEIGHBORS records, or signed PINGs from generated keys) cannot grow
+	// it without limit. When full, evict one unbonded entry to make room rather than
+	// dropping the new node: otherwise a flood that pins the map at MaxNodes would
+	// lock out genuine new peers (their node is never retained, so their inbound PING
+	// can never lead to a bond). Bonded entries are real, endpoint-proven peers and
+	// are never evicted here; if every entry is bonded (genuine load, not a flood) we
+	// leave the map as-is and return the node without retaining it.
 	if len(h.nodes) >= h.config.MaxNodes {
-		return n
+		evicted := false
+		for eid, en := range h.nodes {
+			if !en.IsBonded() {
+				delete(h.nodes, eid)
+				evicted = true
+				break
+			}
+		}
+		if !evicted {
+			return n
+		}
 	}
 
 	h.nodes[id] = n

discv4/protocol/remainder_fixes_test.go

@@ -0,0 +1,137 @@
+package protocol
+
+import (
+	"context"
+	"net"
+	"testing"
+	"time"
+
+	ethcrypto "github.com/ethereum/go-ethereum/crypto"
+	"github.com/ethpandaops/bootnodoor/discv4/node"
+	"github.com/ethpandaops/bootnodoor/enr"
+)
+
+// captureTransport records every packet sent so a test can decode it.
+type captureTransport struct{ sent [][]byte }
+
+func (c *captureTransport) SendTo(b []byte, _ *net.UDPAddr) error {
+	c.sent = append(c.sent, b)
+	return nil
+}
+func (c *captureTransport) Send(b []byte, _ *net.UDPAddr, _ *net.UDPAddr) error {
+	c.sent = append(c.sent, b)
+	return nil
+}
+
+// TestSendNeighborsAlwaysSendsAtLeastOnePacket verifies BUG3: an empty result
+// still produces exactly one (empty) NEIGHBORS packet, so a go-ethereum querier
+// returns immediately instead of waiting out its request timeout on silence.
+func TestSendNeighborsAlwaysSendsAtLeastOnePacket(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	key, _ := ethcrypto.GenerateKey()
+	ct := &captureTransport{}
+	h := NewHandler(ctx, HandlerConfig{PrivateKey: key}, ct)
+
+	pub, id := makeNodeID(t)
+	to := h.lookupOrCreateNode(id, pub, testAddr())
+
+	if err := h.sendNeighbors(to, testAddr(), nil, nil); err != nil {
+		t.Fatalf("sendNeighbors(empty): %v", err)
+	}
+	if len(ct.sent) != 1 {
+		t.Fatalf("empty result sent %d packets, want exactly 1", len(ct.sent))
+	}
+	pkt, err := DecodePacket(ct.sent[0])
+	if err != nil {
+		t.Fatalf("decode NEIGHBORS: %v", err)
+	}
+	nb, ok := pkt.(*Neighbors)
+	if !ok {
+		t.Fatalf("wrong packet type %T", pkt)
+	}
+	if len(nb.Nodes) != 0 {
+		t.Fatalf("empty NEIGHBORS carried %d nodes", len(nb.Nodes))
+	}
+}
+
+// TestSendNeighborsAdvertisesEnrTCPPort verifies BUG4: the NEIGHBORS record
+// advertises the node's real TCP port from its ENR, not the UDP port.
+func TestSendNeighborsAdvertisesEnrTCPPort(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	key, _ := ethcrypto.GenerateKey()
+	ct := &captureTransport{}
+	h := NewHandler(ctx, HandlerConfig{PrivateKey: key}, ct)
+
+	// Build a peer whose ENR advertises tcp=40404 at udp=30303.
+	peerKey, _ := ethcrypto.GenerateKey()
+	rec := enr.New()
+	_ = rec.Set("id", "v4")
+	_ = rec.Set("ip", net.IPv4(203, 0, 113, 7).To4())
+	_ = rec.Set("udp", uint16(30303))
+	_ = rec.Set("tcp", uint16(40404))
+	if err := rec.Sign(peerKey); err != nil {
+		t.Fatalf("sign: %v", err)
+	}
+	peer := node.New(&peerKey.PublicKey, &net.UDPAddr{IP: net.IPv4(203, 0, 113, 7), Port: 30303})
+	peer.SetENR(rec)
+
+	pub, id := makeNodeID(t)
+	to := h.lookupOrCreateNode(id, pub, testAddr())
+
+	if err := h.sendNeighbors(to, testAddr(), nil, []*node.Node{peer}); err != nil {
+		t.Fatalf("sendNeighbors: %v", err)
+	}
+	if len(ct.sent) != 1 {
+		t.Fatalf("sent %d packets, want 1", len(ct.sent))
+	}
+	pkt, _ := DecodePacket(ct.sent[0])
+	nb := pkt.(*Neighbors)
+	if len(nb.Nodes) != 1 {
+		t.Fatalf("NEIGHBORS carried %d nodes, want 1", len(nb.Nodes))
+	}
+	if nb.Nodes[0].TCP != 40404 {
+		t.Fatalf("advertised TCP=%d, want 40404 (the ENR tcp port, not the UDP port)", nb.Nodes[0].TCP)
+	}
+	if nb.Nodes[0].UDP != 30303 {
+		t.Fatalf("advertised UDP=%d, want 30303", nb.Nodes[0].UDP)
+	}
+}
+
+// TestFloodDoesNotEvictBondedPeers verifies the #34 follow-up: when the node map
+// is full, inserts evict a stale unbonded entry (so genuine new peers are never
+// locked out) while bonded, endpoint-proven peers are retained.
+func TestFloodDoesNotEvictBondedPeers(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	const maxNodes = 10
+	h := NewHandler(ctx, HandlerConfig{MaxNodes: maxNodes, NodeTTL: time.Hour}, nil)
+
+	// A genuine, bonded peer.
+	pub, bondedID := makeNodeID(t)
+	bonded := h.lookupOrCreateNode(bondedID, pub, testAddr())
+	bonded.MarkPongReceived(time.Hour, testAddr())
+
+	// Fill the rest with unbonded nodes, then flood well past the cap.
+	for i := 0; i < maxNodes*20; i++ {
+		p, id := makeNodeID(t)
+		h.lookupOrCreateNode(id, p, testAddr())
+	}
+
+	if got := len(h.AllNodes()); got != maxNodes {
+		t.Fatalf("map not bounded under flood: got %d want %d", got, maxNodes)
+	}
+	if h.GetNode(bondedID) == nil {
+		t.Fatal("bonded peer was evicted by an unbonded-ID flood")
+	}
+	// A brand-new node still gets retained (evicting an unbonded entry).
+	p, freshID := makeNodeID(t)
+	h.lookupOrCreateNode(freshID, p, testAddr())
+	if h.GetNode(freshID) == nil {
+		t.Fatal("new peer not retained when map full")
+	}
+}

discv5/protocol/handler.go

@@ -1065,10 +1065,19 @@ func (h *Handler) handleFindNode(msg *FindNode, remoteID node.ID, from *net.UDPA
 		}).Debug("handler: FINDNODE lookup completed via callback")
 	}
 
-	// Split nodes into multiple packets if needed to stay under max packet size
-	// Each ENR is typically 200-400 bytes, so we limit to 3 nodes per packet to be safe
+	// Split nodes into multiple packets if needed to stay under max packet size.
+	// Each ENR is typically 200-400 bytes, so we limit to 3 nodes per packet to be safe.
 	const maxNodesPerPacket = 3
 
+	// Cap the total response so it never exceeds what real clients consume. go-ethereum
+	// honours only the first packet's `total` and reads at most 5 NODES packets
+	// (totalNodesResponseLimit); anything beyond that is dropped as unsolicited. sigp/discv5
+	// caps at 16 nodes. Keep to <=5 packets / <=15 nodes so no served node is silently lost.
+	const maxNodesPerResponse = 15
+	if len(nodes) > maxNodesPerResponse {
+		nodes = nodes[:maxNodesPerResponse]
+	}
+
 	// Calculate total number of packets needed
 	totalPackets := (len(nodes) + maxNodesPerPacket - 1) / maxNodesPerPacket
 	if totalPackets == 0 {