fix(kad): iterator progress to be decided by any of new peers

[maqi]

The progress var is against the whole set of incoming closer_peers.
Hence, currently the last new peer will always overwrites the previous result.
i.e. the progress of the iterator is only decided by the last entry of the incoming closer_peers.

This commit is aimed to make the progress to be decided by any of the incoming closer_peer that turns out to be closer to the target.

Description

Notes & open questions

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

[mxinden]

Well spotted. Out of curiosity, mind sharing how you found the bug?

As far as I can tell, this fortunately only concerns the QueryStats.

[mxinden]

CI failure is unrelated. Seems like cargo semver-checks does not take our Cargo.lock into account. It uses libp2p-yamux v0.12.1 which deprecated WindowUpdateMode, even thought the corresponding dependabot pull request has not yet been merged. #4930

[maqi]

Well spotted. Out of curiosity, mind sharing how you found the bug?

As far as I can tell, this fortunately only concerns the QueryStats.

We ran a testnet of 2k nodes, and collected the PeerId of each node and the correspondent kBucketsTable from all peers.
According to the collected PeerIds, the avg discovered peers per node is at 150.
Which matchs the mathamatic ideal projection of the nodes' IDs distribution.
However, the real avg discovered peers according to the collected kBucketsTable is just at 124.

We also compared the result of expected peers in kBucketsTable and real collected.
The real collected ones are always short of some peers.

Hence I double checked the closest query flow and spot this suspicious point.

[thomaseizinger]

CI failure is unrelated. Seems like cargo semver-checks does not take our Cargo.lock into account. It uses libp2p-yamux v0.12.1 which deprecated WindowUpdateMode, even thought the corresponding dependabot pull request has not yet been merged. #4930

cc @obi1kenobi

[obi1kenobi]

Thanks for the ping. This is a bit of a tricky situation.

The proximate cause of the semver issue is RUSTFLAGS: '-Dwarnings' being set in the job that runs cargo-semver-checks: https://github.com/libp2p/rust-libp2p/blob/master/.github/workflows/ci.yml#L15

I don't think it's safe in general for the cargo-semver-checks action to unset RUSTFLAGS. I think it's possible that the user may be passing flags other than -Dwarnings (e.g. --cfg <something>) and removing them may break other use cases.

At the same time, the current behavior is unfortunate and a user-unfriendly default. I have some ideas for this and I'd love your thoughts as well! I've opened obi1kenobi/cargo-semver-checks#589 so we can continue the discussion without derailing this PR further.

Seems like cargo semver-checks does not take our Cargo.lock into account.

Correct, it does not take Cargo.lock into account. This is preferable — the alternative would cause both false-positives and false-negatives.

A few reasons: (click to expand)

• Users of a library experience semver as defined by the crate dependency bounds, not its Cargo.lock. If our library re-exports another library's APIs, upgrading that other library to a newer version within the same bounds can cause semver violations we'd like to find. Our users would find them otherwise, since cargo add doesn't use the lockfile of libraries.
• Only the "current" version of the library (in cargo-semver-checks terminology) has a lockfile. There's no lockfile for the "baseline" version — the one we're comparing against — so we'd end up using "latest" dependency versions for the baseline. When those dependencies' APIs appear in our own public API, we'll get false positives:
  • the baseline is using a newer version of the dependency which might have made API additions
  • those additions are not present in our library's "current" version, since they aren't in the dependency version in the lockfile
  • therefore the "current" version's public API is smaller than the baseline's public API — this is indistinguishable from a semver-breaking API removal, so it would (falsely) get reported as a breaking change
  • this has happened in the real world: Semver errors when exposing types from 3rd party crates that are not at the latest version obi1kenobi/cargo-semver-checks#167

[obi1kenobi]

#4942 will avoid this problem for the future in this repo, though I'd still love your feedback on solving the issue within cargo-semver-checks itself!

[thomaseizinger]

Seems like cargo semver-checks does not take our Cargo.lock into account.

Correct, it does not take Cargo.lock into account. This is preferable — the alternative would cause both false-positives and false-negatives.

Thanks for explaining! I guess we just need to update our dependencies ASAP then always :)

[maqi]

Hi, @mxinden ,

any idean if this PR is acceptable and to be merged?
thx.

[mxinden]

Well done!

Approvals have been dismissed because the PR was updated after the send-it label was applied.