Description

As part of developing #4718, the question came up whether we actually need to report the following as events:

• ReservationReqAcceptFailed
• ReservationReqDenyFailed
• CircuitReqDenyFailed
• CircuitReqAcceptFailed
• CircuitReqOutboundConnectFailed

The first four can only happen as part of a protocol violation of the remote, i.e. sending badly formatted messages. The last one might occur if the remote rejects the circuit we are trying to establish for another node.

Hypothesis

The hypothesis is: These events are unnecessary. Instead of surfacing them to the user, we should log them on an appropriate level.

Events are useful to programmatically tie into the behaviour of libp2p. The question is, what would you want to do about these errors other than logging them? Chances are, because a typical rust-libp2p application is emitting a lot of events, you are not handling all events individually but instead have a e => log::debug!("{e:?}" catch-all branch at the end of your event loop. That doesn't surface errors like this using an appropriate level.

These events are all about errors before we created some kind of resource or entity (a circuit or a reservation). The lack of state changes to the system make it unlikely that you need to perform some kind of corrective action when encountering such an event.

Suggestion

My suggestion is to remove these events and instead log them at the warn level. Most users will enable all warn messages within a system as those mean that nothing bad has happened yet but an increased number of warn logs might indicate a problem down the line.

From the PoV of a relay-node operator, an increased number of warnings for failed reservations or circuits means that for some reason, those other nodes are not using a compatible wire-format or - in the case of CircuitReqOutboundConnectFailed are denying inbound circuits with an increased rate. On a higher level, this means the relay server is not fulfilling its job by connecting other nodes with each other. Whilst not fatal to the system, this will likely lead to connectivity problems in a larger network. Thus, I think the use of warn! is well justified here.

Alternatives

Do you (reader) use these events for something else other than logging them? Please speak up if removing these somehow conflicts with your usecase.

Motivation

Simplify the public API for our users and notify them about potential problems using appropriate log levels.

Current Implementation

We just forward the errors to the user.

Are you planning to do it yourself in a pull request ?

Yes