feat: Swarm.RelayService (circuit v2)

[marten-seemann]

• Need this for WIP: update go-libp2p to v0.16.0 kubo#8522
• circuit v2: https://github.com/libp2p/specs/blob/master/relay/circuit-v2.md

@vyzo How should we expose configuration options for the limited relay? Should we have a RelayConfig that mirrors https://github.com/libp2p/go-libp2p/blob/4f129401a3e4828eaec6818e5a2a1a8f9db1bd8c/p2p/protocol/circuitv2/relay/resources.go#L7-L41?

[vyzo]

Yeah, let's have the struct wholesale. Can we use the actual type directly?

[marten-seemann]

Yeah, let's have the struct wholesale. Can we use the actual type directly?

Two problems:

• we need to embed the RelayLimit in the Resources struct. That's easy.
• we need to find a solution for the time.Duration, otherwise users will have to enter durations in nanoseconds

Do we have any good solution for dealing with JSON marshaling of time.Durations?

[vyzo]

perhaps we can define a json marshaller for the limit struct? that would be useful elsewhere too, e.g the relay daemon.

…

On Sun, Oct 17, 2021, 18:04 Marten Seemann ***@***.***> wrote: Yeah, let's have the struct wholesale. Can we use the actual type directly? Two problems: - we need to embed the RelayLimit in the Resources struct. That's easy. - we need to find a solution for the time.Duration, otherwise users will have to enter durations in nanoseconds Do we have any good solution for dealing with JSON marshaling of time.Durations? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#146 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAI4SSHT7SYKL26FLW4RU3UHLQXVANCNFSM5GEY66YQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[marten-seemann]

perhaps we can define a json marshaller for the limit struct?

The RelayLimit is not a problem. We can just embed it in the struct, and we'll be fine (we lose the ability to compare to nil, but we can just add a IsZero method).
The problem here is the time.Duration. If we want a custom marshaller for that, we have define our own Duration type, something like in https://stackoverflow.com/questions/48050945/how-to-unmarshal-json-into-durations.

[marten-seemann]

The problem here is the time.Duration. If we want a custom marshaller for that, we have define our own Duration type, something like in https://stackoverflow.com/questions/48050945/how-to-unmarshal-json-into-durations.

Actually, we don't, we could do something like this: https://penkovski.com/post/go-unmarshal-custom-types/. Still wondering if it wouldn't be easier to just define a new config struct here...

[vyzo]

dunno, we would have to keep it up to date if we make changes and its not reusable by the relay daemon -- up to you nonetheless, i dont feel strongly about it.

…

On Sun, Oct 17, 2021, 20:42 Marten Seemann ***@***.***> wrote: The problem here is the time.Duration. If we want a custom marshaller for that, we have define our own Duration type, something like in https://stackoverflow.com/questions/48050945/how-to-unmarshal-json-into-durations . Actually, we don't, we could do something like this: https://penkovski.com/post/go-unmarshal-custom-types/. Still wondering if it wouldn't be easier to just define a new config struct here... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#146 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAI4SRT4DJNMUSSMNCDAELUHMDHVANCNFSM5GEY66YQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[aschmahmann]

we need to find a solution for the time.Duration, otherwise users will have to enter durations in nanoseconds

This seems to already be done in

go-ipfs-config/types.go

Line 217 in a412831

type Duration time.Duration

and used by

go-ipfs-config/autonat.go

Line 80 in a412831

Interval Duration `json:",omitempty"`

We also have some older examples like

go-ipfs-config/ipns.go

Lines 4 to 5 in 6b35ac3

	RepublishPeriod string
	RecordLifetime string

that are parsed manually in go-ipfs and could probably be switched over.

Side note: When this PR is ready for review/merging please tag me or one of the other go-ipfs stewards.

[marten-seemann]

we need to find a solution for the time.Duration, otherwise users will have to enter durations in nanoseconds

This seems to already be done in

go-ipfs-config/types.go

Line 217 in a412831

type Duration time.Duration

and used by

go-ipfs-config/autonat.go

Line 80 in a412831

Interval Duration `json:",omitempty"`

That's awesome! That's exactly what I was looking for!

[marten-seemann]

How do we usually deal with default values. Currently, this is what ends up in the JSON:

{
  "AddrFilters": null,
  "DisableBandwidthMetrics": false,
  "DisableNatPortMap": false,
  "DisableRelayService": false,
  "RelayServiceOpts": {
    "Limit": {
      "Duration": "0s",
      "Data": 0
    },
    "ReservationTTL": "0s",
    "MaxReservations": 0,
    "MaxCircuits": 0,
    "BufferSize": 0,
    "MaxReservationsPerPeer": 0,
    "MaxReservationsPerIP": 0,
    "MaxReservationsPerASN": 0
  },
  "EnableAutoRelay": false,
  "Transports": {
    "Network": {},
    "Security": {},
    "Multiplexers": {}
  },
  "ConnMgr": {
    "Type": "basic",
    "LowWater": 600,
    "HighWater": 900,
    "GracePeriod": "20s"
  }
}

Is it ok to have all 0s there and use our default values (as defined in the circuit v2 implementation) if the user doesn't configure anything other?

[aschmahmann]

How do we usually deal with default values

Usually is a strong word here 😄. There have been multiple approaches taken in the past and IMO it'd be nice to move towards a config file that's basically empty unless the user has made explicit modifications since it'll let us change the defaults within the client and the user will benefit from the changes without needing to mess with their config files or force us to make awkward decisions in a repo migration (e.g. did they set the value to X because they wanted to or because it was the default, how might they feel about us changing it to Y?).

Is it ok to have all 0s there and use our default values (as defined in the circuit v2 implementation) if the user doesn't configure anything other?

It can be ok, and we've done it in the past as long as we document the default values. For example, https://github.com/ipfs/go-ipfs/blob/master/docs/config.md#autonatthrottleinterval.

However, a better approach is probably to use some of our types here like https://github.com/ipfs/go-ipfs/blob/master/docs/config.md#optionalinteger so we can use JSON's sentinel values rather than defining them ourselves (e.g. "zero seems like a safe sentinel here, but not there" kinds of problems). If this doesn't cover a value then we can create or modifying one of the existing types.

We can also use "omit empty" on some of the structs if we think they're too gross/bothersome to expose to the user although it's use is situational since it's still at least a little nice to have an exposure point to users about a config option that isn't only in the doc.

[vyzo]

LGTM. We should probably note the defaults in the documentation.

[aschmahmann]

Before merging this could we get a PR to go-ipfs that integrates the options and the modifies https://github.com/ipfs/go-ipfs/blob/master/docs/config.md?

Really these repos should be combined, so it's basically part of the same PR here.

[marten-seemann]

Before merging this could we get a PR to go-ipfs that integrates the options and the modifies https://github.com/ipfs/go-ipfs/blob/master/docs/config.md?

Here's the PR that updates the docs, and uses the values read from the config to configure libp2p: ipfs/kubo#8522

[lidel]

Applied the changes from the review so far (ipfs/kubo@8821d8b) and tested how it behaves with ipfs init and ipfs config in ipfs/kubo#8522

Remaining work here (I'll continue poking tomorrow):

• refactor + update relevant config docs in WIP: update go-libp2p to v0.16.0 kubo#8522
• clarify the function of EnableAutoRelay – WIP: update go-libp2p to v0.16.0 kubo#8522 (comment) and update comment here
• switch to improved Duration type from feat: add an OptionalDuration type #148

[marten-seemann]

Rebased.

Note that we still need to change the OptionalInteger to deal with pointers, and then change all occurrences of OptionalInteger in the RelayService struct to pointers. I'll leave this to a later PR.

[Stebalien]

This seems redundant. I'd define the type as type OptionalInteger *int64.

[lidel]

Could be my lack of go skills, but I was unable to make that work with pointer type and have WithDefault that pass this omitempty unmarshal test. Go does not seem to allow methods on pointer types, I was getting invalid receiver type errors.

The nil check here feels safer than introducing some creative hackery with unsafe.Pointer.
Lmk if there is a simple way I don't see (otherwise will merge as-is, and we can refine this in the future).

[lidel]

This LGTM.

• Rebased this PR so it includes recent improvements around Optional.. fields
• Switched WIP: update go-libp2p to v0.16.0 kubo#8522 to the latest revision here, so Libp2pForceReachability can be wired up for use in tests.

@marten-seemann when you are back, lmk if you plan to change anything config-wise here. If not, I'd like to squash and merge this and prep for the new go-ipfs-config release.

[marten-seemann]

I'm going to merge this PR. We need more options for configuring static relays and to enable hole punching, but that's independent of (although dependent on) this change.