Observability, internal and external

[rkuhn]

Hi everyone!

I’m currently working on bringing ipfs-embed up to date with libp2p 0.41 — we used a fork for a while due to an unresolved issue with TCP simultaneous open that I’m now working around in a completely different way. My perspective onto ipfs-embed (which may be markedly different from @dvc94ch’s, who created it) is that it basically ties a neat bundle of IFPS functionality out of existing transports and protocols and offers a shim layer to shield users from changes to the details. The main additional feature it offers is the address book, which I now completely overhauled, also due to the nature of the aforementioned workaround: the basic idea is that peer addresses are only considered “confirmed” (and used for general dialling) when a successful address-based dailling attempt has been made — an unsuccessful attempt will remove the address (it may be added again and confirmed later). This procedure allows me to switch off PortReuse, i.e. outgoing connections now originate from random ports instead of the listen port, which in turn removes the danger of TCP sim-open.

---

Now to my question.

There are two mechanisms for observing the swarm state: SwarmEvents can be seen from the outside, while (sub)behaviours see callbacks. These two interfaces are driven from the very same piece of code in Swarm::poll_next_event. The mechanisms have large overlap, but also notable differences — in practice I need to use both of them to get all the information I need. Since there does not seem to be a technical reason for this discrepancy, I’m asking for clarification of what the respective purpose is, so that I can then propose concrete improvements for the shortcomings I currently observe.

BTW: in 0.39 there was a way for the behaviour to find out whether all addresses have been dialled without success, in which case ipfs-embed emitted the Unreachable(peer) event. This is no longer possible since there is no API to figure out whether a given peer is currently being dialled and the events/callbacks also don’t carry this information. Again, I’ll propose improvements once I understand the purpose of the various pieces involved.

---

In closing, I’d like to say that 0.41 has markedly improved the visibility into when and why connections are going awry, this direction is definitely very good for everyone who operates libp2p-based systems in production! There are some minor things that can be improved, of course, and I’ll open PRs once the dust settles.

[thomaseizinger]

From my perspective, there should be feature-parity between both of these mechanisms. It really comes down to how you use rust-libp2p.

If you are developing a reusable and hence pluggable behaviour, creating a new NetworkBehaviour is the way to go in which case you override the callbacks you need. This is what all the protocols within this repository do.

For prototyping and application development, I tend to compose existing behaviours together through the custom derive, forward all events by combining them in enums and poll the swarm in an event loop, matching on all emitted events and reacting accordingly.

Your usecase sounds like you could make use of either, given that you are hiding the Swarm API completely.

[rkuhn]

That makes sense to me, I agree with the intention. Regarding the implementation, especially in light of the inclusion of handler arguments to some callbacks, this raises some more questions, though (the behaviour and/or its protocol handler would need to expose relevant parts of the handler state in such a way that they can be included in the SwarmEvent — I’m using handler state to associate my own dailling goal with the attempt and use that to interpret the result).

[mxinden]

in practice I need to use both of them to get all the information I need. Since there does not seem to be a technical reason for this discrepancy, I’m asking for clarification of what the respective purpose is, so that I can then propose concrete improvements for the shortcomings I currently observe.

Agreed with @thomaseizinger that the two should have feature parity. Thus, please file a bug or propose a patch for any discrepancy.

Regarding the implementation, especially in light of the inclusion of handler arguments to some callbacks, this raises some more questions, though (the behaviour and/or its protocol handler would need to expose relevant parts of the handler state in such a way that they can be included in the SwarmEvent — I’m using handler state to associate my own dailling goal with the attempt and use that to interpret the result).

The handler would be an exception, as we want to return the handler to its creator (the NetworkBehaviour implementation). We don't want to require Cloneon the handler to return it both to theNetworkBehaviourimplementation and as aSwarmEvent. I would argue that the handler should be an implementation detail of the NetworkBehaviour` implementation and thus should not be exposed to the outside in the first place. Let me know if that makes sense @rkuhn.

BTW: in 0.39 there was a way for the behaviour to find out whether all addresses have been dialled without success, in which case ipfs-embed emitted the Unreachable(peer) event. This is no longer possible since there is no API to figure out whether a given peer is currently being dialled and the events/callbacks also don’t carry this information. Again, I’ll propose improvements once I understand the purpose of the various pieces involved.

I am not sure I follow. The returned DialError in OutgoingConnectionError contains the dial failure reason. When the failure is on the transport layer DialError::Transport contains the addresses that failed.

rust-libp2p/swarm/src/lib.rs

Lines 192 to 198 in e19391e

	/// Outgoing connection attempt failed.
	OutgoingConnectionError {
	/// If known, [`PeerId`] of the peer we tried to reach.
	peer_id: Option<PeerId>,
	/// Error that has been encountered.
	error: DialError,
	},

[rkuhn]

I’ll have to think more about the role of the Handler. On the last point: My assumption is that I get an OutgoingConnectionError for every dial that is started, and I’d like to raise the Unreachable event only when the network transitions from “some dials in progress” to “no dials in progress, all failed”. The issue here is keeping track of ongoing dial attempts from within a sub-behaviour — since the swarm event only contains the PeerId and not the DialOpts (I may have a general dial running plus one with an address that is not yet known to the swarm, for validation).

[mxinden]

My assumption is that I get an OutgoingConnectionError for every dial that is started,

To make sure there is no confusion, with #2248, a dial can have many addresses. Those addresses are dialed in sequence (default) or in parallel. In case all addresses of a dial fail OutgoingConnectionError is reported. In case the dial failed on the transport level, the error field of OutgoingConnectionError is DialError::Transport with each address that failed.

since the swarm event only contains the PeerId and not the DialOpts

Sorry, I am not following. Which information are you missing in the SwarmEvent?

[rkuhn]

So is the following correct:

• for every dial (regardless of what kind), there will be exactly one of
  • inject_connection_established + ConnectionEstablished
  • inject_dial_failure + OutgoingConnectionError
• these will have been preceded by exactly one Dialing event, which has not NetworkBehaviour equivalent

The second one is where I have doubts. There is a doc bug for Dialing in that it mentions OutgoingConnectionError{attempts_remaining} which doesn’t exist anymore. And no such event is generated when calling Swarm::dial. So from what I can see, a (sub)behaviour cannot know how many dialling attempts are currently ongoing.

For ipfs-embed I now opted for tagging all dial failures that were not initiated by the address book as Unreachable events, which seems like a useful approximation.

[mxinden]

So is the following correct:

• for every dial (regardless of what kind), there will be exactly one of

  • inject_connection_established + ConnectionEstablished
  • inject_dial_failure + OutgoingConnectionError

Correct.

• these will have been preceded by exactly one Dialing event, which has not NetworkBehaviour equivalent

When the NetworkBehaviour emits a NetworkBehaviourAction::Dial and there is no synchronous (immediate) error, then the Swarm emits a SwarmEvent::Dialing. When the user calls Swarm::dial no SwarmEvent::Dialing is emitted.

I don't think this behavior (mismatch between NetworkBehaviourAction::Dial and Swarm::dial) is intuitive. I would guess it stems from the fact that one can not return SwarnEvents from Swarm::dial. In my eyes we should remove the inconsistency.

The second one is where I have doubts. There is a doc bug for Dialing in that it mentions OutgoingConnectionError{attempts_remaining} which doesn’t exist anymore.

Fixed via #2429.

And no such event is generated when calling Swarm::dial. So from what I can see, a (sub)behaviour cannot know how many dialling attempts are currently ongoing.

Which event are you referring to with "no such event"? In case a dial (triggered via NetworkBehaviourAction::Dial or Swarm::dial) fails, both NetworkBehaviour::inject_dial_failure and SwarmEvent::OutgoingConnectionErrror is called/emitted.