Dual-funding, accepter side only #3973
Conversation
common/subdaemon.h
Outdated
| @@ -2,6 +2,8 @@ | |||
| #define LIGHTNING_COMMON_SUBDAEMON_H | |||
| #include "config.h" | |||
| #include <common/daemon.h> | |||
| #include <stdbool.h> | |||
|
|
|||
There was a problem hiding this comment.
? Doesn't seem to belong in this commit?
openingd/dual_fund_roles.h
Outdated
| enum dual_fund_roles { | ||
| ACCEPTER, | ||
| OPENER, | ||
| NUM_DF_RULES |
There was a problem hiding this comment.
I tend to put the NUM outside, so you can switch() on the other values without complaint. i.e.
#define DUAL_FUND_ROLES_NUM (OPENER + 1)
openingd/dualopend.c
Outdated
| u16 serial_id; | ||
| u8 *msg; | ||
|
|
||
| if ((count = tal_count((*set)->added_ins)) && count > 0) { |
There was a problem hiding this comment.
Overloaded if is ugly, and test in redundant anyway: if the first is true the second must also be true.
openingd/dualopend.c
Outdated
| * - MUST fail the transaction collaboration if: | ||
| * ... | ||
| * - it receives an unconfirmed input | ||
| */ |
There was a problem hiding this comment.
This mattered when we weren't receiving the whole tx, as we looked up the tx on-chain to assure segwitness. Now that we're sending the whole tx, you're right, we can relax it.
openingd/dualopend.c
Outdated
| * ... | ||
| * | ||
| * - The input's sequence number will be set to 0xFDFFFFFF | ||
| * (little endian), which signals RBF. |
There was a problem hiding this comment.
Assume this is fixed later?
openingd/dualopend.c
Outdated
| u16 input_serial; | ||
| if (!psbt_get_serial_id(&psbt->inputs[i].unknowns, | ||
| &input_serial)) { | ||
| peer_failed(state->pps, &state->channel_id, |
There was a problem hiding this comment.
Wrong msg, isn't this an internal error if there's somehow no serial?
There was a problem hiding this comment.
This needs a guard rail to make sure the parity is valid only for their inputs/outputs; it's an error on our part if the input missing a serial is ours... hm
openingd/dualopend.c
Outdated
| * ... | ||
| * - it recieves a duplicate `serial_id` | ||
| */ | ||
| if (psbt_has_serial_input(psbt, serial_id)) |
There was a problem hiding this comment.
Instead, implement psbt_find_serial_input() which returns -1 if not found, otherwise an index.
This makes remove much simpler, too.
And similarly for _output.
| tx = pull_bitcoin_tx(state, &tx_bytes, &len); | ||
| if (!tx) | ||
| peer_failed(state->pps, &state->channel_id, | ||
| "Invalid tx sent."); |
There was a problem hiding this comment.
If len != 0 also fail (implies junk at the end)
lightningd/dual_open_control.c
Outdated
| for (size_t i = 0; i < psbt->num_inputs; i++) { | ||
| if (!psbt_get_serial_id(&psbt->inputs[i].unknowns, &serial_id)) | ||
| fatal("dual funding PSBT must have serial_id for each " | ||
| "input, none found for input %zu", i); |
There was a problem hiding this comment.
Since we get this psbt from openingd, we MUST NOT trust it. This should log_broken and return NULL.
lightningd/dual_open_control.c
Outdated
| if (stack_index == 0) | ||
| return tal_free(stacks); | ||
|
|
||
| if (!tal_resize(&stacks, stack_index)) |
lightningd/dual_open_control.c
Outdated
| for (size_t i = 0; i < psbt->num_inputs; i++) { | ||
| if (!psbt_get_serial_id(&psbt->inputs[i].unknowns, &serial_id)) | ||
| fatal("dual funding PSBT must have serial_id for each " | ||
| "input, none found for input %zu", i); |
lightningd/dual_open_control.c
Outdated
| rcvd->pps, | ||
| rcvd->commitment_msg, | ||
| fwd_msg_2, false); | ||
| tal_free(ws); |
There was a problem hiding this comment.
prefer tmpctx for this, TBH. Or take() in peer_start_channeld (if it supports that?)
lightningd/dual_open_control.c
Outdated
|
|
||
| /* Sort first so stacks are ordered correctly */ | ||
| psbt_sort_by_serial_id(psbt); | ||
|
|
There was a problem hiding this comment.
Can we insist that openingd does this? Then this (and caller) can take const wally_psbt *...
There was a problem hiding this comment.
Good point -- if we permute the PSBT at this point it'll invalidate any signatures we've gotten. It shouldn't be updated/mutated by the signer. We can check for this.
channeld/channel_wire.csv
Outdated
| @@ -57,6 +57,8 @@ msgdata,channel_init,final_scriptpubkey,u8,final_scriptpubkey_len | |||
| msgdata,channel_init,flags,u8, | |||
| msgdata,channel_init,init_peer_pkt_len,u16, | |||
| msgdata,channel_init,init_peer_pkt,u8,init_peer_pkt_len | |||
| msgdata,channel_init,init_peer_pkt_2_len,u16, | |||
| msgdata,channel_init,init_peer_pkt_2,u8,init_peer_pkt_2_len | |||
There was a problem hiding this comment.
commit 01f9955 can be merged with the commit two prior...
| * are now two ways to do it, we save the derived channel id. | ||
| */ | ||
| static void fillin_missing_channel_id(struct lightningd *ld, struct db *db, | ||
| const struct ext_key *bip32_base) |
There was a problem hiding this comment.
I'm sure you could implement derive_channel_id in SQL!
(I AM JOKING, do not try this!)
There was a problem hiding this comment.
I'm sure you could implement derive_channel_id in SQL!
😟
common/utxo.c
Outdated
| * plus the key (33) plus the key len (1) | ||
| * 71 + 1 + 33 + 1 = 106 */ | ||
| /* FIXME: will this be true post anchors ? */ | ||
| 71 + 1 + 33 + 1); |
There was a problem hiding this comment.
I don't think this is true for delayed-to-us outputs? And yes, we'll need the witness script for anchor outputs...
| @@ -95,7 +95,7 @@ | |||
| - The `nLocktime` for the transaction has already been communicated, or is | |||
| established via convention. | |||
| - The transaction version is 2. | |||
| - The each input's sequence number will be set to 0xFEFFFFFF (little endian), which signals RBF. | |||
| - The each input's sequence number will be set to 0xFDFFFFFF (little endian), which signals RBF. | |||
There was a problem hiding this comment.
This is not true, right? And weird... 0xFFFFFFFE is the bitcoind default, and anyway this is now set as per tx_add_input?
And endian probably shouldn't be mentioned here anyway: you're describing the value, not the representation.
There was a problem hiding this comment.
This should have gone away when I added the sequence to the spec, will update.
niftynei
force-pushed
the
nifty/df-accept
branch
3 times, most recently
from
01d5d3e
to
babee9b
Compare
niftynei
force-pushed
the
nifty/df-accept
branch
3 times, most recently
from
1f2fb57
to
ba27a5e
Compare
| if (msg) | ||
| goto sendmsg; | ||
|
|
||
| /* If we don't have any changes cached, go ask Alice for |
There was a problem hiding this comment.
"Bob, just one question. WTF is Alice?"
There was a problem hiding this comment.
the plugin, in this case! lol
| * - MUST fail the transaction collaboration if: | ||
| * ... | ||
| * - it receives a duplicate input to one it sent previously | ||
| */ |
There was a problem hiding this comment.
But there are many things it shouldn't be sending us. Why check this one?
|
Hmm, Travis fails because it tries to build the dual_openingd wiregen file as !EXPERIMENTAL. I'll think of something. |
|
Let me now just do the rebase... |
The bits of a transaction that are paid by the opener!
v2 channel open uses a different method to derive the channel_id, so now we save it to the database so that we dont have to remember how to derive it for each. includes a migration for existing channels
We're going to reuse all this code for dualopend, which is coming soon.
We'll re-use it for dualopend!
v2 of channel open uses the channel revocation basepoints to calculate the channel_id, instead of the funding_txid + outnum Moving away from the funding_txid opens the way for splicing + rbf
imported from ??? TODO: FIXME niftynei/lightning-rfc:nifty/interactive-dual-funding
rustyrussell
force-pushed
the
nifty/df-accept
branch
from
ba27a5e
to
7e3f2fb
Compare
oof. needs validation.
The accepter has to send 2 messages over to channeld to send at start -- their commitment_signatures and tx_signatures
v2 of channel establishment, in the accpeter case, now sends 2 messages to our peer after saving the information to disk (our commitment signatures and our funding transaction signatures)
wherein we add the dual_open_control functions
turn off until we're ready to test both sides
dual funding needs the max-witness-len and utxo fields set for every input. we should add them when we create a 'fundpsbt', so that every psbt that c-lightning generates is dual-funding ready
FIXME: requires wallycore
At some point, it's ok to add more extra info to a psbt and still not have that be counted as 'diff'd.
it's just neater if it's not all wrapped up together, simplifies the interface a smidge
Greatly simplify the changeset API. Instead of 'diff' we simply generate the changes. Also pulls up the 'next message' method, as at some point the interactive tx protocol will be used for other things as well (splices/closes etc) Suggested-By: @rustyrussell
Cleans up some awkward spots in the code, makes the footprint a bit neater Suggested-By: @rustyrussell
We can use a fixed value and close the channel if they don't cover their amount; this wasn't really helping with anything other than setting a floor for an expected feerate
rustyrussell
force-pushed
the
nifty/df-accept
branch
from
7e3f2fb
to
a8acab8
Compare
Hello, it hath arrived. This is just the accepter side of the "anyone can pay" channel open protocol. It includes a half (literally) working implementation of the protocol, the 'accepter' side.
The spec is still in draft, so this is liable to change in future revisions. The option flag is currently not signaled (even if built in experimental), as the opener side does not exist.
Still to come:
Edit: Requires #3971 to build as --enable-experimental