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

Fundchannel complete psbt not txid #4428

Conversation

rustyrussell
Copy link
Contributor

This prevents the footgun from #4416

@rustyrussell rustyrussell added this to the v0.9.4 milestone Mar 12, 2021
@rustyrussell rustyrussell force-pushed the guilt/fundchannel-complete-psbt-not-txid branch 2 times, most recently from 1787092 to 00121a6 Compare March 15, 2021 01:23
@rustyrussell
Copy link
Contributor Author

Trivial rebase to fix un-rebuilt man page.

…_e_s

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We'll need it in next patch to identify the funding output.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the guilt/fundchannel-complete-psbt-not-txid branch from 00121a6 to f51a6fd Compare March 15, 2021 04:26
…h PSBT

Requiring the user to calculate the txid of the PSBT is a horrible, bad,
no-good idea.

Doesn't deprecate yet, so I can test that this path works while
multifundchannel still uses it.

Fixes: ElementsProject#4416 (at least for future users!)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `fundchannel_complete` takes a psbt parameter.
I didn't change the callers yet, so I can test the backwards compat code
works.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In particular, txprepare gives us a nice way to get a valid PSBT for
testing.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `txprepare` and `withdraw` now return a `psbt` field.
And update all the in-tree callers.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Deprecated: JSON-RPC: `fundchannel_complete` `txid` and `txout` parameters (use `psbt`)
@rustyrussell rustyrussell force-pushed the guilt/fundchannel-complete-psbt-not-txid branch from f51a6fd to e9ecb9c Compare March 15, 2021 04:28
@rustyrussell
Copy link
Contributor Author

OK, had to add more PSBT JSON outputs to make our tests work (probably a good thing anyway!)

@rustyrussell
Copy link
Contributor Author

... and check fails on elements. Fixed.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the guilt/fundchannel-complete-psbt-not-txid branch from 9c13d6f to d5a8b4d Compare March 15, 2021 23:59
Copy link
Collaborator

@niftynei niftynei left a comment

Choose a reason for hiding this comment

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

ACK d5a8b4d

@@ -1192,7 +1252,7 @@ static const struct json_command fundchannel_complete_command = {
"channels",
json_fundchannel_complete,
"Complete channel establishment with peer {id} for funding transaction"
"with {txid}. Returns true on success, false otherwise."
"with {psbt}. Returns true on success, false otherwise."
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this will actually make us incompatible with LN Pool? surely not... as the pool probably produces PSBTs for peers to sign for the collectively built opening tx?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can always re-add a txid arg, but I really think that it's a great sanity-check!


# Can't complete with incorrect address.
wrongaddr = l1.rpc.txprepare([{l1.rpc.newaddr()['bech32']: amount}])
with pytest.raises(RpcError, match=r'No output to open channel'):
Copy link
Collaborator

Choose a reason for hiding this comment

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

😅

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.

2 participants