BOLT #7: receiving node requirements related to timestamp for channel_update message by ugamkamat · Pull Request #621 · lightning/bolts

ugamkamat · 2019-06-16T13:57:26Z

While going through BOLT #7 I was confused by this receiving node requirement for channel_update message: "if the timestamp is equal to the last-received channel_update AND the fields (other than signature) differ: MAY blacklist this node_id; MAY forget all channels associated with it." On the outset it might read to "timestamp should be same, the signature should be same as previous but if the other data is different; then blacklist the node_id". However, if signature is same as previous update but data is different, then the signature would be rendered invalid. And since signature check is done prior to the timestamp check, this situation of checking may not arise. So this check seems to be redundant. However, Mark H corrected me on stackexchange (https://bitcoin.stackexchange.com/questions/88350/bolt-7-can-receiving-node-requirements-in-channel-update-message-give-rise-to) by saying that a valid signature must be provided for this channel_update and timestamp to go through. This pull request slightly corrects the language behind this check and adds a paragraph in the rationale to explain that point.

In correcting the point, I have said fields below timestamp differs for the following reason: (1) short_channel_id and chain_hash requirements are processed prior to timestamp and (2) valid signature check is also done prior to the timestamp.

While creating this pull request, I also noticed a slight typo in the same section. The sentence prior to the one I mentioned above says that if the timestamp is not greater than the previous message, the node should ignore the message. And the second sentence is about comparing if timestamp is equal. But again, the equal to check might not be processed as the message will be discarded if it is not greater. I have changed that to "greater than or equal to". Would love your comments and suggestions.

While going through the BOLT lightning#7 I was confused by this receiving node requirement for `channel_update` message: "if the `timestamp` is equal to the last-received `channel_update` AND the fields (other than `signature`) differ: MAY blacklist this `node_id`; MAY forget all channels associated with it." On the outset it might read to "`timestamp` should be same, the `signature` should be same as previous but if the other data is different; then blacklist the `node_id`". However, if `signature` is same as previous update but data is different, then the `signature` would be rendered invalid. And since `signature` check is done prior to the `timestamp` check, this situation of checking may not arise. So this check seems to be redundant. However, Mark H corrected me on stackexchange (https://bitcoin.stackexchange.com/questions/88350/bolt-7-can-receiving-node-requirements-in-channel-update-message-give-rise-to) by saying that a valid signature must be provided for this update and timestamp to go through. This pull request corrects the language behind this check. While creating this pull request, I also noticed a slight typo in the same section. The sentence prior to the one I mentioned above says that if the timestamp is not greater than the previous message, the node should ignore the message. And the second sentence is about comparing if timestamp is equal. But again, this check might not be processed as the message will be discarded if it is not greater. I have changed that to "greater than or equal to".

…or channel_update message While going through the BOLT lightning#7 I was confused by this receiving node requirement for `channel_update` message: "if the `timestamp` is equal to the last-received `channel_update` AND the fields (other than `signature`) differ: MAY blacklist this `node_id`; MAY forget all channels associated with it." On the outset it might read to "`timestamp` should be same, the `signature` should be same as previous but if the other data is different; then blacklist the `node_id`". However, if `signature` is same as previous update but data is different, then the `signature` would be rendered invalid. And since `signature` check is done prior to the `timestamp` check, this situation of checking may not arise. So this check seems to be redundant. However, Mark H corrected me on stackexchange (https://bitcoin.stackexchange.com/questions/88350/bolt-7-can-receiving-node-requirements-in-channel-update-message-give-rise-to) by saying that a valid signature must be provided for this update and timestamp to go through. This pull slightly corrects the language behind this check and adds a paragraph in the rationale to explain that point. While creating this pull request, I also noticed a slight typo in the same section. The sentence prior to the one I mentioned above says that if the timestamp is not greater than the previous message, the node should ignore the message. And the second sentence is about comparing if timestamp is equal. But again, the equal to check might not be processed as the message will be discarded if it is not greater. I have changed that to "greater than or equal to".

07-routing-gossip.md

Co-Authored-By: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>

cfromknecht · 2019-06-19T11:18:49Z

07-routing-gossip.md

  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


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 the otherwise 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:

if blacklist: ... if timestamp <=: ...

@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.

cfromknecht · 2019-06-19T12:01:20Z

07-routing-gossip.md

+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 


important to note that since ECDSA signatures are malleable, the signatures can be different even if the node only signed once.

Very fair point. I'm incorporating this in the rationale.

t-bast

ACK 2393f3a

07-routing-gossip.md

t-bast · 2019-07-09T08:38:12Z

@ugamkamat could you update to fix the typo (which would make the build succeed)?
@cfromknecht could you provide feedback/ack?

Co-Authored-By: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>

ugamkamat · 2019-07-09T09:16:26Z

@t-bast Done, typo is fixed

t-bast · 2019-07-09T09:37:10Z

The build is still failing because you introduce words that aren't in the spellcheck list yet.
Could you add "DoS" and "ECDSA" to the .aspell.en.pws file?

ugamkamat · 2019-07-09T13:08:13Z

@t-bast Done. Since this was my first time adding a commit, I wasn't entirely sure how to update that file. I have created a PR with those words added.

Updated for DoS and ECDSA

ugamkamat · 2019-07-09T17:30:46Z

@t-bast So, I have pushed the file to this PR. Let me know if that is correct.

t-bast · 2019-07-09T19:26:56Z

ACK 839d5b3
We need two approvals to commit, so I'll wait for @cfromknecht or someone else to chime in.

cfromknecht

LGTM 🏖

jtimon · 2019-07-17T19:09:54Z

07-routing-gossip.md

+  - 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:


I guess this otherwise could have been removed too?

@jtimon No that otherwise is needed. Just that both the if and otherwise should be indented on the previous otherwise. I think there was a formatting error when creating the pull request.

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

Is reverting the commit and making changes the right way to go?

No we don't rewrite history. You can submit a tiny indentation fix PR and it should be approved very quickly.

@t-bast cool. Will submit one soon.

Related to pull lightning#621 Corrected the indentation in the otherwise section in the `timestamp` check of the `channel_update` message. Both the if appeared at the same level of otherwise.

ugamkamat added 2 commits June 16, 2019 14:21

t-bast reviewed Jun 17, 2019

View reviewed changes

07-routing-gossip.md Outdated Show resolved Hide resolved

07-routing-gossip.md Outdated Show resolved Hide resolved

07-routing-gossip.md Outdated Show resolved Hide resolved

ugamkamat and others added 3 commits June 17, 2019 21:34

Update 07-routing-gossip.md

fc10d25

Co-Authored-By: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>

Update 07-routing-gossip.md

7373ccd

Co-Authored-By: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>

Update 07-routing-gossip.md

536b8f5

cfromknecht reviewed Jun 19, 2019

View reviewed changes

ugamkamat added 4 commits June 23, 2019 23:20

Update 07-routing-gossip.md

c7dd689

Update 07-routing-gossip.md

50913b4

Update 07-routing-gossip.md

ca9e7bd

Update 07-routing-gossip.md

2393f3a

t-bast reviewed Jul 8, 2019

View reviewed changes

07-routing-gossip.md Outdated Show resolved Hide resolved

t-bast mentioned this pull request Jul 8, 2019

BOLT7: Signing channel updates with the same timestamp may get you bl… #614

Closed

t-bast added the spelling These changes may be merged without additional sign off from the weekly meeting label Jul 9, 2019

Update 07-routing-gossip.md

8554d5c

Co-Authored-By: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>

t-bast mentioned this pull request Jul 9, 2019

Update .aspell.en.pws #632

Closed

Update .aspell.en.pws

437deba

Updated for DoS and ECDSA

Merge branch 'master' into ugamkamat-patch-2

839d5b3

cfromknecht approved these changes Jul 15, 2019

View reviewed changes

t-bast approved these changes Jul 16, 2019

View reviewed changes

t-bast merged commit 1db481f into lightning:master Jul 16, 2019

jtimon reviewed Jul 17, 2019

View reviewed changes

ugamkamat mentioned this pull request Jul 18, 2019

BOLT #7: Correct indentation typo in channel_update message #649

Merged

Conversation

ugamkamat commented Jun 16, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

t-bast commented Jul 9, 2019

Uh oh!

ugamkamat commented Jul 9, 2019

Uh oh!

t-bast commented Jul 9, 2019

Uh oh!

ugamkamat commented Jul 9, 2019

Uh oh!

ugamkamat commented Jul 9, 2019

Uh oh!

t-bast commented Jul 9, 2019

Uh oh!

cfromknecht left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ugamkamat Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ugamkamat Jul 17, 2019 •

edited

Loading