Upgrading from 0.50.0 to 0.52.3

[MitchTurner]

We are currently upgrading our libp2p version and I thought I would start a discussion here for any ongoing questions we have in the process. LMK if there would be a better way of organizing our questions.

It is possible that our architecture is also out of date/wrong. Hopefully this will also be an opportunity to update the architecture as well.

---

I'm not sure of all the issues we are going to have; we have already successfully converted a lot of the code. But I'll start with the current problem:

Background

We have a custom network behavior PeerReportBehavior which on_swarm_event, will take the peer_id from a FromSwarm::ConnectionEstablished and lookup the addresses of that peer_id using addresses_of_peer which under the hood is:

    fn addresses_of_peer(&mut self, peer_id: &PeerId) -> Vec<Multiaddr> {
        self.identify.addresses_of_peer(peer_id)
    }

Obviously, addresses_of_peer is no longer available from PeerReportBehavior or identify::Behavior, so I thought maybe identify::Behavior might expose a getter for discovered_peers which is what the old addresses_of_peer used to do here. It doesn't.

Question

What is the correct way of getting those peer addresses?

I could just hit handle_pending_outbound_connections which will return the desired addresses as seen here. It just has a bunch of additional (albeit unused in this case) parameters, which makes me feel like this isn't the intended use.

I would appreciate your perspective and guidance avoiding any pitfalls here :)

[thomaseizinger]

We are currently upgrading our libp2p version and I thought I would start a discussion here for any ongoing questions we have in the process. LMK if there would be a better way of organizing our questions.

All good! This is what discussions are for :)

I could just hit handle_pending_outbound_connections which will return the desired addresses as seen here. It just has a bunch of additional (albeit unused in this case) parameters, which makes me feel like this isn't the intended use.

Correct. All APIs on NetworkBehaviou are "internal" in that, you should never call them yourself unless you are wrapping another NetworkBehaviour and are just forwarding calls.

There is an open issue and ongoing effort about sharing peer addresses across NetworkBehaviours: #4302. That will make it possible to easily implement a PeerStore which can gather and save addresses of peers that are discovered by other peers.

For the moment, it is unfortunately only possible to be notified about the addresses you get connected to.

What function does your PeerReportBehaviour fulfill?

[MitchTurner]

Move identify::Behaviour and Heartbeat into FuelBehaviour

Update handle_behaviour_event to make use if the data in identify::Event and the output of your Heartbeat behaviour to make whatever decisions you need to make. For example, identify::Event will contain the self-advertised addresses of each peer.

Yes. I have a feeling you might be right. I just want to make sure that the events are emitting what we need. I have a sneaking suspicion that our Heartbeat behavior, as well as another Discovery behavior might be obsolete as well, which would simplify our code a lot. It's just not ideal ripping things out at the same time we're upgrading, so trying to triage as best as I can.

I'm going to keep digging so I can talk more authoritatively about how this should work.

[thomaseizinger]

Do you know about libp2p-swarm-test? We use it internally to test all our behaviours: https://github.com/libp2p/rust-libp2p/blob/master/swarm-test/src/lib.rs

Makes it really easy to spin up several Swarms that are connected via memory-addresses. This is how we test the events of our network behaviours. See for example:

rust-libp2p/protocols/identify/tests/smoke.rs

Lines 81 to 129 in 82902fc

	#[async_std::test]
	async fn identify_push() {
	let _ = env_logger::try_init();
	let mut swarm1 = Swarm::new_ephemeral(|identity| {
	identify::Behaviour::new(identify::Config::new("a".to_string(), identity.public()))
	});
	let mut swarm2 = Swarm::new_ephemeral(|identity| {
	identify::Behaviour::new(
	identify::Config::new("a".to_string(), identity.public())
	.with_agent_version("b".to_string()),
	)
	});
	swarm1.listen().await;
	swarm2.connect(&mut swarm1).await;
	// First, let the periodic identify do its thing.
	let ([e1, e2], [e3, e4]) = libp2p_swarm_test::drive(&mut swarm1, &mut swarm2).await;
	{
	use identify::Event::{Received, Sent};
	// These can be received in any order, hence assert them here.
	assert!(matches!(e1, Received { .. } | Sent { .. }));
	assert!(matches!(e2, Received { .. } | Sent { .. }));
	assert!(matches!(e3, Received { .. } | Sent { .. }));
	assert!(matches!(e4, Received { .. } | Sent { .. }));
	}
	// Second, actively push.
	swarm2
	.behaviour_mut()
	.push(iter::once(*swarm1.local_peer_id()));
	let swarm1_received_info = match libp2p_swarm_test::drive(&mut swarm1, &mut swarm2).await {
	([identify::Event::Received { info, .. }], [identify::Event::Pushed { .. }]) => info,
	other => panic!("Unexpected events: {other:?}"),
	};
	assert_eq!(
	swarm1_received_info.public_key.to_peer_id(),
	*swarm2.local_peer_id()
	);
	assert_eq!(swarm1_received_info.protocol_version, "a");
	assert_eq!(swarm1_received_info.agent_version, "b");
	assert!(!swarm1_received_info.protocols.is_empty());
	assert!(swarm1_received_info.listen_addrs.is_empty());
	}

[thomaseizinger]

There is a lot of type-inference magic going on though so let me know if you have any questions on how to use it 😅

[MitchTurner]

Do you know about libp2p-swarm-test?

Yes! We had a nice conversation about it a few weeks ago (I think I've changed my icon since then :)).

That's part of the reason I'm pushing for this upgrade: I'm not happy about the state of the tests rn and I'm hoping we can get some better coverage and maintainability. libp2p-swarm-test wasn't supported at 0.50.0 IIRC.

[thomaseizinger]

Do you know about libp2p-swarm-test?

Yes! We had a nice conversation about it a few weeks ago (I think I've changed my icon since then :)).

Ah sorry about not recognizing you! Your nick was familiar but I didn't make the connection back to that thread :)

[mxinden]

Closing here. Seems resolved. Please let us know if you have more questions.

[MitchTurner]

We had to put the work on pause. Will be returning to it as soon as possible.