Drop address descriptor ordering/count limits

[TheBlueMatt]

These aren't enforced in practice anyway so there's little value in
keeping them in the spec.

[TheBlueMatt]

Relatedly, should we have some suggested limit where we shouldn't relay a node_announcement if it has more than, say, 10 addresses?

[t-bast]

ACK

[rustyrussell]

Ordering is a requirement for parsing, if we ever add a new addr format. Existing implementations may be slack (since we've never added one), but we should fix them.

Agree we can allow dups though.

[TheBlueMatt]

Ordering is a requirement for parsing

Indeed, but only for types that you don't know. There was a long discussion about this issue in the meeting, but at least from our end, I don't think we're going to build a whole read/write API just to enforce ordering on sending if we cannot enforce it on receiving (which we cannot, currently, do). I'm not sure that its reasonable to fix this by telling people to upgrade, instead any newly-added address formats can be required to be after the current 4, which don't need an order.

[TheBlueMatt]

I updated the Rationale section to mention that future types need to be ordered in ascending order to allow for deserialization, but due to compatibility we can't enforce that on the current 4.

[t-bast]

ACK, I still think it's worth fixing implementations anyway, I'll ensure eclair writes these in order.

[rustyrussell]

Just leave the requirement there, please. It still applies, and is still the Right thing to do.

You can't put a requirement in a subnote like this. That makes things worse, not better, for future implementers. If you want, note that some implementations did not obey this ordering for the initially-defined types 1-4.

There's no requirement in the spec that readers enforce this (though it's always good to detect such sender fails and handle them gracefully).

[rustyrussell]

Huh, weird.

c-lightning removed the "only allow one addr per type" requirement back on 0.8.2 (a year ago) as ElementsProject/lightning@a430abf.

That commit msg refers to spec update 86c2ebc. Where did that change go?

[TheBlueMatt]

Hmm, we could change it to explicitly say that addresses with type > 4 should be ordered. Alternatively, we could add a requirement on the receiver end that you have to handle such messages. Either way, I’d really appreciate some clarity in one way or another - I don’t really accept that the state of things today (where users would expect to route through nodes that do this but the spec silently assumes no such nodes exist) is fine. Spec was changed to allow multiple addresses per type (but with no change to ordering) in #751. There was some discussion there about using ordering to specify the node’s preference between addresses.

…

On Feb 21, 2021, at 19:47, Rusty Russell ***@***.***> wrote:  Huh, weird. c-lightning removed the "only allow one addr per type" requirement back on 0.8.2 (a year ago) as ***@***.*** That commit msg refers to spec update 86c2ebc. Where did that change go? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[rustyrussell]

@TheBlueMatt The spec already says exactly this, though:

The origin node:
...
  - MUST place address descriptors in ascending order.

And the receiving side:

The receiving node:
...
  - SHOULD ignore the first `address descriptor` that does NOT match the types
  defined above.

(To be fair, it should explicitly say to stop parsing addresses if this happens, rather than implying it!).

But it does not say that the receiver should do any additional validation on addresses.

There are, indeed, some places where you need to read the writer requirements to intuit the reader requirements, but those are all spec flaws. They should be implementable separately, and definitely defined separately, since we use the tighter restrictions on writers than readers to let us enhance things in future.

Tests can definitely be stricter, but not live code on the network!

[TheBlueMatt]

Hmm, maybe I'm just still uncomfortable with the "undefined space" in between sending and receiving in so many places in the spec. This certainly isn't the venue to debate that, though, so I'll close it, noting only that, for API simplicity's sake, we'll definitely be taking advantage of the "undefined space" here and doing things we MUST NOT do, while still being guaranteed by the spec that we'll be able to talk to anyone...which just feels gross.