swarm/network: light nodes are not dialed, saved and requested from (#17975)

* RequestFromPeers does not use peers marked as lightnode

* fix warning about variable name

* write tests for RequestFromPeers

* lightnodes should be omitted from the addressbook

* resolve pr comments regarding logging, formatting and comments

* resolve pr comments regarding comments and added a missing newline

* add assertions to check peers in live connections

swarm/network/kademlia.go

@@ -261,7 +261,7 @@ func (k *Kademlia) On(p *Peer) (uint8, bool) {
 		// found among live peers, do nothing
 		return v
 	})
-	if ins {
+	if ins && !p.BzzPeer.LightNode {
 		a := newEntry(p.BzzAddr)
 		a.conn = p
 		// insert new online peer into addrs
@@ -329,14 +329,18 @@ func (k *Kademlia) Off(p *Peer) {
 	k.lock.Lock()
 	defer k.lock.Unlock()
 	var del bool
-	k.addrs, _, _, _ = pot.Swap(k.addrs, p, pof, func(v pot.Val) pot.Val {
-		// v cannot be nil, must check otherwise we overwrite entry
-		if v == nil {
-			panic(fmt.Sprintf("connected peer not found %v", p))
-		}
+	if !p.BzzPeer.LightNode {
+		k.addrs, _, _, _ = pot.Swap(k.addrs, p, pof, func(v pot.Val) pot.Val {
+			// v cannot be nil, must check otherwise we overwrite entry
+			if v == nil {
+				panic(fmt.Sprintf("connected peer not found %v", p))
+			}
+			del = true
+			return newEntry(p.BzzAddr)
+		})
+	} else {
 		del = true
-		return newEntry(p.BzzAddr)
-	})
+	}
 
 	if del {
 		k.conns, _, _, _ = pot.Swap(k.conns, p, pof, func(_ pot.Val) pot.Val {

swarm/network/kademlia_test.go

@@ -46,19 +46,19 @@ func newTestKademlia(b string) *Kademlia {
 	return NewKademlia(base, params)
 }
 
-func newTestKadPeer(k *Kademlia, s string) *Peer {
-	return NewPeer(&BzzPeer{BzzAddr: testKadPeerAddr(s)}, k)
+func newTestKadPeer(k *Kademlia, s string, lightNode bool) *Peer {
+	return NewPeer(&BzzPeer{BzzAddr: testKadPeerAddr(s), LightNode: lightNode}, k)
 }
 
 func On(k *Kademlia, ons ...string) {
 	for _, s := range ons {
-		k.On(newTestKadPeer(k, s))
+		k.On(newTestKadPeer(k, s, false))
 	}
 }
 
 func Off(k *Kademlia, offs ...string) {
 	for _, s := range offs {
-		k.Off(newTestKadPeer(k, s))
+		k.Off(newTestKadPeer(k, s, false))
 	}
 }
 
@@ -254,6 +254,56 @@ func TestSuggestPeerFindPeers(t *testing.T) {
 
 }
 
+// a node should stay in the address book if it's removed from the kademlia
+func TestOffEffectingAddressBookNormalNode(t *testing.T) {
+	k := newTestKademlia("00000000")
+	// peer added to kademlia
+	k.On(newTestKadPeer(k, "01000000", false))
+	// peer should be in the address book
+	if k.addrs.Size() != 1 {
+		t.Fatal("known peer addresses should contain 1 entry")
+	}
+	// peer should be among live connections
+	if k.conns.Size() != 1 {
+		t.Fatal("live peers should contain 1 entry")
+	}
+	// remove peer from kademlia
+	k.Off(newTestKadPeer(k, "01000000", false))
+	// peer should be in the address book
+	if k.addrs.Size() != 1 {
+		t.Fatal("known peer addresses should contain 1 entry")
+	}
+	// peer should not be among live connections
+	if k.conns.Size() != 0 {
+		t.Fatal("live peers should contain 0 entry")
+	}
+}
+
+// a light node should not be in the address book
+func TestOffEffectingAddressBookLightNode(t *testing.T) {
+	k := newTestKademlia("00000000")
+	// light node peer added to kademlia
+	k.On(newTestKadPeer(k, "01000000", true))
+	// peer should not be in the address book
+	if k.addrs.Size() != 0 {
+		t.Fatal("known peer addresses should contain 0 entry")
+	}
+	// peer should be among live connections
+	if k.conns.Size() != 1 {
+		t.Fatal("live peers should contain 1 entry")
+	}
+	// remove peer from kademlia
+	k.Off(newTestKadPeer(k, "01000000", true))
+	// peer should not be in the address book
+	if k.addrs.Size() != 0 {
+		t.Fatal("known peer addresses should contain 0 entry")
+	}
+	// peer should not be among live connections
+	if k.conns.Size() != 0 {
+		t.Fatal("live peers should contain 0 entry")
+	}
+}
+
 func TestSuggestPeerRetries(t *testing.T) {
 	k := newTestKademlia("00000000")
 	k.RetryInterval = int64(300 * time.Millisecond) // cycle

swarm/network/stream/delivery.go

@@ -245,7 +245,10 @@ func (d *Delivery) RequestFromPeers(ctx context.Context, req *network.Request) (
 	} else {
 		d.kad.EachConn(req.Addr[:], 255, func(p *network.Peer, po int, nn bool) bool {
 			id := p.ID()
-			// TODO: skip light nodes that do not accept retrieve requests
+			if p.LightNode {
+				// skip light nodes
+				return true
+			}
 			if req.SkipPeer(id.String()) {
 				log.Trace("Delivery.RequestFromPeers: skip peer", "peer id", id)
 				return true

swarm/network/stream/delivery_test.go

@@ -29,10 +29,13 @@ import (
 
 	"github.com/ethereum/go-ethereum/node"
 	"github.com/ethereum/go-ethereum/p2p"
+	"github.com/ethereum/go-ethereum/p2p/enode"
+	"github.com/ethereum/go-ethereum/p2p/protocols"
 	"github.com/ethereum/go-ethereum/p2p/simulations/adapters"
 	p2ptest "github.com/ethereum/go-ethereum/p2p/testing"
 	"github.com/ethereum/go-ethereum/swarm/log"
 	"github.com/ethereum/go-ethereum/swarm/network"
+	pq "github.com/ethereum/go-ethereum/swarm/network/priorityqueue"
 	"github.com/ethereum/go-ethereum/swarm/network/simulation"
 	"github.com/ethereum/go-ethereum/swarm/state"
 	"github.com/ethereum/go-ethereum/swarm/storage"
@@ -274,6 +277,86 @@ func TestStreamerUpstreamRetrieveRequestMsgExchange(t *testing.T) {
 	}
 }
 
+// if there is one peer in the Kademlia, RequestFromPeers should return it
+func TestRequestFromPeers(t *testing.T) {
+	dummyPeerID := enode.HexID("3431c3939e1ee2a6345e976a8234f9870152d64879f30bc272a074f6859e75e8")
+
+	addr := network.RandomAddr()
+	to := network.NewKademlia(addr.OAddr, network.NewKadParams())
+	delivery := NewDelivery(to, nil)
+	protocolsPeer := protocols.NewPeer(p2p.NewPeer(dummyPeerID, "dummy", nil), nil, nil)
+	peer := network.NewPeer(&network.BzzPeer{
+		BzzAddr:   network.RandomAddr(),
+		LightNode: false,
+		Peer:      protocolsPeer,
+	}, to)
+	to.On(peer)
+	r := NewRegistry(addr.ID(), delivery, nil, nil, nil)
+
+	// an empty priorityQueue has to be created to prevent a goroutine being called after the test has finished
+	sp := &Peer{
+		Peer:     protocolsPeer,
+		pq:       pq.New(int(PriorityQueue), PriorityQueueCap),
+		streamer: r,
+	}
+	r.setPeer(sp)
+	req := network.NewRequest(
+		storage.Address(hash0[:]),
+		true,
+		&sync.Map{},
+	)
+	ctx := context.Background()
+	id, _, err := delivery.RequestFromPeers(ctx, req)
+
+	if err != nil {
+		t.Fatal(err)
+	}
+	if *id != dummyPeerID {
+		t.Fatalf("Expected an id, got %v", id)
+	}
+}
+
+// RequestFromPeers should not return light nodes
+func TestRequestFromPeersWithLightNode(t *testing.T) {
+	dummyPeerID := enode.HexID("3431c3939e1ee2a6345e976a8234f9870152d64879f30bc272a074f6859e75e8")
+
+	addr := network.RandomAddr()
+	to := network.NewKademlia(addr.OAddr, network.NewKadParams())
+	delivery := NewDelivery(to, nil)
+
+	protocolsPeer := protocols.NewPeer(p2p.NewPeer(dummyPeerID, "dummy", nil), nil, nil)
+	// setting up a lightnode
+	peer := network.NewPeer(&network.BzzPeer{
+		BzzAddr:   network.RandomAddr(),
+		LightNode: true,
+		Peer:      protocolsPeer,
+	}, to)
+	to.On(peer)
+	r := NewRegistry(addr.ID(), delivery, nil, nil, nil)
+	// an empty priorityQueue has to be created to prevent a goroutine being called after the test has finished
+	sp := &Peer{
+		Peer:     protocolsPeer,
+		pq:       pq.New(int(PriorityQueue), PriorityQueueCap),
+		streamer: r,
+	}
+	r.setPeer(sp)
+
+	req := network.NewRequest(
+		storage.Address(hash0[:]),
+		true,
+		&sync.Map{},
+	)
+
+	ctx := context.Background()
+	// making a request which should return with "no peer found"
+	_, _, err := delivery.RequestFromPeers(ctx, req)
+
+	expectedError := "no peer found"
+	if err.Error() != expectedError {
+		t.Fatalf("expected '%v', got %v", expectedError, err)
+	}
+}
+
 func TestStreamerDownstreamChunkDeliveryMsgExchange(t *testing.T) {
 	tester, streamer, localStore, teardown, err := newStreamerTester(t, &RegistryOptions{
 		Retrieval: RetrievalDisabled,