enhancement(Identify): add a behavior event that reports the list of locally supported protocols whenever that changes #4332
Comments
|
Thanks for opening this issue! There are two things we need to consider:
But it is difficult to make this a You could make a custom behaviour that forwards these events from the connection to the user but I am guessing you wanted this out of the box. Perhaps we should extend the |
This doesn't necessarily help newcomers though because they'd need to understand that they need to compose something here. Perhaps a dedicated behaviour is better because we can make that one specific to diffing the list of protocols directly which makes that more discoverable? |
|
First of all I found it weird that a developer...(ehem)... ...thought it was useful to have a debug message when the local set of supported protocols changed but not emit an event. It looks like this could easily be reported as a behavior event with the peer id, added/removed, and maybe the connection id. It could potentially be useful information for a connection manager type bit of code. Just like the Ping behavior is designed to support a connection manager, why not Identify? |
|
So something weird is going on that I think is a bug. Now that I have a behavior event, my test peer and identify client shows some strange behavior. What's happening is I run my test peer and then I run my identify client which is just an app that dials the test peer, runs identify and then dumps the protocols the peer reports. My test peer has keep_alive, ping, identify (w/ local protocols changed event), and kademlia behaviors. It starts up and does nothing but list. I doesn't dial nor do a kademlia.bootstrap(). The first time I run the identify client, I get the following output: You can see that it reports my test peer advertises three protocols: Now it is reporting that my test peer also advertises You can clearly see that the first connection results in a |
Also, to rule out the above, you can simply set the rust-libp2p/examples/file-sharing/src/network.rs Lines 72 to 75 in 2c4c961 If the above is not the case, mind sharing your code? That would make remote debugging a bit easier @dhuseby. |
The main information we are logging is that we are pushing those changes to the remote node, not that the protocols changed. See #3613 and linked issues for details :) I'll respond on the PR to the particular feature request. |
You're right, that is what is happening, however I'm erroneously adding a loop back address as as the observed address. That's incorrect. The peer and the test client are both running locally. What strategy do production servers use to avoid adding non-routable addresses as swarm external addresses? It seems like relying entirely on the identify response is error prone as illustrated in this example. The output confirms your hunch:
Sure, all of my code is here: https://github.com/dhuseby/fleyg |
Correct. That is why we changed that behaviour in the last release and no longer do that automatically.
AutoNAT solves this problem. Unfortunately, AutoNATv1 has certain issues which is why AutoNATv2 is in the works. Additionally, our AutoNATv1 implementation has some issues when it used together with port reuse. See #3889. In preparation for AutoNATv2, we need to rework the In the meantime, you have two options:
|
Starting in #3980, we make more use of the identify push protocol by actively notifying the peer when our locally supported protocols change. We emit an event (`Pushed`) to the user in that case. This event however does not include **what** we pushed. Users might be interested in the internal changes that we push to remote peers. This patch adds the identify `Info` to the `Pushed` variant. Fixes #4332. Pull-Request: #4527.
Description
Modify the behavior/handler to emit a behavior event with the new set of local protocols whenever it changes.
Motivation
Some behaviors--most notably the kademlia behavior--only adds its protocol to the local set when it gets into the correct state. Having the protocol list change be reported via an event would make it simple to not only report the protocols the peer is currently supporting but also to track/confirm state changes in other behaviors.
Current Implementation
Currently, the identify behavior only logs and pushes whenever the local_supported_protocols set changes.
Are you planning to do it yourself in a pull request?
Maybe.
The text was updated successfully, but these errors were encountered: