kademlia: Track success of PUT_VALUE queries
#427
Conversation
| @@ -428,6 +460,29 @@ impl QueryEngine { | |||
| query_id | |||
| } | |||
|
|
|||
| /// Start `PUT_VALUE` requests tracking. | |||
| pub fn start_put_record_to_found_nodes_requests_tracking( | |||
There was a problem hiding this comment.
Nice! IIUC, these changes are only impacting the newly added API via start_put_record_to_found_nodes_requests_tracking call?
There was a problem hiding this comment.
Yes, this PR only affects the last stage of PUT_VALUE — tracking individual requests to target peers after calling this function.
| // Send ACK even if the record was/will be filtered out to not reveal any | ||
| // internal state. | ||
| let message = KademliaMessage::put_value(record.clone()); | ||
| self.executor.send_message(peer, message, substream); |
There was a problem hiding this comment.
dq: Is libp2p sending the ACK as well? Im wondering if this is really needed, since we clone the record here and use some bandwidth?
There was a problem hiding this comment.
Yes, this is what libp2p does. I don't understand either, why the whole message is sent back instead of a small ACK message.
Due to these ACKs not currentty being sent by litep2p we will likely need a two-stage upgrade process: first start sending out ACKs and use some alternative mechanism for determining if the record was received (not relying on ACKs being received), and after most of the network upgrades switch to using received ACKs. I am going to implement "stage 1" in a follow-up PR.
There was a problem hiding this comment.
LGTM, nice work here! Left a few minor nits, feel free to merge at any time 🙏
Maybe we don't need to send back the ACK message to optimize the kademlia layer a bit, although we are diverging from the spec here 🤔 While at it, probably we don't need to track failed_peers for put records, but shouldn't add any noticible impact
|
@lexnv thanks for the review! All comments make sense, suggestions applied. As for ACKs, this is part of the protocol, and litep2p currently doesn't behave up to spec. This will cause some difficulties when upgrading — see the inline comment for the explanation. |
dmitry-markin
changed the title
PUT_VALUE queriesPUT_VALUE queries
Track success of
PUT_VALUEqueries and emitPutRecordSuccessevent.This PR add checking of the final stage of
PUT_VALUEquery — storing records at the target peers. When integrating the changes into polkadot-sdk, it should be taken into account that queries that previously unconditionally succeeded can (and will) now fail when not all the target peers returnPUT_VALUEACK. In order to keep authority-discovery operational it is advised to reduce the quorum of queries from "all" to 50% of target peers.