-
Notifications
You must be signed in to change notification settings - Fork 20
kademlia: Track success of ADD_PROVIDER
queries
#432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dm-put-record-substream-tracking
Are you sure you want to change the base?
kademlia: Track success of ADD_PROVIDER
queries
#432
Conversation
@@ -1,5 +1,3 @@ | |||
// Copyright 2023 litep2p developers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Should we do the same for all other files and just keep the copyright without the year?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, accidentally deleted these lines. Will revert.
Yes, we can live without a year as in polkadot-sdk.
|
||
/// Kademlia message. | ||
#[derive(Debug, Clone)] | ||
#[derive(Debug, Clone, EnumDisplay)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dq: This is used to display the message in the logs?
context.register_response_failure(peer); | ||
} | ||
Some(QueryType::PutRecordToFoundNodes { context, .. }) => { | ||
Some(QueryType::PutRecordToFoundNodes { context }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Maybe we can introduce a fn context()
on the QueryType
, such that we can turn this code into:
match self.queries.get_mut(&query) {
None => ...,
Some(query) => query.context.register_response_failure(peer);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job here! The changes look solid to me!
Just a tiny bit more testing on the kusama validators and we are good to go and include them in a litep2p release 🙏
Track success of
ADD_PROVIDER
queries and emitAddProviderSuccess
&QueryFailed
events.This PR adds tracking of the last stage of
ADD_PROVIDER
query: putting provider records to the target peers. Because libp2p spec doesn't haveADD_PROVIDER
ACK messages, we track only that the requests were sent out to the target peers without errors.Quorum
is respected when determining whether the query was successful or not.Builds upon #430.