From b19a416c15fd6521caffbbcb07f4693e68669335 Mon Sep 17 00:00:00 2001 From: pk910 Date: Thu, 30 Jul 2026 15:39:03 +0000 Subject: [PATCH 1/3] discv4: always send NEIGHBORS and advertise the real TCP port An empty routing table previously produced a silent (zero-packet) response, so go-ethereum's querier waited out its full request timeout instead of returning early on the first reply. Always send >=1 NEIGHBORS packet. Also advertise the node's real TCP port from its ENR instead of copying the UDP port. --- discv4/protocol/handler.go | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/discv4/protocol/handler.go b/discv4/protocol/handler.go index 21283b1..c4e9ad9 100644 --- a/discv4/protocol/handler.go +++ b/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()), } } From b0240b478c369db8c1b8e805ac66b4f4522553fb Mon Sep 17 00:00:00 2001 From: pk910 Date: Thu, 30 Jul 2026 15:41:01 +0000 Subject: [PATCH 2/3] discv5: cap NODES response to <=15 nodes / <=5 packets go-ethereum honours only the first NODES packet's total and reads at most 5 packets; sigp/discv5 caps at 16 nodes. Cap the served set at 15 nodes / <=5 packets so no served node is silently dropped by a requester. --- discv5/protocol/handler.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/discv5/protocol/handler.go b/discv5/protocol/handler.go index 0c1311f..29d7569 100644 --- a/discv5/protocol/handler.go +++ b/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 { From 8bcac67ad74a553d7b7fc53066bf2e4c59382176 Mon Sep 17 00:00:00 2001 From: pk910 Date: Thu, 30 Jul 2026 15:42:27 +0000 Subject: [PATCH 3/3] discv4: evict a stale node instead of dropping new peers when the map is full lookupOrCreateNode previously returned a non-retained node once the map was full, so under a flood of distinct signed IDs a genuinely new peer's inbound PING marked bond state on a discarded object and could never bond (memory-growth DoS turned into a bonding-lockout DoS). When full, evict one unbonded entry to admit the new node; bonded, endpoint-proven peers are never evicted this way. Adds regression tests for this and the two preceding NEIGHBORS fixes. --- discv4/protocol/handler.go | 24 ++++- discv4/protocol/remainder_fixes_test.go | 137 ++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 5 deletions(-) create mode 100644 discv4/protocol/remainder_fixes_test.go diff --git a/discv4/protocol/handler.go b/discv4/protocol/handler.go index c4e9ad9..b3667bd 100644 --- a/discv4/protocol/handler.go +++ b/discv4/protocol/handler.go @@ -1168,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 diff --git a/discv4/protocol/remainder_fixes_test.go b/discv4/protocol/remainder_fixes_test.go new file mode 100644 index 0000000..eb4314c --- /dev/null +++ b/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") + } +}