-
Notifications
You must be signed in to change notification settings - Fork 128
Portal Client: Add banOtherClients debug parameter to be used for testing and debugging #3312
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
@@ -924,6 +925,9 @@ proc ping*( | |||
|
|||
p.radiusCache.put(dst.id, payload.data_radius) | |||
|
|||
if p.config.banOtherClients and payload.client_info != NIMBUS_PORTAL_CLIENT_INFO: | |||
p.banNode(dst.id, NodeBanDurationBanOtherClients) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also look at the 'c' value in the ENR? It might provide a faster way to filter out nodes. I'm not sure which way is better or perhaps using both would be ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The c value is nowhere really specified and I think clients might remove this in the future now that there is the ping extension which provides this information
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I understand the usefulness of this for testing whether some issues only occur with other clients, I'm still a bit in doubt whether we should add this in the actual code base.
It's true that it is a hidden
and debug
cli option and it isn't too invasive, so perhaps its fine.
@@ -924,6 +925,9 @@ proc ping*( | |||
|
|||
p.radiusCache.put(dst.id, payload.data_radius) | |||
|
|||
if p.config.banOtherClients and payload.client_info != NIMBUS_PORTAL_CLIENT_INFO: | |||
p.banNode(dst.id, NodeBanDurationBanOtherClients) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The c value is nowhere really specified and I think clients might remove this in the future now that there is the ping extension which provides this information
I understand the concern, perhaps there is a better way to achieve a similar result? Not sure. Anyway the driver for this was that I was trying to debug the failed content lookups today and thought it would be interesting to see if I could get the data from Nimbus Portal nodes (by filtering out the Trin nodes) and ended up writing this code to do that. I think it will be useful for various use cases and it would be nice to have it in the code base so I can just start up the node without having to copy in a code stash manually each time. Another example would be the portal bridge use case. If we have a bridge connected to a portal client that gossips content to other nodes in the network, if I want to optimize the performance of the gossip process it would be ideal to only send the content to Nimbus Portal nodes at least while working on performance testing/optimization in Nimbus Portal. |
This adds a new debug parameter (disabled by default) which can be used to ban all non Nimbus Portal nodes from the routing table. This will be useful when investigating issues on mainnet where we might want to only connect to Nimbus Portal clients for various reasons.
It also might be useful to sometimes run our portal bridges with the local Nimbus Portal client only connecting to other Nimbus Portal nodes.