[feature]: Enhance asset_sum report to clarify conservation

[dstadulis]

Background

Strict, asset-amount conservation checks exists in the tapd VM to prevent undue inflation of asset quantities. However, there may be instances where the daemon reports asset amounts that appear to be double counted, even though they are being properly conserved.

Asset Balance Clarification

To avoid confusion, it is essential to clearly delineate between payment-channel balances and L1-only asset balances. This will ensure that users have a comprehensive understanding of their asset holdings.

Task 1: Ensure Mutual Exclusivity

• Verify that the sum of L1-only asset balances and payment-channel balances are mutually exclusive, i.e., there is
  no overlap or double counting between the two types of balances.
  • Ensure that asset balance sum reports, sum(non-channel asset balances + local_balance)

Comprehensive Asset Sum Calculations

To ensure accuracy in asset sum calculations, we need to investigate the comprehensiveness of these calculations.

Task 2: Investigate asset sum Calculation

• [feature]: Enhance asset_sum report to clarify conservation #1106 (comment) Check if the script_key_has_script_path": true flag is being incorrectly included in the asset sum. (Intuition is no)
  • In situ: Assets have been incorporated in the db (due to some other reason). Confirm that these assets are not being counted erroneously in asset sum.
• Verify that leased assets are not showing up in the asset sum, as they may be subject to different conservation rules. [Completed in https://github.com/Add 'include_leased' flag to ListBalances #1119]

Force Close Scenario

Background

In the scenario where two nodes have a channel
Node 1 force closes
Node 2 (the responder) -- even if it didn't have the channel's entire balance, the node insert the force close transaction, into it's own database
These inserted transaction are added to the database fields that are importing the outputs as well
While these outputs cannot be spent due to the presence of a script path.
But litd/tapd displays these as part of the node's balance today

Solution

make another delineated name space, to track the amounts in order for the wallet balance commands to by default not show these balance, by default

Considerations

These values are used during tests to assert certain outcomes have been met
add a flag to enable reporting in order for assertion tests to have the necessary data available to run tests

[ryanthegentry]

Further detail per debugging with @guggero: "maybe we forgot to not count "script_key_has_script_path": true in the asset sum. or maybe the leased assets still show up there."

Context: the output of tapcli assets balance includes amounts from the tapcli assets list --show_leased output that it shouldn't. Namely channel funding outputs (identified by "script_key_is_local": false, AND script_key_has_script_path": true) that have 6d7cd7ee247587eef86766140262685819deebc034ad80664fb74ec2ad6e11d7 listed as the lease_owner (representing a tapd internal lock`), meaning they've since been spent.

[dstadulis]

Updated original issue with more context

[dstadulis]

@gijswijs Will start task 2 - show balance script key path is true and considering

spend flag
is leased situation
and this is a directly spendable
have for asset list
but not asset sum
a) default values
b) RPC call sum
(asset balance)

[dstadulis]

Reopen as more remediation is needed [edit: here]

[gijswijs]

I confirmed in the code that we do not accidentally count non-local script keys towards the balance.
See:

taproot-assets/tapfreighter/interface.go

Lines 199 to 204 in 65539b6

	// ScriptKeyLocal indicates whether the script key is known to the lnd
	// node connected to this daemon. If this is false, then we won't create
	// a new asset entry in our database as we consider this to be an
	// outbound transfer.
	ScriptKeyLocal bool

And:

taproot-assets/tapdb/assets_store.go

Lines 3020 to 3022 in 65539b6

	skipAssetCreation := !isTombstone && !isBurn &&
	!out.ScriptKeyLocal && !isKnown

And:

taproot-assets/tapdb/assets_store.go

Lines 3039 to 3042 in 65539b6

	if skipAssetCreation {
	continue
	}

When the asset is a tombstone, from a non-local script key, a burn, or derived by the local wallet or explicitly declared to be known the asset creation in the database is skipped and therefor those assets will not turn up in the balance queries.

[gijswijs]

Maybe it is prudent to create a test that checks whether those assets are not created in the database. But maybe that's overkill. @dstadulis ?

If the latter, then consider this issue closed, since Task 1 is more of a UX discussion that we decided should be had further down the line (cc: @Roasbeef )

[gijswijs]

Discussed with @Roasbeef
Add another assertion or two here: https://github.com/lightninglabs/lightning-terminal/blob/0-19-staging/itest/litd_custom_channels_test.go#L264

[dstadulis]

Since un completed items is UX clarification, will deprio a bit to focus on adding testing lightninglabs/lightning-terminal#867

[gijswijs]

It is now confirmed that L1-balances and custom channel balances are not reported mutually exclusive.

The listBalances endpoints use a database query to sum the total value of the assets. Non-local assets should not turn up in those queries. To check whether an asset is local we ask LND. Sadly, there is currently no way to do that in the database. I see two solutions:
A) iterate through all assets returned by ListAssets and use that to calculate the balances. ListAssets already hits LND for each asset, so we know for each asset whether it's local or not.
B) Update the database to include a field indicating whether an asset is local or not. During upsert hit LND once to determine whether the asset is local.
Option A will probably be slow(ish) with large numbers of assets.
Option B requires a go-based db migration, which are 👎

This litd PR depends on this issue being resolved: lightninglabs/lightning-terminal#871

[guggero]

I thought about this quite a bit and have come up with the following plan:

• I don't think we always want to exclude script keys that are not local. For example, you could have a MuSig2 combined internal key, or a NUMS internal key with some tapscript leaves on top. So always filtering by script_key_is_local will probably be too restrictive.
• Filtering by keys that have a script path (script_key_has_script_path) also doesn't fully solve the issue, since that should probably be a flag as well (for example --include_script_paths or something like that).

So what I think we want is to just always exclude any script keys that are the exact key of 0350aaeb166f4234650d84a2d8a130987aeaf6950206e0905401ee74ff3f8d18e6, which is equivalent to tapchannel.NewFundingScriptTree().TaprootKey.SerializeCompressed().

We of course wouldn't hard code that in the SQL query but pass it in as an excludeKey from the Go code.
And yes, I know this feels hacky, but IMO it's the only key we'll ever not want to show up. Any other potentially non-local/combined/NUMS key could be important to an app that's using the RPC.