From 81533deae5ee4a7ec08842e2b6647f3affde5a71 Mon Sep 17 00:00:00 2001 From: Mark Vujevits Date: Wed, 7 Nov 2018 20:33:36 +0100 Subject: [PATCH] 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 | 20 ++++--- swarm/network/kademlia_test.go | 58 +++++++++++++++++-- swarm/network/stream/delivery.go | 5 +- swarm/network/stream/delivery_test.go | 83 +++++++++++++++++++++++++++ 4 files changed, 153 insertions(+), 13 deletions(-) diff --git a/swarm/network/kademlia.go b/swarm/network/kademlia.go index 55a0c6f13d..cd94741be3 100644 --- a/swarm/network/kademlia.go +++ b/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 { diff --git a/swarm/network/kademlia_test.go b/swarm/network/kademlia_test.go index 903c8dbdac..d2e051f457 100644 --- a/swarm/network/kademlia_test.go +++ b/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 diff --git a/swarm/network/stream/delivery.go b/swarm/network/stream/delivery.go index 9092ffe3ed..0109fbdef2 100644 --- a/swarm/network/stream/delivery.go +++ b/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 diff --git a/swarm/network/stream/delivery_test.go b/swarm/network/stream/delivery_test.go index 29b4f2f69a..c77682e0e1 100644 --- a/swarm/network/stream/delivery_test.go +++ b/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,