Skip to content

Conversation

dmitry-markin
Copy link
Collaborator

@dmitry-markin dmitry-markin commented Aug 28, 2025

Because previously litep2p didn't send/receive PUT_VALUE ACKs, we can't just enable this mechanism — this would lead to failures when older remote peers don't behave up to spec. This PR deals with this situation by implementing a workaround to handle such peers:

  1. Track PUT_VALUE send successes instead of ACK responses to determine the query success.
  2. Ignore send errors when the remote doesn't read the PUT_VALUE ACK message.

This should be seen as a temporary measure until most of the networks updates to litep2p with PUT_VALUE ACK support (this PR or newer). Suggested timing is in a year after releasing the version with this PR. This is tracked by issue #429.

Builds upon #427.

@dmitry-markin dmitry-markin self-assigned this Aug 28, 2025
@dmitry-markin dmitry-markin requested a review from lexnv August 28, 2025 09:17
@dmitry-markin dmitry-markin changed the title kademlia: Workaround for dealing with missing PUT_VALUE ACKs implementation kademlia: Workaround for dealing with not implemented PUT_VALUE ACKs Aug 28, 2025
let query_id = match action {
// `SendFindNode` includes `FIND_NODE`, `GET_VALUE` and `GET_PROVIDERS`
// queries.
PeerAction::SendFindNode(query_id)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Maybe we could add the fn query_id() in the PeerAction?


/// Send message and ignore sending errors.
///
/// This is a hackish way of dealing with older litep2p nodes not exppecting receiving
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: typo in exppecting

Copy link
Collaborator

@lexnv lexnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants