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

Conversation

TheBlueMatt
Copy link
Collaborator

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
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
Copy link
Collaborator Author

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
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 cfromknecht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@fluxdeveloper
Copy link

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
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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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.

@TheBlueMatt
Copy link
Collaborator Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants