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

option_simple_close (features 60/61) #1096

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

rustyrussell
Copy link
Collaborator

This is a "can't fail!" close protocol, as discussed at the NY Summit, and on @Roasbeef's wishlist. It's about as simple as I could make it: the only complexity comes from allowing each side to indicate whether they want to omit their own output.

It's "taproot ready"(TM) in the sense that shutdown is always sent to trigger it, so that can contain the nonces without any persistence requirement.

I split it into three commits for cleanliness:

  1. Introduce the new protocol
  2. Remove the requirement that shutdown not be sent multiple times (which was already nonsensical)
  3. Remove the older protocols

I recommend reviewing it as separate commits, it'll make more sense!

Pay your own fees, so the peer will sign without caring.  Even if it doesn't relay.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The shutdown section says:

```
  - MUST NOT send multiple `shutdown` messages.
```

But the reconnection section says:

```
  - upon reconnection:
    - if it has sent a previous `shutdown`:
      - MUST retransmit `shutdown`.
```

So clearly, remove the former.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@ProofOfKeags
Copy link
Contributor

Perhaps this is a misunderstanding on my part for how the document is organized, but wouldn't you still have to support the original closing_signed message in the main BOLT docs here? I understand that maybe you can enforce the new simple close protocol gets used if the channel was opened or upgraded to option_simple_close but the way I'm reading this is that it is deleting the old close method from the protocol completely and I'm pretty sure we can't do that unless this is an extension BOLT right?

Otherwise I tend to like this. The game that has been identified where the closer of the channel pays the fee has the side effect of keeping channels open longer which I believe to be a good thing.

ACK up to aa6deb8

@rustyrussell
Copy link
Collaborator Author

There was an expressed desire in NY to more agressively clean up deprecated parts of the spec, even if they're still in use. It keeps the document cleaner, and avoid having to revisit later (when?) to remove the older parts. However, it's still in the git history.

@ProofOfKeags
Copy link
Contributor

ProofOfKeags commented Jul 21, 2023

Yeah I understand the desire to remove deprecated stuff. I just wonder what a "valid" implementation looks like here. If we implement this change, and essentially drop support for the old closing scheme from the spec, doesn't that mean that option_simple_close isn't really an option for a new impl? If it is an option but we remove the old closing scheme, doesn't that mean that there is no way to do a coop close anymore?

Maybe this is more of a meta-question of who the main BOLTs are for. My naive (I'm new here!) perspective is that the BOLTs should be the document I should consult if I wanted to bootstrap a new LN impl and that as long as I implement everything in the main BOLTs without any of the optional stuff or extensions then I should be broadly compatible with the network. Through this lense, it would seem that this change would result in the removal of cooperative closes from the main specification. Is my understanding of the purpose of the spec wrong? If not, are we OK with removing coop closes from the table-stakes features of the lightning network?

@TheBlueMatt
Copy link
Collaborator

TheBlueMatt commented Jul 24, 2023

I'm not really sure who would be served by keeping the old stuff around - if someone is using the BOLTs to implement a fresh Lightning client, by the time they're done doing so (which probably takes a year!) hopefully everyone supports the new features and there's no need for them to even be aware the old crap was there. If the BOLTs exist purely for the engineers working on the existing implementations, we already have all the old code, making us read the old stuff while implementing the new stuff is a lot more confusing than just writing out the new stuff IME. I'm not sure who else the BOLTs "should be for".

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.

Huge concept ACK, I'll really enjoy removing the old-style mutual close from eclair 😁

02-peer-protocol.md Outdated Show resolved Hide resolved
02-peer-protocol.md Outdated Show resolved Hide resolved
02-peer-protocol.md Outdated Show resolved Hide resolved
02-peer-protocol.md Outdated Show resolved Hide resolved
09-features.md Show resolved Hide resolved
02-peer-protocol.md Outdated Show resolved Hide resolved
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

LGTM.

My two cents on what we should keep inside the BOLTs.

I'd like to suggest cleaning up the feature that is somewhat outdated because having an outdated ways to do the same operation in the BOLTs can make it more challenging to read and understand what is the recent version.

If someone is writing a lightning node from scratch (and I promise that I will do it someday), they should start reading from the current state of the BOLT.

Of course, there is the concern of what happens if the BOLTs change while I am implementing the node? I think we all assume that an implementor will stay updated on the development of the spec (at least we hope).


Similarly, if the closer proposes a high fee, it doesn't harm the closee to sign the transaction, as the closer is paying.

There's a slight game where each side would prefer the other side pay the fee, and proposes a minimal fee. If neither side proposes a fee which will relay, the negotiation can occur again, or the final commitment transaction can be spent. In practice, the opener has an incentive to offer a reasonable closing fee, as they would pay the fee for the commitment transaction, which also costs more to spend.

Choose a reason for hiding this comment

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

This discussion of the game theory hints at an issue I've always had with the "opener pays" rule. I do understand the pathological scenario that the rule is meant to avoid (where a malicious node can drain the funds of a victim node by repeatedly opening and abandoning channels to it), but this new proposal creates a perverse incentive.

Olav opens a channel to Kristján. Some time later, Kristján wants the channel closed. Under this new proposal, Kristján's fee-optimal channel-closing strategy would be to transmit a bogus error message to Olav so that Olav will be forced (by protocol convention) to unilaterally close the channel. Thusly, Kristján would obtain an immediately spendable output and would avoid paying any fees.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's always true.

But K can also mutual close at <1 sat per byte.

Choose a reason for hiding this comment

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

How can any mutual closing transaction use <1 sat/vB fee rate? The transaction would not propagate.

And you're right that it's always true. It has always irked me. I swear I have seen some cases where nodes to which I have opened channels have sent me error messages for no reason other than to get me to unilaterally close. I think it was a design mistake to have commitment transactions reward the non-closing party with an immediately spendable output. I think commitment transactions should time-lock both outputs to discourage baiting a peer into unilaterally closing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That will disappear once we have package relay, because at that point commitment transactions will pay no fees. The fees will have to be paid using CPFP on an ephemeral anchor. Also, if you send bogus errors, nodes will start blacklisting you at some point and won't accept new channels from you: this isn't the case today, but will definitely be in the future.

Choose a reason for hiding this comment

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

The opener should at least be on the hook for whatever the commitment fee is. Otherwise, as the non-opener, it's cheaper to FC and wait out the time lock. Also, if there's nothing on the non-opener side, is FC the only options since there's nothing on that side to pay the fee.

Copy link
Contributor

Choose a reason for hiding this comment

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

I want to challenge this idea of "it's cheaper to FC and wait out the time lock". Having money locked up carries opportunity cost of the entire output. Yes you may save in fees, but your outputs are tied up in chain resolution and that is a cost in its own right.

Also, if there's nothing on the non-opener side, is FC the only options since there's nothing on that side to pay the fee.

Why would a node be concerned about aggressively closing a channel that has no capital tied up in it?

Choose a reason for hiding this comment

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

Current coop close is ~168 vB (2 x P2WPKH outputs) to ~192 vB (2 x P2TR outputs). Sweeping the timelock is 121-123 vB.. so ~35-50% smaller tx. I agree with the time cost, but if you are closing due to months of inactivity, what's another up to two weeks. But the opener would be doubly screwed since they would pay the FC (+ anchors) and then have to pay to sweep. I supposed the opener can "deter" this by keeping max commit fees low.

Closing channels should be discouraged, but having dead/inactive channel is worse.

In my mind, game theory would have me coop close at a very low fee if the balance is mostly on the other side or FC if it's mostly on my side and I'm not the opener and commit fee is high enough.

Why would a node be concerned about aggressively closing a channel that has no capital tied up in it?

More of a hypothetical.. how does a closer pay if there's little to nothing on their side.

I agree a change to channel closing is needed, but 100% opener to 100% closer is a pretty drastic change. I would like to see the opener using (some of) the commit fee already reserved and/or splitting the fee based on balance.

02-peer-protocol.md Show resolved Hide resolved
02-peer-protocol.md Outdated Show resolved Hide resolved
02-peer-protocol.md Outdated Show resolved Hide resolved
02-peer-protocol.md Outdated Show resolved Hide resolved
02-peer-protocol.md Outdated Show resolved Hide resolved
02-peer-protocol.md Outdated Show resolved Hide resolved
02-peer-protocol.md Outdated Show resolved Hide resolved
02-peer-protocol.md Outdated Show resolved Hide resolved
02-peer-protocol.md Show resolved Hide resolved
02-peer-protocol.md Outdated Show resolved Hide resolved
* Adds both directions to the diagram.
* Rename closee_output to has_closee_output.
* Refer explicitly to BOLT 3 for "null output" defn.
* Pad the OP_RETURN to make 65 bytes.
You have to give them something which will propagate.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Copy link

@morehouse morehouse left a comment

Choose a reason for hiding this comment

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

Concept ACK

02-peer-protocol.md Outdated Show resolved Hide resolved
02-peer-protocol.md Outdated Show resolved Hide resolved
02-peer-protocol.md Outdated Show resolved Hide resolved
| |--(3a)- closing_complete Fee->| |
| |<-(3b)- closing_complete Fee--| |
| |<-(4a)- closing_sig ----------| |
| |--(4b)- closing_sig --------->| |

Choose a reason for hiding this comment

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

This doesn't seem like the typical use case where one side initiates the close and the other signs. Some explanation somewhere is needed:

  • Is the idea here that both sides can contribute to the closing fee if they want?
  • Does B's closing_complete override A's? And in that case, why does B send closing_sig?
  • Did both A and B send closing_complete at the same time, so we have 2 flows happening at once?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is one flow happening in each direction, where the sender of closing_complete is paying the fee.
Let me know if the proposed diagram here makes it easier to reason about.

Choose a reason for hiding this comment

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

There is one flow happening in each direction, where the sender of closing_complete is paying the fee.

Do we really expect dual flows to happen like that in practice? If the other party offers to pay for the closing transaction, why would we want to send closing_complete at all?

Let me know if the proposed diagram here makes it easier to reason about.

Your diagram helps to clarify corner cases but probably still needs some high-level explanation in the spec.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really expect dual flows to happen like that in practice?

I think we do, because each side should independently decide what fee they'd like to pay and immediately send closing_complete (at least that's how I implemented it). Since the channel has already stopped relaying HTLCs at that point, both peers are likely eager to get their funds back asap to reinvest them into another channel.

#### Rationale
Multiple signatures are given, so each side can choose whether it wants to include its own output. In the case of large fees and tiny channels, where neither side wants its output, the use of an OP_RETURN simply cleans up the dangling unspent transaction output. This is expected to be extremely rare.

Note that there is usually no reason to pay a high fee for rapid processing, since an urgent child could pay the fee on the closing transactions' behalf.

Choose a reason for hiding this comment

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

We could also mention RBF of the closing transaction here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I fail to understand how we could ever have a transaction be urgent enough to push for confirmation if it involved burning all of the outputs in an OP_RETURN.

03-transactions.md Outdated Show resolved Hide resolved
03-transactions.md Outdated Show resolved Hide resolved
@rustyrussell
Copy link
Collaborator Author

Agreed at meeting that we should simply insist the larger output (or lower key as tiebreak) MUST have an output. I guess if both are dust we cannot close, then.

@ajtowns
Copy link

ajtowns commented Aug 17, 2023

Agreed at meeting that we should simply insist the larger output (or lower key as tiebreak) MUST have an output. I guess if both are dust we cannot close, then.

You could still do this in the implementation potentially -- allow OP_RETURN spks in shutdown(), require setting has_closee_output to false in closing_sig if you chose an OP_RETURN spk (and allow signature_with_closee_output to be invalid?). If you choose OP_RETURN and the other doesn't, wait for them to initiate the close; if you both choose OP_RETURN, wait a random amount of time, and send closing_complete with fee_satoshis equal to your entire balance.

(If you did this, could you move the "has_closee_output" decision to be whether or not the closee set the sPK to OP_RETURN[something]? If you change your mind, you can always just send shutdown again, aiui? Must set the sPK to OP_RETURN if balance is dust or below?)

In general, though, for a sufficiently small channel, you can't guarantee closure: a 100k sat channel closed to a single bare OP_RETURN via a musig2 signature would give you a fee rate of about 1300sat/vb; if miners have a backlog of txs above that fee rate (ie, making >13BTC per block in fees), your close tx won't confirm.

@rustyrussell rustyrussell changed the title option_simple_close option_simple_close (features 60/61) Aug 30, 2023
@rustyrussell
Copy link
Collaborator Author

rustyrussell commented Sep 13, 2023

EDIT: This doesn't work well, since you don't know fee yet, you don't know whether you should discard: IGNORE

Maybe this whole situation is simpler if we move "do I want an output?" negotiation to shutdown. This means we allow two new forms of scriptpubkey:

  1. An empty scriptpubkey to indicate "I don't want my output at all".
  2. OP_RETURN <6+ bytes of padding>.

And now, you SHOULD only set an empty scriptpubkey if you have the lesser output (if both are equal, neither can). Note that the initial shutdown can happen while htlcs are still unsettled, but this will work itself out if you re-xmit shutdown afterwards, and have a rule not to send closing_complete if both sides opt for no output.

…output.

If both are dust, you should lowball fees.  The next patch adds OP_RETURN
as a valid shutdown scriptpubkey though if you really want to do this.

This also addresses the case where people send a new `shutdown` with a *different* scriptpubkey.  This could previously cause a race where you receive a bad signature (because it hasn't received the updated shutdown), so we ignore these cases.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
Copy link
Collaborator Author

OK, this time for sure!

We only let one side (the "lesser in msats", if any) discard their output, so we always have one. We make the signature fields a tlv, so we can simply share them across the two messages, and carefully dictate which one to use.

@ajtowns
Copy link

ajtowns commented Sep 18, 2023

(Avoid @'s in commit msgs to reduce github notification spam? cf https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md )

03-transactions.md Outdated Show resolved Hide resolved
02-peer-protocol.md Show resolved Hide resolved
02-peer-protocol.md Outdated Show resolved Hide resolved
The sender of `closing_complete` (aka. "the closer"):
- MUST set `fee_satoshis` to a fee less than or equal to its outstanding balance, rounded down to whole satoshis.
- MUST set `fee_satoshis` so that at least one output is not dust.
- MUST use the last send and received `shutdown` `scriptpubkey` to generate the closing transaction specified in [BOLT #3](03-transactions.md#closing-transaction).
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

Suggested change
- MUST use the last send and received `shutdown` `scriptpubkey` to generate the closing transaction specified in [BOLT #3](03-transactions.md#closing-transaction).
- MUST use the last sent and received `shutdown` `scriptpubkey` to generate the closing transaction specified in [BOLT #3](03-transactions.md#closing-transaction).

02-peer-protocol.md Outdated Show resolved Hide resolved
02-peer-protocol.md Show resolved Hide resolved
02-peer-protocol.md Outdated Show resolved Hide resolved
- otherwise:
- MUST propose a value "strictly between" the received `fee_satoshis`
and its previously-sent `fee_satoshis`.
- MUST sign and broadcast the corrsponding closing transaction.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- MUST sign and broadcast the corrsponding closing transaction.
- MUST sign and broadcast the corresponding closing transaction.

02-peer-protocol.md Outdated Show resolved Hide resolved
02-peer-protocol.md Outdated Show resolved Hide resolved
We don't care, as long as it's RBF-able.  This will be nicer for
Taproot when mutual closes are otherwise indistinguishable from normal
spends.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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.

LGTM, there are still a few typos: I left my previous comments for them unresolved. I think we should add lockTime to closing_complete, apart from that it's looking good.

2. data:
* [`channel_id`:`channel_id`]
* [`u64`:`fee_satoshis`]
* [`u32`:`sequence`]
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about also including lockTime like we do in open_channel2?

Comment on lines +875 to +878
- otherwise, if `closer_and_closee` is present:
- MUST use `closer_and_closee`.
- otherwise:
- MUST use `no_closer_closee`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

That is the gentleman's approach, but that isn't the most efficient one for the closee. If the closee receives a proposal that contains no_closer_closee, that is the option that pays the most fees, so the closee has an incentive to choose this one instead of closer_and_closee?

In practice though, if the sender sends no_closer_closee, they won't send closer_and_closee, so those cases are only theoretical?

Copy link
Collaborator

Choose a reason for hiding this comment

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

How can the sender even send no_closer_closee? IIUC, the side sending the closing_complete needs to pay for fees, so if they remove their output (it all goes to fees), then wouldn't they be better off just not sending the sig at all?

Comment on lines +865 to +867
The receiver of `closing_complete` (aka. "the closee"):
- If `fee_satoshis` is greater than the closer's outstanding balance:
- MUST either send a `warning` and close the connection, or send an `error` and fail the channel.
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth noting here that this introduces an edge case if we decide to rip out the original coop-close protocol. This would make it such that if you needed to coop-close the channel as the target of a channel-open that you would have no choice but to force close it. I think that everyone loses in this scenario.

Suggested change
The receiver of `closing_complete` (aka. "the closee"):
- If `fee_satoshis` is greater than the closer's outstanding balance:
- MUST either send a `warning` and close the connection, or send an `error` and fail the channel.
The receiver of `closing_complete` (aka. "the closee"):
- If `fee_satoshis` is greater than the closer's outstanding balance:
- MAY either send a `warning` and close the connection, or send an `error` and fail the channel.
- MAY choose to cover the deficit from their own output.

This way, if the closer tries to grief you by suggesting an insane fee, you can reject it, but otherwise if it's a fee you'd be willing to pay and it beats your fee liability if you have to force close, you come out ahead. Note that the only realistic scenario where this happens is if you just had a channel opened to you, received no inbound satsflow on the channel but you've decided to close the channel for whatever reason. In this case you may still want to be a good actor and coop-close the channel, but this construction (along with the removal of the existing coop-close) would force you to be more aggressive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Withdrawn. I misunderstood some of the other aspects of this protocol.

- MUST set `fee_satoshis` so that at least one output is not dust.
- MUST set `sequence` to a value other than 0xFFFFFFFF.
- MUST use the last send and received `shutdown` `scriptpubkey` to generate the closing transaction specified in [BOLT #3](03-transactions.md#closing-transaction).
- If it sets `signature` fields, MUST set them as valid signature using its `funding_pubkey` of:
Copy link
Collaborator

Choose a reason for hiding this comment

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

In what case wouldn't the signature fields be set?

Also why are three versions needed at all? If by the time we decide to co-op close, I'm the only one that has a non-dust output, then why would I ever send a signature with my output removed?

- MUST NOT set `closer_no_closee`.
- MUST set exactly one of `no_closer_closee` or `closer_and_closee`.
- MUST set `no_closer_closee` if the local output amount is dust.
- MAY set `no_closer_closee` if it considers the local output amount uneconomic AND its `scriptpubkey` is not `OP_RETURN`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok, reading further, this makes more sense: before we would sort of implicitly decide to remove outputs based on dust values (had some prior ambiguity there w.r.t which dust values were used), but now we make all the cases explicit!

- `no_closer_closee`: closing transaction with only the remote ("closee") output.
- `closer_and_closee`: closing transaction with both the closer and closee outputs.
- If the local outstanding balance (in millisatoshi) is less than the remote outstanding balance:
- MUST NOT set `closer_no_closee`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having trouble understanding the requirement above (local balance < remote balance): shouldn't this instead state that as along as the remote party has a non-dust output, then closer_no_closee should never be set?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or is this just saying that they shouldn't burn the closee's settled output as fees to miners?

@@ -618,6 +632,10 @@ A sending node:
3. if (and only if) `option_shutdown_anysegwit` is negotiated:
* `OP_1` through `OP_16` inclusive, followed by a single push of 2 to 40 bytes
(witness program versions 1 through 16)
4. if (and only if) `option_simple_close` is negotiated:
* `OP_RETURN` followed by one of:
Copy link
Collaborator

Choose a reason for hiding this comment

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

As of bitcoind 25, when submitted via the sendrawtransaction API (which I assume we all use), the value of an OP_RETURN must be zero unless a new param is set: https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-25.0.md#updated-rpcs

sendrawtransaction has a new, optional argument, maxburnamount with a default value of 0. Any transaction containing an unspendable output with a value greater than maxburnamount will not be submitted. At present, the outputs deemed unspendable are those with scripts that begin with an OP_RETURN code (known as 'datacarriers'), scripts that exceed the maximum script size, and scripts that contain invalid opcodes.

Perhaps we should also recommend that if an OP_RETURN is used as the closing addr, then the output of the sender should always be zero. Otherwise, this could prevent a variant of the co-op close transaction from propagating.

Comment on lines +875 to +878
- otherwise, if `closer_and_closee` is present:
- MUST use `closer_and_closee`.
- otherwise:
- MUST use `no_closer_closee`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

How can the sender even send no_closer_closee? IIUC, the side sending the closing_complete needs to pay for fees, so if they remove their output (it all goes to fees), then wouldn't they be better off just not sending the sig at all?

Comment on lines +818 to +826
1. type: 1 (`closer_no_closee`)
2. data:
* [`signature`:`sig`]
1. type: 2 (`no_closer_closee`)
2. data:
* [`signature`:`sig`]
1. type: 3 (`closer_and_closee`)
2. data:
* [`signature`:`sig`]

Choose a reason for hiding this comment

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

I find it easy to confuse these types with the current naming.

Can we make the names more precise? Something like:

Suggested change
1. type: 1 (`closer_no_closee`)
2. data:
* [`signature`:`sig`]
1. type: 2 (`no_closer_closee`)
2. data:
* [`signature`:`sig`]
1. type: 3 (`closer_and_closee`)
2. data:
* [`signature`:`sig`]
1. type: 1 (`closer_output_only`)
2. data:
* [`signature`:`sig`]
1. type: 2 (`closee_output_only`)
2. data:
* [`signature`:`sig`]
1. type: 3 (`closer_and_closee_outputs`)
2. data:
* [`signature`:`sig`]

| |--(3a)- closing_complete Fee->| |
| |<-(3b)- closing_complete Fee--| |
| |<-(4a)- closing_sig ----------| |
| |--(4b)- closing_sig --------->| |

Choose a reason for hiding this comment

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

There is one flow happening in each direction, where the sender of closing_complete is paying the fee.

Do we really expect dual flows to happen like that in practice? If the other party offers to pay for the closing transaction, why would we want to send closing_complete at all?

Let me know if the proposed diagram here makes it easier to reason about.

Your diagram helps to clarify corner cases but probably still needs some high-level explanation in the spec.

@Roasbeef
Copy link
Collaborator

Roasbeef commented Mar 8, 2024

So I'm finally ready for interop here @t-bast!

One thing that came up during my last debugging session was the fact that we allow the Sequence field to now be set to anything by either side.

I'm not sure how y'all handle this in other implementations, but in lnd we use the sequence to decide if a funding output spend is either a: breach, local force close, remote force close, coop close, or an unknown state (SCB flow).

Before this new flow, the sequence was always set to the max possible sequence, which also disabled the RBF (which was want to enable) and also sequence locks semantics. However, if we allow a peer to set an arbitrary sequence, then they can set a sequence that looks like a past breached state (due to extraction of the state hint) or even a future unknown state. So I ended up modifying our implementation to only set/allow a sequence of mempool.MaxRBFSequence (0xfffffffd). This is the largest sequence that still allows opting into the RBF rules. With this change, we're now able to deterministically detect type of spend we see on chain based on the sequence.

IMO, there's no good reason to set your sequence to anything other than 0xfffffffd for the co-op close transaction. Activating the BIP 112 semantics isn't very useful here as why would you want to delay your co-op close with a relative time lock? If we allow peers to set arbitrary values, then this also weakens the anonymity set of a coop close from a taproot channel as you'll stand out if you use anything other than 0xfffffffd.

So what do y'all think about dropping the sequence field from the new messages? We'd then always restrict it to be 0xfffffffd, rejecting a sig if we get any other value.

@t-bast
Copy link
Collaborator

t-bast commented Mar 8, 2024

So I'm finally ready for interop here @t-bast!

🥳

I'm not sure how y'all handle this in other implementations, but in lnd we use the sequence to decide if a funding output spend is either a: breach, local force close, remote force close, coop close, or an unknown state (SCB flow).

We don't rely too much on the nSequence field for that in eclair. When a transaction spends the funding transaction, we match it by txid against:

  • splice transactions we signed
  • the current local or remote commit
  • the next remote commit if we sent commit_sig and didn't receive revoke_and_ack
  • any mutual close transaction we signed
  • otherwise, we check if it's a revoked commit by checking nSequence and nLockTime and the revocation secrets we have
  • if that doesn't match, it's an unknown state (we lost data)

You always sign mutual close transactions, so you can remember the corresponding txids and shouldn't need to rely on the nSequence field for that?

With this change, we're now able to deterministically detect type of spend we see on chain based on the sequence.

I think this is dangerous to rely on nSequence only for that, it's much safer to rely on txids IMO.

IMO, there's no good reason to set your sequence to anything other than 0xfffffffd for the co-op close transaction.

Why restrict it? There are a lot of discussions of leveraging nSequence for future bitcoin capabilities, so we should keep that door open and allow anything IMO. This is the mutual close transaction paid for by our peer, so we should let them use whatever they want?

If we allow peers to set arbitrary values, then this also weakens the anonymity set of a coop close from a taproot channel

I don't think this argument really holds: if your peer wants to do something funny that stands out, they have many other ways to do this anyway, restricting the nSequence of the mutual close tx is not going to fix that.

@Roasbeef
Copy link
Collaborator

Roasbeef commented Mar 9, 2024

I think this is dangerous to rely on nSequence only for that, it's much safer to rely on txids IMO.

We don't only rely on it, but you might not always have the txid (eg: restore from seed, no state, partial data loss, etc). The sequence effectively serves as an authenticated sequence number since only in collaboration with the other party can a new one be signed. All implementations also must use the sequence to detect which date was broadcast assuming they aren't storing the txid of every commitment txn ever signed.

In the past, we also based things purely on txid, but discovered some edge cases, causing us to re-write that logic entirely all together.

we match it by txid against any mutual close transaction we signed
You always sign mutual close transactions, so you can remember the corresponding txids and shouldn't need to rely on the nSequence field for that?

How would you then handle the case of partial or complete data loss, but before that you signed a co-op close transaction, and that hit the chain?

With the new protocol, do y'all also store very RBF'd close created by either party? Right now we only remember the very last one, as with the new flow, the other party can just continue to sign effectively useless updates to make the peer retain more state (soft DoS).

Why restrict it? There are a lot of discussions of leveraging nSequence for future bitcoin capabilities, so we should keep that door open and allow anything IMO. This is the mutual close transaction paid for by our peer, so we should let them use whatever they want?

Can think of two reasons not to:

  1. You're spending the funding output for a co-op close, meaning you want to exit the channel, so why use a relative time lock?
  2. If every implementation picks its own value, then it's easier to finger print. Today everyone uses the max sequence for co-op close. In a post taproot chans everywhere world, using a standard value can help co-op closes blend in with the broader transaction anonymity set.
  3. One less free parameter.

Any future protocol updates that repurpose the sequence for consensus rules would need to bump the transaction version, just like we did originally for the CSV package. If we follow the line of reasoning for allowing a custom sequence, then should we also allow a custom txn version?

@t-bast
Copy link
Collaborator

t-bast commented Mar 11, 2024

How would you then handle the case of partial or complete data loss, but before that you signed a co-op close transaction, and that hit the chain?

It simply falls into the "unknown transaction" case, where you cannot do anything and just let it confirm (which sends funds back to your wallet). I believe there is nothing to do in that case and it is harmless?

With the new protocol, do y'all also store very RBF'd close created by either party?

Yes, we store the txid of every closing candidate, as it's quite useful for monitoring (it's also a good way to see how well RBF works in practice).

Right now we only remember the very last one, as with the new flow, the other party can just continue to sign effectively useless updates to make the peer retain more state (soft DoS).

Since they have to increase the fees every time, that doesn't sound like a dangerous DoS (especially since you only need to store 32 bytes per attempt).

Can think of two reasons not to:

Those are reasonable reasons, I don't feel very strongly either way (especially since right now, we'll always set the sequence to 0xfffffffd). Let's discuss that during today's spec meeting!

@t-bast
Copy link
Collaborator

t-bast commented Mar 12, 2024

This was discussed during yesterday's spec meeting, and there was rough consensus that the sequence should always be set to 0xfffffffd. Then I believe we should update the closing_signed message to be:

1. type: 40 (`closing_complete`)
2. data:
   * [`channel_id`:`channel_id`]
   * [`u64`:`fee_satoshis`]
   * [`u32`:`locktime`]

Since we want the ability to set the nLockTime for anti-fee sniping. We can add the ability to set a different nSequence in the future with a new TLV and a feature bit, if it becomes necessary.

@rustyrussell @Roasbeef does that sound good to you?

@rustyrussell
Copy link
Collaborator Author

This was discussed during yesterday's spec meeting, and there was rough consensus that the sequence should always be set to 0xfffffffd. Then I believe we should update the closing_signed message to be:

1. type: 40 (`closing_complete`)
2. data:
   * [`channel_id`:`channel_id`]
   * [`u64`:`fee_satoshis`]
   * [`u32`:`locktime`]

Since we want the ability to set the nLockTime for anti-fee sniping. We can add the ability to set a different nSequence in the future with a new TLV and a feature bit, if it becomes necessary.

@rustyrussell @Roasbeef does that sound good to you?

Yes, this looks good...


Note that there is usually no reason to pay a high fee for rapid processing, since an urgent child could pay the fee on the closing transactions' behalf.

However, sending a new `shutdown` message overrides previous ones, so you can negotiate again (even changing the output address) if you want: in this case there's a race where you could receive a `closing_complete` for the previous output address, and the signature won't validate. In this case, ignoring the `closing_complete` is the correct behaviour, as the new `shutdown` will trigger a new `closing_complete` with the correct signature. This assumption that we only remember the last-sent of any message is why so many cases of bad signatures are simply ignored.
Copy link
Contributor

Choose a reason for hiding this comment

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

so you can negotiate again (even changing the output address) if you want

doesn't this override the safety feature of upfront shutdown?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is of course if you don't have upfront_shutdown activated, we can make that clearer.

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.

None yet