Add a method to Kademlia to get detailed information about peers

[elferdo]

Addresses #1003

This PR adds peer_state() and peer_states() to Kademlia and allows to query their state within the table. I guess issue #1003 suggests also being able to query values stored in Kademlia but if that is necessary, I would address that in a separate PR.

[tomaka]

Looks good, but the code duplication because of &self vs &mut self is definitely not great.
I'd be in favour of having just one API and requiring &mut self everywhere, instead of having this duplication.

[elferdo]

I can change it, but I feel that having a query interface that takes &mut communicates a misleading intention. In general I prefer assuming complexity in the library rather than shifting an unnecessary requirement to the user. If we assume duplication now, we can still refactor later and users will not be affected. If we offer an interface that takes &mut now (which is a relatively strong requirement) and manage to remove it down the road, we will force our users to change their code.

[tomaka]

Another argument in favour of &mut is that the peer_state_query method you added doesn't have the logic that lazily updates the k-bucket in case of a timeout.
Because of that, the getters can return a state that is not "the correct one", for a certain definition of "correct". Calling entry and calling peer_state_query (which to me are supposed to be equivalent) can return two different states. I have mixed feelings about that, but I also don't know any clear path forward.

I don't think &mut is a strong requirement. Having a non-mut Swarm is virtually useless, and therefore I don't see any situation where the user wouldn't have a &mut reference of the behaviour.

If we offer an interface that takes &mut now (which is a relatively strong requirement) and manage to remove it down the road, we will force our users to change their code.

I don't see how. In the worst case scenario a user will not be able to use the API that requires &mut and would have to wait for us to add one that only requires &.

[elferdo]

Another argument in favour of &mut is that the peer_state_query method you added doesn't have the logic that lazily updates the k-bucket in case of a timeout.

Should it? I would expect the new query methods to just return a current snapshot of the table.

Because of that, the getters can return a state that is not "the correct one", for a certain definition of "correct". Calling entry and calling peer_state_query (which to me are supposed to be equivalent) can return two different states. I have mixed feelings about that, but I also don't know any clear path forward.

I agree that, in terms of information returned both should return the same status. My understanding of entry is that, for an entry not already in the DHT, the user has the possibility of inserting it straight away. For the new method I understood that we just want to know what is in the DHT. I think I'm missing the part with the timeout, will check the code again.

I don't think &mut is a strong requirement. Having a non-mut Swarm is virtually useless, and therefore I don't see any situation where the user wouldn't have a &mut reference of the behaviour.

I think this argument couples the design of the interface with its use. Agreed, if kad is only used in Swarm, then there may not be any other possibility. If for some reason we would like to reuse kad somewhere else, then the mutability requirement would be already there.

[tomaka]

Should it? I would expect the new query methods to just return a current snapshot of the table.

That's the issue that I don't know how to properly solve in terms of the API.

The situation is: we tell the k-bucket "if node A hasn't responded in 20 seconds, then replace it with B". Then if we immediately call entry(A), it will still be there. If we wait 21 seconds and call entry(A), it will no longer be there.

That's a bad way to do it, and in my opinion it should be changed to have some explicit update method. But the details of that are not straight-forward I think.

[elferdo]

That looks like something more complex than the &mut/code duplication discussion. Are we happy if I remove duplication for this PR and leave the timeout issue for another one?

[tomaka]

Are we happy if I remove duplication for this PR and leave the timeout issue for another one?

Yes!

[elferdo]

I'm trying to replace kbuckets::PeerState with the original kbuckets::Entry and I'm stuck trying to find a way to iterate over nodes and generate a collection of Entrys that hold a &mut to the parent. It makes sense, because if that were possible we would end up with many objects holding mutable references to the KBucketsTable. Please correct me if I'm wrong, and if you have an idea how to achieve this let me know.

Alternatively, what we can do to avoid code redundancy is make the Entry structures generic on the type of parent, and let it be mutable or inmutable depending on context:

/// Represents an entry in a k-bucket.                                                                                                                                                                                                                                                    
pub struct EntryInKbucketConn<'a, TParent, TPeerId> {                                                                                                                                                                                                                                     
    //parent: &'a mut KBucketsTable<TPeerId, TVal>,                                                                                                                                                                                                                                   
    parent: TParent,                                                                                                                                                                                                                                                                      
    peer_id: &'a TPeerId,                                                                                                                                                                                                                                                                 
}                                                                                                                                                                                                                                                                                         

This is relatively straightforward, but then type signatures look cumbersome:

/// Returns an iterator to all the peer IDs in the bucket, including the pending nodes.                                                                                                                                                                                               
pub fn entries<'a>(&'a self) -> impl 'a + Iterator<Item = Entry<'a, &'a KBucketsTable<TPeerId, TVal>, TPeerId>>{                                                                                                                                                                      

[tomaka]

I'm stuck trying to find a way to iterate over nodes and generate a collection of Entrys that hold a &mut to the parent.

Ah yeah, I didn't anticipate this.
Considering that entries can modify the table, it would be a bad idea to allow this. Calling a method on one entry could modify the state of another entry, and that would be quite bad.

I guess that this is a good motivation for restoring the code duplication.
Alternatively, we could make entries() return a list of PeerIds, and the user has to access them one by one if they want more information.

[romanb]

This is very much out of date but I believe most if not all of what was done here is already provided by the refactored APIs (in particular since #1117 which included substantial changes). I suggest to close this PR and update the associated ticket with details about what is still missing in the context of the current API.