If I was to add a one-time-use peer to the swarm I'd have to:

1. implement a custom behavior that will actually store it
2. Make a call into behavior before request to make sure request-response can make a dial if it needs to
3. Remove address from the behavior right after this request in order to avoid leaking memory

The request response behavior already persist explicitly added addresses. You just need to call Swarm::add_peer_address.

Edit: Actually, in an out-of-band discussion with @jxs just now we noticed that the request-response behavior doesn't handle the SwarmEvent::NewExternalAddrOfPeer. So that should be added in

Edit2: The request-response behavior does handle relevant swarm events and updates its addresses based on it:

The request response behavior already persist explicitly added addresses. You just need to call Swarm::add_peer_address.

There are multiple issues with that:

• Those addresses are stored in PeerAddresses, which is initialized with defaults, meaning LRU with 100 entries (we sometimes dispatch A LOT more requests than that)
• There is still a need to add addresses there manually (which will also propagate to behaviors that I don't want it to propagate to)
• There is no way to ever delete peers from that cache once .remove_address() is gone (already deprecated)

All for what? I just want to make a request and I happen to know the address to dial too, it is too much wasted CPU and brain cycled doing useless things comparing to what this PR offers.

that is easily addressable right? We can expose choosing the size for the LruCache inside PeerAddresses via Config

It is not exposed right now and generally I see LRU cache as a hack when you can't specify the exact logic explicitly and instead just slap a heuristic number on it and hope it will work out. To be clear, it is a reasonable way to reduce complexity, but it is not optimal either for specialized use cases like this one.

It might work with large enough limit, but such limit will need to be correlated with other parameters elsewhere, increasing swarm configuration complexity and maintenance cost. It will also use more memory than it should and will use unnecessary CPU cycles in the process, not to say the data flow becomes much more convoluted than it needs to be on top of that.

this is interesting, can you explain why propagating to other hypothetical behaviours, (and which) would be a disadvantage to you?

The better question is why would I want to do that to begin with (except the design of request-response that mandates this right now)?

The disadvantages are many:

• notifying a bunch of behaviors and potentially trigger a bunch of logic in them unnecessarily (meaning each request is more expensive to make than it could have been, which might actually be non-negligible for embedded use cases)
• waste RAM and CPU on LRU cache and other storage containers other behaviors might have (will need to analyze behaviors unrelated to request-response to understand this, again probably bad for embedded use cases)
• writing extra code to add/remove addresses that has strictly negative value due to ^ (bad for maintenance)

I simply want for dial to not fail if the address of the peer is not already known to existing behaviors, just dial the explicitly provided address, as simple as that.

I also can't have a shortcut and not add address and not pay the cost of triggering logic in behaviors by checking if established or pending connection is there without writing even more code and spending even more RAM and CPU cycles (I tried that initially and quickly abandoned because I had to store additional hashmaps with non-trivial logic).

Can you also explain why removing would help you? #4302 doesn't mention it, but I don't see an objection to also remove addresses at the Swarm level with a FromSwarm::RemovedExternalAddrOfPeer event, which I also don't think would affect #4103 negatively

As mentioned many times already, these addresses are 100% useless for anything except the request that I want to make. So if I want to keep memory usage under control, I need to clean up the things that I add after they are no longer needed (meaning after request-response request, which is the only purpose here). This gets even more complex though because I need to know if the address wasn't added by something else prior (or else I'll remove it unnecessarily) and handle potential concurrent requests gracefully as well, all of which is very non-trivial.

If this PR goes through none of that is needed and indeed #4302 doesn't inherently require removing anything because it doesn't inherently require adding anything either, only current design of request-response does.

I would even go as far as saying that this cache in request-response is not a feature, but rather a bug, it should be removed without replacement. libp2p users can store addresses anywhere in any way they want (including a simple sidecar behavior) and decide if/how they want to limit number of addresses (LRU, explicit removal, etc.) instead of this storage being forced only everyone unconditionally.

Addresses are added to behaviors quite frequently because we generally build on the assumption that more "knowledge" about peer addresses is favorable. Compared to the workload and memory consumption of establishing a whole connection, I'd argue that reporting/ storing/ deleting a single address is rather cheap.

For instance, the identify behavior reports a ToSwarm::NewExternalAddrOfPeer for every listening address that the remote peer sends, which all behaviors then receive. So if you are using identify, the addresses will end up being added and propagated to all behaviors anyway once the connection is established.

Behaviors can also just ignore the reported address, which most actually do.

For request-response specifically, I think the address cache does make sense and should not be removed. IMO network behaviors should be self contained in the sense that it should possible to use a behavior on its own in the swarm, which wouldn't be possible for request-response when there is no way of storing an address.
I do agree with your point that an LRU cache is not optimal for all use-cases. But then again, for your specific case, wouldn't a small LRU cache size already be sufficient, because an added address is only used once, so it doesn't matter if it's evicted from the cache afterwards?

Compared to the workload and memory consumption of establishing a whole connection, I'd argue that reporting/ storing/ deleting a single address is rather cheap.

I see your point and agree it is not necessarily particularly expensive, but things that allocate (like LRU) are certainly not free and I would also prefer not to think about that. After working on performance-sensitive code for a while I try to avoid unnecessary overhead whenever possible, so I might be biased.

For request-response specifically, I think the address cache does make sense and should not be removed. IMO network behaviors should be self contained in the sense that it should possible to use a behavior on its own in the swarm, which wouldn't be possible for request-response when there is no way of storing an address.

I see the value in current behavior, so was looking at it from the point of view of extracting that logic into a separate behavior that can be composed together with request-response rather than being forced onto users. It'll also help with debugging in some cases since LRU means connection might succeed in some cases, but not others, which is sometimes tricky to find.

But then again, for your specific case, wouldn't a small LRU cache size already be sufficient, because an added address is only used once, so it doesn't matter if it's evicted from the cache afterwards?

Not really, it depends on how many requests are triggered before the dial is actually happening. If I queue over 100 requests to previously unknown peers, dial will annoyingly fail for some of them. Moreover the cognitive distance between where the address comes from and where it is used is way too large in that case, which hurts debugging.

I already have an address ready to be used, the DialOpts was already conditionally constructed before this PR, just let me construct it explicitly and not think about LRU and other non-determinism 🙏