wallet: add asset coin locking #431

guggero · 2023-08-08T18:13:22Z

~~Depends on #429.~~

Fixes #261.

This PR introduces two new "coin locks":

One on the wallet level (similar to btcwallet's WithCoinSelectLock) that allows multiple DB calls to be made while holding an exclusive lock (e.g. do coin selection then "lease" a specific coin to spend, those cannot be done in a single DB transaction the way the code is structured)
One on the managed_utxo level, which is called a "lease" (to reflect the terminology used in lnd: An application can "lease" (=lock/reserve) a bitcoin level UTXO to be spent in a transaction. So all assets committed to that BTC output are not available for other RPC users as long as the UTXO is being leased.

TODOs (might decide to put those into a second PR):

~~Allow releasing a lease (unlock coin) through RPC~~
~~Allow filtering assets by leased=true/false~~

Roasbeef

Great work! Changes look to be headed in the right direction at a high level. I think my major review comment relates to exactly where the coin selection lock should live it. Right now it lives at the bottom of the abstraction layer in the db, which also means we now have a mutex in the way of db code. In the past (eg lnd), we've run into issues with user space mutexes on top of what the db is already doing. I think we should try to lift the mutex up to the highest level, understanding that some calls may just want a snapshot of the db, and don't necessarily need the same level of contextual locking as needed when trying to perform a transfer.

Also as is, it looks like an output can be locked twice. If the lease are a new table, and then we use an outpoint as the primary key, we can reject such behavior as the unique conflict then fails. Finally, I didn't see where the leases are expired, seems to be sort of implicit rn?

tapdb/assets_store.go

Roasbeef · 2023-08-08T23:30:50Z

tapdb/assets_store.go

+		// valid and hasn't expired.
+		owner := sprout.AnchorLeaseOwner
+		expiry := sprout.AnchorLeaseExpiry
+		if len(owner) > 0 && expiry.Valid &&


How/when are leases to be cleaned up?

This isn't currently done. Not sure what the best approach would be. In btcwallet I think we check the leases on certain actions. We could do the same here as well, e.g. whenever we list the coins or insert a new lease.
Or we could do it with a timed job. Going to see what makes most sense while addressing the other comments.

Ah yeah, we went with the lazy approach in btcwallet. One disadvantage I can think of for the lazy approach is that then every time you want to go fetch just the set of leases, you also need to have that be a write transaction, as you might end up deleting them. This may also impact the decision to keep the leases in the ChainAsset struct as well.

tapdb/assets_store.go

tapdb/sqlc/queries/assets.sql

tapdb/assets_store_test.go

tapfreighter/chain_porter.go

tapfreighter/wallet.go

tapdb/sqlc/queries/assets.sql

guggero · 2023-08-10T15:07:48Z

Latest push addresses most comments. Though I think I should address the following TODOs before a full re-review:

Add more unit tests around the leases and the CoinSelect struct
~~Periodically (or upon certain requests) remove old leases~~
~~Allow filtering assets by lease status on the RPC, assert leases in itest~~
~~Update lease to not expire after anchoring TX has been published~~

ffranr

Nice work! Looks good to me, but I don't think I've reviewed the very latest changes. I don't have too much to add beyond Laolu's comments.

I think we should probably add an integration test to ensure that the send process fails if all candidate UTXOs are locked.

guggero · 2023-08-10T16:13:13Z

I think we should probably add an integration test to ensure that the send process fails if all candidate UTXOs are locked.

We do that here:

taproot-assets/itest/send_test.go

Line 766 in 814ea4b

// Before we mine the next block, we'll make sure that we get a proper

guggero · 2023-08-14T14:49:02Z

Only the extended unit tests missing now, re-requesting review to make sure this is what you had in mind, @Roasbeef.

Roasbeef

The change set is looking pretty good! Will pull it down and mess around with some of the new functionality on the CLI to see how intuitive/surprising it is

Roasbeef · 2023-08-15T23:32:47Z

tapdb/assets_store.go

-	var assetFilter QueryAssetFilters
+	assetFilter := QueryAssetFilters{
+		Now: sql.NullTime{
+			Time:  time.Now().UTC(),


Not blocking at all here, but I think we'll want to eventually start to use clock.Clock (from lnd) here so we can eventually mock out the time for unit tests.

Cool, makes sense. Added a commit in the beginning.

tapfreighter/chain_porter.go

tapfreighter/wallet.go

In this commit we introduce a clock to each database that uses time.Now() to simplify mocking out those timestamps in the future.

To avoid an issue that we sometimes ran into in lnd where a UTXO isn't locked/leased after a spend transaction is published, we now lease any spent outputs for a whole year, right before we publish the spend TX.

guggero · 2023-08-16T15:14:24Z

Added all unit tests that I wanted to add, ready for another round of review.

ffranr

I think I've managed to review the changes since my last review. Everything looks good to me. Nice addition of the clock to different db stores. And a test which ensures that locking an outpoint affects each asset anchored at that outpoint 👍

ffranr · 2023-08-16T21:11:13Z

tapdb/assets_store.go

+		owner := sprout.AnchorLeaseOwner
+		expiry := sprout.AnchorLeaseExpiry
+		if len(owner) > 0 && expiry.Valid &&
+			expiry.Time.UTC().After(a.clock.Now().UTC()) {


nit: the if condition is getting complicated here.

I would consider evaluating a bool variable before the if clause:

var ( hasOwner = len(owner) > 0 isExpired = expiry.Valid && expiry.Time.UTC().After(a.clock.Now().UTC()) ) if hasOwner && isExpired {

Or maybe not, it's a style.

ffranr · 2023-08-16T21:16:37Z

tapdb/assets_store.go

+	spend *tapfreighter.OutboundParcel, finalLeaseOwner [32]byte,
+	finalLeaseExpiry time.Time) error {


nit: I don't understand why we need the final prefix here in finalLeaseOwner and finalLeaseExpiry. Consider removing?

ffranr · 2023-08-16T21:25:30Z

tapdb/assets_store_test.go

+	// Now we lease the first asset for 1 hour. This will cause the second
+	// one also to be leased, since it's on the same anchor transaction.


Roasbeef

LGTM 🦆

guggero requested review from Roasbeef and ffranr August 8, 2023 18:13

guggero added the v0.3 label Aug 8, 2023

Neil-LL assigned Neil-LL and guggero and unassigned Neil-LL Aug 8, 2023

Roasbeef requested changes Aug 8, 2023

View reviewed changes

guggero force-pushed the asset-coin-locking branch from a3a9d35 to 210ca4b Compare August 10, 2023 13:11

ffranr reviewed Aug 10, 2023

View reviewed changes

tapdb/sqlc/queries/assets.sql Outdated Show resolved Hide resolved

guggero force-pushed the asset-coin-locking branch from 210ca4b to 814ea4b Compare August 10, 2023 15:02

ffranr reviewed Aug 10, 2023

View reviewed changes

guggero force-pushed the asset-coin-locking branch 2 times, most recently from 5c8c75c to 2691375 Compare August 14, 2023 14:47

guggero requested review from Roasbeef and ffranr August 14, 2023 14:49

Roasbeef reviewed Aug 15, 2023

View reviewed changes

guggero force-pushed the asset-coin-locking branch from 2691375 to c3d8874 Compare August 16, 2023 13:54

guggero added 6 commits August 16, 2023 15:54

tapcfg+tapdb: use clock everywhere for easyer mocking

f5ed6eb

In this commit we introduce a clock to each database that uses time.Now() to simplify mocking out those timestamps in the future.

tapdb: add lease to managed UTXO

da12c2c

tapcfg+tapfreighter: remove unused CoinSelector

ef023e9

tapfreighter: move coin listing and selecting into CoinSelector

6f33042

tapfreighter: add coin lock mutex to CoinSelect

0e62fa1

tapfreighter: move advanceState closer to assetsPorter

6380ad9

guggero force-pushed the asset-coin-locking branch from c3d8874 to a3884b4 Compare August 16, 2023 13:54

guggero added 3 commits August 16, 2023 15:54

tapfreighter: create goroutine per send package

2e636b7

tapfreighter: actually lease asset outputs after coin selection

c6abc0e

itest: add multi parallel send itest

5f353d2

tapdb+taprpc: allow showing leased assets

fe31079

guggero force-pushed the asset-coin-locking branch from a3884b4 to 93a8a8d Compare August 16, 2023 13:57

guggero added 2 commits August 16, 2023 16:07

tapdb+tapfreighter: hold lease for a year when publishing TX

6500cfc

To avoid an issue that we sometimes ran into in lnd where a UTXO isn't locked/leased after a spend transaction is published, we now lease any spent outputs for a whole year, right before we publish the spend TX.

multi: add RemoveUTXOLease RPC

91c3848

guggero force-pushed the asset-coin-locking branch from 93a8a8d to 91c3848 Compare August 16, 2023 14:07

guggero requested a review from Roasbeef August 16, 2023 15:14

ffranr approved these changes Aug 16, 2023

View reviewed changes

Roasbeef approved these changes Aug 16, 2023

View reviewed changes

Roasbeef added this pull request to the merge queue Aug 16, 2023

Roasbeef removed this pull request from the merge queue due to a manual request Aug 16, 2023

Roasbeef merged commit 6bbe56a into main Aug 16, 2023

guggero deleted the asset-coin-locking branch August 17, 2023 08:04

guggero mentioned this pull request Aug 29, 2023

Tapd stuck and not communicating with tapcli #386

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: add asset coin locking #431

wallet: add asset coin locking #431

guggero commented Aug 8, 2023 •

edited

Loading

Roasbeef left a comment

Roasbeef Aug 8, 2023

guggero Aug 10, 2023

Roasbeef Aug 15, 2023

guggero commented Aug 10, 2023 •

edited

Loading

ffranr left a comment •

edited

Loading

guggero commented Aug 10, 2023

guggero commented Aug 14, 2023

Roasbeef left a comment

Roasbeef Aug 15, 2023

guggero Aug 16, 2023

guggero commented Aug 16, 2023

ffranr left a comment

ffranr Aug 16, 2023

ffranr Aug 16, 2023

ffranr Aug 16, 2023

Roasbeef left a comment

		spend *tapfreighter.OutboundParcel, finalLeaseOwner [32]byte,
		finalLeaseExpiry time.Time) error {

		// Now we lease the first asset for 1 hour. This will cause the second
		// one also to be leased, since it's on the same anchor transaction.

wallet: add asset coin locking #431

wallet: add asset coin locking #431

Conversation

guggero commented Aug 8, 2023 • edited Loading

Roasbeef left a 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

guggero commented Aug 10, 2023 • edited Loading

ffranr left a comment • edited Loading

Choose a reason for hiding this comment

guggero commented Aug 10, 2023

guggero commented Aug 14, 2023

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero commented Aug 16, 2023

ffranr left a 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

Roasbeef left a comment

Choose a reason for hiding this comment

guggero commented Aug 8, 2023 •

edited

Loading

guggero commented Aug 10, 2023 •

edited

Loading

ffranr left a comment •

edited

Loading