asset: introduce new asset version v1 for segwit-like encoding #520
Conversation
|
Ah, so we wouldn't completely move the witness to the proof file? We'd still encode it within the asset in the proof file, but wouldn't commit to the witness in the tree to allow for more flexible constructions? Makes sense, I guess and allows for a very compact diff. |
Yep, going for a minimal approach here. It would still always be encoded, but when we go to map it to a leaf in the tree, we don't include the witness information. Universe trees (which sort of stamp the final transition/issuance proof) will still commit to the witness data, as they include the proof file state transition as a leaf. So that'll end up being the sort of canonical version. By using the tapscript VM, we gain all the extra protections for witness malleability as well. We might want to double check that we're using all the strictest flags there. |
Roasbeef
force-pushed
the
tap-segwit
branch
2 times, most recently
from
e6d6557
to
6d409ee
Compare
|
Updated with relevant commits threading the new state through the entire codebase. Removed from draft as well. Two main things still remaining:
The vPSBT test vectors, amongst many others need to be updated (fail rn in the unit tests as we added a new kv pair to the format). |
Roasbeef
force-pushed
the
tap-segwit
branch
from
6d409ee
to
9197582
Compare
| options := defaultNewAssetOptions() | ||
| for _, opt := range opts { | ||
| opt(options) | ||
| } | ||
|
|
||
| // Collectible assets can only ever be issued once. | ||
| if genesis.Type != Normal && amount != 1 { |
There was a problem hiding this comment.
At this point we check that the genesis type corresponds to the amount field value. Maybe we should check that the asset value is valid at this point also.
There was a problem hiding this comment.
What do you mean? That the value doesn't overflow, or something else?
There was a problem hiding this comment.
I think I meant asset version (rather than "value"). I think we should check the version here also. We have an opportunity to make sure that options.assetVersion is sensible before even constructing the asset.
And also that amount > 0 I suppose.
Roasbeef
force-pushed
the
tap-segwit
branch
2 times, most recently
from
5603fdd
to
a0a9705
Compare
|
Addressed initial round of comments! Test vector check seems to be failing still, I modified the random asset gen to also generate random asset versions, but maybe this is tripping it up? |
As mentioned here you need to use |
Roasbeef
force-pushed
the
tap-segwit
branch
5 times, most recently
from
3d6a46d
to
20faabc
Compare
|
Final issues I had to hunt down:
Some of the changes here might also be useful to update your mental model of the relevant sites @jharveyb. |
| batchKey, groupKey, groupGenAmt, genesisPkt, scriptRoot, assetRoot := | ||
| addRandAssets(t, ctx, assetStore, numSeedlings) | ||
| // | ||
| // nolint:lll |
There was a problem hiding this comment.
nit: linter directive not needed anymore?
In this commit, we add a Version field to the AssetDetails. We also add a unit test to ensure that the new asset version properly excludes the witness field when the top-level taproot output is compiled.
We need an asset_version field to ensure that we can optionally create new v1 assets.
We also modify the seedlingsToAssetRoot method to properly account for the asset version.
We also ensure that `dbAssetsToChainAssets` properly applies the asset version read from the DB.
We add the AssetVersion to the Seedling, so callers are able to create the new v1 assets. We also update the tests to generate random asset versions, and also assert that asset versions match up.
In this commit, we add the asset version to both passive and active transfers. Adding it to passive transfers lets us do things like upgrade the asset versions of passive assets as a whole or individually.
Note that for `ApplyPendingOutput` we now swap in the version from the passive transfer, rather than using the existing version. This lets us update the version of existing assets not involved in transfers.
In this commit, we update all the transfers logic to account for asset versions. We also update the unit tests to use heterogeneous asset versions to make sure all the persistence logic properly accounts for asset version changes.
We'll use this to ensure things are populated everywhere applicable on the RPC interface.
In this commit, we now use the AssetVersion enum for the main Asset message and all other applicable messages.
This also let's us test that we can: mint, create addrs for, and also send v1 assets.
In this commit, we update split locator struct to include the asset version. This ensures that if we attempt a send with heterogeneous asset versions, then each vOut is able to ensure that the split is created with the correct asset version. Without this, we might use a different asset version with the proof we have on disk vs what we put in the transaction. We also modify the way the change output is created. The change output will now always take on the max version of all the inputs.
In this commit, we update the termination condition to account for the fact that grouped assets have a distinct witness from non-grouped assets. Without this, we'll fail to fetch the proof files for grouped assets.
In this commit, we update the Update and Delete methods of the tap and asset commitment to always update the max version. We add 4 new tests to exercise the fix.
In this commit, we add a new asset version to introduce segwit-like asset encoding where we don't encode the witness vector of an asset's inputs. This applies on the asset version level, so all v1 asset won't have that vector encoded. Note that this is only for the MS-SMT tree, any other TLV encoding such as the asset proof or state transitions remain the same.
Fixes #464
TODO
TransferOutputs(?) on disk to support v0 -> v1 mapping