Add `Intersection` trait and implement it on `RelayConstraints` #5835

MarkusPettersson98 · 2024-02-20T11:00:34Z

This PR is one in a series of PRs which aims to make working with the relay selector easier.

The goal of this PR is to be able to in a declarative way express a set of RelayConstraints and define a semantic for how to merge different RelayConstraints which are compatible. This is useful for when the app wants to use a set of default RelayConstraints to try on successive reconnection attempts while always respecting user preferences.

This change is

mullvad-types/Cargo.toml

mullvad-types/src/constraints/set.rs

mullvad-types/src/constraints/constraint.rs

mullvad-types/src/relay_constraints.rs

dlon

Reviewed 18 of 24 files at r1, 9 of 9 files at r3, 5 of 5 files at r4, 2 of 3 files at r5, 6 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @MarkusPettersson98 and @Serock3)

MarkusPettersson98

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Serock3)

mullvad-types/Cargo.toml line 31 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Sure 😊

Done

mullvad-types/src/relay_constraints.rs line 685 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

        // otherwise      = None

Done.

mullvad-types/src/relay_constraints.rs line 1018 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

    /// Internal builder state for a [`RelayConstraint`] parametrized over the

Done.

mullvad-types/src/relay_constraints.rs line 1022 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

    /// - The type parameter `VpnProtocol` keeps track of which VPN protocol that

Done.

mullvad-types/src/relay_constraints.rs line 1036 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

    /// Create a new [`RelayConstraintBuilder`] with unopinionated defaults.

Done.

mullvad-types/src/relay_constraints.rs line 1094 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

        /// otherwise unopinionated defaults.

Done.

mullvad-types/src/relay_constraints.rs line 1158 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

        /// otherwise unopinionated defaults.

Done.

mullvad-types/src/constraints/constraint.rs line 95 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

    /// Define the intersection between two arbitrary [`Constraint`]s. This

Done.

dlon

Reviewed 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Serock3)

test/test-manager/src/tests/tunnel.rs line 301 at r7 (raw file):

) -> Result<(), Error> {
    let relay_constraints = relay_constraints::builder::wireguard::new()
        .multihop()

Clean.

MarkusPettersson98

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Serock3)

test/test-manager/src/tests/tunnel.rs line 301 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

Clean.

:D

dlon

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Serock3)

mullvad-types/src/relay_constraints.rs

Serock3 · 2024-02-20T14:11:41Z

mullvad-types/src/relay_constraints.rs

+            fn test_identity(relay_constraints in relay_constraint()) {
+                // The identity element
+                let identity = builder::any().build();
+                prop_assert_eq!(identity.clone().intersection(relay_constraints.clone()), relay_constraints.clone().into());


This becomes a bit less readable because of the .clone()s. I don't suppose that intersection could take references instead of consuming both the arguments?

Serock3 · 2024-02-20T14:14:59Z

mullvad-types/src/constraints/constraint.rs

+            // The identity element
+            let identity = Constraint::Any;
+            prop_assert_eq!(x.intersection(identity), x.into());
+            prop_assert_eq!(identity.intersection(x), x.into());


Both of these are not technically needed since you also test commutativity

True, but I think I want to leave it in for documentation's sake 😊

mullvad-types/src/constraints/constraint.rs

mullvad-types/src/relay_constraints.rs

This commit initializes a series of commits which aims to decompose `relay_constraints.rs` into smaller modules.

Add `Intersection` trait and implement it on the `Constraint` type. Use `proptest` write a "proof" for associativity of `intersection`

Convert `WireguardConstraints::use_multihop` to `Constraint<bool>` in order to cleanly conform to the `identity` property required by `Intersection`. This is important to be able to express the merge between our retry strategy and user-defined relay constraints.

dlon

Reviewed 7 of 11 files at r8, 7 of 7 files at r9, all commit messages.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @MarkusPettersson98 and @Serock3)

dlon

Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @MarkusPettersson98 and @Serock3)

mullvad-types/src/relay_constraints.rs line 1116 at r9 (raw file):

    pub use wireguard::*;

    /// Internal builder state for a [`RelayConstraint`] parameterized over the

Is RelayConstraint (rather than RelayConstraints) a typo? I also think the whole thing should be called RelayConstraintsBuilder, but that might be verging on nitpicking.

MarkusPettersson98 · 2024-03-11T09:04:44Z

Closing this in favor of #5935

MarkusPettersson98 changed the title ~~Add Intersection trait and implement it on RelayConstraint~~ Add Intersection trait and implement it on RelayConstraints Feb 20, 2024

MarkusPettersson98 force-pushed the add-intersection-trait branch 2 times, most recently from 099d5e7 to 5724de5 Compare February 20, 2024 13:05

MarkusPettersson98 requested review from dlon and Serock3 February 20, 2024 13:05

MarkusPettersson98 marked this pull request as ready for review February 20, 2024 13:05