adding datagram and i2cp options to SessionOptions#11
Conversation
src/options.rs
Outdated
| /// IP address where the datagram socket should be bound to | ||
| /// | ||
| /// Defaults to `0`. | ||
| pub datagram_host: u128, |
There was a problem hiding this comment.
This should be String and it should default to 127.0.0.1
There was a problem hiding this comment.
In fact, datagram_host and datagram_port can be removed, they're automatically configured.
There was a problem hiding this comment.
Aren't datagram_host and datagram_port needed for the two methods of #9 ? Unless they are also automatically configured at the datagram message level ?
There was a problem hiding this comment.
datagram_host and datagram_port specify the address of the client's socket and they're sent in SESSION CREATE message to the router so it can relay datagrams received from the network to the client.
DatagramOptions should only have the following flags:
[FROM_PORT=nnn] # SAM 3.2 or higher only, default 0
[TO_PORT=nnn] # SAM 3.2 or higher only, default 0
[PROTOCOL=nnn] # SAM 3.2 or higher only, only for RAW sessions, default 18
[SEND_TAGS=nnn] # SAM 3.3 or higher only, number of session tags to send
# Overrides crypto.tagsToSend I2CP session option
# Default is router-dependent (40 for Java router)
[TAG_THRESHOLD=nnn] # SAM 3.3 or higher only, low session tag threshold
# Overrides crypto.lowTagThreshold I2CP session option
# Default is router-dependent (30 for Java router)
[EXPIRES=nnn] # SAM 3.3 or higher only, expiration from now in seconds
# Overrides clientMessageTimeout I2CP session option (which is in ms)
# Default is router-dependent (60 for Java router)
[SEND_LEASESET={true,false}] # SAM 3.3 or higher only, whether to send our leaseset
# Overrides shouldBundleReplyInfo I2CP session option
# Default is true
There was a problem hiding this comment.
Oops sorry I misread the docs indeed. Will delete.
src/options.rs
Outdated
| /// | ||
| /// Defaults to `2`. | ||
| pub num_outbound: usize, | ||
| pub inbound_qty: usize, |
There was a problem hiding this comment.
| pub inbound_qty: usize, | |
| pub inbound_quantity: usize, |
src/options.rs
Outdated
| /// How many outbound tunnels does the tunnel pool of the session have. | ||
| /// | ||
| /// Defaults to `2`. | ||
| pub outbound_qty: usize, |
There was a problem hiding this comment.
| pub outbound_qty: usize, | |
| pub outbound_quantity: usize, |
src/options.rs
Outdated
| /// Outbound-only sessions (clients) shouldn't be published whereas servers (accepting inbound | ||
| /// connections) need to be published. | ||
| /// | ||
| /// |
src/options.rs
Outdated
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct OtherI2CPOptions { |
There was a problem hiding this comment.
To remain consistent with the current code style, add line breaks between the options
| match &self.options.destination { | ||
| DestinationKind::Transient => { | ||
| command += "DESTINATION=TRANSIENT "; | ||
| "RAW" => { |
There was a problem hiding this comment.
It doesn't look like DESTINATION is specified for RAW. Could you please reduce code duplication between the three protocol kinds? It should match after parameters.options have been handled and only add FROM_PORT, TO_PORT, PROTOCOL and HEADER if the style indicates datagrams. Otherwise the different styles should be able to use the same code
|
I added two methods in datagram.rs that I believe will solve #9 I also deleted the fastreceive option after reading this exchange : eepnet/emissary#28 |
|
I believe the signature type datagram option should be added as well, as new post-quantum crypto types should eventually be approved, numbered from 12 to 20 as per proposal 169 : https://geti2p.net/spec/proposals/169-pq-crypto#signatures |
| .map_err(From::from) | ||
| } | ||
|
|
||
| pub(crate) async fn _send_to_with_options( |
There was a problem hiding this comment.
The idea is to have a public function for Session<Repliable>/Session<Anonymous> like:
#[derive(Default)]
struct DatagramOptions {
from_port: u16,
to_port: u16,
}
impl Session<Repliable> {
pub async fn send_to_with_options(
&mut self,
buf: &[u8],
destination: &str,
options: DatagramOptions,
) -> crate::Result<()> {
style::Repliable::send_to_with_options(&mut self.context, buf, destination, options).await
}
}and the style::Repliable::send_to_with_options() would then fill FROM_PORT/TO_PORT from DatagramOptions. This allows overriding the source/destination port specified in SESSION CREATE message.
These functions must be available on both synchronous and asynchronous APIs
There was a problem hiding this comment.
Ok I see, I will rollback these for now and will submit another PR afterwards.
| let socket = UdpSocket::bind(format!("127.0.0.1:{}", options.datagram_port)).await?; | ||
| let socket = UdpSocket::bind(format!("127.0.0.1:{}", 0u16)).await?; |
There was a problem hiding this comment.
My mistake, sorry. Please revert this change and keep datagram_port in SessionOptions. I forgot that the port for datagrams is taken from the config
src/options.rs
Outdated
| /// (ms) Idle time required | ||
| /// | ||
| /// Defaults to '1800000' ms (i.e. 30 minutes) | ||
| pub close_idle_time: usize, |
There was a problem hiding this comment.
| /// (ms) Idle time required | |
| /// | |
| /// Defaults to '1800000' ms (i.e. 30 minutes) | |
| pub close_idle_time: usize, | |
| /// Idle time required | |
| /// | |
| /// Defaults to 30 minutes. | |
| pub close_idle_time: Duration, |
There was a problem hiding this comment.
Is the comment correct? It's the same for both close_idle_time and reduce_idle_time
src/options.rs
Outdated
| /// (ms) Idle time required | ||
| /// | ||
| /// Defaults to '1200000' ms (i.e. 20 minutes) | ||
| pub reduce_idle_time: usize, |
There was a problem hiding this comment.
| /// (ms) Idle time required | |
| /// | |
| /// Defaults to '1200000' ms (i.e. 20 minutes) | |
| pub reduce_idle_time: usize, | |
| /// Idle time required | |
| /// | |
| /// Defaults to 20 minutes | |
| pub reduce_idle_time: Duration, |
src/options.rs
Outdated
| crypto_low_tag_threshold: str::parse::<usize>("30").unwrap(), | ||
| crypto_ratchet_inbound_tags: str::parse::<usize>("160").unwrap(), | ||
| crypto_ratchet_outbound_tags: str::parse::<usize>("160").unwrap(), | ||
| crypto_tags_to_send: str::parse::<usize>("40").unwrap(), |
There was a problem hiding this comment.
I was a bit tired aha. Fixed it.
src/options.rs
Outdated
| outbound_random_key: None, | ||
| should_bundle_reply_info: true, | ||
| close_on_idle: false, | ||
| close_idle_time: str::parse::<usize>("1800000").unwrap(), |
There was a problem hiding this comment.
| close_idle_time: str::parse::<usize>("1800000").unwrap(), | |
| close_idle_time: Duration::from_millis(1800000), |
src/options.rs
Outdated
| lease_set_secret: None, | ||
| lease_set_signing_private_key: None, | ||
| reduce_on_idle: false, | ||
| reduce_idle_time: str::parse::<usize>("1200000").unwrap(), |
There was a problem hiding this comment.
| reduce_idle_time: str::parse::<usize>("1200000").unwrap(), | |
| reduce_idle_time: Duration::from_millis(1200000), |
src/proto/session.rs
Outdated
| if !self.options.publish { | ||
| match parameters.style.as_str() { | ||
| "PRIMARY" => {} | ||
|
|
There was a problem hiding this comment.
Please remove these line breaks between the styles
src/proto/session.rs
Outdated
|
|
||
| "DATAGRAM" => { | ||
| command += format!( | ||
| "FROM_PORT={} TO_PORT={}", |
There was a problem hiding this comment.
| "FROM_PORT={} TO_PORT={}", | |
| "FROM_PORT={} TO_PORT={} ", |
Whitespace needed at the end so the command is valid
src/proto/session.rs
Outdated
|
|
||
| "RAW" => { | ||
| command += format!( | ||
| "FROM_PORT={} TO_PORT={} PROTOCOL={} HEADER={}", |
|
I would open a PR in emissary to align with the renaming of num_inbound/outbound to inbound/outbound.quantity in yosemite. Very light refactoring. |
|
I need to cut a new release for yosemite in order for emissary to use these changes and I would like to include both this PR and #9 for the next release. Feel free to make a PR if you want, it should be quite doable as |
|
Ah so you mean merging this PR will not automatically cause compatibility issues with emissary-cli>tunnel>client.rs for example ? |
altonen
left a comment
There was a problem hiding this comment.
Left some nits so that documentation looks consistent, otherwise LGTM
src/options.rs
Outdated
|
|
||
| /// Signature type. | ||
| /// | ||
| /// Default to '7' i.e. EdDSA_SHA512_Ed25519 |
There was a problem hiding this comment.
| /// Default to '7' i.e. EdDSA_SHA512_Ed25519 | |
| /// Default to `7`, i.e., EdDSA-SHA512-Ed25519 |
src/options.rs
Outdated
|
|
||
| /// If incoming zero hop tunnel is allowed | ||
| /// | ||
| /// Defaults to 'false' |
There was a problem hiding this comment.
| /// Defaults to 'false' | |
| /// Defaults to `false`. |
src/options.rs
Outdated
| pub inbound_quantity: usize, | ||
|
|
||
| /// How many outbound tunnels does the tunnel pool of the session have. | ||
| /// Number of redundant fail-over for tunnels in |
There was a problem hiding this comment.
This is missing something? It's a half a sentence
There was a problem hiding this comment.
This is how it is described in the I2CP doc. I will rewrite as /// Number of redundant fail-over for inbound tunnels.
There was a problem hiding this comment.
True, sounds weird though. Maybe "Number of redundant, fail-over inbound tunnels" + same for the outbound case
src/options.rs
Outdated
| /// Number of IP bytes to match to determine if two routers should not be in the same tunnel. 0 | ||
| /// to disable. | ||
| /// | ||
| /// Defaults to `0`. | ||
| pub inbound_ip_restriction: usize, |
There was a problem hiding this comment.
| /// Number of IP bytes to match to determine if two routers should not be in the same tunnel. 0 | |
| /// to disable. | |
| /// | |
| /// Defaults to `0`. | |
| pub inbound_ip_restriction: usize, | |
| /// Number of IP bytes to match to determine if two routers should not be in the same tunnel. | |
| /// | |
| /// Defaults to `None`. | |
| pub inbound_ip_restriction: Option<std::num::NonZeroUsize>, |
src/options.rs
Outdated
|
|
||
| /// If outgoing zero hop tunnel is allowed | ||
| /// | ||
| /// Defaults to 'false' |
There was a problem hiding this comment.
| /// Defaults to 'false' | |
| /// Defaults to `false`. |
src/options.rs
Outdated
|
|
||
| /// Close I2P session when idle | ||
| /// | ||
| /// Defaults to 'false' |
There was a problem hiding this comment.
| /// Defaults to 'false' | |
| /// Defaults to `false`. |
src/options.rs
Outdated
| /// (ms) Idle time required before closing session | ||
| /// | ||
| /// Defaults to '1800000' ms (i.e. 30 minutes) | ||
| pub close_idle_time: Duration, |
There was a problem hiding this comment.
| /// (ms) Idle time required before closing session | |
| /// | |
| /// Defaults to '1800000' ms (i.e. 30 minutes) | |
| pub close_idle_time: Duration, | |
| /// Idle time required before closing session | |
| /// | |
| /// Defaults to 30 minutes. | |
| pub close_idle_time: Duration, |
src/options.rs
Outdated
|
|
||
| /// The encryption type to be used. | ||
| /// | ||
| /// Defaults to '4' i.e. ECIES-X25519 |
There was a problem hiding this comment.
| /// Defaults to '4' i.e. ECIES-X25519 | |
| /// Defaults to `4`, i.e., ECIES-X25519. |
src/options.rs
Outdated
|
|
||
| /// Reduce tunnel quantity when idle | ||
| /// | ||
| /// Defaults to 'false' |
There was a problem hiding this comment.
| /// Defaults to 'false' | |
| /// Defaults to `false`. |
src/options.rs
Outdated
| /// (ms) Idle time required before reducing tunnel quantity | ||
| /// | ||
| /// Defaults to '1200000' ms (i.e. 20 minutes) | ||
| pub reduce_idle_time: Duration, |
There was a problem hiding this comment.
| /// (ms) Idle time required before reducing tunnel quantity | |
| /// | |
| /// Defaults to '1200000' ms (i.e. 20 minutes) | |
| pub reduce_idle_time: Duration, | |
| /// Idle time required before reducing tunnel quantity | |
| /// | |
| /// Defaults to 20 minutes. | |
| pub reduce_idle_time: Duration, |
|
Fixed the nits, noticed I had missed a lease set option, and then tried to reorder the options in a somewhat logical order. |
|
Should I define outbound/inbound_len as std::num::NonZeroUsize for optimization (emissary does not support 0-hop tunnels) ? |
yosemite can be used with java/i2pd which support zero-hop tunnels and emissary should eventually support them as well so no need |
An humble proposal to resolve issue #10, adding as a bonus (most of the) i2cp options alongside the datagram ones, in case the former become needed in the future.