kad: Implement put_record_to and try_put_record_to#77
Merged
lexnv merged 11 commits intolexnv/expore-peer-idfrom Apr 19, 2024
Merged
kad: Implement put_record_to and try_put_record_to#77lexnv merged 11 commits intolexnv/expore-peer-idfrom
lexnv merged 11 commits intolexnv/expore-peer-idfrom
Conversation
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
src/protocol/libp2p/kademlia/mod.rs
Outdated
Comment on lines
+757
to
+761
| self.engine.start_put_record( | ||
| query_id, | ||
| record, | ||
| peers | ||
| ); |
Collaborator
There was a problem hiding this comment.
Going once again through start_put_record() implementation and FindNodeContext query used under the hood, this seems to be doing a slightly different thing that what's needed by authority-discovery. Seeding peers with some specific candidates would result in QueryType::PutRecord query initialized by that peers, but if FindNode collects more than 20 peers that are closer to the target than the original "seed" peers, the "seed" peers would end up not updated.
To update specific peers, we can go two routes:
- Update only peers if they are already in the routing table. For this, we just need to send
KademliaMessage::put_valuedirectly to peers from the routing table. This should be enough for authority-discivery needs, as the peers should be already in the routing table. - Update peers, even if they are not in the routing table. Basically, do
FindNodequery for the needed peers first, followed by sendingKademliaMessage::put_valueto the discovered addresses. We likely need this to conform to libp2pput_record_toimplementation, but can do in a follow-up.
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
lexnv
commented
Apr 19, 2024
dmitry-markin
approved these changes
Apr 19, 2024
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements the
put_record_toandtry_put_record_toto selectively pick peers to update their records.The main use-case from substrate would be the following:
This PR provided peers to the engine if the peers are part of the kBucket. The first step of the discovery in substrate motivates this assumption. We can probably do things a bit more optimally since we know the peers part of the kBucket were discovered previously (or currently connected):
PutNodeContextwhich circumvents the need to discover the peers and just forwards a kadPUT_VALUEto those peersWe'd have to double check that with libp2p as well (my brief looking over code points to this direction).
To unblock paritytech/polkadot-sdk#3786 we can merge this and then come back with a better / optimal solution for this
Builds on top of: #76
cc @paritytech/networking