Nicolas/introduced new parameters #4039
Conversation
| pub min_gateway_mixnet_performance: Option<u8>, | ||
| pub min_gateway_vpn_performance: Option<u8>, | ||
| pub residential_exit: bool, | ||
| pub poisson_parameter: Option<f32>, |
| } | ||
|
|
||
| async fn handle_set_poisson_parameter(&mut self, value: f32) { | ||
| tracing::debug!( |
There was a problem hiding this comment.
Please remove these debug logs.
| options, | ||
| } = connect_args; | ||
|
|
||
| let temporar_config = self.config_manager.config(); |
There was a problem hiding this comment.
This all looks bad. Why are you messing with the config here?
There was a problem hiding this comment.
How can I read the current options for poisson, average delay, message sending average delay and propagate them to the newly constructed config?
There was a problem hiding this comment.
You add setters to VpnServiceConfigManager.
But this function is setting the network; so why does it have to get involved with the config at all?
There was a problem hiding this comment.
In the current develop branch, the VpnServiceConfig structure is instantiated here
Given this, I have a question regarding the new fields I introduced:
If I remove the temporary config lookup, should these new fields be initialized as None when constructing VpnServiceConfig?
Are there any scenarios where the configuration might already exist in the config_manager and should be propagated into this newly created config after a connect command is triggered?
My concern is that if I simply pass None, the existing configuration values may never be saved or carried over. Could you advise on the correct approach here?
There was a problem hiding this comment.
The default values for the vpn service configuration is up to you really, however the vpn service configuration does not need to be changed when the network is changed as that occurs in the global config.
So it looks to me like there is no need to change this function at all.
There was a problem hiding this comment.
My question is the following:
The handle_connect function is triggered when the connect command is issued to the Nym VPN client. There is a scenario where the VPN service daemon is already running and receives these options without being connected. Given that, if I set default values for these options before being connected, will they still be saved when a connect command is executed, or will they be omitted if i do not use this way?
There was a problem hiding this comment.
@simonwicky Can you maybe help here Nick on how this should be done properly?
Side note: In general, any changes to the params should be only applied after reconnecting imo
nick1231321
force-pushed
the
nicolas/introduced_new_parameters
branch
from
5ee4fe9 to
f223b58
Compare
| } | ||
|
|
||
| #[derive(Debug, Clone, Eq, PartialEq)] | ||
| #[derive(Debug, Clone, PartialEq)] |
There was a problem hiding this comment.
Unnecessary changes in that file, you can revert the whole file
| network_stats: nym_vpn_lib_types::NetworkStatisticsConfig::from(value.network_stats), | ||
| average_packet_delay: None, | ||
| message_sending_average_delay: None, | ||
| poisson_parameter: None, |
There was a problem hiding this comment.
At some point you need to add your options to a version of VpnServiceConfig.
Current latest is v4, but it's getting code frozen for the release on Thursday Dec 11th so you will probably need a v5
| } | ||
| } | ||
|
|
||
| #[allow(unused)] |
There was a problem hiding this comment.
Empty line between the fns please
| #[cfg(not(windows))] | ||
| run_standalone(run_parameters, remove_log_file_signal, shutdown_token).await?; | ||
|
|
||
| let _worker_guard = if let Some(setup) = logging_setup { |
There was a problem hiding this comment.
Unnecessary changes here, you can revert the whole file
|
|
||
| Ok(tonic::Response::new(())) | ||
| } | ||
| async fn set_poisson_parameter( |
There was a problem hiding this comment.
Empty line between fns please
| self.config_manager.set_poisson_parameter(value).await; | ||
|
|
||
| // Update tunnel settings with throttle | ||
| self.update_tunnel_settings_with_throttle(); |
There was a problem hiding this comment.
@trojanfoe How are we handling changes that are triggering reconnection and changes that aren't? TunnelSettings::diff?
@aniampio Do we want to always reconnect on the new settings? What's the plan there?
There was a problem hiding this comment.
@simonwicky Yes, with helper functions added to TunnelSettingsDiff to make it easier to use. In Connecting and Connected state we general always reconnect on a change to tunnel settings, unless there are explicit exceptions (Allow LAN, for example). I think these settings will require a reconnection in which case no more logic is required when handling TunnelCommand::SetTunnelSettings.
… improved architecture.
Ticket
JIRA-VPN-4558
Description
This update introduces four new configurable parameters to the
MixnetClient.These parameters allow more manual control over timing and rate behaviors that affect packet delivery and cover-traffic generation.
Checklist
Verified no breaking changes to existing
MixnetClientbehaviorVerified debugging output to ensure parameters are applied correctly.
This change is