The Objects.requireNonNull() is not necessary. If peers is null then peers.stream() will throw an exception first.

good point.

change not made in latest commit

Instead of iterating the list to find the index, we should instead create a Map<NodeId, Integer> for O(1) lookup time.

O(1) here would be nice but i still have to keep a list of peers for the neighbors. To use a map, the values of the map would be a set of peers which must still be converted to a list in O(n). No good trade-off here.

O(1) is non-optional for stuff like this.

Perhaps we can get O(1) with a much similar approach. Is there any reason we can't just compare node IDs directly? The index is really just a proxy for node IDs. All we really want to do is figure out if the other node comes first in the address book, and if you have a lower node ID you will always come first in the address book.

This method used to return neighbors, but now it's returning all peers.

the peers are all neighbors, but not necessarily adjacent.

I don't understand this. If all peers are neighbors, why do we have 2 terms here? To me, neighbor implies adjacent

Won't this method have an off by 1 error for every node with index > self index? You're using position in peerNodes, but self is missing.

for every node with index > self index, we need the index returned by this method to be off by 1 (to match their position similar to the original addressBook).
There's an edge case for when being off by 1 can lead to this method returning an index number > peer list upper bound, but we deal with that correctly in the isAdjacent check so it's ok.

I don't understand this either. The previous code in this class did the following, to get the index of the other node in the address book. This computation includes self id in the indices

addressBook.getIndexOfNodeId(nodeId);

But the new code instead defines the index of the other node via its position in the peer list, which doesn't contain self id. So for every node with index greater than this self id, the returned node index will be 1 less.

Ok, I think I understand why this works.

If the node ID of the neighbor is greater than ours, then this method returns a value that is one less than its actual index. If the node ID of the neighbor is equal to our node ID plus one, getIndexOfNodeId() increments the number so that it doesn't actually equal our index.

This class still returns the correct results because we only care if the other node index is greater or less than our own index, so it doesn't matter that it's off by 1 as long as it is greater than our index.

Although this technically yields the correct result, I feel like this is one of those times where the code is "accidentally returning the right results". At the very least, the code here does not return values consistent with the javadocs. Better to refactor this code I think. I can imagine that the reason why/how this works may not be apparent to a developer who looks at this in the future.

Why are we modifying this class right now? We discussed as a group a few weeks back, and as I recall the decision was to make the network fully connected and not to try and modify the partial connectivity code until we had the bandwidth to test it properly.

The danger of modifying this code now is that we don't actually have any integration tests where the network is not fully connected, meaning we may be introducing bugs we don't catch for a long time.

All that this change does is to get rid of the addressBook within the network code so we're operating on peers, making tests so much easier. AB is limiting to the effectiveness of writing tests, and also just a completely orthogonal concept to the network code. I have deliberately made some choices to closely mimic the old functionality (until we decide to revisit fully connected vs otherwise), like doing an index lookup via iteration vs map (for ex).

like doing an index lookup via iteration vs map

The existing code uses AddressBook::getIndexOfNodeId to determine node index, which has a map lookup under the hood. Where are you seeing index lookup via iteration?

StaticTopology.getNeighbors is one.

Anyway, doing away with node indices altogether has made this a lot easier as we no longer have to match the old behavior.

To fully do away with node indeces, we have to drop use of RandomGraph and officially only support fully connected networks until we introduce a new implementation of connecting to less peers than the number of nodes in the address book.

this index is an AB concept, so we should get rid of it

@lpetrovic05 Is the scope of this PR to simply remove the address book references or also drop our support for non-fully connected network topologies? The RandomGraph class uses hard-coded arrays of size addressbook.getSize(). The translation from id to index is to support the use of RandomGraph to decide who is a neighbor when there are more nodes in the network than the degree of edges allowed per node.

If we drop the use of RandomGraph, then we can get rid of the translation from id to index. But if we keep RandomGraph, then we still need to translate from id to index. We can do it without the address book, using the List<PeerInfo>, but we'll have to reconstruct the Map<id, index>, and be sensitive to where the selfId falls into the sequence.

It is also important to note that RandomGraph is returned and used in the FallenBehindManager where it gets the neighbors of the node based off of the selfId's mapping to index in the address book. To truly drop RandomGraph, we'd need to supply a replacement in other locations where it is used.

This definition of a map is not meaningful. If you have the key, then you can call .id() and get the value without using the map.

I think your intent may be to create a new index ordering based on PeerInfo and map to indexes for use in compatibility with RandomGraph?

You've dropped the isAdjacent check with connectionGraph. If the PR is just to remove the address book dependency then you still need to query the index from the map you construct once it has the right value in the key-value pair.

The name of the variable is more appropriately named as nodeIdToIndexMap assuming the intent is to replace the address book lookup for index.

Is there an invariant on this list that the PeerInfo is in monotonically ascending order with respect to NodeId?

yes there is an invariant, but it's that PeerInfo are ordered as they appear in the original addressBook. I understand they should be monotonically ascending as defined in the AB, but I don't know if that's mandated anywhere.

In the previous implementation this method called the predicate argument method. That method has been deleted. In the implementation of that deleted method, the definition of Neighbor was scoped to the connectIonGraph which is a RandomGraph that also models the number of allowed degrees or edges between nodes. If there are 100 nodes and each node has only 10 degrees in the graph then each node has 10 neighbors. The old implementation would provide the 10 neighbors a node is allowed to talk to. This implementation returns all nodes in the List<PeerInfo> as being allowed to talk to.

Discussed with @edward-swirldslabs. The StaticTopology implementation, given nodes ranging from 1-100, returned what's equivalent to the peersList (i.e. the list of peers) all the time. There's a specific test case for this. Agreed it did that based on what's returned by connectionGraph.getNeighbors() but this PR trivially returned the peers list, which is equivalent. It may be helpful to have a discussion on why that may not be right.

I understand there's been some historic discussions around what the future of this class should look like, but the goal of this PR is to introduce an equivalent logic that does not use the AB.