feat(relay)!: relay only mode now configurable #3056

Arqu · 2024-12-17T13:59:27Z

Description

This will allow us to configure relay only mode at runtime. ~~Useful for easy testing without having to rebuild with the DEV_RELAY_ONLY environment variable set.~~

The DEV_RELAY_ONLY compile time env var has been completely dropped and a relay_only option has been threaded through the entire stack when test-utils is enabled. An example of it can be followed with the iroh/examples/transfer.rs where you can set up a provide and fetch node with --relay-only.

Breaking Changes

Notes & open questions

Change checklist

Self-review.
Documentation updates following the style guide, if relevant.
Tests if relevant.
All breaking changes documented.

github-actions · 2024-12-17T14:02:17Z

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3056/docs/iroh/

Last updated: 2025-01-03T20:07:54Z

dignifiedquire · 2024-12-17T15:51:37Z

please also remove the old env variable, to simplify things

flub

I think it is a good move that this is still cfg(any(test, feature = "test-utils")). I think this generally looks fine?

matheus23 · 2024-12-18T08:41:47Z

iroh/bench/src/lib.rs

    /// to test throughput for relay-only traffic locally.
-    /// (e.g. `DEV_RELAY_ONLY=true cargo run --release -- iroh --with-relay`)
    #[clap(long, default_value_t = false)]
    pub with_relay: bool,


Thanks for adjusting the docs here, but these docs are actually the CLI help text.
So saying "hey you can change the endpoint config" doesn't really help when you're reading this as CLI text 😬

I'd appreciate if there'd be two possible solutions:

Add another pub only_relay: bool option, so you can run cargo run --release -- iroh --only-relay (and only-relay should imply with-relay) or

Rename with_relay to only_relay and make that also set .relay_only(true) on the endpoint.

~~Went with option 2 but am not happy about it as it depends on the test/test-utils feature to be active. I'll revert it to option 1.~~
Nvm, works just fine because local-relay pulls in the iroh/test-utils feature.

Nice!
Only other thing that might be nice is documenting that you need the local-relay feature to be active (and telling the user how to run this thing, so cargo run --release --features=local-relay -- iroh --only-relay).

github-actions · 2024-12-19T11:35:29Z

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: bd58539

dignifiedquire · 2024-12-19T12:47:43Z

iroh/src/endpoint.rs

@@ -417,6 +421,14 @@ impl Builder {
        self.insecure_skip_relay_cert_verify = skip_verify;
        self
    }
+
+    /// "relay_only" mode implies we only use the relay to communicate


nit: something like "Setting this, implies ... " reads bettert than repeating the function name

Arqu · 2024-12-19T13:29:50Z

FWIW this PR is nowhere ready to be merged, the pieces checking and utilizing the relay_only flag are disconnected from the endpoint and never get passed in. This was circumvented by just dropping in that env var, but had some random loose bits. I managed to clean up the confusing bits, but need to thread the needle a bit more.

rklaehn · 2025-01-02T12:10:49Z

iroh/src/endpoint.rs

+    /// This implies we only use the relay to communicate
+    /// and do not attempt to do any hole punching.
+    #[cfg(any(test, feature = "test-utils"))]
+    pub fn relay_only(mut self, relay_only: bool) -> Self {


It is not the end of the world since this is a debug feature, but I would probably favour an enum to configure which path to use instead of a bool. E.g. we might in the future want to have more complex logic to select which path to take (IPV4/IPV6/Relay)

I guess that will make more sense to implement once we start dealing with multipath as we currently don't really have an idea of everything we will need to pass in.

Make it an enum with 2 cases, then it is still easier to extend than a bool

rklaehn · 2025-01-03T08:36:55Z

Any particular reason we need the feature flag dance?

#[cfg(any(test, feature = "test-utils"))]

I think in none of the places where this is used a flag check is going to be a performance problem. So let's make the path use (for now just RelayOnly and All) an enum, set it to All by default, and have this enabled even without feature flags.

Arqu · 2025-01-03T09:54:14Z

Any particular reason we need the feature flag dance?
#[cfg(any(test, feature = "test-utils"))]
I think in none of the places where this is used a flag check is going to be a performance problem. So let's make the path use (for now just RelayOnly and All) an enum, set it to All by default, and have this enabled even without feature flags.

We generally wanted to avoid exposing this fluff to end users without specific reason as you generally should never want to do relay only. Im fine with doing that, simplifies the code a nice bit. @dignifiedquire you wanted to have it feature flagged initially, anything against it?

rklaehn · 2025-01-03T10:37:09Z

We generally wanted to avoid exposing this fluff to end users without specific reason as you generally should never want to do relay only. Im fine with doing that, simplifies the code a nice bit. @dignifiedquire you wanted to have it feature flagged initially, anything against it?

I can see why we don't want users to mess with this, but having lots of code gated by feature flags is not nice.

So either mark the builder fn to set this with docs(hidden), or have just the public fn to set this feature gated?

dignifiedquire · 2025-01-03T12:20:13Z

or have just the public fn to set this feature gated?

that usually leads to a bunch of dead code warnings

Arqu · 2025-01-03T14:40:16Z

This is now at a middle ground using and enum for path selection.
What's the verdict at the end, to feature flag or not?

matheus23

Needs some CI massage.

Keeping this behind the test_utils feature flag sets expectations. IMO important ones. Generally we want people to avoid enabling relay-only mode.
If someone really wants relay-only mode, it's just a feature flag away. It's not dangerous to enable test_utils. I'll totally steal this for e.g. iroh-doctor for testing stuff on mobile.

iroh/src/endpoint.rs

dignifiedquire · 2025-01-03T14:59:39Z

iroh/src/magicsock/node_map.rs

    /// Create a new [`NodeMap`] from a list of [`NodeAddr`]s.
    pub(super) fn load_from_vec(nodes: Vec<NodeAddr>) -> Self {
        Self::from_inner(NodeMapInner::load_from_vec(nodes))
    }

+    #[cfg(any(test, feature = "test-utils"))]
+    /// Create a new [`NodeMap`] from a list of [`NodeAddr`]s.
+    pub(super) fn load_from_vec(nodes: Vec<NodeAddr>, path_selection: PathSelection) -> Self {


I would prefer to make this a seperate method, like set_path_selection, instead of changing the signature

Issue is, the inner requires the flag and it needs to be provided at load time. Should I keep it or move forward and unpack the whole thing?

Arqu self-assigned this Dec 17, 2024

flub reviewed Dec 17, 2024

View reviewed changes

matheus23 reviewed Dec 18, 2024

View reviewed changes

matheus23 approved these changes Dec 18, 2024

View reviewed changes

dignifiedquire reviewed Dec 19, 2024

View reviewed changes

Arqu marked this pull request as draft December 19, 2024 13:30

Arqu marked this pull request as ready for review December 20, 2024 14:25

Arqu requested review from dignifiedquire and matheus23 December 20, 2024 14:25

rklaehn reviewed Jan 2, 2025

View reviewed changes

Arqu changed the title ~~feat: relay only mode now configurable~~ feat(relay)!: relay only mode now configurable Jan 3, 2025

matheus23 approved these changes Jan 3, 2025

View reviewed changes

dignifiedquire reviewed Jan 3, 2025

View reviewed changes

iroh/src/endpoint.rs Show resolved Hide resolved

dignifiedquire reviewed Jan 3, 2025

View reviewed changes

Arqu added 7 commits January 3, 2025 21:03

feat: relay only mode now configurable

b73adb7

cleanup

65a6cbd

adjust bench usage

f96f4e3

cleanup

7f30474

drop unused code

e14bdef

make this make sense

b70f369

connect the dots

1d9fff1

Arqu added 5 commits January 3, 2025 21:05

rework to enum

615e4c2

cleanup comments

304ab9a

clippy mad

7f592d2

feature flag path selection

d0358ad

rebase cleanup

9fd242d

Arqu force-pushed the arqu/relay_only branch from 958f06d to 9fd242d Compare January 3, 2025 20:06

Arqu enabled auto-merge January 3, 2025 20:06

Arqu added this pull request to the merge queue Jan 3, 2025

Merged via the queue into main with commit 5aba17e Jan 3, 2025
25 of 26 checks passed

Arqu deleted the arqu/relay_only branch January 4, 2025 21:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(relay)!: relay only mode now configurable #3056

feat(relay)!: relay only mode now configurable #3056

Arqu commented Dec 17, 2024 •

edited

Loading

github-actions bot commented Dec 17, 2024 •

edited

Loading

dignifiedquire commented Dec 17, 2024

flub left a comment

matheus23 Dec 18, 2024

Arqu Dec 18, 2024 •

edited

Loading

matheus23 Dec 18, 2024

github-actions bot commented Dec 19, 2024 •

edited

Loading

dignifiedquire Dec 19, 2024

Arqu commented Dec 19, 2024

rklaehn Jan 2, 2025

Arqu Jan 2, 2025

rklaehn Jan 3, 2025

rklaehn commented Jan 3, 2025

Arqu commented Jan 3, 2025

rklaehn commented Jan 3, 2025

dignifiedquire commented Jan 3, 2025

Arqu commented Jan 3, 2025

matheus23 left a comment

dignifiedquire Jan 3, 2025

Arqu Jan 3, 2025

feat(relay)!: relay only mode now configurable #3056

feat(relay)!: relay only mode now configurable #3056

Conversation

Arqu commented Dec 17, 2024 • edited Loading

Description

Breaking Changes

Notes & open questions

Change checklist

github-actions bot commented Dec 17, 2024 • edited Loading

dignifiedquire commented Dec 17, 2024

flub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arqu Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Arqu commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rklaehn commented Jan 3, 2025

Arqu commented Jan 3, 2025

rklaehn commented Jan 3, 2025

dignifiedquire commented Jan 3, 2025

Arqu commented Jan 3, 2025

matheus23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arqu commented Dec 17, 2024 •

edited

Loading

github-actions bot commented Dec 17, 2024 •

edited

Loading

Arqu Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024 •

edited

Loading