diff --git a/discv4/protocol/handler.go b/discv4/protocol/handler.go index 21283b1..b3667bd 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()), } } @@ -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 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") + } +} 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 {