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: add the possibility to fund a channel with specific utxos #2699

Merged
merged 5 commits into from Jun 11, 2019

Conversation

@darosior
Copy link
Collaborator

commented Jun 2, 2019

Fixes #2682 .

This PR adds the possibility to specify a list of outpoints ([\"txid:vout\", \"txid:vout\"]) as the second parameter of the fundchannel call in addition to 'all' or an amount in satoshis.

@darosior darosior requested a review from cdecker as a code owner Jun 2, 2019

@darosior darosior force-pushed the darosior:fundchannel_by_utxo branch 2 times, most recently from bbe543b to 5ee7dd7 Jun 2, 2019

@rustyrussell rustyrussell requested a review from niftynei Jun 3, 2019

@rustyrussell
Copy link
Contributor

left a comment

I like the idea! Some implementation polish would be great, and it definitely needs a CHANGELOG.md brag line :)

wallet/wallet.c Outdated Show resolved Hide resolved
common/wallet_tx.c Outdated Show resolved Hide resolved
common/wallet_tx.c Outdated Show resolved Hide resolved
common/wallet_tx.c Outdated Show resolved Hide resolved
wallet/wallet.c Outdated Show resolved Hide resolved
wallet/wallet.c Outdated Show resolved Hide resolved
wallet/wallet.c Outdated Show resolved Hide resolved
tests/test_connection.py Outdated Show resolved Hide resolved

@darosior darosior force-pushed the darosior:fundchannel_by_utxo branch from 5ee7dd7 to fabd16d Jun 3, 2019

@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 3, 2019

@rustyrussell thank you for the review. I rebased to apply most of the requested changes :

  • Parse outpoints before calling the utxo selection function
  • Add some test for failing cases

Added a CHANGELOG brag line :)

@niftynei
Copy link
Collaborator

left a comment

Lots of little things, nice work overall!

I think the biggest question is whether or not we want to have this in addition to the 'amount' field. It seems like it'd be nice to be able to specify a utxo set and a total amount to fund as well, which isn't possible with the current design.

How you do you see people using this @darosior ?

common/wallet_tx.c Outdated Show resolved Hide resolved
common/wallet_tx.c Outdated Show resolved Hide resolved
wallet/wallet.c Outdated Show resolved Hide resolved
common/wallet_tx.c Outdated Show resolved Hide resolved
common/wallet_tx.c Outdated Show resolved Hide resolved
common/wallet_tx.c Outdated Show resolved Hide resolved
common/wallet_tx.c Show resolved Hide resolved
lightningd/opening_control.c Show resolved Hide resolved
common/wallet_tx.c Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

Agree with all @niftynei feedback. In particular, I think adding a separate optional 'utxos' param is nicer: if not specified, utxos are selected from current wallet utxos as now. That does allow us to have a change output from UTXOs if we want to (or 'all' if we don't).

@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 4, 2019

Lots of little things, nice work overall!

I think the biggest question is whether or not we want to have this in addition to the 'amount' field. It seems like it'd be nice to be able to specify a utxo set and a total amount to fund as well, which isn't possible with the current design.

How you do you see people using this @darosior ?

Thank you for the review @niftynei .
I see people using it in a few corner cases : a traditional user doing newaddr, connect, and fundchannel won't use it at all. Some users having a lot of tiny inputs wanting more control over them could use it. Actually I think most people who'll use it will consume the whole utxo, that's why I made this command_result of command_result in case the parameter is an array (it would have been easier to just add an optional parameter) : I thought making another parameter will just make them copy paste the exact utxo amount.

EDIT: misread the comment above, especially (or 'all' if we don't).. It's better with another parameter.

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

Ideally, we'd allow them to specify any combination, but that's a bit ambitious. I really think a "utxos" arg is best here.

This will combine well with #2713 too

@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 5, 2019

(Edited) Ok, I'll work on this

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

Sorry, you really can't do the API this way. As @niftynei points out, you may well want to specify both utxos and the amount to spend, so it needs to be a separate 'utxos' argument.

And while txid:outnum is neat on the commandline, it's nasty JSON. You can always create a plugin which does converts from this syntax :)

@rustyrussell rustyrussell self-requested a review Jun 8, 2019

@darosior darosior force-pushed the darosior:fundchannel_by_utxo branch from fabd16d to 8d9e89f Jun 8, 2019

@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 8, 2019

Sorry, you really can't do the API this way. As @niftynei points out, you may well want to specify both utxos and the amount to spend, so it needs to be a separate 'utxos' argument.

When saying "let's do this" I meant "ok I'll work on the separate 'utxos' argument version" (I was not pushing the initial implementation without argument ^^), it's a translated sentence from french and has apparently not the same signification in english.

And while txid:outnum is neat on the commandline, it's nasty JSON. You can always create a plugin which does converts from this syntax :)

I just saw your comment now that I finished to work on the new implementation. I finally kept using JSON as you requested (I added a bitcoin_txid json helper), relying on the fact that a txid would always be 64 hex characters long.

EDIT: A new json helper was already merged along with #2713 , rebased below .

@darosior darosior force-pushed the darosior:fundchannel_by_utxo branch from 8d9e89f to 5091ed3 Jun 8, 2019

common/wallet_tx.c Outdated Show resolved Hide resolved
common/wallet_tx.c Outdated Show resolved Hide resolved
common/wallet_tx.c Outdated Show resolved Hide resolved
common/wallet_tx.c Outdated Show resolved Hide resolved
common/wallet_tx.c Outdated Show resolved Hide resolved
common/wallet_tx.c Outdated Show resolved Hide resolved
common/wallet_tx.c Outdated Show resolved Hide resolved
@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

OK, minor fixes only. I'm trying to tag rc1, but there are a couple of other merges pending review, so I'll wait (was tempted to tweak it myself, but that's a bad habit!).

@rustyrussell rustyrussell self-requested a review Jun 11, 2019

darosior added some commits Jun 11, 2019

fundchannel: Update the manpage and Pylightning's method
Add the new 'utxo' parameter to both
fundchannel: Add a new 'utxo' parameter
This new parameter takes a list of outpoints (as txid:vout) and fund a channel from the corresponding utxos.
Example : fundchannel <id> 10000 normal 1 [10767f0db0e568127fffd7f70a154d4599f42d62babf63230a7c3378bfce3cb0:0, c9e040e0b5fc8c59d5e7834108fbc5583001f414dd83faf0a05cff9d1a92d32c:0]

@darosior darosior force-pushed the darosior:fundchannel_by_utxo branch from 5091ed3 to 035ea5b Jun 11, 2019

@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 11, 2019

@rustyrussell I corrected the minor fixes, added the split function (as a helper) and feerate estimate in errors, I also split the first check about parsing the txid and vout in two parts to have more accurate errors.

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

Ack 035ea5b

GitHub ordered the commits weirdly, but I checked locally and they're in the expected order...

@rustyrussell rustyrussell merged commit 7ce4fcd into ElementsProject:master Jun 11, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@darosior darosior deleted the darosior:fundchannel_by_utxo branch Jul 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.