Update MinFinalCltvExpiryDelta default value and activate wumbo

[t-bast]

Activate wumbo by default

This is safe as max-funding-satoshis is still set to 16777215 sats, which is the non-wumbo limit.
If users want to increase the maximum channel size, they will now only need to update this configuration value.

Update default minFinalCltvExpiryDelta

See lightning/bolts#785
A value of 9 is a bit too optimistic and could be exploited.

Set minFinalCltvExpiryDelta in invoices

It's important to set minFinalCltvExpiryDelta in the invoices we generate to something bigger than our fulfill-safety-before-timeout-blocks, otherwise we'll close channels whenever a block is produced while we were fulfilling a payment for which we're the final recipient.

NB: @dkrm0 keysend payments will be a problem, because we don't control the minFinalCltvExpiryDelta senders will set and currently have no way of letting them know what value we want. When spec-ing keysend, we should add a tlv to our node_announcement to advertize our minFinalCltvExpiryDelta. Meanwhile we may want to reject keysend payments that are below our minFinalCltvExpiryDelta.

[pm47]

On second thought, I'm not too sure about this. The Channel.MIN_CLTV_EXPIRY_DELTA is spec-defined, that's why it is a constant (and not a configurable value). Why don't we change the definition of PaymentRequest.minFinalCltvExpiryDelta to :

lazy val minFinalCltvExpiryDelta: CltvExpiryDelta = tags.collectFirst {
    case cltvExpiry: PaymentRequest.MinFinalCltvExpiry => cltvExpiry.toCltvExpiryDelta
  } getOrElse(Channel.MIN_CLTV_EXPIRY_DELTA)

We could also keep the PaymentRequest.minFinalCltvExpiryDelta optional, and define a new :

lazy val minFinalCltvExpiryDelta: Option[CltvExpiryDelta] = ...
lazy val minFinalCltvExpiryDeltaWithFallback: CltvExpiryDelta = minFinalCltvExpiryDelta.getOrElse(Channel.MIN_CLTV_EXPIRY_DELTA)

But that seems overkill, since we can always iterate on the tags if we want to know whether the minFinalCltvExpiryDelta was explicitely defined in the payment request or not.

[t-bast]

The Channel.MIN_CLTV_EXPIRY_DELTA is spec-defined, that's why it is a constant (and not a configurable value).

This is changing in the spec as we speak :)
We all agreed during yesterday's meeting that hard-coding this in the spec was a bad idea from the start, and we want to remove it entirely.
The last commit of the spec PR now says that writers MUST specify a value in their invoice, and the default value of 18 is only there for backwards-compatibility for readers that receive invoices from legacy wallets/nodes that didn't specify this value.

The goal in the long(er) term is that this will allow us to change this value from an Option[CltvExpiryDelta] to a CltvExpiryDelta once enough nodes have upgraded.

[pm47]

That's fine, let's redefine Channel.MIN_CLTV_EXPIRY_DELTA to 18 and take my first proposal, it is consistent with the decision to make writer MUST define the value?

[t-bast]

And we do need to keep this fallbackFinalExpiryDelta in these request classes to allow senders to tweak this.
Ideally when you receive an expiryTooSoon error you could guess that you need to provide a higher value here.

[t-bast]

Or at least the node operator can manually act on this error when the payment fails, whereas if we hide that inside PaymentRequest.scala we lose this ability.

[pm47]

And we do need to keep this fallbackFinalExpiryDelta in these request classes to allow senders to tweak this.

Why would senders need to tweak this?

Ideally when you receive an expiryTooSoon error you could guess that you need to provide a higher value here.
Or at least the node operator can manually act on this error when the payment fails, whereas if we hide that inside PaymentRequest.scala we lose this ability.

If the minFinalExpiryTooSoon is enforced in the payment request, then the only reason this would fail is a disagreement in the current block height, in that case the payment should be retried as-is? Anyway for the use-case you are describing, this isn't a fallback but an override.

[t-bast]

Senders may need to tweak this when paying invoices that don't specify this value.

I think older nodes will want to increase the values they accept (given the recent noise about flood and loot) which they can likely do in other implementations by just changing a configuration value, without specifying it in invoices (because the code isn't yet released in implementation to add this to invoices). The only way payers learn that is by the expiry_too_soon error they receive.

I agree that we should remove this fallback value at some point, but I think we should wait a bit before doing this because it removes flexibility (if that case occurs and we removed that parameter, payers can't do anything)...

[t-bast]

There are some unknowns that may play into this as well, like hodl invoices, so I'm a bit wary when making the API less flexible than before

[pm47]

Looking deeper into this, I don't think we are handling FinalIncorrectCltvExpiry at all in the payment lifecycle?

We have four possible values for the minFinalCltvExpiry:
1) returned in a FinalIncorrectCltvExpiry
2) manual override (what we are discussing here)
3) in the payment request
4) default suggested value in the spec (9 blocks then, 18 blocks now)

Do we agree on the order of precedence? If we handle the FinalIncorrectCltvExpiry error correctly, I'm not sure we need the manual override.

edit: I'm mistaking FinalIncorrectCltvExpiry with the now deprecated FinalExpiryTooSoon I think. If the final node has no way of telling us what the correct minFinalCltvExpiry value is, then it makes sense to be able to override the value, altough I don't see how it could be used besides testing.

edit 2: Actually yes, it is usable, provided that we define a conservative fallback value in configuration and we it optimistically, which is what this PR does.

[t-bast]

Thanks :), once lnd and c-lightning release support for setting it by default in invoices, I think we'll be able to safely remove those and simplify the code