Add handshake tracking to kademlia protocol#52
Add handshake tracking to kademlia protocol#52konradkonrad wants to merge 23 commits intoethereum:developfrom
Conversation
The `update()` method of `KademliaProtocol` is far too complex for sensible refactoring, this is a first step to untangle all involved state transitions.
Due to bonding (ping/pong), it must be possible to delay sending of find_node requests until bonding succeeded. Delayed requests will be send in `update()` method after bonding.
KademliaProtocol delays `find_node` requests until bonding succeeded. This is now reflected in the test.
At the end of bonding a node is not necessarily in the routing table. But if it is, we ensure the bonding property (i.e. node identity between `node` in scope and `node` from routing).
This fixes the `test_many` test to cater for bonding during the bootstrapping loop.
Note: this passes all of the previous assertions, although I'm unsure whether this really tests the desired behaviour.
At this point I am unsure why this test is set up this way: it triggers a pytest timeout and is then marked xfail, while the test method is worded `test_..._timeout`. This should probably be revisited, but I consider it out of scope for this.
|
|
||
| @property | ||
| def bonded(self): | ||
| return self.ping_recv and self.pong_recv |
There was a problem hiding this comment.
does the protocol specify how long the bonding info is valid?
There was a problem hiding this comment.
informal spec from geth:
bonding state is valid ~indefinitely in protocol version 4, however: when a node is marked for replacement (new candidate for full bucket should be added) and the known, "bonded" node does not respond to the keep alive ping, it will be
- replaced by the new candidate BUT
- kept in the "known" state (i.e. bonded, will receive answers to queries) for up to 24h (outside the table)
There was a problem hiding this comment.
something to add here: as far as I know, geth solves the out-of-table state tracking by persisting node state in a db. we don't have node persistence available. question is, if it was worth adding?
There was a problem hiding this comment.
if a simple pickle works for the data structure, and we already have infrastructure in devp2p to read config from a specified path, i'd say yes.
devp2p/kademlia.py
Outdated
| raise Exception | ||
|
|
||
| def get_node(self, node): | ||
| if node in self: |
There was a problem hiding this comment.
this is unnecessarily looping the self.node twice.
alternative:
return ([n for n in self.nodes if n.id == node.id] or [None])[0]
3 participants
This addresses part 1) of #50.
Changes:
Node(ping_recv+pong_recv)get_nodefor explicitly accessing nodes from kademlia routing table (so handshake tracking deals with the same instances)find_noderequests until after bondingfind_noderequestsupdate()method for better understanding of the flow