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 #7: More complex proposal, using three separate message types. #11

Merged
merged 2 commits into from
Nov 22, 2016

Conversation

rustyrussell
Copy link
Collaborator

Contents stolen from @cdecker 's draft, but tightened requirements and split into three messages.

I added bitcoin-to-node cross-signing, require both sigs for announcing a channel, expanded expiry from 1 to two bytes to match BOLT #2. I think the result is pretty simple, though I can see us later switching to schnorr and having a bulk-update message.

Contents stolen from Christian's draft.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@cdecker
Copy link
Collaborator

cdecker commented Nov 21, 2016

Very nice proposal, it's rather similar to what I had in mind initially, having the individual classes of objects created and updated in separate messages. The reason I moved away from that idea was that it introduces interdependence between messages, i.e., a node announcement is only valid if it is preceded by a channel announcement, and the same goes for the channel update.

Having the node ID sign the pubkey used in the anchor transaction is very nice because it addresses @Roasbeef 's usecase in which every anchor transaction may use completely separate keys. On the other hand it blows up the message size, with the delegation signatures, the pubkeys and the channel signatures. For comparison in order to fully announce a channel and the corresponding node, i.e., MSG_CHANNEL_ANNOUNCEMENT + MSG_NODE_ANNOUNCEMENT + MSG_CHANNEL_UPDATE, with no alias, this results in 616 bytes, while the scheme in #1 requires just 176 bytes, but admittedly lacks the delegation, so this comparison may not be fair :-)

It's not just that we need to transfer all of the information around, but that is also the minimal set that we need to keep in memory at each node, since we might need to bootstrap another node and provide it with the signed state we received. This is also why I dislike having padding in the messages, since we now need to either define that to be null or keep it in memory since it is signed.

As mentioned in #1 I dislike the passive closure, which forces all nodes to track all anchor transactions, in order to be able to cleanup closed channels, and it does not allow for voluntary disabling of channels. But we can simply redefine the pad byte as an option bitfield and use one bit to signal removal :-)

Another difference is that this proposal signs channels independently, which results in more signatures to send and store, but allows a per-channel sync, instead of the per-node sync in #1. This is important when looking at the extensibility of the routing system, since it is a different signed unit of information that we need to recreate when syncing and verifying. I'm not sure which one is nicer.

Copy link
Collaborator

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

I like this proposal better, the separation between channel_announcement and channel_update is very similar to a prototype we built into eclair a few weeks ago.

I also like the fact that we rely on the blockchain to find out which channels are closed instead of announcing both open and close, which was a little cumbersome in PR #1 (and it can still be added).

On the downside, I think this design is more vulnerable to DoS attacks because messages node_announcement and channel_update are costless to produce once a single valid channel has been created. Maybe we could add some rate limiting logic to mitigate this? <- strike this, the issue is already mitigated by the staggered broadcast, and a peer is free to close a connection if its immediate neighbor is too chatty


The node SHOULD accept HTLCs which pay a fee equal or greater than:

fee-base-msat + htlc-amount-msat * fee-proportional-millionths / 1000000
Copy link
Collaborator

@pm47 pm47 Nov 21, 2016

Choose a reason for hiding this comment

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

I think there needs to be a tolerance on the fee, because we need to take into account the time it takes for the information to propagate. Otherwise a node will systematically reject payments received right after it increased its relaying fee.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, but that is the node's local policy isn't it? Either he has a grace period, or he is rejecting things that'd make him some fees. So I'm not sure we need to specify this here.

Copy link
Collaborator

@pm47 pm47 Nov 21, 2016

Choose a reason for hiding this comment

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

Well, in that case shouldn't this grace period be advertised somehow?
Edit: answering to myself: no, not necessarily

respectively.

The creating node MUST set `bitcoin-signature-1` to the signature of
the double-SHA of `node-id-1` using `bitcoin-key-1`, and set
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we specify what kind of SHA?

as a channel, the receiving node SHOULD queue the message for
rebroadcasting. If it has previously received a valid
`channel_announcement` for the same transaction in the same block, but
different `node-id-1` or `node-id-2`, it SHOULD blacklist the the
Copy link
Collaborator

Choose a reason for hiding this comment

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

the the

typo

`channel_announcement` for the same transaction in the same block, but
different `node-id-1` or `node-id-2`, it SHOULD blacklist the the
previous message's `node-id-1` and `node-id-2` as well as this
`node-id-1` and `node-id-2` and forget channels conntected to them,
Copy link
Collaborator

Choose a reason for hiding this comment

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

conntected

typo

* [33:node-id]
* [3:rgb-color]
* [2:pad]
* [4:len]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably enforce a max value on len. FWIW I'd rather put a fixed size (e.g. 32B) null-terminated string.

* [4:timestamp]
* [1:side]
* [1:pad]
* [2:expiry]
Copy link
Collaborator

Choose a reason for hiding this comment

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

* [2:expiry]

leading spaces


The receiving node SHOULD ignore the message if `node-id` is not
previously known from a `channel_announcement` message, or if
`timestamp` is not greater than than the last-received
Copy link
Collaborator

Choose a reason for hiding this comment

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

than than

typo


## Rebroadcasting

Nodes receiving an announcement verify that this is the latest update from the announcing node by comparing the included timestamp and update their local view of the network's topology accordingly.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you mean that the receiving node should keep the latest update per message type and per channel right ?

Which means for announcing node A:

  • latest node_announcement message with node-id=A if any
  • all channel_announcement messages with node-id-1=A OR node-id-2=A not previously broadcast
  • latest channel_update for each unique blockheight \ blockindex \ outputindex

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, although for just the channels you can get away with just the channel_announcement + channel_update if you don't care about nodes, but we shouldn't encourage this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

you can get away with just the channel_announcement + channel_update if you don't care about nodes

Right! I had totally missed that

intelligence services to give their nodes cool monikers like IRATEMONK
and WISTFULTOLL and use the color black.

### Requirements
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should a node_announcement message be sent after every new channel_announcement, or once after the first channel_announcement?


Each node SHOULD flush outgoing announcements once every 60 seconds, independently of the arrival times of announcements, resulting in a staggered announcement and deduplication of announcements.

Nodes MAY re-announce their channels regularly, however this is discouraged in order to keep the resource requirements low.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add some kind of blacklisting based on a max number of node_announcement or channel_update that a peer is allowed to send per second, considering that those messages are costless to produce?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have the dependence between the channel_announcement and the node_announcement, i.e., a node_announcement that is not preceded by a channel_announcement is invalid and discarded. So we don't forward these free messages, which is the real danger here. If our peer wants to spam us with a node_announcements that we'll just drop, that's fine, and can be handled more generically, e.g., close a channel if our peer sends us more than 5kb/s, independently of the message type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I just realized that and striked my main comment above 😖

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
Copy link
Collaborator Author

Pierre-Marie Padiou notifications@github.com writes:

pm47 commented on this pull request.

I like this proposal better, the separation between channel_announcement and channel_update is very similar to a prototype we built into eclair a few weeks ago.

I also like the fact that we rely on the blockchain to find out which channels are closed instead of announcing both open and close, which was a little cumbersome in PR #1 (and it can still be added).

On the downside, I think this design is more vulnerable to DoS attacks because messages node_announcement and channel_update are costless to produce once a single valid channel has been created. Maybe we could add some rate limiting logic to mitigate this?

I think stepped propagation automatically controls this for the moment,
doesn't it? You can only propagate one every 60 seconds.

+The node SHOULD accept HTLCs which pay a fee equal or greater than:
+

  • fee-base-msat + htlc-amount-msat * fee-proportional-millionths / 1000000

I think there need to be a tolerance on the fee, because we need to take into account the time it takes for the information to propagate. Otherwise a node will systematically reject payments received right after it increased its relaying fee.

Yes, and that's why it's a should.

I needed to stick that formula somewhere in the doc, but wasn't sure
where.

I've moved it out to a separate section:

HTLC Fees

The node creating channel_update SHOULD accept HTLCs which pay a fee equal or greater than:

fee-base-msat + htlc-amount-msat * fee-proportional-millionths / 1000000

The node creating channel_update SHOULD accept HTLCs which pay an
older fee for some time after sending channel_update to allow for
propagation delay.

+The creating node MUST set bitcoin-signature-1 to the signature of
+the double-SHA of node-id-1 using bitcoin-key-1, and set

Should we specify what kind of SHA?

Thanks, I s/SHA/SHA256/ everywhere.

the the

typo
..

conntected

typo

Both fixed, thanks.

+## The node_announcement message
+
+This allows a node to indicate extra data associated with it, in
+addition to its public key. To avoid trivial denial of service attacks,
+nodes for which a channel is not already known are ignored.
+
+1. type: 257 (MSG_NODE_ANNOUNCEMENT)
+2. data:

  • * [64:signature]
  • * [4:timestamp]
  • * [16:ipv6]
  • * [2:port]
  • * [33:node-id]
  • * [3:rgb-color]
  • * [2:pad]
  • * [4:len]

We should probably enforce a max value on len. FWIW I'd rather put a fixed size (e.g. 32B) null-terminated string.

Good idea. The longest NSA codename I could find is "EVOLVED MUTANT BROTH"
which fits comfortably in 32 bytes.

insufficent

typo

Removed, due to fixed len.

+After a channel has been initially announced, each side independently
+announces its fees and minimum expiry for HTLCs. It uses the 8-byte
+channel shortid which matches the channel_announcement and one byte
+to indicate which end this is. It can do this multiple times, if
+it wants to change fees.
+
+1. type: 258 (MSG_CHANNEL_UPDATE)
+2. data:

  • * [64:signature]
  • * [4:blockheight]
  • * [3:blockindex]
  • * [1:outputindex]
  • * [4:timestamp]
  • * [1:side]
  • * [1:pad]
    • [2:expiry]
    • [2:expiry]

leading spaces

Tabs v. spaces. Replaced with spaces everywhere.

than than

typo

Fixed.

+## Rebroadcasting
+
+Nodes receiving an announcement verify that this is the latest update from the announcing node by comparing the included timestamp and update their local view of the network's topology accordingly.

I think you mean that the receiving node should keep the latest update per message type and per channel right ?

Yes, I cut and paste from @cdecker overzealously here.

Which means for announcing node A:

  • latest node_announcement message with node-id=A if any
  • all channel_announcement messages with node-id-1=A OR node-id-2=A not previously broadcast
  • latest channel_update for each unique blockheight \ blockindex \ outputindex

How about:

    Nodes receiving a new `channel_announcement` or a
    `channel_update` or `node_update` with an updated timestamp
    update their local view of the network's topology accordingly.

    Once the announcement has been processed it is added to a list
    of outgoing announcements (perhaps replacing older updates) to
    the processing node's peers, which will be flushed at regular
    intervals.  This store and delayed forward broadcast is called a
    _staggered broadcast_

+The timestamp allows ordering in the case of multiple announcements;
+the ipv6 and port allow the node to announce its willingness to
+accept incoming network connections, the rgb-color and alias allow
+intelligence services to give their nodes cool monikers like IRATEMONK
+and WISTFULTOLL and use the color black.
+
+### Requirements

Should a node_announcement message be sent after every new channel_announcement, or once after the first channel_announcement?

It only needs to be sent once, unless details change. I thought about
banning changing of monikers, but that would be less fun.

+Nodes receiving an announcement verify that this is the latest update from the announcing node by comparing the included timestamp and update their local view of the network's topology accordingly.
+
+Once the announcement has been processed it is added to a list of outgoing announcements to the processing node's peers, which will be flushed at regular intervals.
+This store and delayed forward broadcast is called a staggered broadcast
+
+If, after applying the changes from the announcement, there are no channels associated with the announcing node, then the receiving node MAY purge the announcing node from the set of known nodes.
+Otherwise the receiving node updates the metadata and stores the signature associated with the announcement.
+This will later allow the receiving node to rebuild the announcement for its peers.
+
+After processing the announcement the receiving node adds the announcement to a list of outgoing announcements.
+
+### Requirements
+
+Each node SHOULD flush outgoing announcements once every 60 seconds, independently of the arrival times of announcements, resulting in a staggered announcement and deduplication of announcements.
+
+Nodes MAY re-announce their channels regularly, however this is discouraged in order to keep the resource requirements low.

Should we add some kind of blacklisting based on a max number of node_announcement or channel_update that a peer is allowed to send per second, considering that those messages are costless to produce?

They only waste local bandwidth, since we'll only remember the last one.

Eventually we may want a more realistic limit, perhaps based on
N-updates-per-block which is simple to agree on. Meanwhile, staggered
broadcast limits us to one per 60 seconds.

Cheers,
Rusty.

@rustyrussell
Copy link
Collaborator Author

Christian Decker notifications@github.com writes:

+The node SHOULD accept HTLCs which pay a fee equal or greater than:
+

  • fee-base-msat + htlc-amount-msat * fee-proportional-millionths / 1000000

Right, but that is the node's local policy isn't it? Either he has a grace period, or he is rejecting things that'd make him some fees. So I'm not sure we need to specify this here.

Well, the formula needs to go somewhere. I moved it to its own section.

Cheers,
Rusty.

@BitfuryLightning
Copy link
Contributor

Very nice proposal. I like this proposal better than the previous one because it doesn't send duplicating info each time.

@BitfuryLightning BitfuryLightning merged commit 25db643 into lightning:master Nov 22, 2016
fjahr pushed a commit to fjahr/lightning-mw that referenced this pull request Dec 25, 2018
OrfeasLitos added a commit to OrfeasLitos/lightning-rfc that referenced this pull request Jul 8, 2019
Mention that `outgoing_cltv_value` has to be equal to
`min_final_cltv_expiry` and `amt_to_forward` has to be equal to
`amount` if the [BOLT lightning#11](11-payment-encoding.md) invoice is used
t-bast pushed a commit that referenced this pull request Jul 22, 2019
Mention that `outgoing_cltv_value` has to be equal to
`min_final_cltv_expiry` and `amt_to_forward` has to be equal to
`amount` if the [BOLT #11](11-payment-encoding.md) invoice is used
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.

4 participants