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

rustyrussell · 2020-07-23T07:11:34Z

~~On top of #3845~~
~~On top of #3958~~

I've not touched fundchannel: it currently uses txprepare and friend and should instead use fundpsbt/utxopsbt, but I know @ZmnSCPxj wants to convert fundchannel to multifundchannel (Yes!). Then txprepare etc should be deprecated.

But this shows just how much code we can get rid of from the wallet, which is nice.

cdecker · 2020-08-13T11:13:08Z

Converted to draft since #3845 is still work in progress.

niftynei

code looks good to me, need to check the travis failures though

niftynei · 2020-08-17T18:58:01Z

plugins/txprepare.c

+	tal_free(utx->psbt);
+	utx->psbt = json_tok_psbt(utx, buf, psbttok);
+	/* Replace with signed tx. */
+	tal_free(utx->tx);


i'm not seeing where this gets replaced with the signed version. does it happen in psbt_txid?

niftynei · 2020-08-17T19:01:51Z

wallet/reservation.c

+	u8 *scriptSig, *scriptPubkey, *redeemscript;
+	struct bitcoin_tx *tx;
+
+	/* FIXME: Currently the easiest way to get a PSBT is via a tx */


#3902 contains a create_psbt method

Thanks! It's missing an nLocktime arg, so I've appended that to the series.

niftynei · 2020-08-17T19:19:50Z

tests/test_misc.py

@@ -486,10 +486,17 @@ def is_p2wpkh(output):


 def test_withdraw_misc(node_factory, bitcoind, chainparams):
+    def dont_spent_outputs(n, txid):


Suggested change

def dont_spent_outputs(n, txid):

def dont_spend_outputs(n, txid):

ZmnSCPxj · 2020-08-18T03:54:42Z

We currently implement BIP69 via common/permute_tx.c. Is there an equivalent API that works on PSBTs? It looks like this PR has a regression, in that inputs and outputs are no longer sorted in BIP69 order (I think?). In particular it is not shuffled, either, meaning it is the same order as the parameters passed in, I think.

rustyrussell · 2020-08-18T05:24:29Z

Yes, we preserve PSBT order, BUT:

change is added in random place.
fundpsbt selects UTXOs in random order.

Which I think is The Right Thing...

ZmnSCPxj · 2020-08-18T06:38:27Z

I would suggest shuffling the outputs --- you do not want to inadvertently leak any specific ordering the caller has, in case the caller does not shuffle.

Also, deserves a ChangeLog entry that we are no longer using BIP69 for withdrawal transactions.

ZmnSCPxj · 2020-08-18T10:00:36Z

Also utxopsbt does not shuffle inputs too.

rustyrussell · 2020-08-18T11:03:59Z

Also utxopsbt does not shuffle inputs too.

No, that really is the caller's responsibility. You might have sighash single sigs for example....

rustyrussell · 2020-08-20T05:19:22Z

I would suggest shuffling the outputs --- you do not want to inadvertently leak any specific ordering the caller has, in case the caller does not shuffle.

Also, deserves a ChangeLog entry that we are no longer using BIP69 for withdrawal transactions.

Yes, there should be one:

Changelog-Changed: JSON-RPC: `withdraw` now randomizes input and output order, not BIP69.

?

rustyrussell · 2020-08-20T05:21:19Z

OK, fixed the typo in the tests, appended a change to use create_psbt rather than via bitcoin_tx(), and rebased on top of #3958 which allows that.

If all is set, diff may not be: ==1230518== Use of uninitialised value of size 8 ==1230518== at 0x4C5781B: _itoa_word (_itoa.c:179) ==1230518== by 0x4C736F4: __vfprintf_internal (vfprintf-internal.c:1687) ==1230518== by 0x4C88119: __vsnprintf_internal (vsnprintf.c:114) ==1230518== by 0x1D44B6: do_vfmt (str.c:66) ==1230518== by 0x1D45A0: tal_vfmt_ (str.c:92) ==1230518== by 0x1D4401: tal_fmt_ (str.c:44) ==1230518== by 0x15D30F: fmt_amount_sat (amount.c:60) ==1230518== by 0x15D338: fmt_amount_sat_ (amount.c:62) ==1230518== by 0x178C45: type_to_string_ (type_to_string.c:35) ==1230518== by 0x1B8F75: json_fundpsbt (reservation.c:394) ==1230518== by 0x12D0EC: command_exec (jsonrpc.c:602) ==1230518== by 0x12D598: rpc_command_hook_callback (jsonrpc.c:712) Signed-off-by: Rusty Russell <[email protected]>

This uses `fundpsbt` and similar to simulate the txprepare command. It has one difference (when complete), in that it those reservations are now timed and don't get reset on restart. It also doesn't have the restriction that `all` can only be used with no other output, as I didn't realize that when I implemented it! Note that change is now inserted in a random position, not sorted into BIP69 order. Signed-off-by: Rusty Russell <[email protected]>

Very similar to fundpsbt, but takes 'utxos' instead of 'minconf'. Signed-off-by: Rusty Russell <[email protected]>

…ing funds. This is useful, and also makes us pass the testsuite. Signed-off-by: Rusty Russell <[email protected]>

Signed-off-by: Rusty Russell <[email protected]>

Some minor phrasing differences cause test changes. Signed-off-by: Rusty Russell <[email protected]> Changelog-Changed: txprepare reservations stay across restarts: use fundpsbt/reservepsbt/unreservepsbt Changelog-Removed: txprepare `destination` `satoshi` argument form removed (deprecated v0.7.3)

Signed-off-by: Rusty Russell <[email protected]>

We only need a subset of its functionality, so reproduce that here. Signed-off-by: Rusty Russell <[email protected]>

Marking spent means if the transaction doesn't confirm for some reason, the user will need to force a rescan to find the funds. Now we have timed reservations, reserving for (an additional) 12 hours should be sufficient. We also take this opportunity (now we have our own callback path) to record the tx in the wallet only on success. Signed-off-by: Rusty Russell <[email protected]>

Simple moveonly change for next patch. Signed-off-by: Rusty Russell <[email protected]>

This is a little lazy, but simpler than extracting the common parts or making withdraw a plugin which calls txprepare (which should be deprecated soon in favor of fundpsbt etc). Signed-off-by: Rusty Russell <[email protected]>

… failure. Signed-off-by: Rusty Russell <[email protected]>

First, simplify: amount is set to 1000000, but then we deposit 1000000 + 0.01btc (i.e. 2000000), and we always use 2 * amount. Just use a single constant to make it clear. Secondly, we assume that the wallet considers outputs spent as soon as we created the tx: this will not be true once withdraw uses sendpsbt. So, we generate blocks, but now sometimes withdraw will pick up change txs, so we need to reserve them to avoid that messing our coinmovements. Finally, we assumed the withdrawl order was BIP69, which becomes variable. Signed-off-by: Rusty Russell <[email protected]>

These tests appear in both test_misc.py and test_wallet.py. I checked, and they're the only duplcates; they were moved in 0543149 then re-added (merge error?) in 67fc8ff. Signed-off-by: Rusty Russell <[email protected]>

This removes the reservation cleanup at startup, too, now they're all using 'reserved_til'. This changes test_withdraw, since it asserted that outputs were marked spent as soon as we broadcast a transaction: now they're reserved until it's mined. Similarly, test_addfunds_from_block assumed we'd see funds as soon as we broadcast the tx. Signed-off-by: Rusty Russell <[email protected]> Changelog-Changed: JSON-RPC: `withdraw` now randomizes input and output order, not BIP69.

It's now only needed by devtools/mkfunding, so include a reduced one there, and this also means we remove tx_spending_utxos(). Signed-off-by: Rusty Russell <[email protected]>

It's 0 for old dbs, which is the same as "available". Signed-off-by: Rusty Russell <[email protected]>

Signed-off-by: Rusty Russell <[email protected]>

@niftynei

… via bitcoin_tx. Suggested-by: @niftynei Signed-off-by: Rusty Russell <[email protected]>

rustyrussell · 2020-08-28T03:57:13Z

Trivial rebase, fix for python warning.

so the ones that registered db_write hook are kept alive until after the last db transaction issue ElementsProject#4785 TODO: - maybe revert PR ElementsProject#3867, i.e. remove obsolete db wrapper arround freeing jsonrpc the wrapper was added in PR ElementsProject#1755, but since PR ElementsProject#3867 nothing touches the db at this point?

since PR ElementsProject#3867 utxos are unreserved by height, destroy_utxos and related functions are not used anymore so clean them up also TODO: Can test_stop_pending_fundchannel be removed ?

…ite's anymore since PR ElementsProject#3867 utxos are unreserved by height, destroy_utxos and related functions are not used anymore so clean them up also However free(ld->jsonrpc) still needs to happen before free(ld) because its destructors need list_head pointers from ld

…ite's anymore since PR #3867 utxos are unreserved by height, destroy_utxos and related functions are not used anymore so clean them up also However free(ld->jsonrpc) still needs to happen before free(ld) because its destructors need list_head pointers from ld

rustyrussell requested review from niftynei and ZmnSCPxj July 23, 2020 07:11

rustyrussell requested a review from cdecker as a code owner July 23, 2020 07:11

ZmnSCPxj mentioned this pull request Jul 29, 2020

Planning for SPV backend #3858

Open

cdecker marked this pull request as draft August 13, 2020 11:12

rustyrussell force-pushed the guilt/utxo-selection-cleanup branch 3 times, most recently from 91e98ef to 012f0c8 Compare August 14, 2020 05:35

rustyrussell marked this pull request as ready for review August 14, 2020 05:35

rustyrussell force-pushed the guilt/utxo-selection-cleanup branch from 012f0c8 to 3982d52 Compare August 17, 2020 01:48

niftynei reviewed Aug 17, 2020

View reviewed changes

rustyrussell force-pushed the guilt/utxo-selection-cleanup branch from 3982d52 to 48f39a6 Compare August 20, 2020 05:18

ZmnSCPxj mentioned this pull request Aug 20, 2020

Multiwithdraw #3812

Merged

rustyrussell force-pushed the guilt/utxo-selection-cleanup branch 3 times, most recently from 1e15734 to 3f43008 Compare August 21, 2020 03:47

rustyrussell added 6 commits August 28, 2020 12:00

plugins/txprepare: use utxopsbt to handle utxos arg.

143abd6

Very similar to fundpsbt, but takes 'utxos' instead of 'minconf'. Signed-off-by: Rusty Russell <[email protected]>

reservation: return FUNDING_STILL_SYNCING_BITCOIN if we might be miss…

f52c311

…ing funds. This is useful, and also makes us pass the testsuite. Signed-off-by: Rusty Russell <[email protected]>

txprepare: add txdiscard functionality.

cb02964

Signed-off-by: Rusty Russell <[email protected]>

txprepare: add txsend functionality.

2f99fbc

Signed-off-by: Rusty Russell <[email protected]>

rustyrussell added 14 commits August 28, 2020 12:07

walletrpc: remove now-unused txprepare parsing code.

64f4502

Signed-off-by: Rusty Russell <[email protected]>

wallet: wean fundpsbt off tx_spending_utxos.

c4c0270

We only need a subset of its functionality, so reproduce that here. Signed-off-by: Rusty Russell <[email protected]>

plugins/txprepare: move functions higher.

00f455f

Simple moveonly change for next patch. Signed-off-by: Rusty Russell <[email protected]>

plugins/txprepare: create simple variant for "withdraw".

b613d58

This is a little lazy, but simpler than extracting the common parts or making withdraw a plugin which calls txprepare (which should be deprecated soon in favor of fundpsbt etc). Signed-off-by: Rusty Russell <[email protected]>

pytest: allow variable-order coin_moves, and give more information on…

e0123d1

… failure. Signed-off-by: Rusty Russell <[email protected]>

pytest: remove duplicate tests.

d3bfe91

These tests appear in both test_misc.py and test_wallet.py. I checked, and they're the only duplcates; they were moved in 0543149 then re-added (merge error?) in 67fc8ff. Signed-off-by: Rusty Russell <[email protected]>

common: remove funding_tx.

dd0a4cc

It's now only needed by devtools/mkfunding, so include a reduced one there, and this also means we remove tx_spending_utxos(). Signed-off-by: Rusty Russell <[email protected]>

utxo: make reserved_til a u32 not a ptr, now it's compsulory.

321ea8a

It's 0 for old dbs, which is the same as "available". Signed-off-by: Rusty Russell <[email protected]>

utxo: expose is_reserved, make enum constants upper case.

532ce8c

Signed-off-by: Rusty Russell <[email protected]>

wellet: use create_psbt and psbt_append_input instead of constructing…

75bbaa1

… via bitcoin_tx. Suggested-by: @niftynei Signed-off-by: Rusty Russell <[email protected]>

rustyrussell force-pushed the guilt/utxo-selection-cleanup branch from 3f43008 to 75bbaa1 Compare August 28, 2020 03:57

rustyrussell added this to the v0.9.1 milestone Sep 2, 2020

rustyrussell merged commit 73832d1 into ElementsProject:master Sep 8, 2020

SimonVrouwe mentioned this pull request Sep 13, 2021

lightningd question: Can we expect db_write's after a "shutdown" notification? #4785

Closed

SimonVrouwe mentioned this pull request Sep 17, 2021

lightningd: shutdown plugin in two steps, db_write hook registered plugins last #4790

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rustyrussell commented Jul 23, 2020 •

edited by cdecker

Loading

cdecker commented Aug 13, 2020

niftynei left a comment

niftynei Aug 17, 2020

niftynei Aug 17, 2020

rustyrussell Aug 20, 2020

niftynei Aug 17, 2020

ZmnSCPxj commented Aug 18, 2020 •

edited

Loading

rustyrussell commented Aug 18, 2020

ZmnSCPxj commented Aug 18, 2020

ZmnSCPxj commented Aug 18, 2020

rustyrussell commented Aug 18, 2020

rustyrussell commented Aug 20, 2020

rustyrussell commented Aug 20, 2020

rustyrussell commented Aug 28, 2020

		@@ -486,10 +486,17 @@ def is_p2wpkh(output):


		def test_withdraw_misc(node_factory, bitcoind, chainparams):
		def dont_spent_outputs(n, txid):

	def dont_spent_outputs(n, txid):
	def dont_spend_outputs(n, txid):

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

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

Conversation

rustyrussell commented Jul 23, 2020 • edited by cdecker Loading

cdecker commented Aug 13, 2020

niftynei left a comment

Choose a reason for hiding this comment

niftynei Aug 17, 2020

Choose a reason for hiding this comment

niftynei Aug 17, 2020

Choose a reason for hiding this comment

rustyrussell Aug 20, 2020

Choose a reason for hiding this comment

niftynei Aug 17, 2020

Choose a reason for hiding this comment

ZmnSCPxj commented Aug 18, 2020 • edited Loading

rustyrussell commented Aug 18, 2020

ZmnSCPxj commented Aug 18, 2020

ZmnSCPxj commented Aug 18, 2020

rustyrussell commented Aug 18, 2020

rustyrussell commented Aug 20, 2020

rustyrussell commented Aug 20, 2020

rustyrussell commented Aug 28, 2020

rustyrussell commented Jul 23, 2020 •

edited by cdecker

Loading

ZmnSCPxj commented Aug 18, 2020 •

edited

Loading