prevent loops in assimiliation

[sanity]

If a node receives a requestNewConnection() for a session that it has previously forwarded, it should be rejected, and the previous node in the chain should pick a different node to forward it to.

[sanity]

I think we need to add a unique identifier as a parameter to requestNewConnection() which is forwarded through the chain - such that if we see the same one twice we can reject it. We can't use the joinerAddress for this purpose because it is legitimate that a joiner might want us to forward two assimilation requests at the same time.

[sanity]

A few notes:

The unique identifier will be unique to a particular assimilation request chain - so we create it randomly in the first short AssimilateSessionImpl.startAssimilation() method (the one called on the joiner itself), and it should be forwarded as a parameter through the second startAssimilation() method and down the chain.

It may be tempting to store the "seen assimilation UIDs" in a static field of AssimilateSessionImpl, and while this would work, it is bad architectural design for the reasons explained here: https://code.google.com/p/google-singleton-detector/wiki/WhySingletonsAreControversial

Instead, dependencies like this UID cache need to be "injected", and fortunately we already have an object suitable for this, the TrNode class that is accessible from within any TrSessionImpl class.

So I think a reasonable approach would be to add the UID cache as a field to the TrNode object, from where it can be accessed from within the AssimilateSessionImpl object.

At some point TrNode might become a bit messy if we're using it for a bunch of "global" datastructures and we'll need to tidy it up somehow, but I think it is acceptable for the moment to put such stuff in there.

Oh, and don't forget that we'll need a unit test to test this functionality in AssimilateSessionImpl. Remember that individual unit tests should try to be as specific as possible in the functionality they test (so that when they fail we will know exactly what is broken).

[nomel7]

Would it make more sense for this cache to be in TrPeerManger? Speaking of which it would probably make sense to refactor all assimilation stuff out of TrPeerManager and have an AssimilationManager instead. The AssimilationManager would have a public accessor for the cache. We would also then refactor all topology maintenance stuff out into TopologyMaintenanceManager. If there's methods that don't make sense to be in one of the "manager" classes they could be moved into helper classes.

[sanity]

@nomel7 Yes, I think that's a good idea. What do you think @ravisvi ?

[sanity]

1. a new random UID is created in requestNewConnection(final RSAPublicKey requestorPubkey) method
2. This UID is passed as a second parameter to the requestNewConnection(final RemoteNodeAddress joinerAddress) method
3. At the start of requestNewConnection(final RemoteNodeAddress joinerAddress), we check to make sure that this UID is not in the "seen assimilation UIDs" cache in node.peerManager.seenAssimilationUids
4. If it's not, we add it to this cache. This UID is passed on down the assimilation chain in the second parameter of requestNewConnection(final RemoteNodeAddress joinerAddress)
5. If it is, we need to reject the assimilation request, there is currently no method to do this in the AssimilateSession interface, so we must add one
6. A node must handle the rejectAssimilation request by trying to forward the request to another node

[sanity]

@ravisvi Is this issue ready to be closed?

[ravisvi]

@sanity No, still have to write a test for it. All the methods are implemented though.