-
Notifications
You must be signed in to change notification settings - Fork 951
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
protocols/kad: Do not attempt to store expired record in record store #1496
Changes from 2 commits
d8e61ef
103cdd8
960d3a3
bfc4707
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -981,31 +981,44 @@ where | |
// overridden as it avoids having to load the existing record in the | ||
// first place. | ||
|
||
// The record is cloned because of the weird libp2p protocol requirement | ||
// to send back the value in the response, although this is a waste of | ||
// resources. | ||
match self.store.put(record.clone()) { | ||
Ok(()) => { | ||
debug!("Record stored: {:?}; {} bytes", record.key, record.value.len()); | ||
self.queued_events.push_back(NetworkBehaviourAction::NotifyHandler { | ||
peer_id: source, | ||
handler: NotifyHandler::One(connection), | ||
event: KademliaHandlerIn::PutRecordRes { | ||
key: record.key, | ||
value: record.value, | ||
request_id, | ||
}, | ||
}) | ||
} | ||
Err(e) => { | ||
info!("Record not stored: {:?}", e); | ||
self.queued_events.push_back(NetworkBehaviourAction::NotifyHandler { | ||
peer_id: source, | ||
handler: NotifyHandler::One(connection), | ||
event: KademliaHandlerIn::Reset(request_id) | ||
}) | ||
if !record.is_expired(Instant::now()) { | ||
// The record is cloned because of the weird libp2p protocol | ||
// requirement to send back the value in the response, although this | ||
// is a waste of resources. | ||
match self.store.put(record.clone()) { | ||
Ok(()) => debug!("Record stored: {:?}; {} bytes", record.key, record.value.len()), | ||
Err(e) => { | ||
info!("Record not stored: {:?}", e); | ||
self.queued_events.push_back(NetworkBehaviourAction::NotifyHandler { | ||
peer_id: source, | ||
handler: NotifyHandler::One(connection), | ||
event: KademliaHandlerIn::Reset(request_id) | ||
}); | ||
|
||
return | ||
romanb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
} | ||
|
||
// The remote receives a [`KademliaHandlerIn::PutRecordRes`] even in the | ||
// case where the record is discarded due to being expired. Given that | ||
// the remote sent the local node a [`KademliaHandlerEvent::PutRecord`] | ||
// request, the remote perceives the local node as one node among the k | ||
// closest nodes to the target. Returning a [`KademliaHandlerIn::Reset`] | ||
// instead of an [`KademliaHandlerIn::PutRecordRes`] to have the remote | ||
// try another node would only result in the remote node to contact an | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the current implementation, there would not be a retry on another node. Once the iterative query produces the |
||
// even more distant node. In addition returning | ||
// [`KademliaHandlerIn::PutRecordRes`] does not reveal any internal | ||
// information to a possibly malicious remote node. | ||
self.queued_events.push_back(NetworkBehaviourAction::NotifyHandler { | ||
peer_id: source, | ||
handler: NotifyHandler::One(connection), | ||
event: KademliaHandlerIn::PutRecordRes { | ||
key: record.key, | ||
value: record.value, | ||
request_id, | ||
}, | ||
}) | ||
} | ||
|
||
/// Processes a provider record received from a peer. | ||
|
@@ -1911,4 +1924,3 @@ impl QueryInfo { | |
} | ||
} | ||
} | ||
|
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.