experimental: dual-funding (II of IV) #3122

niftynei · 2019-10-01T02:32:49Z

first pass at implementing dual-funding. working, but has some bugs.

32e6a29..c6ee3d5 are also in #3121. For best results, it's suggested that reviewers begin at 09d5125.

RFC specification can be found at lightning/bolts#524

conscott · 2019-10-01T09:50:02Z

lightningd/peer_control.c

@@ -628,36 +629,23 @@ static void json_add_channel(struct lightningd *ld,
 	    response, "private",
 	    !(channel->channel_flags & CHANNEL_FLAGS_ANNOUNCE_CHANNEL));

-	// FIXME @conscott : Modify this when dual-funded channels


Saibato · 2019-10-01T12:19:32Z

Locks good 👍 Minor nits, Withdraw funds tx has change output set to zero, all funds gone to miner fees, then crash exit with

WIT) txid f302f72f5820b9c89f74af16333da7601ec6b78c134ad94c6d127cba9d99aafc
lightningd: wallet/walletrpc.c:63: wallet_withdrawal_broadcast: Assertion `amount_sat_greater_eq(change, utx->wtx->change)' failed.

lightningd: wallet/walletrpc.c:63: wallet_withdrawal_broadcast: Assertion `amount_sat_greater_eq(change, utx->wtx->change)' failed.
lightningd: FATAL SIGNAL 6 (version v0.7.2.1-404-g2ea533b-modded)
0x563845fcb554 send_backtrace
   common/daemon.c:40
0x563845fcb5fa crashdump
   common/daemon.c:53
0x7fa31c48a05f ???
   ???:0
0x7fa31c489fff ???
   ???:0
0x7fa31c48b429 ???
   ???:0
0x7fa31c482e66 ???
   ???:0
0x7fa31c482f11 ???
   ???:0
0x56384600ccca wallet_withdrawal_broadcast
   wallet/walletrpc.c:63
0x563845f87ccf process_sendrawtx
   lightningd/bitcoind.c:449
0x563845f87303 bcli_finished
   lightningd/bitcoind.c:227
0x56384601dc1f destroy_conn
   ccan/ccan/io/poll.c:244
0x56384601dc3f destroy_conn_close_fd
   ccan/ccan/io/poll.c:250
0x56384602bdc5 notify
   ccan/ccan/tal/tal.c:235
0x56384602c2b4 del_tree
   ccan/ccan/tal/tal.c:397
0x56384602c640 tal_free
   ccan/ccan/tal/tal.c:481
0x56384601c444 io_close
   ccan/ccan/io/io.c:450
0x56384601e35a io_loop
   ccan/ccan/io/poll.c:449
0x563845f99e4d io_loop_with_timers
   lightningd/io_loop_with_timers.c:24
0x563845fa027f main
   lightningd/lightningd.c:845
0x7fa31c4772e0 ???
   ???:0
0x563845f862e9 ???
   ???:0
0xffffffffffffffff ???
   ???:0
2019-10-01T12:02:07.363Z **BROKEN** lightningd(30843): FATAL SIGNAL 6 (version v0.7.2.1-404-g2ea533b-modded)


lightning-cli listfunds
{
  "outputs": [
     {
        "txid": "e84f9d9cd2b9664daed2c40ca5372e5018ca14bee01a06d8df83d580b913f4fc",
        "output": 0,
        "value": 543210000,
        "amount_msat": "543210000000msat",
        "address": "bcrt1qmeh8rsvhzxytvpyvja55w230x3st8wv8csx8z4",
        "status": "confirmed",
        "blockheight": 2399
     }
  ],
  "channels": []
}


lightning-cli withdraw bcrt1qn3krl672n9j9fjjl59rgnenr7850ssgqvtp049 123456 2500perkw
lightning-cli: reading response: socket closed

, if full node excepts high fees

 "4256388d9ebf7f1dd1c129101f6d7f0adcd23ef5c8780dad43e8a32a43fca997": {
    "fees": {
      "base": 5.43086544,
      "modified": 5.43086544,
      "ancestor": 5.43086544,
      "descendant": 5.43086544
    },
    "size": 141,
    "fee": 5.43086544,
    "modifiedfee": 5.43086544,
    "time": 1569931327,
    "height": 2408,
    "descendantcount": 1,
    "

Try to withdraw send 123456 sats at 2500perkw, from a 5,4321 BTC wallet
5.43086544 fees

Miner say's thanks ... 💰

rustyrussell · 2019-10-10T05:58:59Z

~~Let me rebase this first...~~ That got messy fast. I'll just review as-is...

rustyrussell · 2019-10-10T06:17:25Z

openingd/openingd.c

@@ -113,6 +113,15 @@ struct state {
 	u32 feerate_per_kw_funding;
 };

+static struct amount_sat total_funding(struct state *state)


const struct state *state. ABC: Always Be Const.

rustyrussell · 2019-10-10T06:17:47Z

openingd/openingd.c

+{
+	struct amount_sat total;
+	assert(amount_sat_add(&total, state->opener_funding,
+			      state->accepter_funding));


it's a bad habit to wrap active code in assert(), because if you define NDEBUG, assert() vanishes along with its contents.

prefer a simple if(!xxx) abort(); or status_failed().

rustyrussell · 2019-10-10T06:21:03Z

openingd/openingd.c

@@ -1350,7 +1355,8 @@ static u8 *handle_master_in(struct state *state)
 		if (!fromwire_opening_funder_start(msg, &state->funding,
 						   &state->push_msat,
 						   &state->feerate_per_kw,
-						   &channel_flags))
+						   &channel_flags,
+						   &state->use_v2))


For the benefit of the next patch, we should init accepter_funding to AMOUNT_SAT(0) if !use_v2.

rustyrussell · 2019-10-10T06:22:56Z

lightningd/opening_control.c

+					    "Cannot fund a v2 channel with external tx.",
+					    type_to_string(tmpctx, struct bitcoin_txid, funding_txid),
+					    funding_txout);
+		tx = utx->tx;


Yech. We were too soon with fundchannel_start API, it seems :(

rustyrussell · 2019-10-10T06:32:22Z

Partial review: ack 51d903f

If we can't broadcast the transaction, we free the utx anyway. You're going to have to start over regardless.

see title.

Previously we've used the term 'funder' to refer to the peer paying the fees for a transaction; v2 of openchannel will make this no longer true. Instead we rename this to 'opener', or the peer sending the 'open_channel' message, since this will be universally true in a dual-funding world.

plus tests, with some of the corruption tests taken out, since it crashes. will be re-enabled in the next commit.

Since wires can now have TLVs, we need to change up the truncation check to account for the fact that truncated TLVs are kosher.

niftynei · 2019-10-10T20:09:22Z

lightningd/opening_control.c

@@ -736,14 +741,22 @@ static void opening_fundee_finished(struct subd *openingd,
 		goto failed;
 	}

+	assert(amount_sat_add(&total_funding, opener_funding, accepter_funding));


this needs to be moved to a fail/abort also

we need a way to tell openingd to use v2 for channel open.

We've now got two amounts we need to keep track of: the opener's funding and the accpeter's. We add a utlity to help keep track of the full funding. Also wires in the calls for open/accept v2 for when we're the opener.

so we can refer to it later when doing 'complete' things

for v2 we need to know the inputs/outputs of the transaction so we look them up. there's more problems here that we need to unwind still: figuring out which the funding output is plus how to handle change.

going to need to re-use this later.

We're not using the change_outnum for withdraw tx's (and the way we were calculating it was broken as of the addition of 'multiple outputs'). This removes the change output knowhow from withdraw_tx entirely, and pushes the responsibility up to the caller to include the change output in the output set if desired. Consequently, we also remove the change output knowhow from hsmd.

Make it such that we can include other inputs on a transaction, sort them, and sign the ones we need. We're going to need this for dual-funded transactions.

pull up output parser into reusable method

Function and helpers for making a dual funded funding tx

Update some assumtions of wallet_commit_channel to better take the dual-funded reality into account

dual funding needs an output who's value is set to zero in order to correctly specify change for the opener. this modifies txprepare to allow for generating such an output

allows us to use them in wire definitions other than where they are defined.

We'll need it to represent to user in `listpeers`

Allow the utxo object to bear the scriptSig as well; also fixes broken scriptPubkey parsing on utxo

We need the change amount to be correct when we send the tx, so we save the change amount and pipe it through.

method plus wire calls

Allow openingd to call hmsd for dual_funding tx's (accepter flow)

When we're the accepter, we need to be able to tell a plugin hook what the total available sats we have for an open channel offer. This new wallet method will calculate the total amount of sats we have available, based on our top X highest valued utxos.

Update fundchannel to correctly update the txid for txsend and to know whether or not we're using v2 (needed for knowing whether or not to use the zero_output flag)

Calculate the correct funding amounts to display for dual funded channels.

Send info that we need for doing the right thing in the openchannel plugin back to opening_control

Implements accepter side of df

Adds fields that are only relevant for v2 of openchannel protocol to the openchannel hook.

Add documentation for new openchannel fields to doc/PLUGINS.md

rustyrussell

Some minor cleanups, but I'd like this in for -rc1 please!

Ack db88f68

rustyrussell · 2019-10-10T22:12:24Z

common/funding_tx.c

+		input_weight += inputs[i]->max_witness_len;
+		weight += input_weight;
+
+		assert(amount_sat_add(total, *total, inputs[i]->input_satoshis));


Not assert(): see previous "no code inside assert()" advice.

But what if they were to get a tx through to us claiming BIGNUM satoshis incoming? You need to be explicit and return bool here, all the way through the callchain.

rustyrussell · 2019-10-10T22:13:14Z

common/funding_tx.c

+	for (i = 0; i < tal_count(outputs); i++) {
+		if (amount_sat_eq(outputs[i]->output_satoshis, AMOUNT_SAT(0)))
+			return outputs[i];
+	}


And if they were to give us a 0-value output? That's certainly possible...

~~are OP_RETURN outputs zero-value?~~ 🤔

The way that the 'change' output is specified is via a zero-value output in the opener's change set; it's specified in the spec.

rustyrussell · 2019-10-10T22:13:39Z

common/funding_tx.c

+	struct amount_sat total = AMOUNT_SAT(0);
+
+	for (i = 0; i < tal_count(outputs); i++) {
+		assert(amount_sat_add(&total, total, outputs[i]->output_satoshis));


Definitely needs to hand back the failure...

rustyrussell · 2019-10-10T22:15:15Z

common/funding_tx.c

+	}
+}
+
+struct bitcoin_tx *dual_funding_funding_tx(const tal_t *ctx,


I think this is missing a few BOLT quotes, too. That way if the spec changes, you will get a hint as to where to change things.

rustyrussell · 2019-10-10T22:15:42Z

common/funding_tx.h

@@ -5,6 +5,9 @@
 #include <ccan/short_types/short_types.h>
 #include <ccan/tal/tal.h>
 #include <common/amount.h>
+#if EXPERIMENTAL_FEATURES
+#include <wire/gen_peer_wire.h>
+#endif /* EXPERIMENTAL_FEATURES */


Don't need #if around includes (do we?)

rustyrussell · 2019-10-10T22:41:49Z

wallet/walletrpc.c

@@ -179,6 +179,7 @@ static struct command_result *json_prepare_tx(struct command *cmd,
 	u32 *feerate_per_kw = NULL;
 	struct command_result *result;
 	u32 *minconf, maxheight;
+	bool *zero_out_change = NULL;


Don't need to initialize?

rustyrussell · 2019-10-10T22:41:54Z

wallet/walletrpc.c


 		changekey = tal(tmpctx, struct pubkey);
 		if (!bip32_pubkey(cmd->ld->wallet->bip32_base, changekey,
 				  (*utx)->wtx->change_key_index))
 			return command_fail(cmd, LIGHTNINGD, "Keys generation failure");

-		change_output = new_tx_output(outputs, (*utx)->wtx->change,
+
+		if (zero_out_change && *zero_out_change)


zero_out_change will be non-NULL, since you used p_opt_def().

rustyrussell · 2019-10-10T22:44:38Z

wallet/db.c

@@ -477,6 +477,7 @@ static struct migration dbmigrations[] = {
 	 ");"), NULL},
    {SQL("ALTER TABLE channels ADD shutdown_scriptpubkey_local BLOB;"),
 	 NULL},
+    {SQL("ALTER TABLE channels ADD COLUMN our_funding_satoshi INTEGER;"), NULL},


Should we initialize it to one of the other fields? Or is the null value a special flag indicating a v1 channel? (If so, a comment here would be great).

rustyrussell · 2019-10-10T22:49:42Z

wallet/wallet.c

+	available = sorted_confirmed_utxos(NULL, w, output_state_available);
+
+	*sat = AMOUNT_SAT(0);
+	/* Mark them all as reserved and sum up total */


I can't see us marking them as reserved here?

this comment was made in error, it's been removed. :D

rustyrussell · 2019-10-10T22:51:53Z

lightningd/opening_control.c

@@ -344,6 +344,8 @@ static void funding_started_success(struct funding_channel *fc,
 		json_add_hex_talarr(response, "scriptpubkey", scriptPubkey);
 	}

+	json_add_string(response, "open_channel_version", fc->is_v2 ? "2" : "1");


json_add_number perhaps?

niftynei added the funding label Oct 1, 2019

niftynei added this to the 0.7.3 milestone Oct 1, 2019

conscott reviewed Oct 1, 2019

View reviewed changes

niftynei force-pushed the nifty/df branch from ba70d8b to 24fb6d4 Compare October 8, 2019 22:23

niftynei marked this pull request as ready for review October 8, 2019 23:13

niftynei requested a review from cdecker as a code owner October 8, 2019 23:13

niftynei force-pushed the nifty/df branch from 24fb6d4 to 438a05e Compare October 8, 2019 23:15

niftynei mentioned this pull request Oct 8, 2019

dual-funding: track and clean up shared txs (III of IV) #3144

Closed

niftynei changed the title ~~WIP experimental: dual-funding~~ experimental: dual-funding (II of IV) Oct 8, 2019

rustyrussell reviewed Oct 10, 2019

View reviewed changes

niftynei added 5 commits October 10, 2019 14:05

walletrpc: free utx on unsuccessful broadcast

ec51067

If we can't broadcast the transaction, we free the utx anyway. You're going to have to start over regardless.

amount: helper method for adding sats to an msat amount

4938ac6

see title.

df: add wires for dual funding messages

1e50e27

plus tests, with some of the corruption tests taken out, since it crashes. will be re-enabled in the next commit.

df: add truncation check for wire messages

64443ac

Since wires can now have TLVs, we need to change up the truncation check to account for the fact that truncated TLVs are kosher.

niftynei force-pushed the nifty/df branch from cedf099 to db88f68 Compare October 10, 2019 19:32

niftynei commented Oct 10, 2019

View reviewed changes

niftynei added 10 commits October 10, 2019 15:27

df: pipe through whether or not we're using v2

b8e6154

we need a way to tell openingd to use v2 for channel open.

df: switch from funding -> opener_funding and wire in v2 calls

903ebdb

We've now got two amounts we need to keep track of: the opener's funding and the accpeter's. We add a utlity to help keep track of the full funding. Also wires in the calls for open/accept v2 for when we're the opener.

df: stash 'is_v2' into fundchannel object

b2970dc

so we can refer to it later when doing 'complete' things

df: lookup transaction for the fundchannel_complete

b74767b

for v2 we need to know the inputs/outputs of the transaction so we look them up. there's more problems here that we need to unwind still: figuring out which the funding output is plus how to handle change.

hsmd: pull up utxo signing

c0f2828

going to need to re-use this later.

withdraw: add ability to have additional inputs on a tx

f2467f1

Make it such that we can include other inputs on a transaction, sort them, and sign the ones we need. We're going to need this for dual-funded transactions.

walletrpc: break out output extraction code

eafbeb9

pull up output parser into reusable method

funding: add method for building a dual-funded tx

b3c5438

Function and helpers for making a dual funded funding tx

opening: update wallet_commit_channel for df

0755cfa

Update some assumtions of wallet_commit_channel to better take the dual-funded reality into account

niftynei added 20 commits October 10, 2019 15:27

txprepare: zero out change if asked

38ed050

dual funding needs an output who's value is set to zero in order to correctly specify change for the opener. this modifies txprepare to allow for generating such an output

zero_out_change: add to pylightning

d24a7c7

wire: add dual-funding subtypes to list

9409550

allows us to use them in wire definitions other than where they are defined.

wallet-df: save our_funds amount to channel record

8c0942f

We'll need it to represent to user in `listpeers`

utxo: add scriptSig field

6602fa7

Allow the utxo object to bear the scriptSig as well; also fixes broken scriptPubkey parsing on utxo

channeld: add flag for whether channel has our funds

0ed1635

df: pass opener's change through so we can update utx

e99893d

We need the change amount to be correct when we send the tx, so we save the change amount and pipe it through.

hsmd: add method for signing dual funded txs

bd55d30

method plus wire calls

hsmd: allow openingd to call hsm to sign dual-funded tx

b776158

Allow openingd to call hmsd for dual_funding tx's (accepter flow)

fundchannel: update to work with dual funding

83becfc

Update fundchannel to correctly update the txid for txsend and to know whether or not we're using v2 (needed for knowing whether or not to use the zero_output flag)

listpeers: show funding_allocation for df'd channels

a35d298

Calculate the correct funding amounts to display for dual funded channels.

df-accepter: pass v2 info back to opening_control

cb4f0bc

Send info that we need for doing the right thing in the openchannel plugin back to opening_control

df-accepter: if payload is v2, calculate available funds

37b6fe1

fundchannel_start: include version of open protocol

df57686

df-accepter: accepter logic

b5f4c78

Implements accepter side of df

openchannel: add dual-funding fields to hook

d6c8317

Adds fields that are only relevant for v2 of openchannel protocol to the openchannel hook.

plugins: add documentation for openchannel v2 fields

c63b5bf

Add documentation for new openchannel fields to doc/PLUGINS.md

fixup b5f4c78

451748d

fixup! f2467f1

5a66dc4

rustyrussell approved these changes Oct 10, 2019

View reviewed changes

niftynei added 5 commits October 10, 2019 18:09

fixup! df-accepter: if payload is v2, calculate available funds

0bf122f

fixup! funding: pass utxos through to openingd

5f5a3a5

fixup! df: lookup transaction for the fundchannel_complete

9b8d540

fixup! df: pipe through whether or not we're using v2

5d72838

fixup! fundchannel: update to work with dual funding

0e88f38

niftynei force-pushed the nifty/df branch from db88f68 to 0e88f38 Compare October 10, 2019 23:11

niftynei modified the milestones: 0.7.3, 0.7.4 Oct 11, 2019

niftynei closed this Oct 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

experimental: dual-funding (II of IV) #3122

experimental: dual-funding (II of IV) #3122

niftynei commented Oct 1, 2019 •

edited

conscott Oct 1, 2019

Saibato commented Oct 1, 2019

rustyrussell commented Oct 10, 2019 •

edited

rustyrussell Oct 10, 2019

rustyrussell Oct 10, 2019

rustyrussell Oct 10, 2019

rustyrussell Oct 10, 2019

niftynei Oct 10, 2019

rustyrussell commented Oct 10, 2019

niftynei Oct 10, 2019

rustyrussell left a comment

rustyrussell Oct 10, 2019

rustyrussell Oct 10, 2019

niftynei Nov 13, 2019 •

edited

rustyrussell Oct 10, 2019

rustyrussell Oct 10, 2019

rustyrussell Oct 10, 2019

rustyrussell Oct 10, 2019

rustyrussell Oct 10, 2019

niftynei Nov 14, 2019

rustyrussell Oct 10, 2019

rustyrussell Oct 10, 2019

niftynei Oct 10, 2019

rustyrussell Oct 10, 2019

experimental: dual-funding (II of IV) #3122

experimental: dual-funding (II of IV) #3122

Conversation

niftynei commented Oct 1, 2019 • edited

Choose a reason for hiding this comment

Saibato commented Oct 1, 2019

rustyrussell commented Oct 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustyrussell commented Oct 10, 2019

Choose a reason for hiding this comment

rustyrussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niftynei Nov 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niftynei commented Oct 1, 2019 •

edited

rustyrussell commented Oct 10, 2019 •

edited

niftynei Nov 13, 2019 •

edited