Skip to content

Commit

Permalink
kad: Do not update memory store on incoming GetRecordSuccess (#190)
Browse files Browse the repository at this point in the history
This PR ensures that the records discovered by the `GetRecord` query are
not propagated to the memory store.
This makes the litep2p behavior similar to the libp2p.

Another approach could be to put the record into the memory store if the
validation mode is `Automatic`. However, for networks with a large
number of records, this will exhaust the limit of our memory store.

Thanks @alexggh for catching this 🙏 

Fix for:
- #190

cc @paritytech/networking

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
  • Loading branch information
lexnv and dmitry-markin committed Jul 31, 2024
1 parent 171e36a commit 19bf131
Showing 1 changed file with 4 additions and 31 deletions.
35 changes: 4 additions & 31 deletions src/protocol/libp2p/kademlia/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -656,31 +656,6 @@ impl Kademlia {
Ok(())
}
QueryAction::GetRecordQueryDone { query_id, records } => {
// Considering this gives a view of all peers and their records, some peers may have
// outdated records. Store only the record which is backed by most
// peers.
let now = std::time::Instant::now();
let rec = records
.iter()
.filter_map(|peer_record| {
if peer_record.record.is_expired(now) {
None
} else {
Some(&peer_record.record)
}
})
.fold(HashMap::new(), |mut acc, rec| {
*acc.entry(rec).or_insert(0) += 1;
acc
})
.into_iter()
.max_by_key(|(_, v)| *v)
.map(|(k, _)| k);

if let Some(record) = rec {
self.store.put(record.clone());
}

let _ = self
.event_tx
.send(KademliaEvent::GetRecordSuccess {
Expand Down Expand Up @@ -976,9 +951,8 @@ mod tests {
let action = QueryAction::GetRecordQueryDone { query_id, records };
assert!(kademlia.on_query_action(action).await.is_ok());

// Check the local storage was updated.
let record = kademlia.store.get(&key).unwrap();
assert_eq!(record.value, vec![0x1]);
// Check the local storage should not get updated.
assert!(kademlia.store.get(&key).is_none());
}

#[tokio::test]
Expand Down Expand Up @@ -1017,8 +991,7 @@ mod tests {
let action = QueryAction::GetRecordQueryDone { query_id, records };
assert!(kademlia.on_query_action(action).await.is_ok());

// Check the local storage was updated.
let record = kademlia.store.get(&key).unwrap();
assert_eq!(record.value, vec![0x2]);
// Check the local storage should not get updated.
assert!(kademlia.store.get(&key).is_none());
}
}

0 comments on commit 19bf131

Please sign in to comment.