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

BOLT 2&7: Cleaner separation of concerns wrt announcement signatures #97

Merged
merged 5 commits into from
Feb 7, 2017

Conversation

cdecker
Copy link
Collaborator

@cdecker cdecker commented Jan 31, 2017

So far we did not have any indication on what to do if a node does not
allow announcing the channel and we had a mix of concerns in the
funding_locked message, which would also transfer the signatures
needed for the announcement. This is a proposal about splitting the
signatures into their own message, so that simple omission is an
opt-out of announcements, and it does not mix announcement/gossip
stuff into the peer-protocol.

This builds on top of #96. There is some concern that pulling this
message out of the strictly controlled state machine of the channel
lifecycle may have unwanted consequences.

So far we did not have any indication on what to do if a node does not
allow announcing the channel and we had a mix of concerns in the
`funding_locked` message, which would also transfer the signatures
needed for the announcement. This is a proposal about splitting the
signatures into their own message, so that simple omission is an
opt-out of announcements, and it does not mix announcement/gossip
stuff into the peer-protocol.
Copy link
Collaborator

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

This is good, but incomplete:

  • You need to move the following from 02 to 07 (removing the part that lets the sigs be zeros):

    The sender MUST set announcement-node-signature and announcement-bitcoin-signature to the signatures for the channel_announcement message, or all zeroes if it does not want the
    channel announced.

    The recipient SHOULD fail the channel if the announcement-node-signature and announcement-bitcoin-signatures are incorrect (and not all zeroes). The recipient SHOULD queue the channel_announcement message for its peers if it has sent and received a non-zero announcement-node-signature and announcement-bitcoin-signature.

  • You need to specify that it MUST follow funding-locked (specifically, after you have BOTH received and sent funding-locked successfully).

  • Please also edit bolt 02 "Once both nodes have exchanged funding_locked" to add "(and optionally announcement_signatures)".

  • Finally, "Message Retransmission" needs updating: the line "funding_locked: acknowledged by" should be duplicated to "announcement_signatures: acknowledged by" and announcement_signatures added to the list of things which acknowledge funding_locked.

@cdecker
Copy link
Collaborator Author

cdecker commented Feb 1, 2017

I definitely need new reading glasses, sorry for missing those :-)

I amended your first two points, but I am a bit wary of mixing the channel lifecycle messages with the routing messages. The exchange of the routing signatures is not time-critical, so we could just move the signature exchange at a later point in the execution, outside of the strict state machine. Retransmission is also rather simple: if we haven't seen our channel announced we can just send the signatures again.

@rustyrussell
Copy link
Collaborator

rustyrussell commented Feb 2, 2017 via email

@cdecker
Copy link
Collaborator Author

cdecker commented Feb 2, 2017

I like the "just send signature on open/reopen" much better, it's an event we are hooking into anyway to do the initial announcement.

@pm47
Copy link
Collaborator

pm47 commented Feb 2, 2017

I am a bit wary of mixing the channel lifecycle messages with the routing messages

I share Christian's concerns, but I am still slightly in favor of the existing explicit opt-out mechanism because I think it makes the channel creation flow simpler and easier to understand (even if it comes at the expense of the separation of concerns).

Also I like the fact that we know from the get-go if the channel will ever be 'public' or 'private' from a user's perspective.

Copy link
Collaborator

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Excellent, I really like this change. With the mechanism we were using before, there wasn't an explicit way for a node to op-out of advertising a channel or not.

In order to resolve the ambiguity issues that Rusty brought up, we could add a bit to the open_channel and accept_channel which signals if the message carrying the announcement signatures are to be expected or not.

* [64:node-signature]
* [64:bitcoin-signature]

An `announcement_signatures` message MUST NOT be sent before the channel is fully established, i.e., before both ends have received the corresponding `channel_locked` messages.
Copy link
Collaborator

Choose a reason for hiding this comment

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

channel_locked should be funding_locked

@rustyrussell
Copy link
Collaborator

OK, I think we want to add a feature bit. That should be easy:
(1) document the feature bit, say bit #1 (odd is OK). This is a localfeature, IIUC.
(2) if both sides set feature bit, announcement_signatures MUST follow funding_locked, otherwise it MUST NOT.
(3) amend retranmission section and that sentence in bolt 02 "Once both nodes have exchanged funding_locked" to add "(and optionally announcement_signatures)".

@cdecker
Copy link
Collaborator Author

cdecker commented Feb 6, 2017

Ok, amended the PR to include the opt-in localfeature, added a tracking document for the assigned feature flags and added the restriction to send it inbetween the funding_locked and any state update.

One minor concern is that the featureflag now impacts all channels on this connection (open_channel and accept_channel currently don't have a features field), but that's likely not a problem. Still think we can do without the strict ordering requirements, but that's also a minor thing.

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.

None yet

4 participants