-
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?
Changes from all commits
1b52974
bb78e82
2ef2cbe
2964429
f74b150
8120455
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,178 @@ | ||
| package itest | ||
|
|
||
| import ( | ||
| "context" | ||
|
|
||
| "github.com/lightninglabs/taproot-assets/asset" | ||
| "github.com/lightninglabs/taproot-assets/taprpc" | ||
| "github.com/lightninglabs/taproot-assets/taprpc/mintrpc" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // testZeroValueAnchorSweep tests that zero-value anchor outputs | ||
| // are automatically swept when creating new on-chain transactions. | ||
| func testZeroValueAnchorSweep(t *harnessTest) { | ||
| ctxb := context.Background() | ||
|
|
||
| // First, mint some simple asset. | ||
| rpcAssets := MintAssetsConfirmBatch( | ||
| t.t, t.lndHarness.Miner().Client, t.tapd, | ||
| []*mintrpc.MintAssetRequest{simpleAssets[0]}, | ||
| ) | ||
| genInfo := rpcAssets[0].AssetGenesis | ||
| assetAmount := simpleAssets[0].Asset.Amount | ||
|
|
||
| // Create a second tapd node. | ||
| bobLnd := t.lndHarness.NewNodeWithCoins("Bob", nil) | ||
| secondTapd := setupTapdHarness(t.t, t, bobLnd, t.universeServer) | ||
| defer func() { | ||
| require.NoError(t.t, secondTapd.stop(!*noDelete)) | ||
| }() | ||
|
|
||
| bobAddr, err := secondTapd.NewAddr(ctxb, &taprpc.NewAddrRequest{ | ||
| AssetId: genInfo.AssetId, | ||
| Amt: assetAmount, | ||
| AssetVersion: rpcAssets[0].Version, | ||
| }) | ||
| require.NoError(t.t, err) | ||
|
|
||
| // Send ALL assets to Bob, which should create a tombstone. | ||
| sendResp, _ := sendAssetsToAddr(t, t.tapd, bobAddr) | ||
|
|
||
| ConfirmAndAssertOutboundTransfer( | ||
| t.t, t.lndHarness.Miner().Client, t.tapd, sendResp, | ||
| genInfo.AssetId, | ||
| []uint64{0, assetAmount}, 0, 1, | ||
| ) | ||
| AssertNonInteractiveRecvComplete(t.t, secondTapd, 1) | ||
|
|
||
| // Alice should have 1 tombstone UTXO from the full-value send. | ||
| AssertBalances( | ||
| t.t, t.tapd, 0, WithScriptKeyType(asset.ScriptKeyTombstone), | ||
| WithNumUtxos(1), WithNumAnchorUtxos(1), | ||
| ) | ||
|
|
||
| // Test 1: Send transaction sweeps tombstones. | ||
| rpcAssets2 := MintAssetsConfirmBatch( | ||
| t.t, t.lndHarness.Miner().Client, t.tapd, | ||
| []*mintrpc.MintAssetRequest{simpleAssets[0]}, | ||
| ) | ||
| genInfo2 := rpcAssets2[0].AssetGenesis | ||
|
|
||
| // Send full amount of the new asset. This should sweep Alice's | ||
| // first tombstone and create a new one. | ||
| bobAddr2, err := secondTapd.NewAddr(ctxb, &taprpc.NewAddrRequest{ | ||
| AssetId: genInfo2.AssetId, | ||
| Amt: assetAmount, | ||
| AssetVersion: rpcAssets2[0].Version, | ||
| }) | ||
| require.NoError(t.t, err) | ||
|
|
||
| sendResp2, _ := sendAssetsToAddr(t, t.tapd, bobAddr2) | ||
|
|
||
| ConfirmAndAssertOutboundTransfer( | ||
| t.t, t.lndHarness.Miner().Client, t.tapd, sendResp2, | ||
| genInfo2.AssetId, | ||
| []uint64{0, assetAmount}, 1, 2, | ||
| ) | ||
| AssertNonInteractiveRecvComplete(t.t, secondTapd, 2) | ||
|
|
||
| // Check Alice's tombstone balance. The first tombstone should have been | ||
| // swept (spent on-chain as an input), and a new one created. We now | ||
| // have 1 tombstone UTXO (the new one from the second send). | ||
| AssertBalances( | ||
| t.t, t.tapd, 0, WithScriptKeyType(asset.ScriptKeyTombstone), | ||
| WithNumUtxos(1), WithNumAnchorUtxos(1), | ||
| ) | ||
|
|
||
| // Get the new tombstone outpoint. | ||
| utxosAfterSend, err := t.tapd.ListUtxos(ctxb, &taprpc.ListUtxosRequest{ | ||
| ScriptKeyType: &taprpc.ScriptKeyTypeQuery{ | ||
| Type: &taprpc.ScriptKeyTypeQuery_ExplicitType{ | ||
| ExplicitType: taprpc. | ||
| ScriptKeyType_SCRIPT_KEY_TOMBSTONE, | ||
| }, | ||
| }, | ||
| }) | ||
| require.NoError(t.t, err) | ||
| require.Len(t.t, utxosAfterSend.ManagedUtxos, 1) | ||
|
|
||
| // Test 2: Burning transaction sweeps tombstones. | ||
| rpcAssets3 := MintAssetsConfirmBatch( | ||
| t.t, t.lndHarness.Miner().Client, t.tapd, | ||
| []*mintrpc.MintAssetRequest{simpleAssets[0]}, | ||
| ) | ||
| genInfo3 := rpcAssets3[0].AssetGenesis | ||
|
|
||
| // Full burn the asset to create a zero-value burn UTXO | ||
| // and sweep the second tombstone. | ||
| burnResp, err := t.tapd.BurnAsset(ctxb, &taprpc.BurnAssetRequest{ | ||
| Asset: &taprpc.BurnAssetRequest_AssetId{ | ||
| AssetId: genInfo3.AssetId, | ||
| }, | ||
| AmountToBurn: assetAmount, | ||
| ConfirmationText: "assets will be destroyed", | ||
| }) | ||
| require.NoError(t.t, err) | ||
|
|
||
| AssertAssetOutboundTransferWithOutputs( | ||
| t.t, t.lndHarness.Miner().Client, t.tapd, burnResp.BurnTransfer, | ||
| [][]byte{genInfo3.AssetId}, | ||
| []uint64{assetAmount}, 2, 3, 1, true, | ||
| ) | ||
|
|
||
| // Alice should have 0 tombstones remaining and 1 burn UTXO. | ||
| AssertBalances( | ||
| t.t, t.tapd, 0, WithScriptKeyType(asset.ScriptKeyTombstone), | ||
| WithNumUtxos(0), WithNumAnchorUtxos(0), | ||
| ) | ||
| AssertBalances( | ||
| t.t, t.tapd, assetAmount, | ||
| WithScriptKeyType(asset.ScriptKeyBurn), | ||
| WithNumUtxos(1), WithNumAnchorUtxos(1), | ||
| ) | ||
|
|
||
| // Get the burn UTXO outpoint for the next test. | ||
| burnUtxos, err := t.tapd.ListUtxos(ctxb, &taprpc.ListUtxosRequest{ | ||
| ScriptKeyType: &taprpc.ScriptKeyTypeQuery{ | ||
| Type: &taprpc.ScriptKeyTypeQuery_ExplicitType{ | ||
| ExplicitType: taprpc. | ||
| ScriptKeyType_SCRIPT_KEY_BURN, | ||
| }, | ||
| }, | ||
| }) | ||
| require.NoError(t.t, err) | ||
| require.Len(t.t, burnUtxos.ManagedUtxos, 1) | ||
|
|
||
| // Test 3: Send transactions sweeps zero-value burns. | ||
| rpcAssets4 := MintAssetsConfirmBatch( | ||
| t.t, t.lndHarness.Miner().Client, t.tapd, | ||
| []*mintrpc.MintAssetRequest{simpleAssets[0]}, | ||
| ) | ||
| genInfo4 := rpcAssets4[0].AssetGenesis | ||
|
|
||
| // Send partial amount. This should NOT create a tombstone output | ||
| // and sweep the burn UTXO. | ||
| partialAmount := assetAmount / 2 | ||
| bobAddr3, err := secondTapd.NewAddr(ctxb, &taprpc.NewAddrRequest{ | ||
| AssetId: genInfo4.AssetId, | ||
| Amt: partialAmount, | ||
| AssetVersion: rpcAssets4[0].Version, | ||
| }) | ||
| require.NoError(t.t, err) | ||
|
|
||
| sendResp3, _ := sendAssetsToAddr(t, t.tapd, bobAddr3) | ||
|
|
||
| ConfirmAndAssertOutboundTransfer( | ||
| t.t, t.lndHarness.Miner().Client, t.tapd, sendResp3, | ||
| genInfo4.AssetId, | ||
| []uint64{partialAmount, partialAmount}, 3, 4, | ||
| ) | ||
| AssertNonInteractiveRecvComplete(t.t, secondTapd, 3) | ||
|
|
||
| // The burn UTXO should have been swept. | ||
| AssertBalances( | ||
| 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. agree but to test this we would need to start |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2671,8 +2671,17 @@ func (r *rpcServer) AnchorVirtualPsbts(ctx context.Context, | |
| prevID.OutPoint.String()) | ||
| } | ||
|
|
||
| // 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) | ||
| } | ||
|
|
||
|
Comment on lines
+2674
to
+2680
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
This I believe should be a daemon-level flag and should extend all the way to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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:
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:
|
||
| resp, err := r.cfg.ChainPorter.RequestShipment( | ||
| tapfreighter.NewPreSignedParcel(vPackets, inputCommitments, ""), | ||
| tapfreighter.NewPreSignedParcel( | ||
| vPackets, inputCommitments, zeroValueInputs, "", | ||
| ), | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error requesting delivery: %w", err) | ||
|
|
@@ -3785,7 +3794,8 @@ func (r *rpcServer) BurnAsset(ctx context.Context, | |
|
|
||
| resp, err := r.cfg.ChainPorter.RequestShipment( | ||
| tapfreighter.NewPreSignedParcel( | ||
| fundResp.VPackets, fundResp.InputCommitments, in.Note, | ||
| fundResp.VPackets, fundResp.InputCommitments, | ||
| fundResp.ZeroValueInputs, in.Note, | ||
| ), | ||
| ) | ||
| if err != nil { | ||
|
|
||
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