swarm/network: remove kademlia.EachBin; fix RequestSubscriptions and add unit test

Author: holisticode

swarm/network/kademlia.go

@@ -356,40 +356,6 @@ func (k *Kademlia) Off(p *Peer) {
 	}
 }
 
-// EachBin is a two level nested iterator
-// The outer iterator returns all bins that have known peers, in order from shallowest to deepest
-// The inner iterator returns all peers per bin returned by the outer iterator, in no defined order
-// TODO the po returned by the inner iterator is not reliable. However, it is not being used in this method
-func (k *Kademlia) EachBin(base []byte, pof pot.Pof, o int, eachBinFunc func(conn *Peer, po int) bool) {
-	k.lock.RLock()
-	defer k.lock.RUnlock()
-
-	var startPo int
-	var endPo int
-	kadDepth := depthForPot(k.conns, k.NeighbourhoodSize, k.base)
-
-	k.conns.EachBin(base, pof, o, func(po, size int, f func(func(val pot.Val, i int) bool) bool) bool {
-		//if the peer's bin is smaller than the kademlia depth,
-		//only the peer's bin should be considered
-		if po < kadDepth {
-			startPo = po
-			endPo = po
-		} else {
-			//if the peer's bin is equal or higher than the kademlia depth,
-			//each bin from the depth up to k.MaxProxDisplay should be considered
-			startPo = kadDepth
-			endPo = k.MaxProxDisplay
-		}
-
-		for bin := startPo; bin <= endPo; bin++ {
-			f(func(val pot.Val, _ int) bool {
-				return eachBinFunc(val.(*Peer), bin)
-			})
-		}
-		return true
-	})
-}
-
 // EachConn is an iterator with args (base, po, f) applies f to each live peer
 // that has proximity order po or less as measured from the base
 // if base is nil, kademlia base address is used

swarm/network/kademlia_test.go

@@ -888,155 +888,3 @@ func newTestDiscoveryPeer(addr pot.Address, kad *Kademlia) *Peer {
 	}
 	return NewPeer(bp, kad)
 }
-
-/*
-TestEachBin is a unit test for the kademlia's `EachBin` function.
-That function is actually used only for streamer subscriptions.
-
-Thus, the test does:
-	* assigns each known connected peer to a bin map
-  * build up a known kademlia in advance
-	* runs the EachBin function, which returns supposed subscription bins
-	* store all supposed bins per peer in a map
-	* check that all peers have the expected subscriptions
-
-This kad table and its peers are copied from TestKademliaCase1,
-it represents an edge case but for the purpose of a unit test for
-the `EachBin` function it is ok.
-
-Addresses used in this test are discovered as part of the simulation network
-in higher level tests for streaming. They were generated randomly.
-
-=========================================================================
-Thu Dec 13 14:21:47 UTC 2018 KΛÐΞMLIΛ hive: queen's address: 7efef1
-population: 49 (49), MinProxBinSize: 2, MinBinSize: 2, MaxBinSize: 4
-000 18 8196 835f 8958 8e23          | 18 8196 (0) 835f (0) 8958 (0) 8e23 (0)
-001 14 2690 28f0 2850 3a51          | 14 2690 (0) 28f0 (0) 2850 (0) 3a51 (0)
-002 11 4d72 4a45 4375 4607          | 11 4d72 (0) 4a45 (0) 4375 (0) 4607 (0)
-003  1 646e                         |  1 646e (0)
-004  3 769c 76d1 7656               |  3 769c (0) 76d1 (0) 7656 (0)
-============ DEPTH: 5 ==========================================
-005  1 7a48                         |  1 7a48 (0)
-006  1 7cbd                         |  1 7cbd (0)
-007  0                              |  0
-008  0                              |  0
-009  0                              |  0
-010  0                              |  0
-011  0                              |  0
-012  0                              |  0
-013  0                              |  0
-014  0                              |  0
-015  0                              |  0
-=========================================================================
-*/
-func TestEachBin(t *testing.T) {
-	//the pivot address; this is the actual kademlia node
-	pivotAddr := "7efef1c41d77f843ad167be95f6660567eb8a4a59f39240000cce2e0d65baf8e"
-
-	//a map of bin number to addresses from the given kademlia
-	binMap := make(map[int][]string)
-	binMap[0] = []string{
-		"835fbbf1d16ba7347b6e2fc552d6e982148d29c624ea20383850df3c810fa8fc",
-		"81968a2d8fb39114342ee1da85254ec51e0608d7f0f6997c2a8354c260a71009",
-	}
-	binMap[1] = []string{
-		"28f0bc1b44658548d6e05dd16d4c2fe77f1da5d48b6774bc4263b045725d0c19",
-		"2690a910c33ee37b91eb6c4e0731d1d345e2dc3b46d308503a6e85bbc242c69e",
-	}
-	binMap[2] = []string{
-		"4a45f1fc63e1a9cb9dfa44c98da2f3d20c2923e5d75ff60b2db9d1bdb0c54d51",
-		"4d72a04ddeb851a68cd197ef9a92a3e2ff01fbbff638e64929dd1a9c2e150112",
-	}
-	binMap[3] = []string{
-		"646e9540c84f6a2f9cf6585d45a4c219573b4fd1b64a3c9a1386fc5cf98c0d4d",
-	}
-	binMap[4] = []string{
-		"7656caccdc79cd8d7ce66d415cc96a718e8271c62fb35746bfc2b49faf3eebf3",
-		"76d1e83c71ca246d042e37ff1db181f2776265fbcfdc890ce230bfa617c9c2f0",
-		"769ce86aa90b518b7ed382f9fdacfbed93574e18dc98fe6c342e4f9f409c2d5a",
-	}
-	binMap[5] = []string{
-		"7a48f75f8ca60487ae42d6f92b785581b40b91f2da551ae73d5eae46640e02e8",
-	}
-	binMap[6] = []string{
-		"7cbd42350bde8e18ae5b955b5450f8e2cef3419f92fbf5598160c60fd78619f0",
-	}
-
-	//create the pivot's kademlia
-	addr := common.FromHex(pivotAddr)
-	k := NewKademlia(addr, NewKadParams())
-
-	//construct the peers and the kademlia
-	for _, binaddrs := range binMap {
-		for _, a := range binaddrs {
-			addr := common.FromHex(a)
-			k.On(NewPeer(&BzzPeer{BzzAddr: &BzzAddr{OAddr: addr}}, k))
-		}
-	}
-
-	//TODO: check kad table is same
-	//currently k.String() prints date so it will never be the same :)
-	//--> implement JSON representation of kad table
-	log.Debug(k.String())
-
-	//simulate that we would do subscriptions: just store the bin numbers
-	fakeSubscriptions := make(map[string][]int)
-	//define the function which should run for each connection
-	eachBinFunc := func(p *Peer, bin int) bool {
-		//get the peer ID
-		peerstr := fmt.Sprintf("%x", p.Over())
-		//create the array of bins per peer
-		if _, ok := fakeSubscriptions[peerstr]; !ok {
-			fakeSubscriptions[peerstr] = make([]int, 0)
-		}
-		//store the (fake) bin subscription
-		fakeSubscriptions[peerstr] = append(fakeSubscriptions[peerstr], bin)
-		return true
-	}
-	//run the k.EachBin function
-	k.EachBin(addr[:], pot.DefaultPof(k.MaxProxDisplay), 0, eachBinFunc)
-	//calculate the kademlia depth
-	kdepth := k.NeighbourhoodDepth()
-
-	//now, check that all peers have the expected (fake) subscriptions
-	//iterate the bin map
-	for bin, peers := range binMap {
-		//for every peer...
-		for _, peer := range peers {
-			//...get its (fake) subscriptions
-			fakeSubs := fakeSubscriptions[peer]
-			//if the peer's bin is below the kademlia depth...
-			if bin < kdepth {
-				//(iterate all (fake) subscriptions)
-				for _, subbin := range fakeSubs {
-					//...only the peer's bin should be "subscribed"
-					//(and thus have only one subscription)
-					if subbin != bin || len(fakeSubs) != 1 {
-						t.Fatalf("Did not get expected subscription for bin < depth; bin of peer %s: %d, subscription: %d", peer, bin, subbin)
-					}
-				}
-			} else { //if the peer's bin is equal or higher than the kademlia depth...
-				//(iterate all (fake) subscriptions)
-				for i, subbin := range fakeSubs {
-					//...each bin from the peer's bin number up to k.MaxProxDisplay should be "subscribed"
-					// as we start from depth we can use the iteration index to check
-					if subbin != i+kdepth {
-						t.Fatalf("Did not get expected subscription for bin > depth; bin of peer %s: %d, subscription: %d", peer, bin, subbin)
-					}
-					//the last "subscription" should be k.MaxProxDisplay
-					if i == len(fakeSubs)-1 && subbin != k.MaxProxDisplay {
-						t.Fatalf("Expected last subscription to be: %d, but is: %d", k.MaxProxDisplay, subbin)
-					}
-				}
-			}
-		}
-	}
-
-	//print some output
-	for p, subs := range fakeSubscriptions {
-		log.Debug(fmt.Sprintf("Peer %s has the following fake subscriptions: ", p))
-		for _, bin := range subs {
-			log.Debug(fmt.Sprintf("%d,", bin))
-		}
-	}
-}

swarm/network/stream/snapshot_sync_test.go

@@ -538,10 +538,10 @@ func startSyncing(r *Registry, conf *synctestConfig) (int, error) {
 	kad := r.delivery.kad
 	subCnt := 0
 	//iterate over each bin and solicit needed subscription to bins
-	kad.EachBin(r.addr[:], pof, 0, func(conn *network.Peer, po int) bool {
+	kad.EachConn(nil, 255, func(p *network.Peer, po int, _ bool) bool {
 		//identify begin and start index of the bin(s) we want to subscribe to
 		subCnt++
-		err = r.RequestSubscription(conf.addrToIDMap[string(conn.Address())], NewStream("SYNC", FormatSyncBinKey(uint8(po)), true), NewRange(0, 0), High)
+		err = r.RequestSubscription(conf.addrToIDMap[string(p.Address())], NewStream("SYNC", FormatSyncBinKey(uint8(po)), true), NewRange(0, 0), High)
 		if err != nil {
 			log.Error(fmt.Sprintf("Error in RequestSubsciption! %v", err))
 			return false

swarm/network/stream/stream.go

@@ -33,7 +33,6 @@ import (
 	"github.com/ethereum/go-ethereum/swarm/log"
 	"github.com/ethereum/go-ethereum/swarm/network"
 	"github.com/ethereum/go-ethereum/swarm/network/stream/intervals"
-	"github.com/ethereum/go-ethereum/swarm/pot"
 	"github.com/ethereum/go-ethereum/swarm/state"
 	"github.com/ethereum/go-ethereum/swarm/storage"
 )
@@ -467,24 +466,8 @@ func (r *Registry) updateSyncing() {
 	}
 	r.peersMu.RUnlock()
 
-	// request subscriptions for all nodes and bins
-	kad.EachBin(r.addr[:], pot.DefaultPof(256), 0, func(p *network.Peer, bin int) bool {
-		log.Debug(fmt.Sprintf("Requesting subscription by: registry %s from peer %s for bin: %d", r.addr, p.ID(), bin))
-
-		// bin is always less then 256 and it is safe to convert it to type uint8
-		stream := NewStream("SYNC", FormatSyncBinKey(uint8(bin)), true)
-		if streams, ok := subs[p.ID()]; ok {
-			// delete live and history streams from the map, so that it won't be removed with a Quit request
-			delete(streams, stream)
-			delete(streams, getHistoryStream(stream))
-		}
-		err := r.RequestSubscription(p.ID(), stream, NewRange(0, 0), High)
-		if err != nil {
-			log.Debug("Request subscription", "err", err, "peer", p.ID(), "stream", stream)
-			return false
-		}
-		return true
-	})
+	// start requesting subscriptions from peers
+	r.requestPeerSubscriptions(kad, subs, r.doRequestSubscription)
 
 	// remove SYNC servers that do not need to be subscribed
 	for id, streams := range subs {
@@ -505,6 +488,69 @@ func (r *Registry) updateSyncing() {
 	}
 }
 
+// requestPeerSubscriptions calls on each live peer in the kademlia table
+// and sends a `RequestSubscription` to peers according to their bin
+// and their relationship with kademlia's depth.
+// Also check `TestRequestPeerSubscriptions` in order to understand the
+// expected behavior.
+// The function expects:
+//   * the kademlia
+//   * a map of subscriptions
+//   * the actual function to subscribe
+//     (in case of the test, it doesn't do real subscriptions)
+func (r *Registry) requestPeerSubscriptions(
+	kad *network.Kademlia,
+	subs map[enode.ID]map[Stream]struct{},
+	subscriptionFunc func(p *network.Peer, bin uint8, subs map[enode.ID]map[Stream]struct{}) bool) {
+
+	var startPo int
+	var endPo int
+	var ok bool
+
+	// kademlia's depth
+	kadDepth := kad.NeighbourhoodDepth()
+	// request subscriptions for all nodes and bins
+	// nil as base takes the node's base; we need to pass 255 as `EachConn` runs
+	// from deepest bins backwards
+	kad.EachConn(nil, 255, func(p *network.Peer, po int, _ bool) bool {
+		//if the peer's bin is shallower than the kademlia depth,
+		//only the peer's bin should be subscribed
+		if po < kadDepth {
+			startPo = po
+			endPo = po
+		} else {
+			//if the peer's bin is equal or deeper than the kademlia depth,
+			//each bin from the depth up to k.MaxProxDisplay should be subscribed
+			startPo = kadDepth
+			endPo = kad.MaxProxDisplay
+		}
+
+		for bin := startPo; bin <= endPo; bin++ {
+			//do the actual subscription
+			ok = subscriptionFunc(p, uint8(bin), subs)
+		}
+		return ok
+	})
+}
+
+// doRequestSubscription sends the actual RequestSubscription to the peer
+func (r *Registry) doRequestSubscription(p *network.Peer, bin uint8, subs map[enode.ID]map[Stream]struct{}) bool {
+	log.Debug(fmt.Sprintf("Requesting subscription by: registry %s from peer %s for bin: %d", r.addr, p.ID(), bin))
+	// bin is always less then 256 and it is safe to convert it to type uint8
+	stream := NewStream("SYNC", FormatSyncBinKey(uint8(bin)), true)
+	if streams, ok := subs[p.ID()]; ok {
+		// delete live and history streams from the map, so that it won't be removed with a Quit request
+		delete(streams, stream)
+		delete(streams, getHistoryStream(stream))
+	}
+	err := r.RequestSubscription(p.ID(), stream, NewRange(0, 0), High)
+	if err != nil {
+		log.Debug("Request subscription", "err", err, "peer", p.ID(), "stream", stream)
+		return false
+	}
+	return true
+}
+
 func (r *Registry) runProtocol(p *p2p.Peer, rw p2p.MsgReadWriter) error {
 	peer := protocols.NewPeer(p, rw, r.spec)
 	bp := network.NewBzzPeer(peer)