Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Jul 23, 2020

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 cdecker marked this pull request as draft August 13, 2020 11:12
@cdecker
Copy link
Member

cdecker commented Aug 13, 2020

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

@rustyrussell rustyrussell force-pushed the guilt/utxo-selection-cleanup branch 3 times, most recently from 91e98ef to 012f0c8 Compare August 14, 2020 05:35
@rustyrussell rustyrussell marked this pull request as ready for review August 14, 2020 05:35
Copy link
Collaborator

@niftynei niftynei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

u8 *scriptSig, *scriptPubkey, *redeemscript;
struct bitcoin_tx *tx;

/* FIXME: Currently the easiest way to get a PSBT is via a tx */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3902 contains a create_psbt method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ZmnSCPxj
Copy link
Collaborator

ZmnSCPxj commented Aug 18, 2020

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
Copy link
Contributor Author

Yes, we preserve PSBT order, BUT:

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

Which I think is The Right Thing...

@ZmnSCPxj
Copy link
Collaborator

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
Copy link
Collaborator

Also utxopsbt does not shuffle inputs too.

@rustyrussell
Copy link
Contributor Author

Also utxopsbt does not shuffle inputs too.

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

@rustyrussell
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@ZmnSCPxj ZmnSCPxj mentioned this pull request Aug 20, 2020
@rustyrussell rustyrussell force-pushed the guilt/utxo-selection-cleanup branch 3 times, most recently from 1e15734 to 3f43008 Compare August 21, 2020 03:47
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 <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
Very similar to fundpsbt, but takes 'utxos' instead of 'minconf'.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…ing funds.

This is useful, and also makes us pass the testsuite.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Some minor phrasing differences cause test changes.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
We only need a subset of its functionality, so reproduce that here.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
Simple moveonly change for next patch.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
… failure.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
It's 0 for old dbs, which is the same as "available".

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
… via bitcoin_tx.

Suggested-by: @niftynei
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
Copy link
Contributor Author

Trivial rebase, fix for python warning.

@rustyrussell rustyrussell added this to the v0.9.1 milestone Sep 2, 2020
@rustyrussell rustyrussell merged commit 73832d1 into ElementsProject:master Sep 8, 2020
SimonVrouwe added a commit to SimonVrouwe/lightning that referenced this pull request Sep 17, 2021
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?
SimonVrouwe added a commit to SimonVrouwe/lightning that referenced this pull request Nov 1, 2021
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 ?
SimonVrouwe added a commit to SimonVrouwe/lightning that referenced this pull request Nov 3, 2021
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 ?
SimonVrouwe added a commit to SimonVrouwe/lightning that referenced this pull request Nov 5, 2021
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 ?
SimonVrouwe added a commit to SimonVrouwe/lightning that referenced this pull request Nov 15, 2021
…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
SimonVrouwe added a commit to SimonVrouwe/lightning that referenced this pull request Nov 17, 2021
…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
SimonVrouwe added a commit to SimonVrouwe/lightning that referenced this pull request Nov 18, 2021
…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
SimonVrouwe added a commit to SimonVrouwe/lightning that referenced this pull request Nov 24, 2021
…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
SimonVrouwe added a commit to SimonVrouwe/lightning that referenced this pull request Nov 26, 2021
…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
rustyrussell pushed a commit that referenced this pull request Nov 30, 2021
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants