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

Add advisory section for wumbo channels confirmations #746

Closed
wants to merge 4 commits into from

Conversation

araspitzu
Copy link
Contributor

This PR is a follow up on #596 adding an advisory section where we suggest implementors to provide the means to scale the number of confirmation along with the channel size. This is a security measure to protect against chain reorgs when opening large channels.

We advise the implementors to provide the means to increase the number confirmations
needed to confirm large channels
Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt @moneyball @Roasbeef can you please provide some input?

Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also advise wumbo channels operators to diversify their block headers sources instead of relying only on one full-node connected to broadband internet (like multi-homing or satellites). Likely out of scope here, but maybe we should have some spec annex with general security advices.

@@ -192,6 +192,10 @@ know this node will accept `funding_satoshis` greater than or equal to 2^24.
Since it's broadcast in the `node_announcement` message other nodes can use it to identify peers
willing to accept large channel even before exchanging the `init` message with them.

Implementers are advised to provide the means to scale the number of confirmations, tweaking
`accept_channel.minimum_depth`, with the size of the funding amount. A rule of thumb is to
wait enough blocks until the cumulative block reward exceeds the size of the channel.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should also recommend to scale to_self_delay in consequence ? You may have watchtowers to alleviate the liveliness requirement but mempool congestion is still a concern.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the issue apply to to_self_delay? I don't think it does, I think only funding is important as you don't want to start using the channel, relay payments and then see the channel's funding tx be double spent. But what could the attack be when closing?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eclipsing your node and slowing block your block reception. That way I pin you with a delayed view of blockchain view, can broadcast a revoked commitment transaction and you won't react until it's too late. An adversary would also beneficiate from a congestioned mempool for your justice txn getting confirmed. This attack may be hard to setup but for a high-value channel they shouldn't be excluded, increasing to_self_delay means increasing period during which eclipsing must be sustained.

02-peer-protocol.md Outdated Show resolved Hide resolved
@araspitzu
Copy link
Contributor Author

I agree it would be nice to mention having different sources for block headers as a security principle, but it seems a bit out of scope in the advisory. In general the implementers should take the same precautions as when handling a large incoming bitcoin payment, perhaps we could express this more explicitly in the advisory but i couldn't find a better way to give some general advice while keeping it technical (especially mentioning accept_channel.minimum_depth which is important for implementers).

@@ -192,6 +192,10 @@ know this node will accept `funding_satoshis` greater than or equal to 2^24.
Since it's broadcast in the `node_announcement` message other nodes can use it to identify peers
willing to accept large channel even before exchanging the `init` message with them.

Implementers are advised to provide the means to scale the number of confirmations, tweaking
`accept_channel.minimum_depth`, with the size of the funding amount. A rule of thumb is to
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just drop the sentence starting with "A rule of thumb". I don't think its sufficiently conservative for many users, and its probably too conservative for others.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious on why it wouldn't be sufficiently conservative for many users (we're also adding largely exceeds), but if this is too generic and can't cover most cases then I agree we should remove it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's mainly that it's too opinionated. Looking at the cumulative block reward to evaluate cost is one way among several ways of evaluating your scaling scenario. Having this in the spec may be influencing too much the implementers.

IIUC what was suggested during the meeting, this advisory should only say something like:

Implementers are advised to provide the means to scale the number of confirmations, tweaking 
`accept_channel.minimum_depth` and `accept_channel.to_self_delay`, with the size of the funding amount.

And let implementers completely free of choosing what scaling strategies they offer to their users.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me 👍

Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6cc31bb

@ariard
Copy link

ariard commented Mar 19, 2020

ACK 6cc31bb

@cdecker
Copy link
Collaborator

cdecker commented Mar 30, 2020

Personally I don't find these advisories useful, they tend to bloat the spec and not add much value. Don't write every little detail into the Constitution, that's what associated works are for.

@t-bast
Copy link
Collaborator

t-bast commented Mar 30, 2020

Personally I don't find these advisories useful, they tend to bloat the spec and not add much value.

I'd generally agree, but here we're keeping this to two lines and I think it's really, really important future implementers don't miss that so worth quickly mentioning...

@rustyrussell
Copy link
Collaborator

rustyrussell commented Mar 30, 2020

Minimum depth makes sense, ofc. But tweaking the to-self delay never did (from an economic PoV): in theory the danger of complete funds loss scales just like the danger of loss of access to funds.

So while a 1BTC channel is worth protecting, it's also a PITA to have the funds inaccessable for a month.

[ ariard on IRC pointed out that a large enough channel may incentivize attackers to perform an eclipse attack on your node, so having more time for out-of-band penalty broadcast makes sense. This is a valid point ]

@t-bast
Copy link
Collaborator

t-bast commented Mar 30, 2020

We should probably update the wording to something like:

"Implementers should consider scaling the number of confirmations (accept_channel.minimum_depth) and/or accept_channel.to_self_delay with the size of the funding amount."

WDYT?

@araspitzu
Copy link
Contributor Author

On a second thought: do we really need to mention the to-self delay and the eclipsing attack here? It seems to me that this kind of attack can be already performed on any node with enough funds at stake, an attacker that can pin you to a delayed view of the blockchain can take advantage of this to broadcast an old state of all the channels it has with you, so this kind of attack is not strictly related to large channels. WDYT @ariard?

@araspitzu
Copy link
Contributor Author

Closing for lack of traction and since it's overlapping with #772

@araspitzu araspitzu closed this Jun 3, 2020
@araspitzu araspitzu deleted the wumbo_advisory_section branch June 3, 2020 14:37
@ariard
Copy link

ariard commented Jun 5, 2020

@araspitzu seeing this only now. My SECURITY.md is about direct security vulnerabilities disclosure. In this PR, it's more LN node OPSEC.md, I would put down such write-up soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants