swarm/network: hive bug: needed shallow peers are not sent to nodes beyond connection's proximity order

[zelig]

The current swarm hive would in some occasions not connect to peers in shallow bins causing unhealthy connectivity
The reason is that needed shallow peers are not sent to nodes beyond connection's proximity order.
The initial response to subPeersMsg needs to be modified to allow enumeration of peers up to the advertised depth not the connection PO.

This PR introduces a one-liner fix to this bug.
It also adds extensive protocol exchange tests for initial subPeersMsg-peersMsg exchange
It modifies bzzProtocolTester to allow pregenerated overlay addresses

[frncmx]

As the PR lacks in-progress or ready for review labels, but a review has been requested from me I decided to give one. Please, forgive me if I came too early.

[frncmx]

typo: 'iff' => 'if'

[zelig]

iff means if and only if

[frncmx]

minor: then I would prefer 'if and only if' because that requires no knowledge of the abbreviation; that has to be written just once and read many times

[holisticode]

This PR should have in its description why it makes #19274 obsolete

[holisticode]

I can't find a compelling explanation why this is the correct function to use for this test and not hive.depth. And why is hive.depth accessible at all? Why was NeighbourhoodDepth() wrong and/or producing flaky results?

[zelig]

This is the logic. Polemic on the ambiguous naming aside.
saturation is the depth that the peer is advertising, basically, the shallowest bin, that has less than MinBinSize connections

[holisticode]

I deem the solution in the other PR with a channel to be more elegant than this polling.

[zelig]

i think your channel is closed before the protocol run function is called so it is too early

[holisticode]

I can't follow your reasoning here:

	protocol := func(p *p2p.Peer, rw p2p.MsgReadWriter) error {
		...		
		bzzAddr := preConns[n]
		// pop the address from the "stack"
		preConns = preConns[:n]
		// if there are no more addresses left, inform the caller
		if len(preConns) == 0 {
			waitC <- struct{}{}
		}		

The channel is "closed" inside the protocol function?

[holisticode]

Clarifying my suggestion, If you pass a chan to newBzzBaseTesterWithAddrs, then you could just close that channel when all addresses have been assigned instead of polling on the caller.

Also. len(addrs) will always be one, so maybe we don't need to pass a [][]byte, but I guess you want to keep it generic?

[frncmx]

I've just one major concern: the potential resource leak. However, if I'm right on that I think all the calls for NewProtocolTester() share the problem, we might defer that fix and create an issue.

All other comments are minor with the addition that, please, drop the unused *testing.T param.

Don't block on me.

[frncmx]

minor: I still hold the intuition that maybe this test tries to test too many things at once. (clean code: test one concept per test, ideally with one assert) But I let that go. On the other hand, please consider my version for the same logic. What do you think? Is it simpler?

• I put the simplest case first, so I can "forget" about that quickly. (smaller working memory)
• I did not like the return statement was 3 indents deep.

if len(expBzzAddrs) > 0 {
  if err != nil {
    t.Fatal(err)
   }
} else {
  if err == nil {   // <= stating we are expecting an error when `len(expBzzAddrs)` is not nill (I usually try to aoid `err == nil` but I think in this case it fits.
    t.Fatalf("expected timeout, got no error")
  }
  
   timeoutMessage := "exchange #1 \"trigger subPeersMsg and expect peersMsg\": timed out" 
   if err.Error() != timeoutMessage {
      t.Fatalf("expected timeout, got %v", err)
   } 
}

Note:

• Since the function scope is big 100+ lines, I use if...else to press that I'm checking related stuff, i.e., I'm still making decisions based on len(expBzzAddrs) not something else coming from the previous lines.