-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
base: master
Are you sure you want to change the base?
Conversation
…ing way. Sorry for that, see <libp2p#6176 (comment)> for some details.
Record with None publisher in a confus…Record with None publisher in a confus…
Record with None publisher in a confus…
jxs
left a comment
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.
Hi, and thanks for the cleaned up version.
left some suggestions
protocols/kad/src/behaviour/test.rs
Outdated
| // this test relies on counting republished `Record` against `records.len()` | ||
| records.retain(|r| r.publisher.is_some()); |
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.
can we then add a new test for new_anonymous?
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.
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.
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.
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 wouldThere 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.
Have it work for you? Looking at the checks below I read it that it doesn't pass anymore.
Co-authored-by: João Oliveira <[email protected]>
Description
superseeds #6176, see #6176 (comment) for some details.