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: receiving node requirements related to timestamp for channel_update message #621

Merged
merged 12 commits into from Jul 16, 2019

Conversation

Projects
None yet
3 participants
@ugamkamat
Copy link
Contributor

commented Jun 16, 2019

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.

ugamkamat added some commits Jun 16, 2019

Update 07-routing-gossip.md
While going through the 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 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".
BOLT #7 : Receiving node requirements related to timestamp for channe…
…l_update message

While going through the 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 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".
Show resolved Hide resolved 07-routing-gossip.md Outdated
Show resolved Hide resolved 07-routing-gossip.md Outdated
Show resolved Hide resolved 07-routing-gossip.md Outdated

ugamkamat and others added some commits Jun 17, 2019

Update 07-routing-gossip.md
Co-Authored-By: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>
Update 07-routing-gossip.md
Co-Authored-By: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>
@@ -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

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jun 19, 2019

Collaborator

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 <=:
  ...

This comment has been minimized.

Copy link
@ugamkamat

ugamkamat Jun 23, 2019

Author Contributor

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

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

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jun 19, 2019

Collaborator

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

This comment has been minimized.

Copy link
@ugamkamat

ugamkamat Jun 20, 2019

Author Contributor

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

ugamkamat added some commits Jun 23, 2019

@t-bast
Copy link
Collaborator

left a comment

ACK 2393f3a

Show resolved Hide resolved 07-routing-gossip.md Outdated
@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2019

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

@t-bast t-bast added the spelling label Jul 9, 2019

Update 07-routing-gossip.md
Co-Authored-By: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>
@ugamkamat

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

@t-bast Done, typo is fixed

@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

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

@t-bast t-bast referenced this pull request Jul 9, 2019

Closed

Update .aspell.en.pws #632

Update .aspell.en.pws
Updated for DoS and ECDSA
@ugamkamat

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

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

@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2019

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

@cfromknecht
Copy link
Collaborator

left a comment

LGTM 🏖

@t-bast

t-bast approved these changes Jul 16, 2019

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.