swarm/pss: refactored according to review

swarm/network/simulation/kademlia.go

@@ -100,7 +100,7 @@ func (s *Simulation) kademlias() (ks map[enode.ID]*network.Kademlia) {
 }
 
 // WaitTillSnapshotRecreated is blocking until all the connections specified
-// in the snapshot are actually up and running.
+// in the snapshot are registered in the kademlia.
 // It differs from WaitTillHealthy, which waits only until all the kademlias are
 // healthy (it might happen even before all the connections are established).
 func (s *Simulation) WaitTillSnapshotRecreated(ctx context.Context, snap simulations.Snapshot) error {
@@ -138,7 +138,6 @@ func (s *Simulation) getActualConnections() (res []uint64) {
 func getSnapshotConnections(conns []simulations.Conn) (res []uint64) {
 	for _, c := range conns {
 		res = append(res, getConnectionHash(c.One, c.Other))
-		c.String()
 	}
 	return res
 }

swarm/network/simulation/kademlia_test.go

@@ -145,6 +145,16 @@ func createSimServiceMap(discovery bool) map[string]ServiceFunc {
 	}
 }
 
+// TestWaitTillSnapshotRecreated tests that we indeed have a network
+// configuration specified in the snapshot file, after we wait for it.
+//
+// First we create a first simulation
+// Run it as nodes connected in a ring
+// Wait until the network is healthy
+// Then we create a snapshot
+// With this snapshot we create a new simulation
+// Call WaitTillSnapshotRecreated() function and wait until it returns
+// Iterate the nodes and check if all the connections are successfully recreated
 func TestWaitTillSnapshotRecreated(t *testing.T) {
 	var err error
 	sim := New(createSimServiceMap(true))
@@ -185,6 +195,7 @@ func TestWaitTillSnapshotRecreated(t *testing.T) {
 	}
 }
 
+// exist returns true if val is found in arr
 func exist(arr []uint64, val uint64) bool {
 	for _, c := range arr {
 		if c == val {
@@ -195,8 +206,28 @@ func exist(arr []uint64, val uint64) bool {
 }
 
 func TestRemoveDuplicatesAndSingletons(t *testing.T) {
-	singletons := []uint64{0x3c127c6f6cb026b0, 0x0f45190d72e71fc5, 0xb0184c02449e0bb6, 0xa85c7b84239c54d3, 0xe3b0c44298fc1c14, 0x9afbf4c8996fb924, 0x27ae41e4649b934c, 0xa495991b7852b855}
+	singletons := []uint64{
-	doubles := []uint64{0x1b879f878de7fc7a, 0xc6791470521bdab4, 0xdd34b0ee39bbccc6, 0x4d904fbf0f31da10, 0x6403c2560432c8f8, 0x18954e33cf3ad847, 0x90db00e98dc7a8a6, 0x92886b0dfcc1809b}
+		0x3c127c6f6cb026b0,
+		0x0f45190d72e71fc5,
+		0xb0184c02449e0bb6,
+		0xa85c7b84239c54d3,
+		0xe3b0c44298fc1c14,
+		0x9afbf4c8996fb924,
+		0x27ae41e4649b934c,
+		0xa495991b7852b855,
+	}
+
+	doubles := []uint64{
+		0x1b879f878de7fc7a,
+		0xc6791470521bdab4,
+		0xdd34b0ee39bbccc6,
+		0x4d904fbf0f31da10,
+		0x6403c2560432c8f8,
+		0x18954e33cf3ad847,
+		0x90db00e98dc7a8a6,
+		0x92886b0dfcc1809b,
+	}
+
 	var arr []uint64
 	arr = append(arr, doubles...)
 	arr = append(arr, singletons...)
@@ -222,3 +253,57 @@ func TestRemoveDuplicatesAndSingletons(t *testing.T) {
 		}
 	}
 }
+
+func TestIsAllDeployed(t *testing.T) {
+	a := []uint64{
+		0x3c127c6f6cb026b0,
+		0x0f45190d72e71fc5,
+		0xb0184c02449e0bb6,
+		0xa85c7b84239c54d3,
+		0xe3b0c44298fc1c14,
+		0x9afbf4c8996fb924,
+		0x27ae41e4649b934c,
+		0xa495991b7852b855,
+	}
+
+	b := []uint64{
+		0x1b879f878de7fc7a,
+		0xc6791470521bdab4,
+		0xdd34b0ee39bbccc6,
+		0x4d904fbf0f31da10,
+		0x6403c2560432c8f8,
+		0x18954e33cf3ad847,
+		0x90db00e98dc7a8a6,
+		0x92886b0dfcc1809b,
+	}
+
+	var c []uint64
+	c = append(c, a...)
+	c = append(c, b...)
+
+	if !isAllDeployed(a, c) {
+		t.Fatal("isAllDeployed failed")
+	}
+
+	if !isAllDeployed(b, c) {
+		t.Fatal("isAllDeployed failed")
+	}
+
+	if isAllDeployed(c, a) {
+		t.Fatal("isAllDeployed failed: false positive")
+	}
+
+	if isAllDeployed(c, b) {
+		t.Fatal("isAllDeployed failed: false positive")
+	}
+
+	c = c[2:]
+
+	if isAllDeployed(a, c) {
+		t.Fatal("isAllDeployed failed: false positive")
+	}
+
+	if !isAllDeployed(b, c) {
+		t.Fatal("isAllDeployed failed")
+	}
+}

swarm/pss/prox_test.go

@@ -64,7 +64,7 @@ type testData struct {
 
 var (
 	pof   = pot.DefaultPof(256) // generate messages and index them
-	topic = BytesToTopic([]byte{0x00, 0x00, 0x06, 0x82})
+	topic = BytesToTopic([]byte{0xf3, 0x9e, 0x06, 0x82})
 )
 
 func (d *testData) getMsgCount() int {
@@ -123,7 +123,7 @@ func readSnapshot(t *testing.T, nodeCount int) simulations.Snapshot {
 	return snap
 }
 
-func initializeTestData(d *testData, msgCount int) {
+func (d *testData) init(msgCount int) {
 	log.Debug("TestProxNetwork start")
 	d.nodeAddrs = make(map[enode.ID][]byte)
 	d.recipients = make(map[int][]enode.ID)
@@ -187,6 +187,24 @@ func initializeTestData(d *testData, msgCount int) {
 	log.Debug("msgs to receive", "count", d.requiredMessages)
 }
 
+// Here we test specific functionality of the pss, setting the prox property of
+// the handler. The tests generate a number of messages with random addresses.
+// Then, for each message it calculates which nodes in the network the msg address
+// within its nearest neighborhood depth, and stores those nodes as possible
+// recipients. Those nodes that are the closest to the message address (nodes
+// belonging to the deepest PO wrt the msg address) are stored as required
+// recipients. The difference between allowed and required recipients results
+// from the fact that the nearest neighbours are not necessarily reciprocal.
+// Upon sending the messages, the test verifies that the respective message is
+// passed to the message handlers of these required recipients. Test will fail
+// if a message is handled by recipient which is not listed among the allowed
+// recipients of this particular message. It also fails after timeout, if not
+// all the required recipients have received their respective messages.
+//
+// For example, if proximity order of certain msg address is 4, and node X
+// has PO=5 wrt the message address, and nodes Y and Z have PO=6, then:
+// nodes Y and Z will be considered required recipients of the msg,
+// whereas nodes X, Y and Z will be allowed recipients.
 func TestProxNetwork(t *testing.T) {
 	t.Run("16/16", testProxNetwork)
 }
@@ -200,23 +218,9 @@ func TestProxNetworkLong(t *testing.T) {
 	t.Run("16/100", testProxNetwork)
 	t.Run("32/100", testProxNetwork)
 	t.Run("64/100", testProxNetwork)
+	t.Run("128/100", testProxNetwork)
 }
 
-// This tests generates a number of messages with random addresses. Then,
-// for each message it calculates which nodes in the network the msg address
-// within its nearest neighborhood depth, and stores those nodes as possible
-// recipients. Those nodes that are the closest to the message address (nodes
-// belonging to the deepest PO wrt msg address) are stored as required recipients.
-// Upon sending the messages, the test verifies that the respective message is
-// passed to the message handlers of these required recipients. Test will fail
-// if a message is handled by recipient which is not listed among the allowed
-// recipients of this particular message. It also fails after timeout, if not
-// all the required recipients have received their respective messages.
-//
-// For example, if proximity order of certain msg address is 4, and node X
-// has PO=5 wrt the message address, and nodes Y and Z have PO=6, then:
-// nodes Y and Z will be considered required recipients of the msg,
-// whereas nodes X, Y and Z will be allowed recipients.
 func testProxNetwork(t *testing.T) {
 	var tstdata testData
 	msgCount, nodeCount := getCmdParams(t)
@@ -237,11 +241,11 @@ func testProxNetwork(t *testing.T) {
 	if err != nil {
 		t.Fatalf("failed to recreate snapshot: %s", err)
 	}
-	initializeTestData(&tstdata, msgCount)
+	tstdata.init(msgCount) // initialize the test data
-	wrapper := func(c context.Context, s *simulation.Simulation) error {
+	wrapper := func(c context.Context, _ *simulation.Simulation) error {
-		return runFunc(&tstdata, c, s)
+		return runFunc(&tstdata, c)
 	}
-	result := tstdata.sim.Run(ctx, wrapper)
+	result := tstdata.sim.Run(ctx, wrapper) // call the main test function
 	if result.Error != nil {
 		// context deadline exceeded
 		// however, it might just mean that not all possible messages are received
@@ -255,12 +259,12 @@ func testProxNetwork(t *testing.T) {
 	t.Logf("completed %d", result.Duration)
 }
 
-func sendAllMsgs(sim *simulation.Simulation, msgs [][]byte, senders map[int]enode.ID) {
+func sendAllMsgs(tstdata *testData) {
-	for i, msg := range msgs {
+	for i, msg := range tstdata.msgs {
-		log.Debug("sending msg", "idx", i, "from", senders[i])
+		log.Debug("sending msg", "idx", i, "from", tstdata.senders[i])
-		nodeClient, err := sim.Net.GetNode(senders[i]).Client()
+		nodeClient, err := tstdata.sim.Net.GetNode(tstdata.senders[i]).Client()
 		if err != nil {
-			log.Crit(err.Error())
+			tstdata.errC <- err
 		}
 		var uvarByte [8]byte
 		binary.PutUvarint(uvarByte[:], uint64(i))
@@ -269,9 +273,10 @@ func sendAllMsgs(sim *simulation.Simulation, msgs [][]byte, senders map[int]enod
 	log.Debug("all messages sent")
 }
 
-func runFunc(tstdata *testData, ctx context.Context, sim *simulation.Simulation) error {
+// runFunc is the main test function, called by Simulation.Run()
+func runFunc(tstdata *testData, ctx context.Context) error {
 	go handlerChannelListener(tstdata, ctx)
-	go sendAllMsgs(sim, tstdata.msgs, tstdata.senders)
+	go sendAllMsgs(tstdata)
 	received := 0
 
 	// collect incoming messages and terminate with corresponding status when message handler listener ends
@@ -283,7 +288,6 @@ func runFunc(tstdata *testData, ctx context.Context, sim *simulation.Simulation)
 			received++
 			log.Debug("msg received", "msgs_received", received, "total_expected", tstdata.requiredMessages, "id", hn.id, "serial", hn.serial)
 			if received == tstdata.allowedMessages {
-				tstdata.doneC <- struct{}{}
 				close(tstdata.doneC)
 				return nil
 			}
@@ -388,11 +392,10 @@ func newProxServices(tstdata *testData, allowRaw bool, handlerContextFuncs map[T
 		"bzz": func(ctx *adapters.ServiceContext, b *sync.Map) (node.Service, func(), error) {
 			// normally translation of enode id to swarm address is concealed by the network package
 			// however, we need to keep track of it in the test driver as well.
-			// if the translation in the network package changes, that can cause thiese tests to unpredictably fail
+			// if the translation in the network package changes, that can cause these tests to unpredictably fail
 			// therefore we keep a local copy of the translation here
 			addr := network.NewAddr(ctx.Config.Node())
 			addr.OAddr = nodeIDToAddr(ctx.Config.Node().ID())
-
 			hp := network.NewHiveParams()
 			hp.Discovery = false
 			config := &network.BzzConfig{