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) #1205

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

Conversation

t-bast
Copy link
Collaborator

@t-bast t-bast commented Oct 11, 2024

This PR is a continuation of #1096, that @rustyrussell asked me to take over. The original description was:

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!

I believe it is still useful to review as separate commits: however, we initially allowed setting nSequence, which we removed in favor of setting nLockTime. That part can probably be skipped. I squashed the fixup commits from the previous PR, but kept the rest.

rustyrussell and others added 10 commits October 11, 2024 10:14
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>
You have to give them something which will propagate.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…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>
This gets around "but both our outputs are dust!" problems, as
recommended by Anthony Towns.

I hope I interpreted the standardness rules correctly (technically,
you can have multiple pushes in an OP_RETURN as long as the total is
under 83 bytes, but let's keep it simple).

Add an explicit note that "OP_RETURN" is never considered "uneconomic".

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
- Make it clear why the OP_RETURN restrictions have two forms.
- Cross-reference existing dust threshold
- Lots of typo fixes
- Don't set closer_and_closee if we're larger/equal, and closee is dust.
- Remove Rationale on delete zero-output tx hack.
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>
Bitcoin Core version 25+ will not broadcast transactions containing
`OP_RETURN` outputs if their amount is greater than 0, because this
amount would then be unspendable. We thus require that the output
amount is set to 0 when using `OP_RETURN`.
We always set `nSequence` to `0xFFFFFFFD`, but each node can choose the
`nLockTime` they want to use for the transactions for which they are
paying the fees.
- add more detailed protocol flow diagram
- rename sigs TLVs as suggested by @morehouse
- mention `upfront_shutdown_script` as suggested by @Crypt-iQ
- fix typos
- reformat
@t-bast
Copy link
Collaborator Author

t-bast commented Oct 11, 2024

As described in #1096 (comment), the main question is whether we want to have stricter requirements on exchanging shutdown whenever one side sends a new one. This is probably required to ensure that we can correctly exchange nonces to produce partial signatures for taproot channels: we want to make sure we get this right, as the goal of this protocol is to be compatible with taproot channels!

@Roasbeef let me know what you think: I'm currently leaning towards your initial implementation where you must receive shutdown after sending one. If we decide on that, I'll clarify the spec!

It was previously unclear whether a node could send `shutdown` and
`closing_complete` immediately after that whenever RBF-ing their
previous closing transaction. While this worked for non-taproot
channels, it doesn't allow a clean exchange of fresh musig2 nonces
for taproot channels. We now require that whenever a node wants to
start a new signing round, `shutdown` must be sent *and* received
before sending `closing_complete`.
@t-bast
Copy link
Collaborator Author

t-bast commented Oct 22, 2024

I added the requirement to strictly exchange shutdown before sending closing_complete again in a8fd1ab

This is implemented in ACINQ/eclair#2747, waiting for lnd for cross-compatibility tests (may need to update the feature bit either on the lnd side to use 60 or on the eclair side to use 160)!

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

I'm starting to look into implementing option_simple_close for LDK. For now I just have one question and a few nits after an initial round of review.

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
09-features.md Outdated Show resolved Hide resolved
03-transactions.md Outdated Show resolved Hide resolved
Clarify strict `shutdown` exchange requirements and fix typos.
Comment on lines +1540 to +1554
| | <Bob updates his script> | |
| | | |
| | shutdown(scriptB2) | |
| |<-----------------------------| |
| | shutdown(scriptA2) | |
| |----------------------------->| |
| | closing_complete | |
| | <-------------------| |
| | | |
| | <Alice updates her script> | | (*) This is a concurrent update while Bob is sending closing_complete
| | | |
| | shutdown(scriptA3) | |
| |-------------------> | |
| | closing_complete | |
| |<------------------- | | (*) A doesn't answer with closing_sig because B's sig doesn't use scriptA3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why won't Alice just proceed with scriptA2? It's what she sent in order to respond to Bob's shutdown initiation above.

If if Alice just ignores the closing_complete, then Bob has no way of knowing why she ignored it. Alice sent scriptA2 as part of this new RBF session, so should stick with it until the sigs are changed, then afterwards she can send her new script. This simplifies the state machine IMO. Otherwise you need to handle extra transitions each time the other party sends a new shutdown while you're already prepping to sign a new coop close txn.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why won't Alice just proceed with scriptA2? It's what she sent in order to respond to Bob's shutdown initiation above.

She could, but it's more complex because that means Alice must remember the state she had when she sent shutdown(scriptA2), whereas it's simpler to drop the old state as soon as you send shutdown. Note that the spec currently doesn't forbid Alice from doing so, but she isn't forced to do it if she doesn't want to store the state from shutdown(scriptA2) after sending her shutdown(scriptA3).

If if Alice just ignores the closing_complete, then Bob has no way of knowing why she ignored it.

Yes he does: he sees that he receives shutdown after sending closing_complete (while he's waiting for closing_sig), which means Alice is starting a new signing round and may not answer to his closing_complete. It doesn't matter, because Bob will answer with shutdown and will then re-send closing_complete with the new parameters.

Alice sent scriptA2 as part of this new RBF session, so should stick with it until the sigs are changed, then afterwards she can send her new script.

That seems dangerous, because it introduces a risk of deadlock: if Bob never sends closing_complete, Alice would never be allowed to send a new shutdown?

Otherwise you need to handle extra transitions each time the other party sends a new shutdown while you're already prepping to sign a new coop close txn.

You have to implement those state transitions anyway, because there is no requirement for both nodes to re-sign with the new parameters, so at least one of the two nodes cannot know in advance if the other node will send closing_complete or not.


Overall I think that what you're suggesting is that you'd like to change the protocol to be more strict and require both nodes to re-sign their mutual close tx every time shutdown is exchanged, is that correct? That would indeed be a simpler state machine, but if any side doesn't send one of the expected messages, we're stuck and cannot send shutdown again to restart the signing session, so it feels less robust than the currently proposed protocol, where we can always restart by sending shutdown. Otherwise the only option is to disconnect, which negatively impacts the other channels you may have with that peer.

I don't know if I'm being too careful about avoiding deadlocks here, but I like having the property that we can restart the state machine without disconnecting...

Copy link
Contributor

Choose a reason for hiding this comment

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

but it's more complex because that means Alice must remember the state she had when she sent shutdown(scriptA2), whereas it's simpler to drop the old state as soon as you send shutdown.

Yeah this does look cleaner.

That seems dangerous, because it introduces a risk of deadlock: if Bob never sends closing_complete, Alice would never be allowed to send a new shutdown?

Could you clarity a bit? What prevents Alice from sending a new shutdown given that node can now send multiple shutdown msgs?

02-peer-protocol.md Show resolved Hide resolved
- If the signature field is non-compliant with LOW-S-standard rule<sup>[LOWS](https://github.com/bitcoin/bitcoin/pull/6769)</sup>:
- MUST either send a `warning` and close the connection, or send an `error` and fail the channel.
- MUST sign and broadcast the corresponding closing transaction.
- MUST send `closing_sig` with a single valid signature in the same TLV field as the `closing_complete`.
Copy link
Contributor

Choose a reason for hiding this comment

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

The sender of closing_sig needs to make sure the received closing_complete.locktime is equal to the closing_complete.locktime sent.

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 you may be misunderstanding something about the proposal here: both nodes send closing_complete independently, with whatever value they choose for locktime, which don't have to match. The only thing that needs to be done is that when receiving closing_complete, a node must use the received locktime and fee_satoshis to create the closing transaction that they sign, for which they send closing_sig.

02-peer-protocol.md Show resolved Hide resolved
- MAY send `closing_complete` afterwards.

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the sender MUST set closee_output_only if the local output amount is dust when the local outstanding balance is less than the remote outstanding balance, I interpret it as,

  • the local node is the closer and,
  • we allow the local balance to be dust or zero

Then this would make the fee_satoshis here either dust or zero because we require it to be <= local balance?

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 don't understand what the issue is here. You can never set fee_satoshis to something greater than your balance (because otherwise you simply cannot pay those fees). That's the only thing that this requirement says.

If, after deducing the fee_satoshis, the remaining value of your output is too small, then you're allowed to omit this output, which means the closing transaction will only have an output for the closee (closee_output_only).

Copy link
Contributor

@yyforyongyu yyforyongyu Dec 16, 2024

Choose a reason for hiding this comment

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

I guess it's easier to look at this question - if I don't have local balance, am I allowed to start a shutdown session to kick off the coop close flow?

Based on this requirement here, since I am required to set the fee_satoshi to be no greater than my local balance in my closing_complete, it means the value I choose here is 0 and set the closee_output_only?

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 guess it's easier to look at this question - if I don't have local balance, am I allowed to start a shutdown session to kick off the coop close flow?

I guess that what you mean by not having a local balance is having a channel balance of 0 sat, correct? Which by the way can only happen on a new channel opened to you or when using 0-reserve.

If that's the case then no, you don't have any funds in that channel, so it doesn't make any sense for you to create a closing transaction, you have nothing at stake that you'd like to get back. So you will not send closing_complete. Your peer will send closing_complete though because they have funds in the channel. When they do, you will respond with closing_sig to let them get their funds back.

If by "I don't have a local balance" you mean that you have some funds in the channel, but they are trimmed (e.g. 200 sats), then you're allowed to create a closing transaction where you put all of your funds to mining fees, and in that case you are only allowed to include your peer's output, hence the use of closee_output_only.


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.
Copy link
Contributor

Choose a reason for hiding this comment

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

As a receiver of fee_satoshi I'll send an error or warning if the specified value is below the min relay fee.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, you don't validate what your peer chooses for their closing transaction. It's their choice, they're paying the fees, they can do whatever they want even if it doesn't seem sane to you. You will use what you consider sane values in your closing transaction, when you send closing_complete.

| | closing_complete | |
| |<-----------------------------| |
| | closing_sig | |
| |<-----------------------------| |
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need for Bob to send the closing_sig since he just takes over?

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 you're misunderstanding how this works, I believe you think that a single closing transaction is created (like the previous mutual close protocol). In this protocol, two independent closing transaction are created: one for each node. Each node independently sends closing_complete with the parameters they want for their closing transaction, and the receiver just signs it.

@@ -1508,23 +1509,68 @@ Closing happens in two stages:
2. once all HTLCs are resolved, the final channel close negotiation begins.

+-------+ +-------+
| |--(1)----- shutdown ------->| |
| |<-(2)----- shutdown --------| |
| | shutdown(scriptA1) | |
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice diagram! And I think it may be more helpful if we could break it down case by case, also addresses the comment here, to make it clear that,

  1. a pair of closing_complete and closing_sig completes the flow, and,
  2. whoever sends the closing_complete pays the fees.

In a hindsight it probably makes more sense to name closing_complete closing_params and closing_sig closing_complete, well, but the code is written so nvm.

Copy link
Contributor

@yyforyongyu yyforyongyu Dec 16, 2024

Choose a reason for hiding this comment

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

During normal flow, once the shutdown scripts are exchanged via shutdown messages, Alice sends a closing_complete to specify the fees she is willing to pay, and Bob replies with a closing_sig to complete Alice's closing transaction. Bob also sends a closing_complete to express his fee preference, and Alice replies with a closing_sig to provide signature for Bob's closing transaction.

 +-------+                              +-------+
 |       | shutdown(scriptA1)           |       |
 |       |----------------------------->|       |
 |       |           shutdown(scriptB1) |       |
 |       |<-----------------------------|       |
 |       |                              |       |
 |       | <complete all pending HTLCs> |       |
 |   A   |             ....             |   B   |
 |       |                              |       |
 |       | closing_complete             |       |
 |       |----------------------------->|       |
 |       |             closing_complete |       |
 |       |<-----------------------------|       |
 |       |                  closing_sig |       |
 |       |<-----------------------------|       |
 |       | closing_sig                  |       |
 |       |----------------------------->|       |
 +-------+                              +-------+

It's allowed to specify shutdown scripts multiple times.

+-------+                              +-------+
|       | shutdown(scriptA1)           |       |
|       |----------------------------->|       |
|       |           shutdown(scriptB1) |       |
|       |<-----------------------------|       |
|       |                              |       |
|       | <complete all pending HTLCs> |       |
|   A   |             ....             |   B   |
|       |                              |       |
|       |   <A updates their script>   |       |
|       |                              |       |
|       | shutdown(scriptA2)           |       |
|       |----------------------------->|       |
|       |           shutdown(scriptB1) |       | (*) Bob doesn't update his script
|       |<-----------------------------|       |
|       | closing_complete             |       |
|       |----------------------------->|       |
|       |             closing_complete |       |
|       |<-----------------------------|       |
|       |                  closing_sig |       |
|       |<-----------------------------|       |
|       | closing_sig                  |       |
|       |----------------------------->|       |
+-------+                              +-------+

It's possible to encounter a race condition when updating the shutdown script. This is handled by always sending an updated closing_complete with the latest shutdown script used.

+-------+                              +-------+
|       | shutdown(scriptA1)           |       |
|       |----------------------------->|       |
|       |           shutdown(scriptB1) |       |
|       |<-----------------------------|       |
|       |                              |       |
|       | <complete all pending HTLCs> |       |
|   A   |             ....             |   B   |
|       |                              |       |
|       |   <Bob updates his script>   |       |
|       |                              |       |
|       |           shutdown(scriptB2) |       |
|       |<-----------------------------|       |
|       | shutdown(scriptA2)           |       |
|       |----------------------------->|       |
|       |             closing_complete |       |
|       |          <-------------------|       |
|       |                              |       |
|       |  <Alice updates her script>  |       | (*) This is a concurrent update while Bob is sending closing_complete
|       |                              |       |
|       | shutdown(scriptA3)           |       |
|       |------------------->          |       |
|       |   closing_complete           |       |
|       |<-------------------          |       | (*) A doesn't answer with closing_sig because B's sig doesn't use scriptA3
|       |           shutdown(scriptA3) |       |
|       |          ------------------->|       |
|       |           shutdown(scriptB2) |       |
|       |<-----------------------------|       |
|       | closing_complete             |       |
|       |----------------------------->|       | (*) A now uses scriptB2 and scriptA3 for closing_complete 
|       |             closing_complete |       |
|       |<-----------------------------|       | (*) B now uses scriptB2 and scriptA3 for closing_complete 
|       | closing_sig                  |       |
|       |----------------------------->|       |
|       |                  closing_sig |       |
|       |<-----------------------------|       |
+-------+                              +-------+

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See my answers to your other comments to clarify some misunderstandings. There is one open comment that we will discuss during today's spec meeting that may affect the whole flow: #1205 (comment)

Let's see after this discussion is resolved if you still have open issues with the resulting protocol.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I did realize it's a dual flow - updated the diagram and description, could you verify if it's correct?

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