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 <[email protected]>
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 <[email protected]>
You have to give them something which will propagate.

Signed-off-by: Rusty Russell <[email protected]>
…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 <[email protected]>
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 <[email protected]>
- 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 <[email protected]>
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
2. data:
* [`channel_id`:`channel_id`]
* [`u64`:`fee_satoshis`]
* [`u32`:`locktime`]
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems the locktime isn't really explained anywhere, might make sense to add a sentence to the rationale section?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a requirement in 1a3e5d8

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...

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