-
Notifications
You must be signed in to change notification settings - Fork 493
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
BOLT #7: More complex proposal, using three separate message types. #11
Conversation
Contents stolen from Christian's draft. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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., 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 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. |
There was a problem hiding this 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 <- 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 chattynode_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?
|
||
The node SHOULD accept HTLCs which pay a fee equal or greater than: | ||
|
||
fee-base-msat + htlc-amount-msat * fee-proportional-millionths / 1000000 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 withnode-id
=A if any - all
channel_announcement
messages withnode-id-1
=A ORnode-id-2
=A not previously broadcast - latest
channel_update
for each uniqueblockheight
\blockindex
\outputindex
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
Pierre-Marie Padiou notifications@github.com writes:
I think stepped propagation automatically controls this for the moment,
Yes, and that's why it's a should. I needed to stick that formula somewhere in the doc, but wasn't sure I've moved it out to a separate section: HTLC FeesThe node creating
The node creating
Thanks, I s/SHA/SHA256/ everywhere.
Both fixed, thanks.
Good idea. The longest NSA codename I could find is "EVOLVED MUTANT BROTH"
Removed, due to fixed len.
Tabs v. spaces. Replaced with spaces everywhere.
Fixed.
Yes, I cut and paste from @cdecker overzealously here.
How about:
It only needs to be sent once, unless details change. I thought about
They only waste local bandwidth, since we'll only remember the last one. Eventually we may want a more realistic limit, perhaps based on Cheers, |
Christian Decker notifications@github.com writes:
Well, the formula needs to go somewhere. I moved it to its own section. Cheers, |
Very nice proposal. I like this proposal better than the previous one because it doesn't send duplicating info each time. |
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
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
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.