Skip to content

fix(server): properly disable kademlia record storage#5987

Open
VolodymyrBg wants to merge 14 commits intolibp2p:masterfrom
VolodymyrBg:bgg
Open

fix(server): properly disable kademlia record storage#5987
VolodymyrBg wants to merge 14 commits intolibp2p:masterfrom
VolodymyrBg:bgg

Conversation

@VolodymyrBg
Copy link
Contributor

Description

Replace the hack using zero-second TTL with proper record filtering in Kademlia configuration.

This PR addresses a TODO comment in misc/server/src/behaviour.rs where we were previously
using a workaround to disable storing records and provider records in Kademlia by setting
their TTL to zero seconds. The proper approach is to use the StoreInserts::FilterBoth
option, which is specifically designed for this purpose.

The changes:

  1. Replace set_record_ttl(Some(Duration::from_secs(0))) and set_provider_record_ttl(Some(Duration::from_secs(0))) with set_record_filtering(kad::StoreInserts::FilterBoth)
  2. Remove unused import time::Duration

These changes make the code cleaner and more maintainable by using the proper API rather than a workaround.

Notes & open questions

None

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@elenaf9 elenaf9 changed the title fix: properly disable kademlia record storage fix(server): properly disable kademlia record storage Apr 17, 2025
Copy link
Member

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

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

Thanks!
This needs a patch version bump and changelog entry.

@VolodymyrBg VolodymyrBg requested a review from elenaf9 April 17, 2025 13:21
@VolodymyrBg
Copy link
Contributor Author

Thanks! This needs a patch version bump and changelog entry.

Done

@mergify
Copy link
Contributor

mergify bot commented Apr 28, 2025

This pull request has merge conflicts. Could you please resolve them @VolodymyrBg? 🙏

@VolodymyrBg
Copy link
Contributor Author

This pull request has merge conflicts. Could you please resolve them @VolodymyrBg? 🙏

thanks bot, I resolved it)))

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

You also need to update Cargo.lock, you can do it with
cargo metadata

@VolodymyrBg
Copy link
Contributor Author

You also need to update Cargo.lock, you can do it with cargo metadata

Done

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

CI is still failing:
Version of 'libp2p-server' has been bumped more than once since last release v0.12.6.

0.12.7 hasn't been released no need to bump the Cargo.toml and a new changelog entry on the CHANGELOG.md only an entry with the change under the 0.12.7 release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants