From 74862941e52294f82d5553e4dd8ee2e8a55117dc Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Fri, 15 Dec 2023 17:04:53 +0100 Subject: [PATCH 1/6] p2p/discover: add liveness check in collectTableNodes --- p2p/discover/table.go | 19 +++++++++++++++++++ p2p/discover/v5_udp.go | 17 ++++------------- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/p2p/discover/table.go b/p2p/discover/table.go index e6dafb0dcaa..564fe86172b 100644 --- a/p2p/discover/table.go +++ b/p2p/discover/table.go @@ -459,6 +459,25 @@ func (tab *Table) findnodeByID(target enode.ID, nresults int, preferLive bool) * return nodes } +func (tab *Table) appendBucketNodes(dist uint, result []*enode.Node) []*enode.Node { + if dist > 256 { + return result + } + if dist == 0 { + return append(result, tab.self()) + } + + tab.mutex.Lock() + defer tab.mutex.Unlock() + for _, n := range tab.bucketAtDistance(int(dist)).entries { + if n.livenessChecks > 1 { + node := n.Node // avoid handing out pointer to struct field + result = append(result, &node) + } + } + return result +} + // len returns the number of nodes in the table. func (tab *Table) len() (n int) { tab.mutex.Lock() diff --git a/p2p/discover/v5_udp.go b/p2p/discover/v5_udp.go index 6ba7a906188..4b96a1dec78 100644 --- a/p2p/discover/v5_udp.go +++ b/p2p/discover/v5_udp.go @@ -852,6 +852,7 @@ func (t *UDPv5) handleFindnode(p *v5wire.Findnode, fromID enode.ID, fromAddr *ne // collectTableNodes creates a FINDNODE result set for the given distances. func (t *UDPv5) collectTableNodes(rip net.IP, distances []uint, limit int) []*enode.Node { var nodes []*enode.Node + var bn []*enode.Node var processed = make(map[uint]struct{}) for _, dist := range distances { // Reject duplicate / invalid distances. @@ -860,20 +861,10 @@ func (t *UDPv5) collectTableNodes(rip net.IP, distances []uint, limit int) []*en continue } - // Get the nodes. - var bn []*enode.Node - if dist == 0 { - bn = []*enode.Node{t.Self()} - } else if dist <= 256 { - t.tab.mutex.Lock() - bn = unwrapNodes(t.tab.bucketAtDistance(int(dist)).entries) - t.tab.mutex.Unlock() - } - processed[dist] = struct{}{} - - // Apply some pre-checks to avoid sending invalid nodes. + bn = t.tab.appendBucketNodes(dist, limit, bn[:0]) for _, n := range bn { - // TODO livenessChecks > 1 + // Apply some pre-checks to avoid sending invalid nodes. + // Note liveness is checked by appendBucketNodes. if netutil.CheckRelayIP(rip, n.IP()) != nil { continue } From 0d46156af4a19a979a76f883085e0728ce798d11 Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Fri, 15 Dec 2023 17:10:25 +0100 Subject: [PATCH 2/6] p2p/discover: fix test --- p2p/discover/table_test.go | 2 +- p2p/discover/table_util_test.go | 5 ++++- p2p/discover/v4_lookup_test.go | 6 +++--- p2p/discover/v4_udp_test.go | 2 +- p2p/discover/v5_udp.go | 4 ++-- p2p/discover/v5_udp_test.go | 8 ++++---- 6 files changed, 15 insertions(+), 12 deletions(-) diff --git a/p2p/discover/table_test.go b/p2p/discover/table_test.go index 2781dd4225b..3ba34222513 100644 --- a/p2p/discover/table_test.go +++ b/p2p/discover/table_test.go @@ -199,7 +199,7 @@ func TestTable_findnodeByID(t *testing.T) { tab, db := newTestTable(transport) defer db.Close() defer tab.close() - fillTable(tab, test.All) + fillTable(tab, test.All, true) // check that closest(Target, N) returns nodes result := tab.findnodeByID(test.Target, test.N, false).entries diff --git a/p2p/discover/table_util_test.go b/p2p/discover/table_util_test.go index 8f3813bdcf0..d6309dfd6c6 100644 --- a/p2p/discover/table_util_test.go +++ b/p2p/discover/table_util_test.go @@ -109,8 +109,11 @@ func fillBucket(tab *Table, n *node) (last *node) { // fillTable adds nodes the table to the end of their corresponding bucket // if the bucket is not full. The caller must not hold tab.mutex. -func fillTable(tab *Table, nodes []*node) { +func fillTable(tab *Table, nodes []*node, setLive bool) { for _, n := range nodes { + if setLive { + n.livenessChecks = 1 + } tab.addSeenNode(n) } } diff --git a/p2p/discover/v4_lookup_test.go b/p2p/discover/v4_lookup_test.go index 1f9ad69d0a2..8867a5a8ac7 100644 --- a/p2p/discover/v4_lookup_test.go +++ b/p2p/discover/v4_lookup_test.go @@ -40,7 +40,7 @@ func TestUDPv4_Lookup(t *testing.T) { } // Seed table with initial node. - fillTable(test.table, []*node{wrapNode(lookupTestnet.node(256, 0))}) + fillTable(test.table, []*node{wrapNode(lookupTestnet.node(256, 0))}, true) // Start the lookup. resultC := make(chan []*enode.Node, 1) @@ -74,7 +74,7 @@ func TestUDPv4_LookupIterator(t *testing.T) { for i := range lookupTestnet.dists[256] { bootnodes[i] = wrapNode(lookupTestnet.node(256, i)) } - fillTable(test.table, bootnodes) + fillTable(test.table, bootnodes, true) go serveTestnet(test, lookupTestnet) // Create the iterator and collect the nodes it yields. @@ -109,7 +109,7 @@ func TestUDPv4_LookupIteratorClose(t *testing.T) { for i := range lookupTestnet.dists[256] { bootnodes[i] = wrapNode(lookupTestnet.node(256, i)) } - fillTable(test.table, bootnodes) + fillTable(test.table, bootnodes, true) go serveTestnet(test, lookupTestnet) it := test.udp.RandomNodes() diff --git a/p2p/discover/v4_udp_test.go b/p2p/discover/v4_udp_test.go index 53ecb1bc6e2..361e3796264 100644 --- a/p2p/discover/v4_udp_test.go +++ b/p2p/discover/v4_udp_test.go @@ -269,7 +269,7 @@ func TestUDPv4_findnode(t *testing.T) { } nodes.push(n, numCandidates) } - fillTable(test.table, nodes.entries) + fillTable(test.table, nodes.entries, false) // ensure there's a bond with the test node, // findnode won't be accepted otherwise. diff --git a/p2p/discover/v5_udp.go b/p2p/discover/v5_udp.go index 4b96a1dec78..ab429502df1 100644 --- a/p2p/discover/v5_udp.go +++ b/p2p/discover/v5_udp.go @@ -851,8 +851,8 @@ func (t *UDPv5) handleFindnode(p *v5wire.Findnode, fromID enode.ID, fromAddr *ne // collectTableNodes creates a FINDNODE result set for the given distances. func (t *UDPv5) collectTableNodes(rip net.IP, distances []uint, limit int) []*enode.Node { - var nodes []*enode.Node var bn []*enode.Node + var nodes []*enode.Node var processed = make(map[uint]struct{}) for _, dist := range distances { // Reject duplicate / invalid distances. @@ -861,7 +861,7 @@ func (t *UDPv5) collectTableNodes(rip net.IP, distances []uint, limit int) []*en continue } - bn = t.tab.appendBucketNodes(dist, limit, bn[:0]) + bn = t.tab.appendBucketNodes(dist, bn[:0]) for _, n := range bn { // Apply some pre-checks to avoid sending invalid nodes. // Note liveness is checked by appendBucketNodes. diff --git a/p2p/discover/v5_udp_test.go b/p2p/discover/v5_udp_test.go index 18d8aeac6dc..eaa969ea8b6 100644 --- a/p2p/discover/v5_udp_test.go +++ b/p2p/discover/v5_udp_test.go @@ -159,9 +159,9 @@ func TestUDPv5_findnodeHandling(t *testing.T) { nodes253 := nodesAtDistance(test.table.self().ID(), 253, 16) nodes249 := nodesAtDistance(test.table.self().ID(), 249, 4) nodes248 := nodesAtDistance(test.table.self().ID(), 248, 10) - fillTable(test.table, wrapNodes(nodes253)) - fillTable(test.table, wrapNodes(nodes249)) - fillTable(test.table, wrapNodes(nodes248)) + fillTable(test.table, wrapNodes(nodes253), true) + fillTable(test.table, wrapNodes(nodes249), true) + fillTable(test.table, wrapNodes(nodes248), true) // Requesting with distance zero should return the node's own record. test.packetIn(&v5wire.Findnode{ReqID: []byte{0}, Distances: []uint{0}}) @@ -589,7 +589,7 @@ func TestUDPv5_lookup(t *testing.T) { // Seed table with initial node. initialNode := lookupTestnet.node(256, 0) - fillTable(test.table, []*node{wrapNode(initialNode)}) + fillTable(test.table, []*node{wrapNode(initialNode)}, true) // Start the lookup. resultC := make(chan []*enode.Node, 1) From b5acff0fad8c2ab313c41ea21d3ac086d759a434 Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Sat, 16 Dec 2023 02:50:13 +0100 Subject: [PATCH 3/6] p2p/discover: rename to appendLiveNodes --- p2p/discover/table.go | 3 ++- p2p/discover/v5_udp.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/p2p/discover/table.go b/p2p/discover/table.go index 564fe86172b..dbbcf30b519 100644 --- a/p2p/discover/table.go +++ b/p2p/discover/table.go @@ -459,7 +459,8 @@ func (tab *Table) findnodeByID(target enode.ID, nresults int, preferLive bool) * return nodes } -func (tab *Table) appendBucketNodes(dist uint, result []*enode.Node) []*enode.Node { +// appendLiveNodes adds nodes at the given distance to the result slice. +func (tab *Table) appendLiveNodes(dist uint, result []*enode.Node) []*enode.Node { if dist > 256 { return result } diff --git a/p2p/discover/v5_udp.go b/p2p/discover/v5_udp.go index ab429502df1..6e6ed7f08f9 100644 --- a/p2p/discover/v5_udp.go +++ b/p2p/discover/v5_udp.go @@ -861,7 +861,7 @@ func (t *UDPv5) collectTableNodes(rip net.IP, distances []uint, limit int) []*en continue } - bn = t.tab.appendBucketNodes(dist, bn[:0]) + bn = t.tab.appendLiveNodes(dist, bn[:0]) for _, n := range bn { // Apply some pre-checks to avoid sending invalid nodes. // Note liveness is checked by appendBucketNodes. From ba833c1fde2f7d854f200b995ff6d0b545b7c186 Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Sat, 16 Dec 2023 02:50:25 +0100 Subject: [PATCH 4/6] p2p/discover: add dedup logic back --- p2p/discover/v5_udp.go | 1 + 1 file changed, 1 insertion(+) diff --git a/p2p/discover/v5_udp.go b/p2p/discover/v5_udp.go index 6e6ed7f08f9..a929a577e05 100644 --- a/p2p/discover/v5_udp.go +++ b/p2p/discover/v5_udp.go @@ -860,6 +860,7 @@ func (t *UDPv5) collectTableNodes(rip net.IP, distances []uint, limit int) []*en if seen || dist > 256 { continue } + processed[dist] = struct{}{} bn = t.tab.appendLiveNodes(dist, bn[:0]) for _, n := range bn { From d39ef3c8f44a941dde8684c8a73d6a1478f28d01 Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Sat, 16 Dec 2023 03:00:06 +0100 Subject: [PATCH 5/6] p2p/discover: simplify --- p2p/discover/v5_udp.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/p2p/discover/v5_udp.go b/p2p/discover/v5_udp.go index a929a577e05..8b3e33d37cf 100644 --- a/p2p/discover/v5_udp.go +++ b/p2p/discover/v5_udp.go @@ -862,10 +862,9 @@ func (t *UDPv5) collectTableNodes(rip net.IP, distances []uint, limit int) []*en } processed[dist] = struct{}{} - bn = t.tab.appendLiveNodes(dist, bn[:0]) - for _, n := range bn { + for _, n := range t.tab.appendLiveNodes(dist, bn[:0]) { // Apply some pre-checks to avoid sending invalid nodes. - // Note liveness is checked by appendBucketNodes. + // Note liveness is checked by appendLiveNodes. if netutil.CheckRelayIP(rip, n.IP()) != nil { continue } From e95a2657f4a7a289b0a3984411161a0853fb31c8 Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Sat, 16 Dec 2023 03:00:14 +0100 Subject: [PATCH 6/6] p2p/discover: fix issue found by test --- p2p/discover/table.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/p2p/discover/table.go b/p2p/discover/table.go index dbbcf30b519..2b7a28708b8 100644 --- a/p2p/discover/table.go +++ b/p2p/discover/table.go @@ -471,7 +471,7 @@ func (tab *Table) appendLiveNodes(dist uint, result []*enode.Node) []*enode.Node tab.mutex.Lock() defer tab.mutex.Unlock() for _, n := range tab.bucketAtDistance(int(dist)).entries { - if n.livenessChecks > 1 { + if n.livenessChecks >= 1 { node := n.Node // avoid handing out pointer to struct field result = append(result, &node) }