-
Notifications
You must be signed in to change notification settings - Fork 138
garbage collect zero-value UTXOs #1832
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
base: 0-8-0-staging
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 18382547235Details
💛 - Coveralls |
f3a3bff
to
f55b6c5
Compare
3bd769d
to
6fe11a5
Compare
a8f3ce4
to
afbfebf
Compare
bf3e3ee
to
d812a8e
Compare
d812a8e
to
0ceaf76
Compare
a0a71be
to
1a7100d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit message is incomplete for the commit with subject tapdb: add support for zero-value UTXO sweeping
.
// Fetch zero-value UTXOs that should be swept as additional inputs. | ||
zeroValueInputs, err := r.cfg.AssetStore.FetchZeroValueAnchorUTXOs(ctx) | ||
if err != nil { | ||
return nil, fmt.Errorf("unable to fetch zero-value "+ | ||
"UTXOs: %w", err) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is probably fine as the default behaviour for now, even though it does have privacy implications. Maybe we should add a flag to AnchorVirtualPsbtsRequest
to skip spending zero value inputs? default to: spend zero value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure what to do with this endpoint either. Adding a flag bothers me because zero value utxos are an internal side effect of the protocol and the notion should never be exposed externally so I added the same behavior here as we would in burning or transfers.
Exposing the zero value utxos notion externally implies adding descriptions to the API, adding to the CLI and asking users to be aware of what they are.
Let's see what @GeorgeTsagk says.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zero value utxos are an internal side effect of the protocol
The task of cleaning up UTXOs, regardless of why they exist (whether as an internal side effect of the protocol or not), is separate from anchoring a virtual PSBT. It’s distinct from the primary function of the endpoint. The current change opportunistically extends the endpoint’s core behavior by adding cleanup logic, which introduces privacy implications.
I think we should let the user choose whether to at least skip that cleanup so that the expected (and previous) behaviour is at least still available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this now. I'm not even sure if we should sweep zero value UTXOs in AnchorVirtualPsbts
at all. Maybe it should be a completely distinct cleanup focused endpoint. Maybe for now we can just make this opt-in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current change opportunistically extends the endpoint’s core behavior by adding cleanup logic
I do agree with this and personally hate unintended side effects. I agree we should remove this. Only issue is if some user is always using low-level endpoints to create transactions, then he will never be able to sweep I think. A dedicated endpoint might make sense or a way to get the UTXOs when querying ListUtxos
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping the cleanup integrated with other wallet actions feels more natural. It is also more cost-efficient as we naturally batch on-the-go.
I think we should definitely wrap all these behind a config flag. Whether it should be on/off by default I'm not sure about, we can start with off-by-default and maybe enable in a future release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrap all these behind a config flag.
This I believe should be a daemon-level flag and should extend all the way to tapfreighter
/tapsend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue I see here is we're starting to diverge from the basic need that is avoiding "garbage" to grow indefinitely.
Zero-value UTXOs garbage collection is part of the realm of coin selection. Most Bitcoin wallets perform coin selection on behalf of the user and only few advanced wallets allow selecting and freezing specific UTXOs (like Sparrow). This is even more important for tapd
where the coin selection is tied to the assets.
We initially started with a feature that naturally preserved liveness and safety properties and abstracted from the user the entire notion of zero-value UTXOs, which is a notion internal to the protocol, but now we are:
- Worrying about privacy when the user calls
AnchorVirtualPsbts
if we select zero value UTXOs the same way we do for burn or send. - Adding a global config that defines whether or not zero-value UTXOs are sweeped, which implies that the set of zero-value UTXOs can now grow outside the limits of a transaction, breaking liveness and safety so we need to verify the size of the tx not to exceed the weight limit and maybe other side effects.
Taproot assets inherits from the same coin selection behavior as Bitcoin. If you have a small change UTXO lying around, a simple thing to do is consolidate it when sending a transaction. but the most efficient thing to do can be quite complex. Unless we want to implement advanced coin selection, I say we:
- do not add a global flag
- do not add a flag to the requests
- always sweep in send and burn
- never expose the notion of zero-value UTXOs in the API.
- think about coin selection strategies (optimize fees, minimize utxos, consolidate, etc).
AssertBalances( | ||
t.t, t.tapd, burnAmt+simpleCollectible.Amount+multiBurnAmt, | ||
WithNumUtxos(3), WithNumAnchorUtxos(3), | ||
WithScriptKeyType(asset.ScriptKeyBurn), | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why remove this assertion? (and below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion is not relevant anymore for burns because it sums the amounts of the assets in the UTXOs it finds. Here it's used to verify the amounts burnt but you can't do that anymore unless you keep track of the swept UTXOs which is not the scope of this test.
As an e2e test, you only care that after you burn, the balance of the asset is correctly decreased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another reason to wrap the sweeping behind a flag is to make sure we preserve old behavior and not alter test coverage
this way we'd disable sweeping for tests like this one and not have to delete the assertion
Please rebase on to the latest in |
f4fb800
to
127946d
Compare
965998d
to
83a4116
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome work!
@@ -0,0 +1,2 @@ | |||
-- Add swept flag to managed_utxos table to track when UTXOs have been swept | |||
ALTER TABLE managed_utxos ADD COLUMN swept BOOLEAN NOT NULL DEFAULT FALSE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we also add a golang-level migration step to automatically detect which of our managed utxos are already spent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally not because zero-value anchor UTXOs cannot have been swept by normal usage of tapd
. We're not responsible if a user happened to sweep these UTXOs somehow manually and corrupted the state of his DB.
// Bip32Derivation is the BIP32 derivation info for the internal key. | ||
// This is used to preserve the key derivation through the allocation | ||
// flow so that PSBTs can be properly signed. | ||
Bip32Derivation []*psbt.Bip32Derivation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before, we were not persisting the necessary information in the DB to be able to sign the input for zero value UTXOs. Laolu described the bug here: https://github.com/lightninglabs/taproot-assets/blob/main/docs/debug-handbook/psbt-finalization-failures.md#a-bip32-key-derivation-mismatch.
Specifically the KeyFamily or KeyIndex not preserved when storing keys
.
// Fetch zero-value UTXOs that should be swept as additional inputs. | ||
zeroValueInputs, err := r.cfg.AssetStore.FetchZeroValueAnchorUTXOs(ctx) | ||
if err != nil { | ||
return nil, fmt.Errorf("unable to fetch zero-value "+ | ||
"UTXOs: %w", err) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping the cleanup integrated with other wallet actions feels more natural. It is also more cost-efficient as we naturally batch on-the-go.
I think we should definitely wrap all these behind a config flag. Whether it should be on/off by default I'm not sure about, we can start with off-by-default and maybe enable in a future release.
// Fetch zero-value UTXOs that should be swept as additional inputs. | ||
zeroValueInputs, err := r.cfg.AssetStore.FetchZeroValueAnchorUTXOs(ctx) | ||
if err != nil { | ||
return nil, fmt.Errorf("unable to fetch zero-value "+ | ||
"UTXOs: %w", err) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrap all these behind a config flag.
This I believe should be a daemon-level flag and should extend all the way to tapfreighter
/tapsend
AssertBalances( | ||
t.t, t.tapd, burnAmt+simpleCollectible.Amount+multiBurnAmt, | ||
WithNumUtxos(3), WithNumAnchorUtxos(3), | ||
WithScriptKeyType(asset.ScriptKeyBurn), | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another reason to wrap the sweeping behind a flag is to make sure we preserve old behavior and not alter test coverage
this way we'd disable sweeping for tests like this one and not have to delete the assertion
t.t, t.tapd, 0, WithScriptKeyType(asset.ScriptKeyBurn), | ||
WithNumUtxos(0), WithNumAnchorUtxos(0), | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the config flag that controls whether sweeping is activated will also help us add slightly better coverage here too.
We could accumulate multiple burn UTXOs / tombstones and then turn the flag on and see them all being swept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree but to test this we would need to start tapd
with a given config, stop it in the middle of the test and restart it with a different config. Is this what you mean?
also needs a rebase |
This includes the addition of a "swept" field in the "managed_utxos" table with the corresponding migration and the "MarkManagedUTXOAsSwept" function.
…ng family and index
127946d
to
8120455
Compare
Garbage collect the residue zero-value UTXOs when creating transactions. Zero-value UTXOs occur when creating tombstones or full burns.
Currently, these UTXOs accumulate in the DB and are never cleaned. This PR introduces a garbage collection mechanism to collect these UTXOs and use them as inputs of transactions initiated by
tapd
:The PR adds a new
swept
flag to themanaged_utxo
table because UTXOs are not removed from the table when spent. This flag is also returned by theListUtxos
RPC endpoint.The mechanism preserves the liveness and safety properties, ensuring that zero-value UTXOs can never accumulate in the DB. Adding garbage collection to Mint transactions is not necessary to ensure these properties.
Fixes #514
Note to reviewers
unspent
, keeping the spent outputs in a table calledmanage_utxos
seems like a contradiction, same logic applies for the need of aswept
flag in that table. We should either rename the table or store spent utxos somewhere else?pkScript
is retrieved by querying thechain_txs
table. Is it possible/more efficient to avoid this and somehow recompute it from the data we have?