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

Reserve/unreserve/fundpsbt #3825

Merged

Conversation

rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Jul 8, 2020

(Note: this fails due to libwally failing to marshal 0-output psbts, but should otherwise be complete). -- Fixed in master by @niftynei !

Based on #3821

This changes the (newly-introduced!) reserveinputs and unreserveinputs to be far more primitive, and adds fundpsbt which is a low-level psbt creator. Unfortunately, the rework I had planned which migrates everything else on top of these will not be finished before the release.

Nonetheless, it's better to ship with these APIs now, so progress can continue: shortly after 0.9.0 I will complete the rewrite which makes txprepare/txdiscard/txcancel a plugin on top of these, and @ZmnSCPxj can do the same for multiwithdraw and multifundchannel!

Closes: #3789
Fixes: #3415

@rustyrussell rustyrussell added this to the v0.9.0 milestone Jul 8, 2020
struct bitcoin_tx *tx;

if (!param(cmd, buffer, params,
p_req("satoshi", param_sat_or_all, &amount),
p_req("feerate", param_feerate_val, &feerate_per_kw),
p_opt_def("minconf", param_number, &minconf, 1),
p_opt_def("reserve", param_bool, &reserve, true),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I approve

tx = tx_spending_utxos(cmd, chainparams, utxos,
cmd->ld->wallet->bip32_base,
false, 0, locktime,
BITCOIN_TX_RBF_SEQUENCE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why RBF-flagged? Am I able to actually RBF now?? How does RBF work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can RBF once everyone uses this...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, let me see if my understanding is correct.

To implement RBF, I would have to go all the way up to signpsbt, but I would have to independently extract the transaction and broadcast it, such as by the bcli sendrawtransaction method. Then if I decide to RBF, I would mutate the PSBT directly, then call signpsbt again with the RBFed PSBT, then sendrawtransaction again.

(we cannot call sendpsbt more than once, since it marks consumed coins as spent, with an assertion that the consumed coin must currently be reserved)

Then, if any of the transactions get confirmed onchain, this automatically marks the consumed inputs as spent. Does that happen?

Looking at lightningd/chaintopology.c it seems we have the rough framework to do so, but while it calls into wallet_outpoint_spend, that function does not update the status of UTXOs. It also calls into wallet_extract_owned_outputs, which will get any unknown outputs, but not what a transaction spends.

wallet_update_update_output_status is otherwise only called from within wallet/wallet.c, or from dev-rescan-outputs.

I don't see any changes in wallet/wallet.c that would allow the above flow to be used.

Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj Jul 9, 2020

Choose a reason for hiding this comment

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

And then there is the issue where one version of the transaction is seen by one miner, the other version is seen by the other miner, then we see the block created by the first, but ultimately the second miner gets a second block, so now we have to rewind our chaintopology and destroy UTXOs in that chaintopology and recreate it as we go to the winning branch. Do we have code that handles that? What happens to PSBTs that were spending UTXOs from the losing branch? RBF is hard, so I am wary of signaling it until our codebase can handle all the little problems.

(Indeed, since RBF is signaled if any input signals it, we should also refuse to sign if any input is RBF-signalled, else the transaction is still RBF).

(granted the above problem already exists with incoming transactions that might be RBFed as well, but marking our own transactions RBF seems to me to make that even more likely)

@ZmnSCPxj
Copy link
Collaborator

ZmnSCPxj commented Jul 8, 2020

Does this effectively fix #3415 ? The underlying issue in #3415 is that we want to have an atomic reservation of funds all the way to sign+broadcast without using the current discard+re-prepare trick in fundchannel. My understanding is that it does, in which case this should be tagged "fixes #3415"?

@rustyrussell
Copy link
Contributor Author

Does this effectively fix #3415 ? The underlying issue in #3415 is that we want to have an atomic reservation of funds all the way to sign+broadcast without using the current discard+re-prepare trick in fundchannel. My understanding is that it does, in which case this should be tagged "fixes #3415"?

Yes, good point.

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.

ACK cd65293

* - less distinguishable transactions (with this we create
* general-purpose transactions which looks like bitcoind:
* native segwit, nlocktime set to tip, and sequence set to
* 0xFFFFFFFE by default. Other wallets are likely to implement
Copy link
Collaborator

Choose a reason for hiding this comment

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

nota bene: we set the sequence to 0xFFFFFFFD below

@@ -1248,8 +1248,12 @@ static struct command_result *match_psbt_inputs_to_utxos(struct command *cmd,
wally_tx_input_get_txid(&psbt->tx->inputs[i], &txid);
utxo = wallet_utxo_get(*utxos, cmd->ld->wallet,
&txid, psbt->tx->inputs[i].index);
if (!utxo)
if (!utxo) {
log_debug(cmd->ld->log, "utxo %zu DNE", i);
Copy link
Collaborator

Choose a reason for hiding this comment

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

these seem extraneous?

@rustyrussell
Copy link
Contributor Author

@ZmnSCPxj for some reason I can't quote you on mobile...

Yes, it also requires the change to reserve, rather than mark spent, utxos when we sendpsbt. (This is the Right Thing anyway, in case the tx never confirms).

This is also good, since reserving them twice means we'll extend the reservation, which makes sense.

@ZmnSCPxj
Copy link
Collaborator

ZmnSCPxj commented Jul 10, 2020

@rustyrussell Okay but does spending a utxo we own without passing the spend through wallet/walletrpc.c interfaces work?

One way to implement #475 would be to keep the PSBT around until some transaction confirms. That means keeping the reservation around until the target transaction confirms.

For another matter, suppose we are building a transaction that spends coins owned by multiple parties (e.g. a CoinJoin). So we signpsbt because we happen to have drawn the short straw and must be the one to sign first, send the PSBT off to the next participant, and then we get disconnected from the Internet because gremlins exist, and then the rest of the participants complete the PSBT signing and they all send the completed PSBT to each other and broadcast, but because we have no Internet because gremlins, we cannot receive the completed PSBT (and thus cannot sendpsbt the tx). We only learn about the successful spend later when we see the tx onchain. So maybe signpsbt should extend reservations as well.

string "all" meaning use all unreserved inputs). If a value, it can
be a whole number, a whole number ending in *sat*, a whole number
ending in *000msat*, or a number with 1 to 8 decimal places ending in
*btc*.
Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj Jul 11, 2020

Choose a reason for hiding this comment

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

We should probably mention that:

  • fundpsbt factors in the fees needed for each input it proposes to add.
  • the given satoshi value should include the fee for the outputs, plus the fee for the common parts (nLockTime, nVersion etc) of the transaction.

The latter point implies that plugins need access to the fee estimates. While plugins can call the bcli estimatefees directly, I think it is best if lightningd/chaintopology also exposes the actual feerates it would use, especially since estimatefees is allowed to return null and chaintopology, to my understanding, papers over that case. (I think? @darosior )

Alternately, since e.g. Liquid apparently has explicit fee outputs, maybe we can add another parameter, outputs, which is an array of strings or nulls:

  • An entry can be an address, in which case fundpsbt allocates fees enough for an output going to that address.
  • An entry can be any of the following strings, indicating fundpsbt allocates fees enough for an output going to that kind of address:
    • p2pkh
    • p2pk - or not who uses this nowadays
    • p2sh
    • p2wpkh
    • p2wsh
    • op-return-# where # is a number from 0 to 64 (or whatever the OP_RETURN max is).
  • An entry can be null, in which case fundpsbt will allocate fees for the largest known valid address type scriptPubKey..

Then fundpsbt can consider whether there is some excess and see if it is large enough to (1) pay for its existence as a change output and (2) remains large enough to keep itself above the dust limit. If not, it adds the excess to fees (which it adds as an explicit output for Liquid txes).

With this outputs parameter we can also have fundpsbt consider the common parts of the transaction (nLockTime and friends). This removes any remaining reasons that a robust plugin would want to loop around fundpsbt.

Copy link
Collaborator

@darosior darosior Jul 11, 2020

Choose a reason for hiding this comment

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

The latter point implies that plugins need access to the fee estimates. While plugins can call the bcli estimatefees directly, I think it is best if lightningd/chaintopology also exposes the actual feerates it would use, especially since estimatefees is allowed to return null and chaintopology, to my understanding, papers over that case. (I think? @darosior )

Right, better to use the feerate command which returns a human error in case of estimation failure and smoothed estimation otherwise.

estimatefees could fail (and we allow it to), while feerates would still return a result based on our cached history (if we have one, ie not startup).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, right, feerates. Will go document it for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably mention that:

* `fundpsbt` factors in the fees needed for each input it proposes to add.

* the given `satoshi` value should include the fee for the outputs, plus the fee for the common parts (`nLockTime`, `nVersion` etc) of the transaction.

The next paragraph does exactly this:

You calculate the value by starting with the amount you want to pay
and adding the fee which will be needed to pay for the base of the
transaction plus that output, and any other outputs and inputs you
will add to the final transaction.

With this outputs parameter we can also have fundpsbt consider the common parts of the transaction (nLockTime and friends). This removes any remaining reasons that a robust plugin would want to loop around fundpsbt.

Indeed. My draft txprepare-as-plugin patch does exactly this, and it's pretty simple (now we have the helpers in place!).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed. My draft txprepare-as-plugin patch does exactly this, and it's pretty simple (now we have the helpers in place!).

Is the code in txprepare or in fundpsbt? I cannot find txprepare-as-plugin branch in your repo or in the ElementsProject repo. My concern is that I believe you mentioned deprecation of txprepare, and in any case fundchannel/multifundchannel will want to move away from txprepare anyway. So a command which wraps fundpsbt and handles as well the common and output weight fees would be quite convenient; if txprepare will be deprecated later, it would be useful for both withdraw and fundchannel (and their multi versions) to use that rather than the existing base fundpsbt.

@rustyrussell
Copy link
Contributor Author

OK, addressed minor feedback from @niftynei and rebased onto master now libwally is fixed...

@rustyrussell rustyrussell force-pushed the reserve-unreserve-fundpsbt branch 4 times, most recently from 9b40fa9 to a732f7c Compare July 13, 2020 05:48
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

I'm not exactly clear how all and the bumping of reservations interact. If I have a wallet with unreserved UTXOs a, b, and c, then I reserve a, will a call to fundpsbt bump the previous a reservation, or will it skip a?

I'd expect the latter (a is already reserved for another purpose, and I just want to gather all other UTXOs), but I could not convince myself that that's the case.

@@ -418,6 +421,84 @@ void wallet_confirm_utxos(struct wallet *w, const struct utxo **utxos)
}
}

static void db_set_utxo(struct db *db, const struct utxo *utxo)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: All functions in here have the prefix wallet_.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but that's silly. Particularly for an internal function which literally hits the db. Note that many of these functions are going to be removed once the users are gone (i.e. once fundchannel, withdraw and txprepare are reworked on top of fundpsbt/reservepbst/unreservepsbt).

Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj Jul 14, 2020

Choose a reason for hiding this comment

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

It helps when naming functions and structs to prepend them with a convenient namespace, even if internal. I get annoyed that we have multiple contradictory struct channels, for example, and I have to do extra digging to figure out which one is being used in a particular piece of code.

wallet/wallet.c Outdated Show resolved Hide resolved
wallet/wallet.c Show resolved Hide resolved
doc/lightning-reserveinputs.7.md Outdated Show resolved Hide resolved
doc/lightning-reserveinputs.7.md Outdated Show resolved Hide resolved
wallet/reservation.c Show resolved Hide resolved
doc/lightning-fundpsbt.7.md Outdated Show resolved Hide resolved
@rustyrussell
Copy link
Contributor Author

I'm not exactly clear how all and the bumping of reservations interact. If I have a wallet with unreserved UTXOs a, b, and c, then I reserve a, will a call to fundpsbt bump the previous a reservation, or will it skip a?

I'd expect the latter (a is already reserved for another purpose, and I just want to gather all other UTXOs), but I could not convince myself that that's the case.

The UTXO selection only considers unreserved UTXOs, so it will skip a.

@rustyrussell
Copy link
Contributor Author

rustyrussell commented Jul 14, 2020

OK, fixups pushed for those issues (force push because I cleaned up a commit msg too)

@cdecker
Copy link
Member

cdecker commented Jul 14, 2020

ACK 01672a9

@cdecker cdecker force-pushed the reserve-unreserve-fundpsbt branch from 01672a9 to 1270c3f Compare July 14, 2020 12:47
@cdecker
Copy link
Member

cdecker commented Jul 14, 2020

Rebased on master, no content changes

@cdecker cdecker force-pushed the reserve-unreserve-fundpsbt branch 2 times, most recently from b83f83a to 095f4e6 Compare July 14, 2020 13:17
@cdecker
Copy link
Member

cdecker commented Jul 14, 2020

Sqashed the fixups, had to manually merge a couple since they were conflicting, but the end-to-end diff is identical now.

ACK 095f4e6

@cdecker
Copy link
Member

cdecker commented Jul 14, 2020

Sadly the rebase seems to have broken something somewhere. I'm now getting these errors:

       # Join and add an output
        joint_psbt = bitcoind.rpc.joinpsbts([l1_funding['psbt'], l2_funding['psbt'],
>                                            output_pbst])
tests/test_wallet.py:644: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
contrib/pyln-testing/pyln/testing/utils.py:311: in f
    return proxy._call(name, *args)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
self = <bitcoin.rpc.RawProxy object at 0x7fd9948c7b90>
service_name = 'joinpsbts'
args = (['cHNidP8BAAsCAAAAAAAAaAAAAAA=', 'cHNidP8BAK4CAAAABI3tfktaNA4PBnGH8ZPYfNsRFTjclLsA41ICPDFzo2HOAQAAAAD9////ri3g6ZylJh2...tWBzibHhwEEFgAUoyPCgqmhl/wRjsCc2vIcorsbiHIA', 'cHNidP8BACkCAAAAAAHAxi0AAAAAABYAFMkJbUP0COpSYCAmLM2tfIUWuSqBAAAAAAAA'],)
postdata = '{"version": "1.1", "method": "joinpsbts", "params": [["cHNidP8BAAsCAAAAAAAAaAAAAAA=", "cHNidP8BAK4CAAAABI3tfktaNA4PBn...EFgAUoyPCgqmhl/wRjsCc2vIcorsbiHIA", "cHNidP8BACkCAAAAAAHAxi0AAAAAABYAFMkJbUP0COpSYCAmLM2tfIUWuSqBAAAAAAAA"]], "id": 1}'
headers = {'Authorization': b'Basic cnBjdXNlcjpycGNwYXNz', 'Content-type': 'application/json', 'Host': 'localhost', 'User-Agent': 'AuthServiceProxy/0.1'}
response = {'error': {'code': -22, 'message': 'TX decode failed Size of value was not the stated size: iostream error'}, 'id': 1, 'result': None}
    def _call(self, service_name, *args):
        [...snip...]
        response = self._get_response()
        if response['error'] is not None:
>           raise JSONRPCError(response['error'])
E           bitcoin.rpc.JSONRPCError: {'code': -22, 'message': 'TX decode failed Size of value was not the stated size: iostream error'}

Any ideas what this might be @rustyrussell ?

@niftynei
Copy link
Collaborator

niftynei commented Jul 14, 2020 via email

@cdecker
Copy link
Member

cdecker commented Jul 14, 2020

This is directly from travis here with the configuration as follows:

$ export VALGRIND=1
$ export ARCH=64
$ export DEVELOPER=1
$ export COMPILER=gcc
$ export TEST_GROUP=12
$ export TEST_GROUP_COUNT=12
$ export SOURCE_CHECK_ONLY=false

@cdecker cdecker force-pushed the reserve-unreserve-fundpsbt branch from 095f4e6 to b38e07c Compare July 14, 2020 16:26
@cdecker cdecker force-pushed the reserve-unreserve-fundpsbt branch from b38e07c to 095f4e6 Compare July 14, 2020 16:27
Allow a utxo to be reserved until explicitly unreserved or until a timer
runs out. Currently unused.

We explicitly do not unreserve these at startup.

# Add some of our own PSBT inputs to it
l1_reserved = l1.rpc.reserveinputs(outputs=[{addr: Millisatoshi(3 * amount * 1000)}])
joint_psbt = bitcoind.rpc.joinpsbts([l1_reserved['psbt'], l2_reserved['psbt']])
l1_funding = l1.rpc.fundpsbt(satoshi=Millisatoshi(0),
Copy link
Collaborator

@niftynei niftynei Jul 14, 2020

Choose a reason for hiding this comment

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

Is this intended to be a zero msat fund call? the PSBT that it's returning is invalid.

cHNidP8BAAsCAAAAAAAAZgAAAAA=

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, that was me. Weird, here's the fix I'm applying:

diff --git a/tests/test_wallet.py b/tests/test_wallet.py
index 5afbb518b..8fabd83b0 100644
--- a/tests/test_wallet.py
+++ b/tests/test_wallet.py
@@ -636,7 +636,7 @@ def test_sign_and_send_psbt(node_factory, bitcoind, chainparams):
         l1.rpc.signpsbt(l2_funding['psbt'])
 
     # Add some of our own PSBT inputs to it
-    l1_funding = l1.rpc.fundpsbt(satoshi=Millisatoshi(0),
+    l1_funding = l1.rpc.fundpsbt(satoshi=Millisatoshi(3 * amount * 1000),
                                  feerate=7500,
                                  reserve=True)
 
@@ -696,8 +696,11 @@ def test_sign_and_send_psbt(node_factory, bitcoind, chainparams):
         {'type': 'chain_mvt', 'credit': 0, 'debit': 3000000000, 'tag': 'withdrawal'},
         {'type': 'chain_mvt', 'credit': 0, 'debit': 1000000000, 'tag': 'chain_fees'},
         {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
-        {'type': 'chain_mvt', 'credit': 0, 'debit': 1000000000, 'tag': 'withdrawal'},
-        {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'chain_fees'},
+        {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
+        {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
+        {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
+        {'type': 'chain_mvt', 'credit': 0, 'debit': 3000000000, 'tag': 'withdrawal'},
+        {'type': 'chain_mvt', 'credit': 0, 'debit': 1000000000, 'tag': 'chain_fees'},
     ]
 
     check_coin_moves(l1, 'wallet', wallet_coin_mvts, chainparams)

These keep the struct utxo in sync with the database, explicitly:
these will be the only places where utxo->status is set.

The old routines will be removed at the end.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is a new fundamental routine to obtain UTXOs from the database.

It's not the most efficient approach, as it returns a single UTXO at a
time, but it can consolidate all our UTXO handling (becoming more
complex by the reservation timeout logic).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
reserveinputs marks UTXOs reserved for 12 hours, so we won't select them
for spending: unreserveinputs marks them available again.

Exposes param_psbt() for wider use.

Disabled the test_sign_and_send_psbt since we're altering the API;
the final patch restores it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is the normal case: you only want to reserve inputs which
are not already reserved.  This saves you iterating through the
results and unreserving some if you weren't exclusive.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Technically, they could do this themselves, but it's much nicer to have one
place to do it (and it makes sure we get the required information into the
PSBT, which is actually not entirely accessible through listfunds, as that
doesn't want to consult with the HSM for close outputs).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON RPC: new low-level coin selection `fundpsbt` routine.
It's easier for us to call it atomically than have the user loop and
retry!

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>
It uses reservations heavily, and assumed we generated change, etc.
It's now a simpler test, in many ways.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We're actually going to deprecate this, so don't add new features!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: ***REMOVE*** JSON-API: `txprepare` returns a psbt version of the created transaction
@rustyrussell
Copy link
Contributor Author

Ack d162739

@rustyrussell rustyrussell merged commit 371cabf into ElementsProject:master Jul 15, 2020
fiatjaf pushed a commit to fiatjaf/mcldsp that referenced this pull request Aug 4, 2020
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.

Feature: txmodifypayee a txprepared transaction
5 participants