make kad::kbucket::Distance public again?

[joshuef]

We rely on the public kbucket::Key and use the .distance() in some functionality.

https://github.com/maidsafe/safe_network/blob/main/sn_protocol/src/lib.rs#L113C1-L116C6

I was have a look at upgrading but as of 0.52 Distance is no longer exposed. #4103

Is there a blocker to reenabling that as a pub API?

[mxinden]

Sorry for the trouble. No objections from my end. Mind creating a pull request?

[joshuef]

Perfect, will do!

[jsantell]

Similarly, there are a few interfaces no longer exposed in 0.52.0 in KBucketRef (Distance, and EntryRefView specifically). Ran into this in our periodic peer dialer; should these KBucketRef interfaces be exposed, or perhaps is there a better way of handling?

  fn dial_next_peer(&mut self) {
        let mut to_dial = None;
        for kbucket in self.swarm.behaviour_mut().kad.kbuckets() {
            if let Some(range) = self.kad_last_range {
                if kbucket.range() == range {
                    continue;
                }
            }

            // find the first disconnected node
            for entry in kbucket.iter() {
                if entry.status == NodeStatus::Disconnected {
                    let peer_id = entry.node.key.preimage();

                    let dial_opts = DialOpts::peer_id(*peer_id)
                        .condition(PeerCondition::Disconnected)
                        .addresses(entry.node.value.clone().into_vec())
                        .extend_addresses_through_behaviour()
                        .build();
                    to_dial = Some((dial_opts, kbucket.range()));
                    break;
                }
            }
        }

        if let Some((dial_opts, range)) = to_dial {
            if let Err(e) = self.swarm.dial(dial_opts) {
                warn!("failed to dial: {:?}", e);
            }
            self.kad_last_range = Some(range);
        }
    }

[mxinden]

@jsantell not opposed to exposing them, though in your above example, given that you don't reference the concrete types, do you actually need them exposed?

[thomaseizinger]

I was have a look at upgrading but as of 0.52 Distance is no longer exposed. #4103

Are you currently running 0.51.3? I am asking because we didn't just remove these types but had deprecation warnings on them before that they might/will be private soon. Our thinking was that people hopefully speak up before we make them private to avoid situations like this :)

But perhaps that didn't work? Are you currently seeing those deprecation warnings?

[cdata]

I am asking because we didn't just remove these types but had deprecation warnings on them before that they might/will be private soon

Just chiming in to mention that we did not see deprecation warnings. The reason is probably because the libp2p_kad implementation was marked as deprecated but we used it via libp2p and it appears that the warnings were silenced during cargo build.

[jsantell]

@jsantell not opposed to exposing them, though in your above example, given that you don't reference the concrete types, do you actually need them exposed?

In this snippet, we check the status of the KBucketRef and compare to NodeStatus and the Distance is stored in self.kad_last_range, though those are the only references. This logic mostly inspired by beetle/iroh, maybe there are better ways (using public interfaces) for a similar result?

[thomaseizinger]

I am asking because we didn't just remove these types but had deprecation warnings on them before that they might/will be private soon

Just chiming in to mention that we did not see deprecation warnings. The reason is probably because the libp2p_kad implementation was marked as deprecated but we used it via libp2p and it appears that the warnings were silenced during cargo build.

Interesting, I just checked out your repo and I can see a deprecation warning in my IDE:

Yet, cargo check does not surface it. I'll investigate.

[thomaseizinger]

This almost seems like a rustc bug to me. Even with double --verbose, the deprecation warning is not visible. Note that double --verbose does show warnings in other crates which tells me that this warning isn't silenced, it is simply never emitted.

[cdata]

tells me that this warning isn't silenced, it is simply never emitted.

Sorry, yes, I am conflating my language here a bit. But, you're right, we don't even see the warning with -vv (though we do see some others). Possibly a rustc bug? 🤷

[thomaseizinger]

Possibly a rustc bug? shrug

I think so yes although a quick search didn't reveal anything.

cc @mxinden It seems that most of our efforts around deprecating modules so far was not very successful.

[mxinden]

Oh, that is surprising. They did show up whenever I upgraded https://github.com/mxinden/libp2p-lookup/, https://github.com/mxinden/kademlia-exporter/ and https://github.com/mxinden/rust-libp2p-server/.