Skip to content
This repository has been archived by the owner on Jan 11, 2024. It is now read-only.

IPC-58: Disable Kademlia storage inserts #62

Merged
merged 1 commit into from
Mar 6, 2023

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Mar 3, 2023

Closes consensus-shipyard/ipc#461

The PR disables the storing of Kademlia records in the store and warns if any peers sends us such requests.

@aakoshh aakoshh changed the title IPC-38: Disable Kademlia storage inserts IPC-58: Disable Kademlia storage inserts Mar 3, 2023
Copy link
Contributor

@adlrocha adlrocha left a comment

Choose a reason for hiding this comment

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

Have a look at my comment (just to double-check we don't break something(. If this is not the case LGTM.

@@ -115,6 +115,10 @@ impl Behaviour {
let protocol_name = format!("/ipc/{}/kad/1.0.0", nc.network_name);
kad_config.set_protocol_names(vec![Cow::Owned(protocol_name.as_bytes().to_vec())]);

// Disable inserting records into the memory store, so peers cannot send `PutRecord`
// messages to store content in the memory of our node.
kad_config.set_record_filtering(KademliaStoreInserts::FilterBoth);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a sanity-check (I haven't looked in-depth to the rust impl): disabling StoreInserts still allow peers to advertise their contact information on a PeerRecord right? If not this may break our discovery service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I can see it will disable PutRecord and AddProvider, but a Record is a key-value pair. It is indeed part of a PeerRecord so it's a bit confusing as to how exactly addresses spread. I will merge the tests first and check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to work. find_nearest_peers, bootstrap, AddProvider and PutRecord all enqueue background queries to find the ClosestPeers to the one in the record, so they have the same effect. It looks to me like if we don't want to advertise nodes providing specific content, then we can disable the store completely.

@aakoshh aakoshh merged commit f1a8a23 into main Mar 6, 2023
@aakoshh aakoshh deleted the ipc-58-disable-kademlia-put branch March 6, 2023 10:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IPLD Resolver: Disable Kademlia content storage
2 participants