feat(kad): expose a kad query facility allowing dynamic num_results

[maqi]

Description

This PR is to expose a kad query facility that allowing specify num_results dynamically.
It is related to the Sybil Defence issue,
that during the attempt of implementation on higher level code, it is find will be useful if libp2p-kad can expose such facility.

The PR try not to cause any interference to the existing work flow, only introduce an extra exposal.

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

[guillaumemichel]

Thanks @maqi for your contribution, this change is indeed needed! I left a few remarks.

[mergify]

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

[maqi]

@guillaumemichel PR updated with using your suggested approach of adding num_results into QueryInfo::GetClosestPeers, to avoid introduce extra functions.

also replied to your another comment regarding the with_config idea, which I think better not to be included in this PR, if really needed ?

btw. any idea why there is a failing CI https://github.com/libp2p/rust-libp2p/actions/runs/10390288352/job/28770402300?pr=5555 complaining failure enum_struct_variant_field_added: pub enum struct variant field added ?
and how to resolve it? thx

[maqi]

seems the failing CI requires a major version up on libp2p-kad, as a new variant added into the struct.

Though I think it is not necessary, I have to increase the libp2p-kad version to 0.47.0, to get an ALL green CI.

[maqi]

Hi, @guillaumemichel

thx for the review.
PR updated according to your new comments.

[guillaumemichel]

Thanks @maqi, it looks good. It would be great to add a test for the cases where num_result is higher and lower than K_VALUE.

[maqi]

Hi, @guillaumemichel

I have updated the PR to add a test covering the num_results range of K_VALUE / 2 .. K_VALUE * 2

However, it shows the returned result is capped after exceeds K_VALUE .
A quick read into the code shows it might needs a deeper refactor of the code base.

However, for us, a capped result is acceptable, hence I only put the test according to the current impl.
And put some comments to explain the situation.

I think to support results not capped , it will be better to have it as a separate PR ?

[guillaumemichel]

@maqi thanks for adding the tests!

However, for us, a capped result is acceptable.

What is your use case? If you need to pass a num_results to receive less peers than K_VALUE you might as well just take a subset of the returned peers with the default get_closest_peers. The feature seems incomplete if it is not possible to increase the number of returned peers to match num_results and I am afraid it will be confusing for users.

[maqi]

Hi, @guillaumemichel

For our case, we need to set the replication_factor to a value lower than the K_VALUE, but still want the query of get_closest gives a definable num of results ranging from that replication_factor to K_VALUE.

Or, you mean the get_closest will gives K_VALUE results anyway (as long as there are enough nodes in the network) ?
but I see the code of set num_results to replication_factor ?

[guillaumemichel]

Alright, I see! A high replication_factor or num_results is never expected to return more than K_VALUE results anyway.

IMO it is fine, because it was never possible to have more than K_VALUE closest peers in response, this change simply allows users to define a number of expected closest peers between 1 and K_VALUE. Previously, the number of returned closest peers was set to replication_factor.

Removing the cap might be tricky, since it would involve forging a new key and additional requests. It can be left to a follow up PR 👍🏻

[maqi]

Hi, @guillaumemichel ,

thx for the summary comment.
PR updated according to your new comment and added some of your word as code comment

[guillaumemichel]

Sorry for the back-and-forth, but it would be good to have different values of replication_factor in the test. K_VALUE (default) and a lower value (e.g K_VALUE / 2) should be enough.

[maqi]

Hi, @guillaumemichel ,

updated the test according to your suggestion.

[guillaumemichel]

Thanks for your contribution @maqi, it looks good to me!

@jxs could you also have a look before we merge?

[mergify]

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

[maqi]

Hi, @guillaumemichel ,

may I know the schedule of when the PR will get the second approval to be merged pls?
trying to resolve the conflicts contstantly is little bit annoying :)

[jxs]

Hi, and thanks for the interest! Left some comments

[maqi]

Hi, @jxs ,

Thx for the review.
PR updated according to your comments.

Replied to one comment, as I'm leading toward not to inline the code block.
Let me know if I have to, thx.

[jxs]

LGTM, thanks!