Dual funding, PSBT work #3902

niftynei · 2020-08-04T18:23:54Z

This PR contains just the PSBT utilities needed for the dual-funding accepter PR.

This allows us to:

add a serial id to psbt inputs + outputs
sort psbt inputs + outputs by serial_id
produce a 'diff' between two psbts (must have serial_ids assigned to inputs + outputs though)

Changelog-None

rustyrussell

Style comments only, and some doc fixes...

rustyrussell · 2020-08-05T02:49:43Z

bitcoin/psbt.c

+	if (wally_tx_init_alloc(WALLY_TX_VERSION_2, 0, num_inputs, num_outputs, &wtx) != WALLY_OK)
+		return NULL;


Abort if this fails. We don't (as a rule) check for allocation failures; in fact, tal() will abort if it can't allocate. Crashing due to NULL on the next line will give us a nice backtrace too :)

rustyrussell · 2020-08-05T02:49:49Z

bitcoin/psbt.c

+				      sizeof(struct bitcoin_txid),
+				      outnum, sequence, NULL, 0, NULL,
+				      &tx_in) != WALLY_OK)
+		return NULL;


rustyrussell · 2020-08-05T02:51:54Z

bitcoin/psbt.c

+					  const u8 *script,
+					  struct amount_sat amount)
+{
+	size_t i = psbt->tx->num_outputs;


Vague preference not to use i as anything but an iterator. In this case, the extra var declaraton is not really worth it, I think.

rustyrussell · 2020-08-05T02:57:28Z

bitcoin/psbt.c

+		struct wally_tx_input *in =
+			&psbt->tx->inputs[i];


Are you sure this doesn't fit in 80 columns on a single line? OTOH, this is more poetic, I guess?

Oh, right. It fits!

rustyrussell · 2020-08-05T02:58:20Z

bitcoin/psbt.h

+ * @num_inputs - number of inputs to allocate
+ * @num_outputs - number of outputs to allocate
+ *
+ * Returns NULL if there's a failure.


Delete this.

rustyrussell · 2020-08-05T03:07:39Z

common/psbt_open.c

+		return false;
+	assert(value_len == sizeof(*serial_id));
+	memcpy(serial_id, result, value_len);
+	return true;


This seems a bit round-about, allocating a key to do the comparison. Either we could implement psbt_get_lightning(), or if we want to be generic, have psbt_get_unknown() take the prefix and the keyvalue?

rustyrussell · 2020-08-05T03:08:06Z

common/psbt_open.c

+	void *result = psbt_get_unknown(map, key, &value_len);
+	if (!result)
+		return false;
+	assert(value_len == sizeof(*serial_id));


I worry this is a vector by which bad PSBTs can crash us. Perhaps return false in this case?

rustyrussell · 2020-08-05T03:10:07Z

common/psbt_open.c

+	void *result = psbt_get_unknown(input->unknowns, key, &value_len);
+	if (!result)
+		return false;
+	assert(value_len == sizeof(*max_witness_len));


rustyrussell · 2020-08-05T03:11:21Z

common/psbt_open.h

+ * If a serial_id has remained the same, but the underlying data
+ * has changed, this WILL NOT detect it.
+ *


Not true any more?

rustyrussell · 2020-08-05T03:12:28Z

common/psbt_open.h

+ * @added_outs - outputs added to {new}
+ * @rm_outs - outputs removed from {new}
+ *
+ * Returns true if no changes are found.


No it doesn't!

Also probably needs warning about sets simply referring into orig and new here, not on implementation?

jgriffiths · 2020-08-06T20:31:26Z

Assuming #3833 is merged (as the wally changes are in master), this PR will need updating to reflect the current wally interface changes. The major ones are:

Maps are all the same type now, see the wally_map_xx functions in wally_psbt.h
You don't need to create new maps if they are empty, just call wally_map_add and it will do the right thing
wally_map_find should replace your hand-rolled search loops
You can now add tx inputs and outputs directly to the psbt at any position and it will update the tx in step.
Some struct members have changed names to reflect the rest of the wally naming, see Jon wally update #3833 for details.

Other comments:

Per Jon wally update #3833 in psbt_append_input you don't need to allocate an input to add it, but you should use the elements init function rather than messing with the features bits yourself (they are an impl detail its better if you don't poke around in)
#define WALLY_PSBT_PROPRIETARY 0xFC you can just use PSBT_PROPRIETARY_TYPE from wally_psbt.h

niftynei · 2020-08-07T04:39:36Z

Ok, updated to the latest things.~~There's a failing unit test I'll have to address tomorrow.~~

libwally now verifies that transactions have inputs before agreeing to serialize them, which impacted how linearize_outputs works. We now add an empty input before attempting serialization.

There's an outstanding issue here still in that our comparison functions will fail to detect maps with identical yet differently ordered entries. Filed ElementsProject/libwally-core#214.

jgriffiths · 2020-08-08T10:22:11Z

bitcoin/psbt.c

+		abort();
+
+	tx_in->features = chainparams->is_elements ? WALLY_TX_IS_ELEMENTS : 0;
+	input = psbt_add_input(psbt, tx_in, insert_at);


features is an internal impl detail and we may end up using it for more than just elements. please use wally_tx_elements_input_init_alloc for elements instead.

ack, updated

jgriffiths · 2020-08-08T10:33:29Z

common/psbt_open.c

+		/* If is P2SH, redeemscript must be present */
+		const u8 *outscript =
+			wally_tx_output_get_script(tmpctx,
+				&input->utxo->outputs[psbt->tx->inputs[i].index]);


I think you want to assert this is in bounds for the utxo outputs before dereferencing here, just to be sure.

ack, updated

jgriffiths · 2020-08-08T23:31:57Z

Note that your lightning keys and values in maps need to be stored endian-independent - so rather than memcpy for your serial id you'll need to use cpu_to/from_ before copying, or things will go badly wrong when swapping PSBTs with an instance on a difference arch.

…data

includes facilities for - sorting psbt inputs by serial_id - sorting psbt outputs by serial_id - adding a serial_id - getting a serial_id - finding the diffset between two psbts - adding a max_len to a psbt input - getting a max_len from a psbt input

Less code is more code!

There's no stable ordering on unknown serialization, so linearizing identical but mis-ordered unknown data will lead to 'wrong' results. Instead, we just ignore any data that's in the psbt unknown struct. There's probably also problems here with other PSBT maps. Really, this needs a finer grained comparison function .... fuck

@jgriffiths

portability for the win Suggested-By: @jgriffiths

jgriffiths · 2020-08-13T20:10:36Z

common/psbt_open.c

 	psbt->outputs[0] = *out;
 	psbt->num_outputs++;
+	/* Blank out unknowns. These are unordered and serializing
+	 * them might have different outputs for identical data */
+	psbt->outputs[0].unknowns.num_items = 0;


wally_map_sort(&psbt->outputs[0].unknowns) can be used now that its been merged to wally master.

Our psbt input/output comparison functions use serialization to compare the things, but if there's a map with things in it and the map isn't sorted exactly the same, it's highly likely you'll mark an identical inputs as different. To fix this, we sort all the input/output maps before linearizing them.

rustyrussell · 2020-08-18T01:38:38Z

Ack f99dbfa

rustyrussell approved these changes Aug 5, 2020

View reviewed changes

rustyrussell mentioned this pull request Aug 5, 2020

amount_sat/amount_msat audit results #3904

Merged

jgriffiths mentioned this pull request Aug 6, 2020

Jon wally update #3833

Merged

niftynei force-pushed the nifty/dual-fund-psbt-work branch 2 times, most recently from 2c83bf7 to 2ca8b30 Compare August 6, 2020 20:55

rustyrussell added the needs-rebase label Aug 7, 2020

niftynei force-pushed the nifty/dual-fund-psbt-work branch 2 times, most recently from 94e5826 to 17e210c Compare August 7, 2020 04:39

niftynei removed the needs-rebase label Aug 7, 2020

niftynei force-pushed the nifty/dual-fund-psbt-work branch 3 times, most recently from 79712de to e9fa0ed Compare August 7, 2020 16:46

jgriffiths reviewed Aug 8, 2020

View reviewed changes

niftynei added 12 commits August 13, 2020 12:18

psbt: add helpers for creating + adding input/outputs

c31f7f1

psbt: utilities for adding unknown info / proprietary c-lightning key…

4f449d0

…data

pubkey: update comment to reflect location

764e512

psbt-common: shared psbt utilities

6971ad4

includes facilities for - sorting psbt inputs by serial_id - sorting psbt outputs by serial_id - adding a serial_id - getting a serial_id - finding the diffset between two psbts - adding a max_len to a psbt input - getting a max_len from a psbt input

psbt: re-use init function for create_psbt

67662d2

Less code is more code!

psbt: rename append_out -> append_output

1a4038d

psbt-common: unit test for diff/sort/add serial etc

abc2e28

psbt: use elements input init alloc

3e7f3e1

tx: use elements_input_init_alloc

7ca23a5

psbt: assertion for index in bounds

17f6dd3

psbt: use platform-independent serialization for psbt fields

9090f78

portability for the win Suggested-By: @jgriffiths

niftynei force-pushed the nifty/dual-fund-psbt-work branch from 9ac2ff9 to 9090f78 Compare August 13, 2020 17:18

jgriffiths reviewed Aug 13, 2020

View reviewed changes

niftynei added 2 commits August 13, 2020 18:19

libwally: update to latest master, includes map sort

ba040cd

niftynei mentioned this pull request Aug 17, 2020

Conversion of withdraw and txprepare to new UTXO primitives (as plugins!) #3867

Merged

rustyrussell merged commit 6371a49 into ElementsProject:master Aug 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dual funding, PSBT work #3902

Dual funding, PSBT work #3902

niftynei commented Aug 4, 2020 •

edited

Loading

rustyrussell left a comment

rustyrussell Aug 5, 2020

rustyrussell Aug 5, 2020

rustyrussell Aug 5, 2020

rustyrussell Aug 5, 2020

niftynei Aug 6, 2020

rustyrussell Aug 5, 2020

rustyrussell Aug 5, 2020

rustyrussell Aug 5, 2020

rustyrussell Aug 5, 2020

rustyrussell Aug 5, 2020

rustyrussell Aug 5, 2020

jgriffiths commented Aug 6, 2020 •

edited

Loading

niftynei commented Aug 7, 2020 •

edited

Loading

jgriffiths Aug 8, 2020

niftynei Aug 13, 2020

jgriffiths Aug 8, 2020

niftynei Aug 13, 2020

jgriffiths commented Aug 8, 2020

jgriffiths Aug 13, 2020

rustyrussell commented Aug 18, 2020

		if (wally_tx_init_alloc(WALLY_TX_VERSION_2, 0, num_inputs, num_outputs, &wtx) != WALLY_OK)
		return NULL;

Dual funding, PSBT work #3902

Dual funding, PSBT work #3902

Conversation

niftynei commented Aug 4, 2020 • edited Loading

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

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

jgriffiths commented Aug 6, 2020 • edited Loading

niftynei commented Aug 7, 2020 • edited Loading

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

jgriffiths commented Aug 8, 2020

Choose a reason for hiding this comment

rustyrussell commented Aug 18, 2020

niftynei commented Aug 4, 2020 •

edited

Loading

jgriffiths commented Aug 6, 2020 •

edited

Loading

niftynei commented Aug 7, 2020 •

edited

Loading