Feature to allow shutdown of channels opened with the wrong funding txid / output number. #4421
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustyrussell
force-pushed
the
guilt/shutdown-wrong-txid
branch
from
ff85516
to
599113e
Compare
cdecker
approved these changes
Mar 10, 2021
There was a problem hiding this comment.
Very good, just found an assert that seems to be triggerable (followed by the same check in an if block) and minor wording.
CI failures seem unrelated, restarted.
ACK 599113e
I was initially curious about why we'd want to remember the funding
alias in the first place, but it is being used to track the funding
instead. I'm wondering if this is worth generalizing into an alias
tracking feature: record tx outpoint aliases in a new table, if an
alias gets confirmed look at the txo_watches and call that with the
alias transaction instead. It'd require a bit of care in the watch
callbacks in case we build on top of that TX, but it could allow us to
use the aliasing to unify outpoint tracking across variants of
transactions for things like HTLC-transactions being re-assembled into
larger batches, these malleation cases and some others.
Not for this PR, but might be a nice cleanup for the next release if we go
more into the direction of babysitting our txs.
| \fBexperimental-shutdown-wrong-funding\fR | ||
|
|
||
|
|
||
| Specifying this allows the \fBwrong_funding\fR field in shutdown: if a |
There was a problem hiding this comment.
For the spec we probably want to call this funding_malleated_txid or funding_alias_txid but for now this is ok.
doc/lightningd-config.5
Outdated
|
|
||
| Specifying this allows the \fBwrong_funding\fR field in shutdown: if a | ||
| remote node has opened a channel using the incorrect txid (and it | ||
| hasn't been used yet at all) this allows them to negotiate a clean |
There was a problem hiding this comment.
Isn't specifying that it hasn't been used implied? We can't perform updates unless we find the correct funding.
There was a problem hiding this comment.
Yes, I should clarify. "If the remote node has opened a channel but claims it used the incorrect txid (and the channel hasn't been used at all)"?
| @@ -90,6 +90,10 @@ msgtype,shutdown,38 | |||
| msgdata,shutdown,channel_id,channel_id, | |||
| msgdata,shutdown,len,u16, | |||
| msgdata,shutdown,scriptpubkey,byte,len | |||
| msgdata,shutdown,tlvs,shutdown_tlvs, | |||
| tlvtype,shutdown_tlvs,wrong_funding,100 | |||
There was a problem hiding this comment.
Shall we use 104 here as well to match the option featurebit?
There was a problem hiding this comment.
Sure, why not.
There was a problem hiding this comment.
(I didn't change this, actually, since @openoms is actively testing now, and easier not to break him).
Comment on lines
+1326
to
+1336
| ", shutdown_wrong_txid" // 57 | ||
| ", shutdown_wrong_outnum" // 58 |
There was a problem hiding this comment.
At some point I'll add an outpoint primitive that subsumes (txid, outnum) since it's a very common thing, and they rarely get queried individually :-)
There was a problem hiding this comment.
If only ALTER TABLE DROP COLUMN were something that sqlite3 can do...
closingd/closingd.c
Outdated
| @@ -75,6 +76,7 @@ static struct bitcoin_tx *close_tx(const tal_t *ctx, | |||
| out_minus_fee[LOCAL], | |||
| out_minus_fee[REMOTE], | |||
| dust_limit); | |||
| assert(tx); | |||
There was a problem hiding this comment.
Whoops, this looks like it could trigger, taking closingd with it.
There was a problem hiding this comment.
Oops, that was me trying to track a bug and force a backtrace.
Good catch!
closingd/closingd.c
Outdated
| @@ -669,6 +695,11 @@ int main(int argc, char *argv[]) | |||
| status_debug("fee = %s", | |||
| type_to_string(tmpctx, struct amount_sat, &offer[LOCAL])); | |||
| status_debug("fee negotiation step = %s", fee_negotiation_step_str); | |||
| if (wrong_funding) | |||
| status_unusual("Seting wrong_funding_txid to %s:%u", | |||
There was a problem hiding this comment.
Suggested change
| status_unusual("Seting wrong_funding_txid to %s:%u", | |
| status_unusual("Setting wrong_funding_txid to %s:%u", |
|
Tried but seems to break unless it is me doing something wrong. built with: And lightningd is running with: Build output: https://pastebin.com/raw/AD6U8RNG The signet transaction sent (with the wrong txid passed to In the log: |
|
Good catch, fix coming! |
rustyrussell
force-pushed
the
guilt/shutdown-wrong-txid
branch
from
5e95855
to
85477a2
Compare
|
Tested sucessfully on signet! Log of the remote peer: Log of the funding peer: And the recovered funds in the wallet: |
|
Minor breakage in the test, otherwise perfect: |
Obvious fix applied. Rebased and folded previous fixes... |
rustyrussell
force-pushed
the
guilt/shutdown-wrong-txid
branch
2 times, most recently
from
319460c
to
0095c48
Compare
… up! It's not unheard of for people to give the wrong funding tx to us, getting their funds stuck. Interestingly, we can allow mutual close using a different txid and output number as long as they (solely) funded the channel, and the channel hasn't been used. This defines a "play area" feature to do just that. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…tpoint. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
If it passes checks, lightningd puts it in the database. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: lightningd: experimental-shutdown-wrong-funding to allow remote nodes to close incorrectly opened channels. Changelog-Added: JSON-RPC: close has a new `wrong_funding` option to try to close out unused channels where we messed up the funding tx. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Fairly easy to do, though we also have to add the watch when we load from the database. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
guilt/shutdown-wrong-txid
branch
from
0095c48
to
06d04f8
Compare
|
Trivial rebase to fix generated file conflicts |
|
Ack 06d04f8 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #4416, though it may not help soon enough for that (the peer will forget the channel after 2016 blocks, so they need to upgrade and try this before then!)
Next step is to remove the footgun which caused it, but @TBast also reported that he had similar problems with users trying to RBF their funding tx...