-
Notifications
You must be signed in to change notification settings - Fork 494
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: receiving node requirements related to timestamp for channel_update message #621
Changes from 5 commits
bfe9032
59c5175
fc10d25
7373ccd
536b8f5
c7dd689
50913b4
ca9e7bd
2393f3a
8554d5c
437deba
839d5b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -494,12 +494,12 @@ The receiving node: | |
- if the specified `chain_hash` value is unknown (meaning it isn't active on | ||
the specified chain): | ||
- MUST ignore the channel update. | ||
- if `timestamp` is NOT greater than that of the last-received | ||
- if `timestamp` is lower than that of the last-received | ||
`channel_update` for this `short_channel_id` AND for `node_id`: | ||
- SHOULD ignore the message. | ||
- otherwise: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this otherwise could have been removed too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jtimon No that otherwise is needed. Just that both the The idea was to have it this way. Timestamp equal to previous do this. Time stamp lower than previous do that. Otherwise: if timestamp too large in future do this, otherwise do that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is reverting the commit and making changes the right way to go? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No we don't rewrite history. You can submit a tiny indentation fix PR and it should be approved very quickly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @t-bast cool. Will submit one soon. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! |
||
- if the `timestamp` is equal to the last-received `channel_update` | ||
AND the fields (other than `signature`) differ: | ||
- if the `timestamp` is equal to the last-received `channel_update` for this | ||
`short_channel_id` AND `node_id`, AND the fields below `timestamp` differ: | ||
- MAY blacklist this `node_id`. | ||
- MAY forget all channels associated with it. | ||
- if the `timestamp` is unreasonably far in the future: | ||
|
@@ -524,6 +524,13 @@ makes sense to have it be a UNIX timestamp (i.e. seconds since UTC | |
1970-01-01). This cannot be a hard requirement, however, given the possible case | ||
of two `channel_update`s within a single second. | ||
|
||
It is assumed that more than one `channel_update` message changing the channel | ||
parameters in the same second may be a DoS attempt, and therefore, the node responsible | ||
for signing such messages may be blacklisted. However, a node can send a same | ||
`channel_update` message with a different signature (changing the nonce in signature | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. important to note that since ECDSA signatures are malleable, the signatures can be different even if the node only signed once. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very fair point. I'm incorporating this in the rationale. |
||
signing), and hence fields apart from signature are checked to see if the channel | ||
parameters have changed for the same timestamp. | ||
|
||
The explicit `option_channel_htlc_max` flag to indicate the presence | ||
of `htlc_maximum_msat` (rather than having `htlc_maximum_msat` implied | ||
by the message length) allows us to extend the `channel_update` | ||
|
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.
this the new rewording leaves a gap, since it doesn't specify what to do if the timestamp is equal to the last-received and the message is indeed a duplicate. in this case, the message should still be ignored.
i think this is resolved keeping the
NOT greater than
here and removing theotherwise
on line 500. i think it also requires switching the order of the checks, since the blacklist check is a stricter subset of the<=
check.to be clear, i'm suggesting:
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.
@cfromknecht Thanks! I included your suggestion for the condition where the
timestamp
is equal. But in order to not leave other holes, I slightly changed it in terms of order than what you suggested. Let me know if its good.