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

More conservative cltv_expiry_delta recommendations #785

Merged
merged 3 commits into from
Aug 20, 2020

Conversation

t-bast
Copy link
Collaborator

@t-bast t-bast commented Jun 10, 2020

Many channels use a value below 6, which is really insecure (there are more than 2k such channels on mainnet).

While less risky, there are more than 7k channels with a value below 12.

This indicates that the spec should probably make the risks a bit more clear to help guide node operators.

@ariard
Copy link

ariard commented Jun 22, 2020

Concept ACK on a far more conservative delta for now.

Many channels use a value below 6, which is really insecure (there are
more than 2k such channels on mainnet).

While less risky, there are more than 7k channels with a value below 12.

This indicates that the spec should probably make the risks a bit more
clear to help guide node operators.
Copy link
Collaborator

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack 07c4493

02-peer-protocol.md Outdated Show resolved Hide resolved
[BOLT #11](11-payment-encoding.md) is 9, which is slightly more
conservative than the 7 that this calculation suggests.
1-3 above don't apply). The default in [BOLT #11](11-payment-encoding.md) is
9, which is less conservative than the 18 that this calculation suggests.
Copy link
Collaborator

Choose a reason for hiding this comment

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

should BOLT 11 also be updated with this more conservative value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hesitated. The fact you're mentioning it shows that it's probably a good idea, I'll update it!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 791801d

Copy link
Collaborator Author

@t-bast t-bast Jul 7, 2020

Choose a reason for hiding this comment

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

NB: that means that when paying an invoice that is not providing a min_final_cltv_expiry, implementations must be updated to use 18 instead of 9. I'm making that change to eclair right now, lnd and c-lightning need to implement it as well. On the receiving side we'll need to accept the old value (9) for a while to give the network time to update. Does that sound ok? @rustyrussell @cfromknecht

Copy link
Collaborator

Choose a reason for hiding this comment

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

@t-bast I looked into this as well, I think the changes to BOLT 11 are more complicated than I initially expected. As you mentioned, raising the default value when min_final_cltv_delta is not present creates incompatibilities when the sender is assuming a lower default than the receiver. It is a bit tricky since there are several places where it is ambiguous which default to return (sender vs receiver), e.g. when decoding an arbitrary invoice.

In hind sight, we probably shouldn't have used these implicit defaults because it makes it very difficult to upgrade. For a smooth upgrade, perhaps there is a preliminary step where all implementations change to always set the min_final_cltv_delta explicitly, and at a later point, fail to parse invoices that omit it. Increasing the default in BOLT 11 could probably be done in tandem with the last stage I think. The same should precautions should probably be taken for expiry while we're at it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@t-bast actually I think I see what you're saying, we can all change to set 18 on new invoices but still assume 9 when not present?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the reason we put a default value in the spec and omitted it from invoices was to make invoices fit in a tweet or something like that, but indeed it makes upgrades more painful.

The change I've done in eclair is to start using 18 when paying an invoice that doesn't specify min_final_cltv_delta.
When we receive a payment for an invoice for which we didn't specify a min_final_cltv_delta, we still accept 9.

If we all release new versions that do that, we can probably completely phase out the old default value of 9 in 6 months or something, and that will prompt outdated software to update to be able to pay invoices as they start being rejected by up-to-date nodes. How does that sound? I still think it's helpful to update the spec so that new-comers are aware of these safe(r) defaults.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it turns out LND has been setting an explicit final CLTV value for several releases, with a default value of 40. i have made the change which now clamps user-chosen final CLTV values to be at least 18 when creating new invoices to be in line with these recommendations.

we still retain the behavior of using 9 if no CLTV is present for backwards compatibility on both sender and receiver side, but it sounds like this is compatible with your changes 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it turns out LND has been setting an explicit final CLTV value for several releases

Great!

we still retain the behavior of using 9 if no CLTV is present for backwards compatibility on both sender and receiver side, but it sounds like this is compatible with your changes 👍

Awesome!

@t-bast
Copy link
Collaborator Author

t-bast commented Jul 21, 2020

Following yesterday's spec meeting discussion, I updated Bolt 11 requirements in eb946f0.

Writers now MUST include min_final_cltv_expiry: this is more future-proof. If not included (for backwards-compatibility) the default value that should be used is now 18.

t-bast added a commit to ACINQ/eclair that referenced this pull request Jul 21, 2020
* Activate wumbo by default

This is safe as `max-funding-satoshis` is set to 16777215 sats, which is
the non-wumbo limit.

If users want to increase the maximum channel size, they can update this
configuration value.

* Update default minFinalCltvExpiryDelta

See lightning/bolts#785

* Set minFinalCltvExpiryDelta in invoices

Our default fulfill-safety-window is now greater than the spec's default
min-final-expiry-delta in invoices, so we need to explicitly tell payers
what value they must use.

Otherwise we may end up closing channels if a block is produced while we're
waiting for our peer to accept an UpdateFulfillHtlc.
@@ -140,7 +140,7 @@ Currently defined tagged fields are:
* `n` (19): `data_length` 53. 33-byte public key of the payee node
* `h` (23): `data_length` 52. 256-bit description of purpose of payment (SHA256). This is used to commit to an associated description that is over 639 bytes, but the transport mechanism for the description in that case is transport specific and not defined here.
* `x` (6): `data_length` variable. `expiry` time in seconds (big-endian). Default is 3600 (1 hour) if not specified.
* `c` (24): `data_length` variable. `min_final_cltv_expiry` to use for the last HTLC in the route. Default is 9 if not specified.
* `c` (24): `data_length` variable. `min_final_cltv_expiry` to use for the last HTLC in the route. Default is 18 if not specified.
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't this need to be conditioned on the context of sender/receiver? i.e., the default is 9 when receiving but 18 when sending? it is also worth asking whether making this distinction is necessary, or just changing to always set explicit values is good enough until we remove the behavior that assumes a CLTV when none is present

Copy link
Collaborator Author

@t-bast t-bast Jul 28, 2020

Choose a reason for hiding this comment

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

doesn't this need to be conditioned on the context of sender/receiver?

This is what's done in eb946f0: the receiver now MUST explicitly specify this value, and the sender should use 18 if it's not specified (backwards-compatibility).

We want the value 9 to completely disappear from the spec (current implementation may be non compliant with that for the moment, but as long as they're working towards compliance in a few releases it's ok).

@cdecker
Copy link
Collaborator

cdecker commented Aug 3, 2020

The change in the suggestions is good, the change in the default if not specified is not. But making the field mandatory further down has the same effect, so keep the default as is to avoid obsoleting existing implementations.

given that block times are irregular, empty blocks still occur, fees may vary
greatly, and the fees cannot be bumped on HTLC transactions, `S=12` should be
considered a minimum. `S` is also the parameter that may vary the most under
attack, so a higher value may be desirable when non-negligible amounts are at
Copy link

Choose a reason for hiding this comment

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

Note, I think you can keep this line but overall if you're a meaningful routing node an attacker can exploit by opening either 1 big channel or 10 small ones, even batching opening fees. Really most of attacks are "batchable" :/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Of course, but that doesn't make these recommendations obsolete, on the contrary! It will need to be coupled with other techniques (e.g. limiting your exposure in the number of channels with nodes you don't trust) but in any case, you'll want high enough deltas.

[BOLT #11](11-payment-encoding.md) is 9, which is slightly more
conservative than the 7 that this calculation suggests.
1-3 above don't apply). The default in [BOLT #11](11-payment-encoding.md) is
18, which matches this calculation.
Copy link

Choose a reason for hiding this comment

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

Should we subtract one block from deadline for received HTLCs compare to minimum cltv_expiry ? You might evaluate both values with different logics/threads and in-between you may process a block thus breaking the channel without letting a bit of "block"-time for a happy resolution ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is more an implementation concern than a spec concern, and all implementations naturally did it the other way around (sending a higher value than what the spec recommends to accommodate for such things).

Copy link
Collaborator

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🗽

@rustyrussell
Copy link
Collaborator

Approved at http://www.erisian.com.au/meetbot/lightning-dev/2020/lightning-dev.2020-08-17-20.08.html

@rustyrussell rustyrussell merged commit b4132ff into master Aug 20, 2020
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Aug 20, 2020
As per lightning/bolts#785

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: config: the default CLTV expiry is now 34 blocks, and final expiry 18 blocks as per new BOLT recommendations.
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Aug 20, 2020
As per lightning/bolts#785

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: protocol: bolt11 invoices always include CLTV fields (see lightning-rfc#785)
rustyrussell added a commit to ElementsProject/lightning that referenced this pull request Aug 24, 2020
As per lightning/bolts#785

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: config: the default CLTV expiry is now 34 blocks, and final expiry 18 blocks as per new BOLT recommendations.
rustyrussell added a commit to ElementsProject/lightning that referenced this pull request Aug 24, 2020
As per lightning/bolts#785

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: protocol: bolt11 invoices always include CLTV fields (see lightning-rfc#785)
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Aug 25, 2020
As per lightning/bolts#785

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: config: the default CLTV expiry is now 34 blocks, and final expiry 18 blocks as per new BOLT recommendations.
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Aug 25, 2020
As per lightning/bolts#785

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: protocol: bolt11 invoices always include CLTV fields (see lightning-rfc#785)
@t-bast t-bast deleted the cltv-expiry-delta-section branch September 1, 2020 07:04
@@ -212,6 +211,8 @@ A reader:
- MUST use the `n` field to validate the signature instead of performing signature recovery.
- if there is a valid `s` field:
- MUST use that as [`payment_secret`](04-onion-routing.md#tlv_payload-payload-format)
- if the `c` field (`min_final_cltv_expiry`) is not provided:
- MUST use an expiry delta of at least 18 when making the payment
Copy link
Contributor

Choose a reason for hiding this comment

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

The writer now MUST include a min_final_cltv_expiry c field (L173), therefore the reader should reject if c is not provided.

On the one hand, this suggestion makes the spec more consistent. On the other, it may be a non-backwards compatible change. But if the two parties use different defaults payments will fail to go through anyway (if I understand correctly).

Copy link
Collaborator Author

@t-bast t-bast Sep 16, 2020

Choose a reason for hiding this comment

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

That's exactly why we kept that fallback to 18 when missing, we don't want to make a backwards-incompatible change here. 18 is bigger than the previous default, so it should work for backwards-compatibility.

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.

7 participants