Add a 'reservation expiration' mechanism #3789

niftynei · 2020-06-25T00:12:55Z

It's useful to be able to 'retire' reservations on utxos after a certain time lapse. It's also useful to be able to RBF transactions. This PR lets us do both now, by adding two new parameters to reserveinputs.

expire_after which will functionally 'unreserve' a utxo after that many blocks have passed, and
allow_rbf which if set will create a psbt that has all of its inputs set to be rbf'able.

RBF'ability forced a few not so nice state breaks in the output status (you have to be able to select utxos that are marked as 'spent' but not in a block yet, etc). But hey, RBF, here it is!

Built ontop of the as of yet unmerged #3775 (new commits start at 37ba229)

wallet/wallet.c

cdecker · 2020-06-25T15:40:26Z

wallet/wallet.c

+					" WHERE status = ?"
+					" AND reserved_til <= ?"));
+	db_bind_int(stmt, 0, output_status_in_db(output_state_reserved));
+	db_bind_int(stmt, 1, w->ld->topology->tip->height);


Would it make sense to pass in the current blockheight explicitly? Could make it a bit more explicit that expirations are blockheight based.

cdecker · 2020-06-25T15:46:34Z

wallet/wallet.c

+	reserved = expired_reserved_utxos_get(ctx, w);
+	available = wallet_get_utxos(ctx, w, output_state_available);
+
+	rcount = tal_count(reserved);
+	tal_resize(&reserved, rcount + tal_count(available));
+	for (size_t i = 0; i < tal_count(available); i++)
+		reserved[rcount + i] = tal_steal(reserved, available[i]);


Shouldn't we rather just expire the reserved ones, and then query them directly as output_state_available? This way we keep the reservation but are also returning them as available.

yes but then we'd have to clean them up on every new block. Another option would be to remove the 'output_state_available'/ status altogether and just use the reserved_til as a marker of whether or not the utxo is still reserved.

cdecker · 2020-06-25T16:05:35Z

wallet/walletrpc.c

 	 * or 24 blocks */
-	u32 expires_at = cmd->ld->topology->tip->height + 6 * 4;
+	u32 expires_at = cmd->ld->topology->tip->height + 6 * 24;


The commit message needs an update if we change this default value.

wallet/walletrpc.c

cdecker · 2020-06-25T16:32:20Z

wallet/walletrpc.c

+		for (size_t i = 0; i < tal_count(utx->wtx->utxos); i++)
+			wallet_output_reservation_update(cmd->ld->wallet,
+							 utx->wtx->utxos[i],
+							 *expire_at);


Seems to me like this uses the relative expiry as absolute blockheight. Should probably be something like the following:

Suggested change

for (size_t i = 0; i < tal_count(utx->wtx->utxos); i++)

wallet_output_reservation_update(cmd->ld->wallet,

utx->wtx->utxos[i],

*expire_at);

for (size_t i = 0; i < tal_count(utx->wtx->utxos); i++)

wallet_output_reservation_update(cmd->ld->wallet,

utx->wtx->utxos[i],

cmd->ld->topology->tip->height + *expire_at);

Ah yes, good spot.

common/withdraw_tx.c

cdecker · 2020-06-25T16:43:57Z

common/wallet_tx.c

-	if (!*utxos)
-		return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
-							"Could not decode all of the outpoints. The utxos"
-							" should be specified as an array of "
-							" 'txid:output_index'.");


This looks like an unrelated change.

cdecker · 2020-06-25T16:51:55Z

tests/test_wallet.py

+    unreserve_psbt = bitcoind.rpc.createpsbt(unreserve_utxos, [])
+    unres = l1.rpc.unreserveinputs(unreserve_psbt)
+
+    # let's not set the fee high enough, should explodes


Suggested change

# let's not set the fee high enough, should explodes

# let's not set the fee high enough, should explode

cdecker · 2020-06-25T16:53:10Z

tests/test_wallet.py

+    l1.rpc.sendpsbt(signed_psbt)
+    assert bitcoind.rpc.getmempoolinfo()['size'] == 1


Should we check that the new tx is in the mempool? Just checking for the size of the mempool might pass despite the old tx still being in there. bitcoind.rpc.getrawmempool() will give you the txids.

rustyrussell · 2020-06-26T06:01:00Z

Note: after discussing with @niftynei I agree with this concept, but she didn't take it far enough.

Because we didn't expire utxo reservations, we had multiple kludges:

On startup, we unreserve all of them.
We reserve them, but use a destructor to unreserve them.
We mark them spent before we see the transaction in the blockchain, leading to missing funds and rescan if it never confirms.

On digging further, I noticed that with a simple change, we can move utxo.h entirely into the wallet dir. This further allows us to simplify our utxo handling.

So I've cherry picked part of the "reservations expire" commit and started ripping things apart...

rustyrussell · 2020-06-26T03:26:29Z

bitcoin/psbt.c

 	tmp_in = tx->inputs[tx->num_inputs - 1];
 	MAKE_ROOM(tx->inputs, insert_at, tx->num_inputs);
+
 	tx->inputs[insert_at] = tmp_in;

+
    	if (psbt->inputs_allocation_len < tx->num_inputs) {
 		struct wally_psbt_input *p = tal_arr(psbt, struct wally_psbt_input, tx->num_inputs);


Commit cf4054f is empty except this whitespace change, remove

rustyrussell · 2020-06-26T03:32:07Z

wallet/wallet.c

+	if (!db_column_is_null(stmt, 12)) {
+		reserved_til = tal(utxo, u32);
+		*reserved_til = db_column_int(stmt, 12);
+		utxo->reserved_til = reserved_til;
+	}


I would do this as:

u32 reserved_til = db_column_int(stmt, 12); utxo->reserved_til = tal_dup(utxo, u32, &reserved_til);

Allow a utxo to be reserved until explicitly unreserved or until a timer runs out. Currently unused

Check that the reservations stick around after a node restarts

If a utxo is unspent, don't remove the reservation unless it's expired (or no expiration was set on it).

we don't pro-actively 'update' the state of a reserved utxo to available when its lease expired, instead we compute whether or not it's now considered eligible for spending, which is what the 'reserve' flag here really is asking

if you haven't used this lease in 4 hours, it'll be eligible to be used in a different utxo.

Now `reserveinputs` can parse its params on its own, which will let us add more specific params for this command

Changelog-Added: JSON-API: `reserveinputs` has a new parameter, `expire_after`, which can be used to adjust or disable the reservation expiration mechansim (setting it to zero disables it)

Changelog-Added: JSON-API: `reserveinputs` has a new parameter, `allow_rbf`, which will make a RBF'able tx. Defaults to true

Adds an RBF test plus associated utxo state tracking fixes needed to make this possible. Mostly, you *must* specify which inputs you're spending when you attempt to RBF (makes sense) -- these inputs are allowed to be marked as 'spent' (which means we've broadcast a tx spending them) but must not be included in a block yet (which would be indicated by the spendheight) being populated. There's a few other places that 'spending' already spent tx's is problematic, e.g. when we mark a utxo as spent we now allow it to be coming from 'any' status -> spent. We also allow psbtsign to pull up any 'already spent' utxos to be signed again

niftynei · 2020-06-30T19:11:44Z

This will largely be subsumed by work @rustyrussell is doing on the utxo infrastructure, but updated with comments for good measure.

cdecker · 2020-07-07T17:29:36Z

This will largely be subsumed by work @rustyrussell is doing on the utxo infrastructure, but updated with comments for good measure.

Thanks for the heads up 👍

ZmnSCPxj · 2020-07-07T17:54:55Z

This will largely be subsumed by work @rustyrussell is doing on the utxo infrastructure, but updated with comments for good measure.

Thanks for the heads up 👍

I thought this was #3798 , which was withdrawn? The nearest equivalent is #3821, which does not look like it has reservation expiration. What is the status? How does this affect #3763 which apparently need to be blocked for the new UTXO things?

niftynei requested a review from cdecker as a code owner June 25, 2020 00:12

niftynei mentioned this pull request Jun 25, 2020

PSBT RPC calls #3775

Merged

cdecker reviewed Jun 25, 2020

View reviewed changes

rustyrussell reviewed Jun 29, 2020

View reviewed changes

cdecker mentioned this pull request Jun 29, 2020

Potential memory leak in wallet_clean_utxos #3780

Closed

niftynei force-pushed the nifty/reserve-utxo branch from cd392cb to 84b0287 Compare June 30, 2020 18:47

niftynei added 11 commits June 30, 2020 14:09

utxos: add a 'reserved_til' marker for utxos

85ab7a5

Allow a utxo to be reserved until explicitly unreserved or until a timer runs out. Currently unused

test: check reservations are sticky on restart

9cb5583

Check that the reservations stick around after a node restarts

wallet-init: don't clear out still reserved, unspent utxos

01e2a86

If a utxo is unspent, don't remove the reservation unless it's expired (or no expiration was set on it).

reserveinputs: set a 24 block (~4hr) lease on utxo reservations

9a85567

if you haven't used this lease in 4 hours, it'll be eligible to be used in a different utxo.

wallet-test: mark test as passing now

e375d03

walletrpc: refactor how prepare_tx() works, splitting it up

90f3e1e

Now `reserveinputs` can parse its params on its own, which will let us add more specific params for this command

reserveinputs: add parameter to adjust or turn off expiration counter

c53daed

Changelog-Added: JSON-API: `reserveinputs` has a new parameter, `expire_after`, which can be used to adjust or disable the reservation expiration mechansim (setting it to zero disables it)

reserveinputs: build a tx with the sequence set to 'rbf-able'

ccf85a0

Changelog-Added: JSON-API: `reserveinputs` has a new parameter, `allow_rbf`, which will make a RBF'able tx. Defaults to true

cleanup: unused import in hsmd

7eaf29c

niftynei force-pushed the nifty/reserve-utxo branch from 84b0287 to b6e759c Compare June 30, 2020 19:11

niftynei added the Work in Progress label Jun 30, 2020

rustyrussell mentioned this pull request Jul 8, 2020

Reserve/unreserve/fundpsbt #3825

Merged

rustyrussell closed this in #3825 Jul 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a 'reservation expiration' mechanism #3789

Add a 'reservation expiration' mechanism #3789

niftynei commented Jun 25, 2020

cdecker Jun 25, 2020

cdecker Jun 25, 2020

niftynei Jun 30, 2020

cdecker Jun 25, 2020

cdecker Jun 25, 2020

niftynei Jun 30, 2020

cdecker Jun 25, 2020

cdecker Jun 25, 2020

cdecker Jun 25, 2020

rustyrussell commented Jun 26, 2020

rustyrussell Jun 26, 2020

rustyrussell Jun 26, 2020

niftynei commented Jun 30, 2020

cdecker commented Jul 7, 2020

ZmnSCPxj commented Jul 7, 2020

	# let's not set the fee high enough, should explodes
	# let's not set the fee high enough, should explode

		l1.rpc.sendpsbt(signed_psbt)
		assert bitcoind.rpc.getmempoolinfo()['size'] == 1

Add a 'reservation expiration' mechanism #3789

Add a 'reservation expiration' mechanism #3789

Conversation

niftynei commented Jun 25, 2020

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

rustyrussell commented Jun 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niftynei commented Jun 30, 2020

cdecker commented Jul 7, 2020

ZmnSCPxj commented Jul 7, 2020