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 -- make into a plugin #3003

Merged
merged 19 commits into from Sep 11, 2019

Conversation

@niftynei
Copy link
Collaborator

@niftynei niftynei commented Aug 28, 2019

Moves fundchannel out to a plugin, instead relying on fundchannel_start/_continue internally.

@niftynei niftynei requested a review from cdecker as a code owner Aug 28, 2019
@niftynei niftynei force-pushed the nifty/fundchannel-plugin branch 3 times, most recently from 6ff2e97 to bac25bb Aug 29, 2019
@@ -29,11 +30,12 @@ struct bitcoin_tx *withdraw_tx(const tal_t *ctx,
const void *map[2];
map[0] = int2ptr(0);
map[1] = int2ptr(1);
bitcoin_tx_add_output(tx, scriptpubkey_p2wpkh(tx, changekey),
change);
script = scriptpubkey_p2wpkh(tx, changekey);
Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj Aug 29, 2019

Choose a reason for hiding this comment

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

Commit 9ebb521 in #3006 and #2964 is a better solution for this (and possibly deserves its own PR).

Copy link
Collaborator Author

@niftynei niftynei Aug 30, 2019

Choose a reason for hiding this comment

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

Removed!

@niftynei niftynei force-pushed the nifty/fundchannel-plugin branch 6 times, most recently from eeecbf6 to b78f621 Sep 4, 2019
@niftynei niftynei changed the title WIP: Fundchannel plugin Fundchannel plugin Sep 4, 2019
@niftynei niftynei changed the title Fundchannel plugin fundchannel -- make into a plugin Sep 4, 2019
@@ -74,6 +75,12 @@ struct command_result *param_sat(struct command *cmd, const char *name,
const char *buffer, const jsmntok_t *tok,
struct amount_sat **sat);

/* Extract node_id from this string. Makes sure *id is valid. */
struct command_result *param_node_id(struct command *cmd,
const char *name,
Copy link
Collaborator

@darosior darosior Sep 5, 2019

Choose a reason for hiding this comment

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

nit: weird indent ?

Copy link
Collaborator Author

@niftynei niftynei Sep 6, 2019

Choose a reason for hiding this comment

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

fixed, thanks!

prep5 = l1.rpc.txprepare('bcrt1qeyyk6sl5pr49ycpqyckvmttus5ttj25pd0zpvg',
Millisatoshi(amount * 3.5 * 1000), utxos=utxos)

print(prep5['unsigned_tx'])
Copy link
Collaborator

@darosior darosior Sep 5, 2019

Choose a reason for hiding this comment

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

Debugging trailing line ? :-)

Copy link
Collaborator Author

@niftynei niftynei Sep 6, 2019

Choose a reason for hiding this comment

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

yikes. 😱


bool *announce_channel;
u32 *minconf;
// todo: utxos
Copy link
Collaborator

@darosior darosior Sep 5, 2019

Choose a reason for hiding this comment

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

nit: /* TODO: utxos */ ?

Copy link
Collaborator Author

@niftynei niftynei Sep 6, 2019

Choose a reason for hiding this comment

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

removed later?

json_out_end(ret, '}');
json_out_finished(ret);

// FIXME: as a nice feature, we should check that the peer
Copy link
Collaborator

@darosior darosior Sep 5, 2019

Choose a reason for hiding this comment

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

nit: Use /* */ comments ? (There are other in this file too)
EDIT: Other are removed in next commits

Copy link
Collaborator Author

@niftynei niftynei Sep 6, 2019

Choose a reason for hiding this comment

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

updated.

@niftynei niftynei force-pushed the nifty/fundchannel-plugin branch 3 times, most recently from 7678972 to 3a0efd5 Sep 6, 2019
@niftynei niftynei force-pushed the nifty/fundchannel-plugin branch 2 times, most recently from 0de46e8 to cedad76 Sep 6, 2019
@rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Sep 9, 2019

Seems to have broken test_txprepare?


\fBtxprepare\fR is similar to the first part of a \fBwithdraw\fR command, but
supports multiple outputs and uses \fIoutputs\fR as parameter\. The second part
is provided by \fBtxsend\fR\.


\fIutxos\fR specifies the utxos to be used to fund the transaction, as an array
of "txid:vout"\. These must be drawn from the node's available UTXO set\.
Copy link
Collaborator

@trueptolemy trueptolemy Sep 9, 2019

Choose a reason for hiding this comment

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

What about adding the explanation for txid and vout here?

@@ -37,6 +37,10 @@ the suffix is equivalent to \fIperkb\fR\.
\fIminconf\fR specifies the minimum number of confirmations that used
outputs should have\. Default is 1\.


\fIutxos\fR specifies the utxos to be used to be withdrawn from, as an array
of "txid:vout"\. These must be drawn from the node's available UTXO set\.
Copy link
Collaborator

@trueptolemy trueptolemy Sep 9, 2019

Choose a reason for hiding this comment

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

Also here. They are the new variables in this doc.

Copy link
Collaborator Author

@niftynei niftynei Sep 9, 2019

Choose a reason for hiding this comment

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

generally, this is a good rule. in this case, however, using a txid:vout to describe a utxo is rather canonical, and should be self-explanatory to a user who's interested in this option.

@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added
- JSON API: `listfunds` now lists a blockheight for confirmed transactions
- JSON API: `fundchannel_start` now includes field `scriptpubkey`
Copy link
Collaborator

@trueptolemy trueptolemy Sep 9, 2019

Choose a reason for hiding this comment

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

What about "The return of fundchannel_start now includes field scriptpubkey " ?

Copy link
Collaborator Author

@niftynei niftynei Sep 9, 2019

Choose a reason for hiding this comment

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

good point. updated to make a bit clearer.

script_tok = json_get_member(buf, result, "scriptpubkey");

if (!addr_tok || !script_tok)
plugin_err("Fundchannel start didn't return a funding_address or scriptpubkey? '%.*s'",
Copy link
Collaborator

@trueptolemy trueptolemy Sep 9, 2019

Choose a reason for hiding this comment

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

"fundchannel_start"?

@niftynei niftynei force-pushed the nifty/fundchannel-plugin branch 2 times, most recently from 0ba7f15 to a2ccfc6 Sep 9, 2019
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Ack ba5d7d7

Minor changes only. This is a really nice series, kudos!

CHANGELOG.md Outdated
@@ -8,7 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added
- JSON API: `listfunds` now lists a blockheight for confirmed transactions
- JSON API: `fundchannel_start` result now includes field `scriptpubkey`
- JSON API: `fundchannel_start` now includes field `scriptpubkey`
- JSON API: `txprepare` and `withdraw` now accept an optional parameter `utxos`, a list of utxos to incldue in the prepared transaction
Copy link
Contributor

@rustyrussell rustyrussell Sep 10, 2019

Choose a reason for hiding this comment

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

incldue sounds like a cross between Mountain Dew and Pocari Sweat...


/* We need to call txdiscard, and forward the actual cause for the
* error after we've cleaned up. We swallow any errors returned by
* this call, as we don't really care if it succeeds or not **/
Copy link
Contributor

@rustyrussell rustyrussell Sep 10, 2019

Choose a reason for hiding this comment

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

Extra *?


ret = json_out_new(NULL);
json_out_start(ret, NULL, '{');
json_out_addstr(ret, "id", type_to_string(tmpctx, struct node_id, fr->id));
Copy link
Contributor

@rustyrussell rustyrussell Sep 10, 2019

Choose a reason for hiding this comment

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

Prefer not to use type_to_string outside logging code, TBH, since it may change. node_id_to_hexstr(tmpctx, fr->id)?

Copy link
Collaborator

@trueptolemy trueptolemy Sep 11, 2019

Choose a reason for hiding this comment

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

Also reminded me.

@@ -179,6 +187,7 @@ static struct command_result *tx_prepare_done(struct command *cmd,
plugin_err("Unable to parse tx %s", hex);

/* Find the txout */
outnum_found = false;
Copy link
Contributor

@rustyrussell rustyrussell Sep 10, 2019

Choose a reason for hiding this comment

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

Seems like a fixup for a previous commit. Would be nice to squash it into that one.

result->end - result->start, buf);
return send_outreq(cmd, "txprepare",
tx_prepare_done, cancel_start,
cast_const(struct funding_req *, fr),
Copy link
Contributor

@rustyrussell rustyrussell Sep 10, 2019

Choose a reason for hiding this comment

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

This cast_const seems like a noop?

const char *placeholder_script = "0020b95810f824f843934fa042acd0becba52087813e260edaeebc42b5cb9abe1464";
const char *placeholder_funding_addr;

/* FIXME: dynamically query */
const struct amount_sat max_funding = AMOUNT_SAT_INIT((1 << 24) - 1);
Copy link
Contributor

@rustyrussell rustyrussell Sep 10, 2019

Choose a reason for hiding this comment

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

Get this from chainparams too?

@@ -360,6 +360,8 @@ static bool encoding_end_zlib(u8 **encoded, size_t off)
/* Successful: copy over and trim */
tal_resize(encoded, off + tal_count(z));
memcpy(*encoded + off, z, tal_count(z));

tal_free(z);
Copy link
Contributor

@rustyrussell rustyrussell Sep 10, 2019

Choose a reason for hiding this comment

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

Suspect this is because run-extended-info.c doesn't setup or cleanup tmpctx, so it's NULL.

It should probably do that (hmm, we should initialize tmpctx to 0xdeadbeef or something so it crashes in such a case). Separate PR though

@niftynei niftynei force-pushed the nifty/fundchannel-plugin branch from a2ccfc6 to 55e371e Sep 10, 2019
@niftynei niftynei force-pushed the nifty/fundchannel-plugin branch 3 times, most recently from 9449d56 to b8004be Sep 11, 2019
niftynei added 19 commits Sep 11, 2019
Going to need this for a plugin that's external
Easier to pass it back than dig it out of the address, since we
have it. Needed for extracting fundchannel
Allow a user to select the utxo set that will be added to a
transaction, via the `utxos` parameter. Optional.

Format for utxos should be of the form ["txid:vout","..."]
libplugin seems to need the time include, so move it up from pay.
Makes it easier to stash an error and then return it after another
RPC call has been made.
New plugin which mimics the functionality of the embedded 'fundchannel'.
Allows a user to specify the utxo's they like to allow for a command.
If txprepare hits an error, we need to cancel the pending request
with our peer.
When the peer sends back an error, we return null, but sending
a NULL message back to lightningd causes a parsing error on their
side. negotiation_failed already calls back the peer, all we need
to do here is exit.
Will allow for more intelligent change policy
In some cases, the funding transaction is affordable only
if we leave out the change amount.
Previously, we'd fail on 'all'. `fundchannel_start` needs an amount
in order to start a funding transaction.

The way that we approach this is to first call `txprepare` with a
placeholder address and the 'all' amount; this will return the maximum
amount available. We then clamp this to the max_funding (currently
hardcoded, in the future we'd want to consult our/the peer's features) and
then use the amount on the output in the prepared transaction as the
funding amount. We then pass this amount to fundchannel_start,
after we've started it successfully we cancel the held placeholder
transaction and prepare a second transaction for the exact amount,
using the funding address that fundchannel_start passed back.
in order to preserve current behavior, we cap at max if specified 'all';
otherwise we fail since the amount requested is larger than the
channel max capacity
`txprepare` verifies that you're sending to an address on the
right network, so this builds a placeholder address for the
right network.
After switching to a plugin, we verify that we can fund a channel
before we check to contact a peer. We'll need to have a funded wallet
to pass the check in this test that verifies that 'fundchannel' cannot
be called for a peer after fundchannel_start is.
Since we pull the chainparams out to get the funding_addr placeholder,
we can also get the max_funding now!
Switch over to using the fundchannel plugin.
These were giving me valgrind errors locally; fixed now.
@niftynei niftynei force-pushed the nifty/fundchannel-plugin branch from b8004be to 4c0de59 Sep 11, 2019
@rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Sep 11, 2019

Ack 4c0de59

@rustyrussell rustyrussell merged commit 904a138 into ElementsProject:master Sep 11, 2019
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants