discv4/discv5 wire fixes + hardening: ENRRESPONSE, NEIGHBORS/NODES, ENR size, bond & flood

discv4/protocol/enrresponse_test.go

@@ -0,0 +1,69 @@
+package protocol
+
+import (
+	"bytes"
+	"net"
+	"testing"
+
+	ethcrypto "github.com/ethereum/go-ethereum/crypto"
+	"github.com/ethereum/go-ethereum/p2p/discover/v4wire"
+	gethenode "github.com/ethereum/go-ethereum/p2p/enode"
+	"github.com/ethereum/go-ethereum/rlp"
+	"github.com/ethpandaops/bootnodoor/enr"
+)
+
+// TestENRResponseEncodesRecord verifies that a discv4 ENRRESPONSE serializes the
+// embedded ENR record correctly (regression for the empty-record bug where
+// enr.Record was encoded as c0 because it does not satisfy rlp.Encoder).
+func TestENRResponseEncodesRecord(t *testing.T) {
+	key, _ := ethcrypto.GenerateKey()
+	rec := enr.New()
+	_ = rec.Set("id", "v4")
+	_ = rec.Set("ip", net.IPv4(127, 0, 0, 1).To4())
+	_ = rec.Set("udp", uint16(30303))
+	if err := rec.Sign(key); err != nil {
+		t.Fatalf("sign: %v", err)
+	}
+
+	resp := &ENRResponse{ReplyTok: []byte{1, 2, 3}, Record: rec}
+
+	packet, _, err := Encode(key, resp)
+	if err != nil {
+		t.Fatalf("encode: %v", err)
+	}
+
+	// Decode as geth would: parse packet, then decode the ENR via geth's enode.
+	decoded, err := DecodePacket(packet)
+	if err != nil {
+		t.Fatalf("self-decode: %v", err)
+	}
+	got, ok := decoded.(*ENRResponse)
+	if !ok {
+		t.Fatalf("wrong type: %T", decoded)
+	}
+
+	// The embedded record must match the original record's canonical encoding.
+	want, _ := rec.EncodeRLP()
+	gotRLP, _ := got.Record.EncodeRLP()
+	if !bytes.Equal(want, gotRLP) {
+		t.Fatalf("record mismatch:\n got %x\nwant %x", gotRLP, want)
+	}
+
+	// And geth's own v4wire decoder must accept it (this is what fails on the wire:
+	// "record contains less than two list elements").
+	sigdata := packet[headSize:] // type || rlp
+	var gethResp v4wire.ENRResponse
+	if err := rlp.DecodeBytes(sigdata[1:], &gethResp); err != nil {
+		t.Fatalf("geth v4wire decode rejected the response: %v", err)
+	}
+	gethNode, err := gethenode.New(gethenode.ValidSchemes, &gethResp.Record)
+	if err != nil {
+		t.Fatalf("geth could not build node from record: %v", err)
+	}
+	if gethNode.Seq() != rec.Seq() {
+		t.Fatalf("geth decoded seq %d, want %d", gethNode.Seq(), rec.Seq())
+	}
+	if !gethNode.IP().Equal(net.IPv4(127, 0, 0, 1)) {
+		t.Fatalf("geth decoded ip %s, want 127.0.0.1", gethNode.IP())
+	}
+}

discv4/protocol/handler.go

@@ -330,13 +330,15 @@ func (h *Handler) handlePing(fromNode *node.Node, from *net.UDPAddr, localAddr *
 		return err
 	}
 
-	// Mark node as bonded: they pinged us, we ponged them.
-	// This allows THEM to query US with FINDNODE immediately.
-	fromNode.MarkPongReceived(h.config.BondExpiration)
-
-	// IMPORTANT: For bidirectional bonding (required by strict clients like reth for ENRRequest),
-	// we also need to establish that WE can reach THEM, not just that they can reach us.
-	// Send a PING back to them to establish bidirectional bond.
+	// Do NOT mark the node bonded here. A received PING only proves the sender
+	// claims this endpoint; the UDP source can be spoofed, so bonding on it would
+	// let a spoofed PING coax a large NEIGHBORS reply to a victim (amplification).
+	// The bond is established only once the peer answers OUR ping-back with a PONG
+	// (handlePong -> MarkPongReceived), matching go-ethereum's endpoint proof.
+
+	// Send a PING back so the peer can prove its endpoint and become bonded. This
+	// also satisfies strict clients (e.g. reth) that require bidirectional bonding
+	// before answering ENRRequest.
 	//
 	// RATE LIMITING: Only send PING back if we haven't pinged them in the last 100ms.
 	// This prevents ping-pong loops (max 10 pings/sec per node).
@@ -782,8 +784,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)
@@ -793,10 +798,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()),
 			}
 		}
@@ -871,12 +884,26 @@ func (h *Handler) getOrCreateNode(id node.ID, pubkey *ecdsa.PublicKey, addr *net
 	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/handler_test.go

@@ -97,3 +97,66 @@ func TestCleanupReclaimsFloodedNodes(t *testing.T) {
 		t.Fatalf("flooded nodes not reclaimed: %d still tracked", got)
 	}
 }
+
+// TestPingDoesNotBondBeforePong verifies that receiving a PING must not by itself
+// bond the sender. The bond is only established once the peer answers our
+// ping-back with a PONG (endpoint proof), matching go-ethereum.
+func TestPingDoesNotBondBeforePong(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	key, _ := crypto.GenerateKey()
+	h := NewHandler(ctx, HandlerConfig{PrivateKey: key, BondExpiration: time.Hour, LocalAddr: testAddr(), RequestTimeout: 20 * time.Millisecond}, stubTransport{})
+
+	pub, id := makeNodeID(t)
+	n := h.getOrCreateNode(id, pub, testAddr())
+
+	// Handling an inbound PING sends a PONG + a ping-back but must not bond.
+	if err := h.handlePing(n, testAddr(), nil, &Ping{Version: 4, Expiration: MakeExpiration(time.Minute)}, []byte{0x01}); err != nil {
+		t.Fatalf("handlePing: %v", err)
+	}
+	if n.IsBonded() {
+		t.Fatal("node became bonded from an inbound PING alone")
+	}
+
+	// Once the peer answers our ping-back with a PONG, the bond is established.
+	n.MarkPongReceived(time.Hour)
+	if !n.IsBonded() {
+		t.Fatal("node should be bonded after receiving a PONG to our ping")
+	}
+}
+
+// TestFloodDoesNotEvictBondedPeers verifies that 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.getOrCreateNode(bondedID, pub, testAddr())
+	bonded.MarkPongReceived(time.Hour)
+
+	// Fill the rest with unbonded nodes, then flood well past the cap.
+	for i := 0; i < maxNodes*20; i++ {
+		p, id := makeNodeID(t)
+		h.getOrCreateNode(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.getOrCreateNode(freshID, p, testAddr())
+	if h.GetNode(freshID) == nil {
+		t.Fatal("new peer not retained when map full")
+	}
+}

discv4/protocol/messages.go

@@ -13,6 +13,7 @@ import (
 	"crypto/ecdsa"
 	"crypto/elliptic"
 	"fmt"
+	"io"
 	"net"
 	"time"
 
@@ -192,6 +193,37 @@ func (e *ENRResponse) Name() string { return "ENRRESPONSE/v4" }
 // Kind returns the packet type byte.
 func (e *ENRResponse) Kind() byte { return ENRResponsePacket }
 
+// EncodeRLP implements rlp.Encoder so the embedded ENR record is serialized via
+// enr.Record's own RLP encoding.
+//
+// Without this, geth's rlp package does not recognize enr.Record as an encoder
+// (its EncodeRLP has signature ([]byte, error), not the io.Writer form the
+// interface requires) and, because enr.Record has no exported fields, would
+// serialize the record as an empty RLP list. Real clients (go-ethereum) then
+// reject the response with "record contains less than two list elements".
+func (e *ENRResponse) EncodeRLP(w io.Writer) error {
+	var recordRLP []byte
+	if e.Record != nil {
+		encoded, err := e.Record.EncodeRLP()
+		if err != nil {
+			return err
+		}
+		recordRLP = encoded
+	}
+
+	type enrResponseRLP struct {
+		ReplyTok []byte
+		Record   rlp.RawValue
+		Rest     []rlp.RawValue `rlp:"tail"`
+	}
+
+	return rlp.Encode(w, &enrResponseRLP{
+		ReplyTok: e.ReplyTok,
+		Record:   recordRLP,
+		Rest:     e.Rest,
+	})
+}
+
 // Supporting Types
 
 // Pubkey represents an encoded 64-byte secp256k1 public key.

discv5/protocol/handler.go

@@ -1040,10 +1040,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 {