Socks5 in GW probe #6298
Socks5 in GW probe #6298
Conversation
|
Thank you for making this first PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
dynco-nym
force-pushed
the
dyn/gw-probe-socks5
branch
from
4b118a7 to
05448ab
Compare
dynco-nym
force-pushed
the
dyn/gw-probe-socks5
branch
from
05448ab to
e4948e6
Compare
| [package] | ||
| name = "nym-node-status-agent" | ||
| version = "1.0.7" | ||
| version = "1.0.8-gw-socks5" |
There was a problem hiding this comment.
does it make sense to annotate the version with gw-socks5 suffix? it will either have it or not. i.e. there wouldn't be 1.0.8 that does not have it
| let available_gateways = self.available_gateways().await?; | ||
| info!("Listing all available gateways in topology:"); | ||
| for node in available_gateways.iter() { | ||
| info!("{}", node.identity_key.to_base58_string()); |
There was a problem hiding this comment.
I don't think this should be at info level. on mainnet we have +300 gateways so on client startup you will see over 300 logs every single time. for debug/trace I think that's useful information though. maybe also include node_id there for debugging purposes?
| #[arg(long)] | ||
| config_dir: Option<PathBuf>, | ||
| }, | ||
| Socks5 { |
There was a problem hiding this comment.
shouldn't socks5 test be part of the "normal" testing suite? maybe just introduce a flag in "normal" args to toggle it
|
|
||
| /// endpoint to test against | ||
| /// https://www.quicknode.com/docs/ethereum/web3_clientVersion | ||
| const TARGET_URL: &str = "https://docs-demo.quiknode.pro"; |
There was a problem hiding this comment.
why are we testing against this quite random website? why not something we control or something that's more used for this purpose
| match tokio::time::timeout( | ||
| self.mixnet_client_timeout, | ||
| client | ||
| .post(TARGET_URL) |
There was a problem hiding this comment.
wouldn't a simple get be easier? then we could, say, test against our own website
| ) -> anyhow::Result<ProbeResult> { | ||
| let exit_gateway = match gateway_key { | ||
| Some(gateway_key) => NodeIdentity::from_base58_string(gateway_key)?, | ||
| None => directory.random_exit_with_nr()?, |
There was a problem hiding this comment.
what's the point of testing a random gateway for socks5?
|
|
||
| // test failure doesn't stop further tests | ||
| let socks5_outcome = { | ||
| if let Some(ref nr_details) = node_info.network_requester_details { |
There was a problem hiding this comment.
nit: you have exactly the same match logic in multiple places
| if let Some(ref nr_details) = node_info.network_requester_details { | ||
| match do_socks5_connectivity_test( | ||
| &nr_details.address, | ||
| NymNetworkDetails::new_from_env(), |
| }; | ||
|
|
||
| info!("Waiting for network topology to be ready..."); | ||
| tokio::time::sleep(Duration::from_secs(10)).await; |
There was a problem hiding this comment.
couldn't we wait for some condition rather than always wait 10s? what if topology takes longer than that? (or way less than that)?
| /// Creates a SOCKS5 proxy connection through the mixnet to the exit GW | ||
| /// and performs necessary tests. | ||
| #[instrument(level = "info", name = "socks5_test", skip_all)] | ||
| async fn do_socks5_connectivity_test( |
There was a problem hiding this comment.
I think we should still investigate just creating a single client and sending NR packets to exit gateways rather than starting local socks5 listeners.
This change is