refactor(iroh-net): Make derp_map not an option in MagicEndpoint

[flub]

Description

This changes the public API to set the derp map to enable_derp(derp_map)
and disable_derp(). As part of this the DerpMap itself is changed so it
can only be constructed as a valid DerpMap and consistency checks are made.
The public endpoints also ensure the derp map is not empty.

Internally this removes the Option from MagicEndpoint::derp_map. An empty
DerpMap is treated the same as a missing DerpMap in all the code so
this is just a noisy indirection.

Notes & open questions

Change checklist

• Self-review.
• Documentation updates if relevant.
• Tests if relevant.

[Arqu]

Allowing None gives us the ability to force non-derp connections only and to talk directly through the provided addresses in tickets.

[flub]

Allowing None gives us the ability to force non-derp connections only and to talk directly through the provided addresses in tickets.

You can still do this by using an empty DerpMap. Though there should probably be a DerpMap::empty() constructor to make this easy and obvious.

[flub]

Allowing None gives us the ability to force non-derp connections only and to talk directly through the provided addresses in tickets.

You can still do this by using an empty DerpMap. Though there should probably be a DerpMap::empty() constructor to make this easy and obvious.

oh wait, the public API still has it as an option, but with a TODO to consider this.

So yes the question is both about the internal and external api. I think internal it makes sense. External maybe too and I could like a DerpMap::emtpy() for this.

[dignifiedquire]

I would like to see sth like this

enum Derp {
  Enabled(DerpMap), // Constructor checks that this is not empty and the node checks that at least the local region id is available
  Disabled,
}

[flub]

I would like to see sth like this

enum Derp {
  Enabled(DerpMap), // Constructor checks that this is not empty and the node checks that at least the local region id is available
  Disabled,
}

So the only really public API is the MagicEndpointBuilder::derp_map(mut self, derp_map: Option<DerpMap) call. Are you saying this for there or for internal as well?

This PR started as just acknowledging that internally the code does not ever distinguish a None DerpMap from an empty one, it does eventually end up as an empty one. So it just peeled back some layers and simplified this. As this is internal I'm largely fine with that.

I think switching internally to this Disabled enum will be a lot more work. Maybe the code will end up being clearer, maybe not. Hard to say ahead of time.

[dignifiedquire]

for the public api I would do sth like disable_derp() and enable_derp(map) which forces map to not be empty

[dignifiedquire]

you are right internal is annoying and we should just drop the optiob there for now

[flub]

I've now updated the public API to be MagicEndpointBuilder::enable_derp(derp_map) and .disable_derp(). This touches a surprising number of places.

I've moved validation of the DerpMap into the constructors itself, the constructor is basically DerpMap::from_regions which is now fallible. This again touches a lot of places but ensures that if the type exists we have a valid one, which was nicer than having a separate validation function.

(I tried having this constructor in TryFrom<T: Into<Iterarator<Item = DerpRegion>>> since the old one was in such a From, but I lost that fight with the compiler)

[dignifiedquire]

can you update the Node::builder api as well, to match this new public api?

[flub]

can you update the Node::builder api as well, to match this new public api?

oh yes, good call. sorry to gloss over that at first.

Note that I also changed the default DerpMap used in iroh::Node::builder: this was also empty originally and now uses defaults::default_derp_map.