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

External wallet funding #2672

Merged
merged 20 commits into from Jun 12, 2019

Conversation

@niftynei
Copy link
Collaborator

commented May 24, 2019

Adds RPC commands for funding a channel via an external wallet.

There's a lot of caveats around the expected behavior of the wallet (uses SegWit ie non-malleable inputs, doesn't broadcast until continue has completed, provides the correct funding amount to fundchannel_start) that this doesn't have any way of enforcing. As such, it's behind an experimental flag -- a good plugin could probably paper over the majority of these issues.

RPCs added are as follows:

  • fundchannel_start. Returns a bech32 address that is the funding tx destination.
  • fundchannel_continue. Takes a funding txid and vout, returns the channel_id and confirmation that we've secured the commitment transactions for this channel.
  • fundchannel_cancel. Instead of continue, halts the fund channel flow.

Fixes #644

@niftynei

This comment has been minimized.

Copy link
Collaborator Author

commented May 24, 2019

Wanted to point out that we have no way of confirming that the amount someone says they're going to fund a channel for is actually the amount that they fund the channel for. We can correct/fix for this once the transaction is broadcast.

We have to be given a funding amount, however, as the channel establishment protocol calls for a funding amount in open_channel.

@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

This needs to be designed carefully. In particular the external wallet must be capable of providing a signed tx without broadcasting it (and clients need to be very aware that it is lightningd that must broadcast the funding tx).

More concretely, it seems a good amount of duplication of code, thiugh I have not checked all details.

I believe the same messages can be leveraged to implement multifundchannel. Given sufficient hooks in the normal lightningd wallet (a flag to withdraw that says "give me the signed tx but do not broadcast it", the ability to specify multiple targets to withdraw, and a new command to unreserve UTXOs of an unbroadcast tx (in case the succeeding funding attempt fails)), plus the ability to tell fundchannel_continue to not broadcast the funding tx once the commitment is signed,, plus a fundchannel_cancel, plus a broadcasttx command. Then it is possible to implement multifundchannel as a plugin. #1936

@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

Wanted to point out that we have no way of confirming that the amount someone says they're going to fund a channel for is actually the amount that they fund the channel for. We can correct/fix for this once the transaction is broadcast.

We cannot fix anything once funding tx is broadcast. The initial commitment tx signed commits to a specific value for the funding txout. If the external wallet pays a different amount to the same address, the initial commitment is useless.

The broadcast of the funding tx needs to be done after the other side signs initial commitment. So the external wallet must sign but not broadcast. If the funding tx is broadcast immediately then the counterparty can refuse to sign the initial commitment unless you push_msat at least half the value to them. Or consider what happens if the peer goes down permanently suddenly after accept_channel.

@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

Xref. https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-May/015925.html

The above link describes CoinSwap but note that the same funding transaction pattern underlies that protocol. It is unsafe to broadcast the funding tx until the backout tx is signed;if you want to to support wallets that do "sign and broadcast" as a single atomic step (e.g. every custodial wallet) you need SIGHASH_ANYPREVOUT. On LN the initial commitment serves as the backout (on LN unilateral close is our backout).

@niftynei

This comment has been minimized.

Copy link
Collaborator Author

commented May 24, 2019

More concretely, it seems a good amount of duplication of code, though I have not checked all details.

The duplication is by design, for a few reasons, namely that it's easier to DRY things out than to shoehorn too much divergence into a singular flow, especially at the work in progress stage. But agreed, it does need some consolidation to preserve some maintainability, especially between this and the ongoing dual funding work.

Wanted to point out that we have no way of confirming that the amount someone says they're going to fund a channel for is actually the amount that they fund the channel for. We can correct/fix for this once the transaction is broadcast.

We cannot fix anything once funding tx is broadcast. The initial commitment tx signed commits to a specific value for the funding txout. If the external wallet pays a different amount to the same address, the initial commitment is useless.

Ah right, my mistake I forgot about the commitment transaction commitments to amounts, I was thinking more from an internal representation standpoint. You're right, that would be unfortunate. A user of this RPC should supply the correct funding amount then, to avoid any of these issues. One way to mitigate this from an interaction standpoint would be to have the user supply a PSBT with the inputs, such that the funding_amount could be calculated internally by c-lightning, but that's really a convenience/sanity check rather than a deterrent.

@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

commented May 27, 2019

Fixes: #644

xref. #644 (comment) for my old proposal of the command interface. The fundingtransaction command there seems to be missing an outnum argument.

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

This needs to be designed carefully. In particular the external wallet must be capable of providing a signed tx without broadcasting it (and clients need to be very aware that it is lightningd that must broadcast the funding tx).

Is that really necessary? I was thinking of a flow like:

  1. Wallet tells lightningd how much it will fund.
  2. Lightningd starts open channel, then tells wallet what address to send to.
  3. Wallet tells lightningd the txid and output number.
  4. Lightningd tells wallet to release tx.

I imagine a fund-a-friend feature on a mobile Lightning, where Bob tells his phone to open a channel for 0.01btc, it starts negotiation and presents a qr code with the address. Alice scans that, produces tx, presents another QR code with txid and outnum. Bob scans that, and if negotiation completed, Alice tells her wallet to release tx.

@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

commented May 27, 2019

Perhaps a reword to "(and clients need to be very aware that it is lightningd that must initiate/signal broadcast of the funding tx)" more clearly expresses my meaning.

@niftynei niftynei force-pushed the niftynei:nifty/external-wallet branch 2 times, most recently from 2c0ff6c to 9fba2ba Jun 1, 2019

@cdecker cdecker changed the title WIP: external wallet funding External wallet funding Jun 3, 2019

@rustyrussell
Copy link
Contributor

left a comment

A few minor issues.

Your approach of duplicating code paths rather than starting with a split the existing code paths makes piecemeal review harder; since I can't tell from the patch if you've cut & pasted correctly. We'll end up in the same place, I think, but despite seeming easier this path is more awkward.

You should shuffle the infrastructure changes to the front, since they're trivial to review.

This should not be behind experimental_features; be proud! We also shouldn't turn those on in our test matrix just yet.

I think funding_continue should be called funding_complete. Maybe?

I'll have a go at reunifying the openingd paths (only using your new path) and see what I end up with?

if (scriptLen == 22 && scriptPubkey[1] != 0x14)
return NULL;
if (scriptLen == 34 && scriptPubkey[1] != 0x20)
return NULL;

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jun 3, 2019

Contributor

We already have these helpers in bitcoin/script.c:

if (!is_p2wsh(scriptPubkey, NULL) && !is_p2wpkh(scriptPubkey, NULL))
    return NULL;
#include <common/bech32.h>

/* Returns NULL if the script is not a P2WPKH or P2WSH */
char * encode_scriptpubkey_to_addr(const tal_t *ctx,

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jun 3, 2019

Contributor

Style nit: char *encode_ with no space after the *


out = tal_arr(ctx, char, 73 + strlen(hrp));
ok = segwit_addr_encode(out, hrp, 0, scriptPubkey + 2, scriptLen - 2);
if (!ok)

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jun 3, 2019

Contributor

Trivial: I think we can skip the 'ok' var here?

NULL))
return command_param_failed();

if (*funding_txout_num > UINT16_MAX)
return command_param_failed();

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jun 3, 2019

Contributor

command_param_failed is kinda magical: you need command_fail() here with a message.

@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

commented Jun 3, 2019

I think funding_continue should be called funding_complete. Maybe?

In case of multifundchannel, it is not necessarily so that the fundchannel process is complete on completion of the second step --- if some other counterparty aborts on the second step, then we should not broadcast the funding transaction. Instead we should support a fundchannel_abort that errors the channel and makes the counterparties that completed the second step forget the channel.

So I would argue that funding_continue is more appropriate name.

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

I think funding_continue should be called funding_complete. Maybe?

In case of multifundchannel, it is not necessarily so that the fundchannel process is complete on completion of the second step --- if some other counterparty aborts on the second step, then we should not broadcast the funding transaction. Instead we should support a fundchannel_abort that errors the channel and makes the counterparties that completed the second step forget the channel.

So I would argue that funding_continue is more appropriate name.

But if funding_continue returns successfully, the channel is funded. It has to be, since we're relying on the external party to broadcast the funding tx.

So the funding_complete name works well.

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

Minor patch needed, to plug memleak:

diff --git a/openingd/openingd.c b/openingd/openingd.c
index 937598a0b..d89dd627f 100644
--- a/openingd/openingd.c
+++ b/openingd/openingd.c
@@ -616,7 +616,7 @@ static u8 *funder_channel_start(struct state *state,
 		return NULL;
 
 	funding_output_script =
-		scriptpubkey_p2wsh(state,
+		scriptpubkey_p2wsh(tmpctx,
-				   bitcoin_redeem_2of2(state,
+				   bitcoin_redeem_2of2(tmpctx,
 						       &state->our_funding_pubkey,
 						       &state->their_funding_pubkey));
@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

commented Jun 4, 2019

But if funding_continue returns successfully, the channel is funded. It has to be, since we're relying on the external party to broadcast the funding tx.

That depends on whenfundchannel_continue returns.

If it returns before funding_locked , then we can use it for multifundchannel and cancel it later if we decide to not broadcast the fubding tx after all.

If it returns after funding_locked then it cannot be used for multifundchannel.

I strongly suggest splitting into smaller steps and just monitor the channel state moving to CHANNELD to determine if funding_locked occurred or not yet.

We can always create a plugin that combines multiple steps later, we cannot create a plugin to split up operations.

@niftynei

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 4, 2019

If it returns before funding_locked, then we can use it for multifundchannel and cancel it later if we decide to not broadcast the funding tx after all.

Here's one way that multifundchannel might work, using the RPC commands that this PR adds. Note that currently, fundchannel_cancel is a no-op if called after fundchannel_complete has already been called.

#  Let's say a user wants to open 3 channels, one with 10 0000sats, 
# one with 5 0000sats, and one with 2 0000sats, with a single funding transaction 
# funded from an external wallet.

# The first step would be to get funding addresses for all the proposed channels.
$ lightning-cli fundchannel_start <node1_id> 100000
   {"funding_address":"bcrt1q6p9umlh85swksnnzymg3pm09cjhqyd8nd6m2ep"}
$ lightning-cli fundchannel_start <node2_id> 50000
      {"funding_address":"bcrt1qe0dmajvx920zwnkdqkwvke5hapsrvxf8hfaffd"}
$ lightning-cli fundchannel_start <node3_id> 20000
      {"funding_address":"bcrt1qkqp5dcgpm55k3tl6matr943whtmpmzy64mdanl"}

# User uses external wallet to compose desired transaction, and derives the
# funding txid for this transaction, then 'completes' the 3 outstanding channel
# opens.

$ lightning-cli fundchannel_complete <node1_id> <funding_txid> <funding_txout1>
$ lightning-cli fundchannel_complete <node2_id> <funding_txid> <funding_txout2>
$ lightning-cli fundchannel_complete <node3_id> <funding_txid> <funding_txout3>

# User broadcasts their transaction, all three channels complete.

Is this an accurate portrayal of a workable multifundchannel workflow?

@niftynei niftynei force-pushed the niftynei:nifty/external-wallet branch 2 times, most recently from 142a5e6 to 3c78a2b Jun 5, 2019

@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2019

What happens if funding_conplete fails for node2_id because node 2 disconnected? Is node1 now stuck waiting for a funding tx that will now never be broadcast? (Until funding timeout but that is a long time)

That is why we need funding_cancel to do an error.

Xref this thread: https://lists.linuxfoundation.org/pipermail/lightning-dev/2018-January/000905.html

In the end I concluded that error had the semantics needed. But it is still necessary.

Funding is not complete until the blockchain says it is complete.

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

I somewhat agree: it might be nice to be able to cancel even though lightningd thinks we're ready. However, that can be added later and we're too close to release.

I also want to remove the fundchannel path from openingd, and always use the other path. But that's easiest done by moving fundchannel to a simple plugin, then ripping it out. This will have to wait for 0.7.2.

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

Still has a memleak, BTW...

@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

commented Jun 9, 2019

Maybe this? 6233855#diff-1130d9d3f990f966967fbc527c10f275R1206 Previous value of peer->uncommitted_channel->fc->cmd is not resolved?

Also, is not modifying if (!peer->uncommitted_channel->fc || !peer->uncommitted_channel->fc->inflight) to if (!peer->uncommitted_channel->fc) sufficient to make fundchannel_cancel work after fundchannel_continue is executed?

niftynei added some commits May 22, 2019

common: pull out scriptPubkey address method
We're going to need this for P2WSH scripts. pull it out into
a common file plus adopt the sanity checks so that it will allow for
either P2WSH or P2WPKH (previously only encoded P2WPKH scripts)
opening: add entry point for `funding_start rpc command
Beginnings of wiring up the funding_start rpc command. missing
the part that actually starts the funding channel dance.
json: correct error message for param parser
Looks like copy-paste from another commit didn't update
the field for this
openingd: allow funding_failed to complete successfully
For the `fundchannel_cancel` we're going to want
to 'successfully' fail a funding channel operation. This allows
us to report it a failure back as an RPC success, instead of
automatically failing the RPC request.
tests: default all addresses to bech32
Needed for composing a transaction externally to c-lightning, using
bitcoind util.
openingd: pull out setup funder checks into separate method
Useful for adding the funder_start stuff
opening: wire up walking through open channel up thru accept
Fill in details to make fundchannel_start work.
pylightning: add fundchannel_start
Add method to rpc handler
test: add initial tests for starting an external fundchannel
Test for getting through the address generation portion.
openingd: update billboard with funding info
Make it easier for a user to know what's going on with
a channel in `listpeers`.
opening: stash amount outside of the wtx
Some channels won't be opened with a wtx struct, so keep
the total funding amount separate from it so we can
show some stats for listpeers.

Note that we're going to need to update/confirm this once
the transaction gets confirmed.
lightningd: add start for fundchannel_continue
Add an RPC method (not working at the moment) called
`fundchannel_continue` that takes as its parameters a
node_id and a txid for a transaction (that ostensibly has an output
for a channel)

@niftynei niftynei force-pushed the niftynei:nifty/external-wallet branch from 3c78a2b to 70fdc5d Jun 12, 2019

niftynei added some commits May 31, 2019

fundchannel: add txout field to RPC/API
We'll need the outpoint for the funding output.
funding: add a 'inflight' marker
We need a way to gate allowing continue to proceed or not
funding: wire up funding_continue
Big wiring re-org for funding-continue

In openingd, we move the 'persistent' state (their basepoints,
pubkey, and the minimum_depth requirement for the opening tx) into
the state object. We also look to keep code-reuse between
'continue' and normal 'fundchannel' as high as possible. Both
of these call the same 'fundchannel_reply' at the end.

In opening_control.c, we remap fundchannel_reply such that it is
now aware of the difference between an external/internally funded
channel open. It's the same return path, with the difference that
one finishes making and broadcasting the funding transaction; the
other is skips this.
opening: add fundchannel_cancel command
Provide the option to cancel a funding-opening with a peer.
Must either call `fundchannel_cancel` or `fundchannel_continue`
tests: finish up test for external funding flow
Add to test for fundchannel with composing and broadcasting
an external transaction.
funding: rename fundchannel_continue -> _complete
Renaming. "complete" more accurately describes what we're doing here.
funding: update CHANGELOG with new RPC calls
Let people know what's changed

@rustyrussell rustyrussell force-pushed the niftynei:nifty/external-wallet branch from 70fdc5d to dcab321 Jun 12, 2019

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

Fixed "%ls" which should have been "%u"...

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

Ack dcab321

@rustyrussell rustyrussell merged commit c0475d0 into ElementsProject:master Jun 12, 2019

2 checks passed

ackbot PR ack'd by rustyrussell
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.