Skip to content

Fix bug in relay selector obfuscation resolver and extend tests#10043

Open
Serock3 wants to merge 3 commits intomainfrom
relay-selector-api-improvements
Open

Fix bug in relay selector obfuscation resolver and extend tests#10043
Serock3 wants to merge 3 commits intomainfrom
relay-selector-api-improvements

Conversation

@Serock3
Copy link
Contributor

@Serock3 Serock3 commented Mar 20, 2026


This change is Reviewable

@Serock3 Serock3 self-assigned this Mar 20, 2026
@Serock3 Serock3 force-pushed the relay-selector-api-improvements branch 2 times, most recently from 4034f79 to a3de656 Compare March 23, 2026 08:50
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Left some minor comments, but logically it seems sound

@MarkusPettersson98 reviewed 2 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on MarkusPettersson98 and Serock3).


mullvad-relay-selector/tests/relay_selector.rs line 1802 at r1 (raw file):

    }

    // TODO: Fix this test, use the relay list builder?

TODO

Code quote:

// TODO: Fix this test, use the relay list builder?

mullvad-relay-selector/src/relay_selector/mod.rs line 1141 at r1 (raw file):

        Other,
        None,
    }

Could you repurpose some of the previous comment to explain what these variants mean? The Ok and None cases are fairly self-explanatory, but what is signaled with Other?:)

Code quote:

    enum IpVersionMatch {
        Ok,
        Other,
        None,
    }

@Serock3 Serock3 force-pushed the relay-selector-api-improvements branch 2 times, most recently from bdd1671 to 5344f05 Compare March 23, 2026 09:30
Copy link
Contributor Author

@Serock3 Serock3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Serock3 made 2 comments and resolved 1 discussion.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on MarkusPettersson98).


mullvad-relay-selector/src/relay_selector/mod.rs line 1141 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Could you repurpose some of the previous comment to explain what these variants mean? The Ok and None cases are fairly self-explanatory, but what is signaled with Other?:)

Done


mullvad-relay-selector/tests/relay_selector.rs line 1802 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

TODO

Nice catch, fixed!

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarkusPettersson98 reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved.


mullvad-relay-selector/src/relay_selector/mod.rs line 1124 at r3 (raw file):

    /// with the given port or IP version.
    Reject(Reason),
}

🤩

Code quote:

/// Verdict for connecting using an obfuscation method.
enum ObfuscationVerdict {
    /// Connect to the relay's "normal" WireGuard IP address.
    AcceptWireguardEndpoint,
    /// Connect to the relay using an IP address dedicated to
    /// this obfuscation method.
    AcceptObfuscationEndpoint,
    /// The requested obfuscation cannot be resolved on the relay
    /// with the given port or IP version.
    Reject(Reason),
}

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

@MarkusPettersson98 reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants