Provable asset burning #477

guggero · 2023-08-30T17:37:45Z

Fixes #24.

~~Depends on #482.~~

Still work in progress, main things missing are:

~~More unit tests~~
~~CLI command tapcli assets burn~~
Integration tests with 4 test cases:
- ~~Burn all coins in a commitment (expect error because of edge case)~~
- ~~Burn all assets of one ID while there are other (passive) assets (=> expect root asset for burn)~~
- ~~Burn some assets, get some change back (=> expect split asset for burn)~~
- ~~Burn assets from multiple inputs~~

asset/burn.go

asset/burn_test.go

asset/asset.go

Roasbeef · 2023-08-31T00:00:54Z

rpcserver.go

+
+	// If there is only a single output, it means that we are attempting to
+	// burn all the assets that were selected and that there are no passive
+	// assets either. This would mean we'd create a BTC level output with


Wouldn't this be the same as the tombstone scenario? With the only difference being this new burn NUMs key is used instead of the existing burn/nums key?

Or it's even just a send as "normal", with no need for an anchor at all, as it looks sort of like UTXO consolidation.

Ah, true. But I guess that way we'd end up sending 1k sats to a BTC output that can't be spent anymore (at least not with the current RPCs). I can remove that restriction if we want and create an issue that we'll eventually need a "sweep BTC from outputs with empty commitments/tombstones/burns".

tapfreighter/parcel.go

asset/burn.go

guggero · 2023-09-04T14:21:23Z

Addressed all comments and fixed all TODOs. This is ready for full review.

lightninglabs-deploy · 2023-09-11T14:40:05Z

@Roasbeef: review reminder
@jharveyb: review reminder

Roasbeef · 2023-09-13T02:19:20Z

Pushed up a rebased version.

asset/burn.go

Roasbeef · 2023-09-13T02:26:36Z

tapfreighter/wallet.go

+			Amount:            0,
+			Type:              tappsbt.TypeSplitRoot,
+			AnchorOutputIndex: 0,
+			ScriptKey:         asset.NUMSScriptKey,


Thought tombstones weren't needed for this case? cc @jharveyb

IIUC the tombstone output is added here so that fundPacketWithInputs handles change calculation on building a valid change output if needed. The branch later on handles the full-value burn situation, so we should never end up with an actual tombstone. I think the comment could be a bit more verbose in nothing that the final result will not be a tombstone.

Correct, they aren't. This is just to give the wallet an asset output it can attach any passive assets to. We remove it again later if there are no passive assets. Added a more explicit comment to it.

Roasbeef · 2023-09-13T02:30:34Z

tapfreighter/wallet.go

+	// without an actual commitment in it. So if we are not getting any
+	// change or passive assets in this output, we'll not want to go through
+	// with it.
+	changeOut := fundedPkt.VPacket.Outputs[0]


If the above is unchanged though, then doesn't this lead to us just sending those other funds to the NUMs key?

Yes, but I think from fundPacketWithInputs and the logic right after this, that key will never stay as NUMS if the amount is non-zero; this either becomes a normal split, or the tombstone is removed.

Correct. We might want to eventually refactor the whole wallet code a bit to either make some of the assumptions less implicit or by just documenting them better.

tapdb/assets_store.go

Roasbeef · 2023-09-13T02:37:02Z

tapscript/send.go

+			// assets. Otherwise, for an interactive send we don't
+			// need a tombstone output and this wouldn't be a two
+			// output collectible send.
+			if !rootOut.Type.CanCarryPassive() &&


Needs unit test

Added a unit test.

jharveyb

Looks good, cool to see how the freighter changes are relatively contained!

Gave my nits offline or in existing threads.

jharveyb · 2023-09-18T16:04:55Z

rpcserver.go

+	if in.AmountToBurn == 0 {
+		return nil, fmt.Errorf("amount to burn must be specified")
+	}
+	if in.ConfirmationText != AssetBurnConfirmationText {


jharveyb · 2023-09-18T16:05:28Z

itest/burn_test.go

+	scriptKey3, anchorInternalKeyDesc3 := deriveKeys(t.t, t.tapd)
+	scriptKey4, _ := deriveKeys(t.t, t.tapd)
+
+	// We create the following outputs:


Very helpful for following the flow & cases here!

Because a burn is an interactive send, we don't need to create a tombstone output if it's a full-value send. So in case there are passive assets in the same commitment, we don't have a split root output to hook them to. With the introduction of the new TypeSimplePassiveAssets we have a new way to signal to the send state machine where to commit passive assets to if there is no actual split or tombstone output.

We want to be able to show/list burned assets, so we want to re-create them in the assets table. But to avoid them showing up in any balances, we also mark burned assets as spent and we'll add an extra is_burn flag to the asset output as well on the RPC response.

The same way we prune tombstone asset outputs from the input commitment when spending an anchor output, we also want to remove any burned assets.

In the case where we have an interactive full-value send of a collectible, we would normally only expect a single output. But if there are passive assets to commit to, then there needs to be a second output just for those.

Roasbeef

LGTM 🦜

Roasbeef reviewed Aug 31, 2023

View reviewed changes

guggero mentioned this pull request Aug 31, 2023

[bug]: proof_at_depth is confusing in decode method #480

Closed

guggero linked an issue Aug 31, 2023 that may be closed by this pull request

[bug]: proof_at_depth is confusing in decode method #480

Closed

guggero self-assigned this Aug 31, 2023

guggero force-pushed the provable-asset-burning branch from 2070c37 to a735502 Compare September 1, 2023 17:11

jharveyb reviewed Sep 1, 2023

View reviewed changes

asset/burn.go Show resolved Hide resolved

guggero mentioned this pull request Sep 4, 2023

Small fixes in freighter and wallet #482

Merged

guggero force-pushed the provable-asset-burning branch from a735502 to 8424ea5 Compare September 4, 2023 14:20

guggero marked this pull request as ready for review September 4, 2023 14:20

guggero requested review from Roasbeef and jharveyb September 4, 2023 14:20

guggero force-pushed the provable-asset-burning branch 4 times, most recently from 43eb682 to b9c7662 Compare September 8, 2023 14:45

Roasbeef force-pushed the provable-asset-burning branch from b9c7662 to 49f7cda Compare September 13, 2023 02:19

Roasbeef reviewed Sep 13, 2023

View reviewed changes

jharveyb approved these changes Sep 18, 2023

View reviewed changes

guggero added 9 commits September 18, 2023 18:19

asset+make: add DeriveBurnKey function

6dea989

asset: add IsBurn method

3ebe229

tapfreighter: add FundBurn method to wallet

e7e8149

tapfreighter: prune burned assets from commitment

c653a8e

The same way we prune tombstone asset outputs from the input commitment when spending an anchor output, we also want to remove any burned assets.

taprpc+tapfreighter: add BurnAsset RPC method

97a8fac

cmd/tapcli: add assets burn sub command

018bef3

itest: add burn tests

2d15923

guggero force-pushed the provable-asset-burning branch from 49f7cda to 2d15923 Compare September 18, 2023 16:42

guggero requested a review from Roasbeef September 18, 2023 16:42

Roasbeef approved these changes Sep 19, 2023

View reviewed changes

Roasbeef added this pull request to the merge queue Sep 19, 2023

Roasbeef removed this pull request from the merge queue due to a manual request Sep 19, 2023

Roasbeef merged commit 0c6cf23 into main Sep 19, 2023

guggero deleted the provable-asset-burning branch September 19, 2023 06:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provable asset burning #477

Provable asset burning #477

guggero commented Aug 30, 2023 •

edited by dstadulis

Loading

Roasbeef Aug 31, 2023

Roasbeef Aug 31, 2023

guggero Sep 1, 2023

guggero commented Sep 4, 2023

lightninglabs-deploy commented Sep 11, 2023

Roasbeef commented Sep 13, 2023

Roasbeef Sep 13, 2023

jharveyb Sep 18, 2023

guggero Sep 18, 2023

Roasbeef Sep 13, 2023

jharveyb Sep 18, 2023

guggero Sep 18, 2023

Roasbeef Sep 13, 2023

guggero Sep 18, 2023

jharveyb left a comment

jharveyb Sep 18, 2023

jharveyb Sep 18, 2023

Roasbeef left a comment

Provable asset burning #477

Provable asset burning #477

Conversation

guggero commented Aug 30, 2023 • edited by dstadulis Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero commented Sep 4, 2023

lightninglabs-deploy commented Sep 11, 2023

Roasbeef commented Sep 13, 2023

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

jharveyb left a 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 30, 2023 •

edited by dstadulis

Loading