dual funding -> API changes/breakage #5670

niftynei · 2022-10-19T20:55:55Z

Built on #5650, updates to latest + greatest dual-funding spec proposal.

Awaiting interop with ACINQ/Eclair, and requires lnprototest updates (so in draft til these are updated).

Biggest change is that anchors aren't assumed for v2 opens now.

cdecker

Very nice cleanups, looking forward to merging this. All I could find was a slight issue with the ok = chaining used to compute some amount_sat values. It can happen that we clobber an amount with an uninitialized value without noticing if I'm not mistaken.

cdecker · 2022-10-26T10:26:56Z

common/features.c

 	 */
-	{ OPT_DUAL_FUND, OPT_ANCHOR_OUTPUTS },


No explicit signalling needed anymore?

We've removed the implicit dependence on anchor_outputs, instead we just assume that static_remotekey is required.

cdecker · 2022-10-26T10:30:24Z

openingd/dualopend.c

 			state->our_role = TX_INITIATOR;
-		else
+			ok = amount_msat_to_sat(&state->tx_state->opener_funding, our_msat);
+			ok = amount_sat_sub(&state->tx_state->accepter_funding,


You probably want to have ok &= amount_sat_sub otherwise the assignment on line 3965 is pointless. More importantly amount_msat_to_sat doesn't actually change opener_funding in case of a failure, thus one may fail, and amount_sat_sub would use uninitialized values and may succeed.

cdecker · 2022-10-26T10:30:34Z

openingd/dualopend.c

 			state->our_role = TX_ACCEPTER;
+			ok = amount_msat_to_sat(&state->tx_state->accepter_funding, our_msat);
+			ok = amount_sat_sub(&state->tx_state->opener_funding,


See comment above.

cdecker · 2022-10-26T14:40:33Z

connectd/connectd.c

-		msg = cryptomsg_encrypt_msg(tmpctx, cs, take(msg));
-		return io_write(conn, msg, tal_count(msg), io_close_cb, NULL);
+		msg = cryptomsg_encrypt_msg(NULL, cs, take(msg));
+		return io_write_wire(conn, take(msg), io_close_cb, NULL);


Good catch, just verified that io_write_wire takes data, despite not being annotated as such. It uses tal_dup_talarr which is marked as TAKES

niftynei · 2023-01-31T02:41:56Z

test failure unrelated to changes:

Valgrind error file: valgrind-errors.24405
==24405== Conditional jump or move depends on uninitialised value(s)
==24405==    at 0x49E4B56: __vfprintf_internal (vfprintf-internal.c:1516)
==24405==    by 0x49F6519: __vsnprintf_internal (vsnprintf.c:114)
==24405==    by 0x1EC4CB: do_vfmt (str.c:66)
==24405==    by 0x1EC5D8: tal_vfmt_ (str.c:92)
==24405==    by 0x11A356: paymod_log (libplugin-pay.c:167)
==24405==    by 0x11B4D2: payment_chanhints_apply_route (libplugin-pay.c:534)
==24405==    by 0x11E9B9: payment_compute_onion_payloads (libplugin-pay.c:1707)
==24405==    by 0x11FF6C: payment_continue (libplugin-pay.c:2135)
==24405==    by 0x1245E0: adaptive_splitter_cb (libplugin-pay.c:3800)
==24405==    by 0x11FF13: payment_continue (libplugin-pay.c:2123)
==24405==    by 0x12061E: retry_step_cb (libplugin-pay.c:2301)
==24405==    by 0x11FF13: payment_continue (libplugin-pay.c:2123)
==24405==

Changelog-None

You can now pick a different amount during the RBF phase

We use them in the RBF case!

We're gonna reuse it in dualopend.

Re-use validation from openingd

There's two anchor flags, we should check both. Also have dualopend check this as well!

Only add the anchor channel_type if it's negotiated separately!

Otherwise we'd have to update the liquidity ads spec to get this shipped.

The `tmpctx` is free'd before the error is read out/sent over the wire; there's a call that will copy the array before sending it, let's use that instead and take() the object? ------------------------------- Valgrind errors -------------------------------- Valgrind error file: valgrind-errors.2181501 ==2181501== Syscall param write(buf) points to unaddressable byte(s) ==2181501== at 0x49E4077: write (write.c:26) ==2181501== by 0x1C79A3: do_write (io.c:189) ==2181501== by 0x1C80AB: do_plan (io.c:394) ==2181501== by 0x1C81BA: io_ready (io.c:423) ==2181501== by 0x1CA45B: io_loop (poll.c:453) ==2181501== by 0x118593: main (connectd.c:2053) ==2181501== Address 0x4afb158 is 40 bytes inside a block of size 140 free'd ==2181501== at 0x483F0C3: free (vg_replace_malloc.c:872) ==2181501== by 0x1D103C: del_tree (tal.c:421) ==2181501== by 0x1D130A: tal_free (tal.c:486) ==2181501== by 0x1364B8: clean_tmpctx (utils.c:172) ==2181501== by 0x1266DD: daemon_poll (daemon.c:87) ==2181501== by 0x1CA334: io_loop (poll.c:420) ==2181501== by 0x118593: main (connectd.c:2053) ==2181501== Block was alloc'd at ==2181501== at 0x483C855: malloc (vg_replace_malloc.c:381) ==2181501== by 0x1D0AC5: allocate (tal.c:250) ==2181501== by 0x1D1086: tal_alloc_ (tal.c:428) ==2181501== by 0x1D124F: tal_alloc_arr_ (tal.c:471) ==2181501== by 0x126204: cryptomsg_encrypt_msg (cryptomsg.c:161) ==2181501== by 0x11335F: peer_connected (connectd.c:318) ==2181501== by 0x118A8A: peer_init_received (peer_exchange_initmsg.c:135) ==2181501== by 0x1C751E: next_plan (io.c:59) ==2181501== by 0x1C8126: do_plan (io.c:407) ==2181501== by 0x1C8168: io_ready (io.c:417) ==2181501== by 0x1CA45B: io_loop (poll.c:453) ==2181501== by 0x118593: main (connectd.c:2053) ==2181501== { <insert_a_suppression_name_here> Memcheck:Param write(buf) fun:write fun:do_write fun:do_plan fun:io_ready fun:io_loop fun:main } --------------------------------------------------------------------------------

we've removed the EXPERIMENTAL_DUAL_FUND requirement

liquidity ads (as proposed) rely on the CSV addition to the to_remote output that anchors introduced.

There's no reason not to use the channel-types (same as v1s) for v2 opens. Brings us into compliance with ACINQ's implementation afaict

state isn't kept around for lease amount, so it should fail

Don't forget the requested lease across restarts.

We need to be able to only use non-wrapped inputs for v2/interactive tx protocol. Changelog-Added: JSONRPC: `fundpsbt` option `nonwrapped` filters out p2sh wrapped inputs

v2 opens require you to use native segwit inputs Changelog-Added: JSONRPC: `upgradewallet` command, sweeps all p2sh-wrapped outputs to a native segwit output

They're not allowed!

rustyrussell · 2023-02-04T04:59:39Z

Ack 62af76d

niftynei force-pushed the nifty/dual-fund-updates branch from 31dab9a to 2ec6bd1 Compare October 20, 2022 22:09

cdecker reviewed Oct 26, 2022

View reviewed changes

niftynei force-pushed the nifty/dual-fund-updates branch from 2ec6bd1 to bd2de7b Compare November 22, 2022 23:26

niftynei force-pushed the nifty/dual-fund-updates branch from bd2de7b to db2b6a2 Compare December 1, 2022 22:01

niftynei marked this pull request as ready for review December 1, 2022 22:11

niftynei mentioned this pull request Dec 1, 2022

dual-funding -> use tx-abort now #5767

Merged

niftynei force-pushed the nifty/dual-fund-updates branch from db2b6a2 to 9e3e006 Compare December 2, 2022 21:07

niftynei force-pushed the nifty/dual-fund-updates branch from 9e3e006 to 6716b5a Compare December 16, 2022 18:35

niftynei force-pushed the nifty/dual-fund-updates branch 2 times, most recently from f3f4018 to 0c82054 Compare January 17, 2023 20:32

niftynei force-pushed the nifty/dual-fund-updates branch 2 times, most recently from 4fefc5d to 6969341 Compare January 31, 2023 01:14

m-schmoock and others added 17 commits February 2, 2023 16:18

dual-fund: update extracted CSVs to latest bolt draft

0a8d942

Changelog-None

dual-fund: update to latest, add in updates to rbf amounts

12843d4

You can now pick a different amount during the RBF phase

dual-fund: on re-init, re-populate opener_funding/accepter_funding

0b00be7

We use them in the RBF case!

df tests: use the amount from the logs to check for!

1d50947

openingd: pull out validation for shutdown script

811720f

We're gonna reuse it in dualopend.

dual-fund: validate upfront shutdown using taproot + anchors

a9249db

Re-use validation from openingd

opening: helper for anchor flagged, use in dualopend also

c92fdb2

There's two anchor flags, we should check both. Also have dualopend check this as well!

dual-fund: check features to print (anchors not assumed)

f4a3de7

dual-fund: remove anchor assumption for all dual-funded channels

db88b7b

Only add the anchor channel_type if it's negotiated separately!

dual-fund: only allow for liquidity ads if both nodes support anchors

3e937d3

Otherwise we'd have to update the liquidity ads spec to get this shipped.

tests: anchors is only EXPERIMENTAL_FEATURES

a7da33d

we've removed the EXPERIMENTAL_DUAL_FUND requirement

tests: check that non-anchor opens can't use liquidity ads

90d7d25

liquidity ads (as proposed) rely on the CSV addition to the to_remote output that anchors introduced.

dual-fund: patch in channel_type logic

4f5b8a7

There's no reason not to use the channel-types (same as v1s) for v2 opens. Brings us into compliance with ACINQ's implementation afaict

test: restart node during rbf

a887dcd

state isn't kept around for lease amount, so it should fail

dual-open-rbf: remember the requested lease amount btw restarts

8b70405

Don't forget the requested lease across restarts.

fundpsbt: add option to filter out wrapped p2sh inputs

04340c5

We need to be able to only use non-wrapped inputs for v2/interactive tx protocol. Changelog-Added: JSONRPC: `fundpsbt` option `nonwrapped` filters out p2sh wrapped inputs

niftynei added 2 commits February 2, 2023 16:18

upgradewallet: JSONRPC call to update p2sh outputs to a native segwit

3f8bec1

v2 opens require you to use native segwit inputs Changelog-Added: JSONRPC: `upgradewallet` command, sweeps all p2sh-wrapped outputs to a native segwit output

v2 opens: don't use p2sh inputs for v2 opens

62af76d

They're not allowed!

niftynei force-pushed the nifty/dual-fund-updates branch from 6969341 to 62af76d Compare February 2, 2023 22:18

rustyrussell merged commit 278fa7a into ElementsProject:master Feb 4, 2023

endothermicdev mentioned this pull request Feb 6, 2023

peer transient failures in master #5958

Closed

vincenzopalazzo mentioned this pull request Feb 6, 2023

cln required a dualfunding update rustyrussell/lnprototest#70

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dual funding -> API changes/breakage #5670

dual funding -> API changes/breakage #5670

niftynei commented Oct 19, 2022

cdecker left a comment

cdecker Oct 26, 2022

niftynei Nov 14, 2022

cdecker Oct 26, 2022

cdecker Oct 26, 2022

cdecker Oct 26, 2022

niftynei commented Jan 31, 2023

rustyrussell commented Feb 4, 2023

dual funding -> API changes/breakage #5670

dual funding -> API changes/breakage #5670

Conversation

niftynei commented Oct 19, 2022

cdecker left a comment

Choose a reason for hiding this comment

cdecker Oct 26, 2022

Choose a reason for hiding this comment

niftynei Nov 14, 2022

Choose a reason for hiding this comment

cdecker Oct 26, 2022

Choose a reason for hiding this comment

cdecker Oct 26, 2022

Choose a reason for hiding this comment

cdecker Oct 26, 2022

Choose a reason for hiding this comment

niftynei commented Jan 31, 2023

rustyrussell commented Feb 4, 2023