-
Notifications
You must be signed in to change notification settings - Fork 493
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
base: master
Are you sure you want to change the base?
Conversation
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
As described in #1096 (comment), the main question is whether we want to have stricter requirements on exchanging @Roasbeef let me know what you think: I'm currently leaning towards your initial implementation where you must receive |
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`.
I added the requirement to strictly exchange 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)! |
There was a problem hiding this 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.
2. data: | ||
* [`channel_id`:`channel_id`] | ||
* [`u64`:`fee_satoshis`] | ||
* [`u32`:`locktime`] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Clarify strict `shutdown` exchange requirements and fix typos.
| | <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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
This PR is a continuation of #1096, that @rustyrussell asked me to take over. The original description was:
I believe it is still useful to review as separate commits: however, we initially allowed setting
nSequence
, which we removed in favor of settingnLockTime
. That part can probably be skipped. I squashed the fixup commits from the previous PR, but kept the rest.