Skip to content

feat(kad): enable putting Record without publisher #6177

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat(kad): enable putting Record without publisher #6177

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
08a3c3f
fix[kad]: enable putting `Record` with `None` `publisher` in a confus…
skaunov Oct 10, 2025
bf0866c
fmt
skaunov Oct 10, 2025
8783cf9
Update protocols/kad/CHANGELOG.md
skaunov Oct 20, 2025
82c7d6d
address review
skaunov Oct 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions protocols/kad/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
- minimized #6176

## 0.49.0

- Remove no longer constructed GetRecordError::QuorumFailed.
Expand Down
4 changes: 3 additions & 1 deletion protocols/kad/src/behaviour.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,9 @@ where
mut record: Record,
quorum: Quorum,
) -> Result<QueryId, store::Error> {
record.publisher = Some(*self.kbuckets.local_key().preimage());
if record.publisher.is_some() {
record.publisher = Some(*self.kbuckets.local_key().preimage())
}
self.store.put(record.clone())?;
record.expires = record
.expires
Expand Down
6 changes: 5 additions & 1 deletion protocols/kad/src/behaviour/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,11 @@ fn get_record_not_found() {
/// is equal to the configured replication factor.
#[test]
fn put_record() {
fn prop(records: Vec<Record>, seed: Seed, filter_records: bool, drop_records: bool) {
fn prop(mut records: Vec<Record>, seed: Seed, filter_records: bool, drop_records: bool) {
tracing::trace!("remove records without a publisher");
// this test relies on counting republished `Record` against `records.len()`
records.retain(|r| r.publisher.is_some());
Copy link
Member

Choose a reason for hiding this comment

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

can we then add a new test for new_anonymous?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not get any new ideas since #6176 (comment).

Notes & open questions

I don't have a fine test for this in mind as it doesn't really do anything (because replication context doesn't yield an event). Simultaneously I'm not really satisfied with the approach I took to adapt/fix the test (retaining only Record with publisher: Some); but I need a hint if that's possible to do better due to the same fact of how silent replication is.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, yeah I missed that, but then we don't need to do double iteration, this should work:

diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs
index 41d03f4f77..f8f1ee0134 100644
--- a/protocols/kad/src/behaviour/test.rs
+++ b/protocols/kad/src/behaviour/test.rs
@@ -562,11 +562,7 @@
 /// is equal to the configured replication factor.
 #[test]
 fn put_record() {
-    fn prop(mut records: Vec<Record>, seed: Seed, filter_records: bool, drop_records: bool) {
-        tracing::trace!("remove records without a publisher");
-        // this test relies on counting republished `Record` against `records.len()`
-        records.retain(|r| r.publisher.is_some());
-
+    fn prop(records: Vec<Record>, seed: Seed, filter_records: bool, drop_records: bool) {
         let mut rng = StdRng::from_seed(seed.0);
         let replication_factor =
             NonZeroUsize::new(rng.gen_range(1..(K_VALUE.get() / 2) + 1)).unwrap();
@@ -613,6 +609,7 @@
         #[allow(clippy::mutable_key_type)] // False positive, we never modify `Bytes`.
         let records = records
             .into_iter()
+            // Exclude records without a publisher.
+            .filter(|r| r.publisher.is_some())
             .take(num_total)
             .map(|mut r| {
                 // We don't want records to expire prematurely, as they would

Copy link
Contributor Author

@skaunov skaunov Oct 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Have it work for you? Looking at the checks below I read it that it doesn't pass anymore.


let mut rng = StdRng::from_seed(seed.0);
let replication_factor =
NonZeroUsize::new(rng.gen_range(1..(K_VALUE.get() / 2) + 1)).unwrap();
Expand Down
10 changes: 10 additions & 0 deletions protocols/kad/src/record.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,16 @@ impl Record {
where
K: Into<Key>,
{
Record {
key: key.into(),
value,
publisher: Some(PeerId::random()),
expires: None,
}
}

/// Creates a new record for insertion into the DHT.
pub fn new_anonymous(key: impl Into<Key>, value: Vec<u8>) -> Self {
Record {
key: key.into(),
value,
Expand Down