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

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

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@jtimon
Copy link
Contributor

commented May 25, 2019

…acklisted

The current text seems to contradict itself.
If equal timestamps should be ignored, there's no chance to blacklist the node because of it as it said right afterwards.
Perhaps this is not the best fix, but it seems like a fix is needed.

@cdecker

This comment has been minimized.

Copy link
Collaborator

commented May 27, 2019

We don't have a concept of blacklisting in the spec. So far we will drop the channel_update if the timestamp is not strictly greater than any prior channel_update timestamp. Notice also that the timestamp is defined as a logical timestamp and not as a unix-timestamp, hence you could for example just have a monotonically increasing version counter instead of using the unix timestamp. c-lightning for example will take max(unix_timestamp, last_timestamp + 1) as the new value to avoid multiple channel_updates being unordered.

@jtimon

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

There's the following right below:

- MAY blacklist this `node_id`.

https://github.com/lightningnetwork/lightning-rfc/pull/614/files#diff-210f53c64b4a7ac8da48642029437668R503

Perhaps that part should be removed then?
Both things seem in contradiction.

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

commented May 27, 2019

hence you could for example just have a monotonically increasing version counter instead of using the unix timestamp

won't this get your channel instapruned as a zombie since it will be much older than two weeks? lnd does the same in taking max(unix, last+1) which works well

@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

I think #621 replaces this PR and adds more details.

@jtimon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

I think #621 replaces this PR and adds more details.

Perhaps. As said, this just seems contradictory, but I'm not sure what the best solution to remove the contradiction is. Perhaps it's #621 , If that one is merged and I haven't closed this one yet, please, remind me.

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

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.