Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow More than one Address of a given type #751

Merged
merged 1 commit into from Mar 20, 2020
Merged

Conversation

@TheBlueMatt
Copy link
Collaborator

TheBlueMatt commented Feb 28, 2020

Its not uncommon to be multi-homed with different addresses, so we should probably allow nodes to do this. Also, it seems like this is pretty much universally not actually enforced on the network.

Its not uncommon to be multi-homed with different addresses, so we should probably allow nodes to do this. Also, it seems like this is pretty much universally not actually enforced on the network.
@t-bast

This comment has been minimized.

Copy link
Collaborator

t-bast commented Mar 2, 2020

At first glance it does make sense to allow multiple addresses of the same type.
But I'm wondering what the reasoning was to explicitly disallow it?

@TheBlueMatt

This comment has been minimized.

Copy link
Collaborator Author

TheBlueMatt commented Mar 5, 2020

I presume to ensure that space isn't wasted. But its not even about "allowing multiple" its about "bring the spec up to date with whats in the wild". AFAICT, lnd has been ignoring this part of the spec for a few years and, thus, no one actually seems to care about this requirement.

@t-bast

This comment has been minimized.

Copy link
Collaborator

t-bast commented Mar 9, 2020

We also allow multiple addresses of the same type in eclair, because why not?
ACK 86c2ebc

Copy link
Collaborator

cfromknecht left a comment

LGTM, LND also allows more than one addr per type.

@fluxdeveloper

This comment has been minimized.

Copy link

fluxdeveloper commented Mar 10, 2020

If allowing multipe addresses of a given type, should it not be required that the addresses are ordered by their value in the message to ensure that the message can be reliably reconstructed for signature verification?

@t-bast

This comment has been minimized.

Copy link
Collaborator

t-bast commented Mar 10, 2020

should it not be required that the addresses are ordered by their value

If think the following requirement already covers that, doesn't it?

MUST place address descriptors in ascending order.

Actually you're totally right, this is needed if we want other users to reconstruct the list order. Otherwise you could brute-force the order until you get a match, since there shouldn't be too many addresses, but it's better if we provide an ordering that everyone follows.

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

cfromknecht commented Mar 11, 2020

If allowing multipe addresses of a given type, should it not be required that the addresses are ordered by their value in the message to ensure that the message can be reliably reconstructed for signature verification?

I’m not sure imposing an order is required, they can just be stored in the order that we received them in. If we were to impose an ordering it’d have to be backwards compatible with existing implementations, which I suspect behave slightly different. Perhaps it does make sense tho to say that addresses of the same type should be ordered by preference?

@TheBlueMatt

This comment has been minimized.

Copy link
Collaborator Author

TheBlueMatt commented Mar 11, 2020

The current spec requires an order, so removing it or ordering by preference would be non-backwards-compatible. I don't see much reason to not leave it as-is. No harm in the connector deciding which they wish to connect over instead of connectee.

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

cfromknecht commented Mar 12, 2020

The current spec requires an order, so removing it or ordering by preference would be non-backwards-compatible.

I was suggesting an intra-type ordering, which is backwards compatible with the spec because a single address is inherently sorted/ordered.

That being said, it’s definitely not critical and I’m totally fine leaving as is.

@TheBlueMatt

This comment has been minimized.

Copy link
Collaborator Author

TheBlueMatt commented Mar 12, 2020

Ah, right. I suppose if there is no order specified it probably makes sense to just call it "announcer's preference". But thats separate.

@rustyrussell

This comment has been minimized.

Copy link
Collaborator

rustyrussell commented Mar 13, 2020

It was actually introduced at @Roasbeef 's request, but apparently that was a misunderstanding. It's certainly valid to have multiple addresses, and as stated, most implementations allow it (c-lightning refuses to produce them, but will accept them).

Ack from me.

@t-bast
t-bast approved these changes Mar 13, 2020
@TheBlueMatt

This comment has been minimized.

Copy link
Collaborator Author

TheBlueMatt commented Mar 20, 2020

Gonna merge given the ACKs. Further changes (@cfromknecht did you want more specificity here?) I'm happy to do as a follow-up PR.

@TheBlueMatt TheBlueMatt merged commit 4107c69 into master Mar 20, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.