chore: refactor tx kernel from ASSET to ASSET_KEY and ASSET_VALUE

[PhilippGackstatter]

This PR refactors the tx kernel internals from the previous ASSET to ASSET_KEY and ASSET_VALUE. I.e. assets are now represented by two instead of one word.

To keep this change managable:

• The asset key is temporarily derived from the asset value at the kernel procedure API boundary (api.masm). In one of the next PRs, this will go away and the kernel procedures will instead take the key in addition to the value.
  • Importantly, the asset value is unchanged: It still has the same layout as before. So, in isolation, this change is weird because the key can always be derived from the value, but this is a temporary thing and in a subsequent PR, the faucet ID will be removed from the asset value.
• Most changes are only in the tx kernel, but it wasn't possible to keep them entirely contained there. The one change that bleeds through in this PR is that the assets commitment is computed over key and value instead of just the value. This affects the get_assets kernel procedure.

Notable changes

• Computing assets commitment is now both conceptually easier and also easier in the implementation. It uses the various double-word hash procedures from miden::core.
  • Simplify NoteAssets by implementing SequentialCommit.
• output_note::add_asset was refactored heavily. It now uses a single procedure for both fungible and non-fungible assets which should prepare nicely for the unified assets in the short-term future.
  • add_asset_raw logic can later be generalized to: if asset is found and can be merged (merge procedure is not EMPTY_WORD) it will call it, otherwise it will abort.
• Semantically, asset origin checks (for faucet mint and burn) or whether an asset is fungible or non-fungible is now checked against the vault key instead of the asset value.
• asset_vault::remove_asset takes the ASSET_KEY and ASSET_VALUE to remove from the vault. The reason the value is required is so an asset can be partially removed from the vault, e.g. "remove 30 tokens from the total of 80 tokens in the vault". This can work similarly in the future with assets that are splittable (e.g. fungible assets).
• faucet::burn currently returns the same ASSET_VALUE that it takes as input. It should either be removed (in line with add_asset_to_note procedure results overhead when calling from Rust #1717) , or return the previous value of the vault. I think the latter makes the most sense as it makes the API strictly more useful. For that reason, I kept the return value so we can more easily change this in the future. The same applies to native_account::remove_asset.
• Fix procedure signature of mint_fungible_asset. It previously suggested it returned the same asset it took, but it actually returns the merged asset.

Review

The most heavily refactored modules are asset, asset_vault, output_note.

Note that asset.masm will change again when changing layouts of asset keys and values, so a very deep review may not be necessary at this point.

I tried to keep the commits organized by the module that was refactored (and what conceptually was updated as a consequence), so reviewing by commit should be mostly fine.

Migration

Calls to {input_note, active_note, output_note}::get_assets must be updated since the returned memory layout has changed from previous [ASSET] to [ASSET_KEY, ASSET_VALUE].

Potential Follow-Ups

• NoteAssets::add_asset recomputes the commitment every time. This currently results in a lot of hashing in the tx host, which seems unnecessary. To make this a bit more efficient:
  • Store assets in a simple Vec in OutputNoteBuilder and delay NoteAssets creation until OutputNoteBuilder::build is called.
  • In addition to the above, make NoteAssets immutable by removing add_asset. It seems to only exist for the purpose of OutputNoteBuilder. I think this seems like the best solution and is more in line with most other types, which are immutable.
• faucet::burn should return previous value. E.g. if we have a total of 80 tokens in the vault and 30 are burnt, then 50 should be returned. This may be useful, for example, to easily check what the remaining balance is after burning. For instance, faucets::burn could set token_supply to that returned value after the burning. The same applies to native_account::remove_asset.

part of #2328

[PhilippGackstatter]

I thought a bit about @bobbinth's suggestion of putting the fee asset into an output note that the batch would consume. I think there are some pros and cons to using a stack output:

• Using a stack output means we can remove the fee very late in the process and we have a pretty good, maybe eventually even a perfect estimate of the remaining number of cycles the transaction will take. So, we can compute the fee from within the transaction with high accuracy. When using an output note, this doesn't become impossible, just a lot harder.
• The con is that the stack output is basically special behavior that bypasses asset preservation checks and is inconsistent with how assets are usually handled, and so an output note is nicer, as the fee asset doesn't violate asset preservation and essentially follows protocol rules.

I think whether this is an issue or not depends on how we move forward with fees in general. If we want to allow payments in any asset, maybe we need to drop the computation of the fee in the tx kernel since computing the fee and then converting it into some asset may end up more complicated than letting users specify some fee asset explicitly via an API. And in that case, I think the above wouldn't apply anymore and the output note handling would be easier.

[bobbinth]

Using a stack output means we can remove the fee very late in the process and we have a pretty good, maybe eventually even a perfect estimate of the remaining number of cycles the transaction will take. So, we can compute the fee from within the transaction with high accuracy. When using an output note, this doesn't become impossible, just a lot harder.

Would it actually be a lot harder? My thinking was that instead of keeping the asset on the stack, we'd just create a new not here in the epilogue and add the asset to it. This should result in an easy-to-estimate (maybe even constant) number of cycles. But maybe I missed some complexity elsewhere?

[PhilippGackstatter]

My assumption was that this fee note would need to be committed to by OUTPUT_NOTES_COMMITMENT and that needs to be signed as part of TX_SUMMARY_COMMITMENT, so that it's a "proper signed" output note.

If we don't do that and create a note directly from the epilogue, we'd probably need to have a "pre-fee output notes commitment" and "post-fee output notes commitment", similar to the account delta, which is not super nice, but possible.

I previously thought we'd have to recompute the post-fee output notes commitment which would be a variable number of cycles, but I guess it could be done in constant cycles by caching the hasher state of the pre-fee output notes commitment and just doing one more permutation.

I would probably try to come up with a design for "fee payments in any asset" before we commit to refactor it to the output note variant, since one or the other might be easier, even though currently I think it's largely orthogonal to that design - just being cautious.

[igamigo]

Awesome work! Couldn't find any potential problems with the implementation. Follow-ups and the PR cutoff also make sense.

[mmagician]

review WIP :)

[mmagician]

LVGTM!

[mmagician]

@copilot work on the nits from @mmagician 's PR review

[Copilot]

@mmagician I've opened a new pull request, #2438, to work on those changes. Once the pull request is ready, I'll request review from you.

[bobbinth]

Looks good! Thank you! I left some comments inline, but pretty much everything could be addressed in follow-up PRs.

[bobbinth]

Using a stack output means we can remove the fee very late in the process and we have a pretty good, maybe eventually even a perfect estimate of the remaining number of cycles the transaction will take. So, we can compute the fee from within the transaction with high accuracy. When using an output note, this doesn't become impossible, just a lot harder.

Would it actually be a lot harder? My thinking was that instead of keeping the asset on the stack, we'd just create a new not here in the epilogue and add the asset to it. This should result in an easy-to-estimate (maybe even constant) number of cycles. But maybe I missed some complexity elsewhere?

[bobbinth]

Could we not use pipe_double_words_preimage_to_memory here instead of doing this manually? (maybe relevant in some other places too)

[PhilippGackstatter]

We could. The main reason I'm not using that is because it is bad for debugging, since it will show the generic error from pipe_double_words_preimage_to_memory if there is a commitment mismatch. If we had VM backtraces/stacktraces in error messages or some other easy way to debug, this would be a no-brainer.

For now, I think the current code is easy enough since it only pulls the equality check out of pipe_double_words_preimage_to_memory.

[bobbinth]

Should we use a constant here rather than 8?

[PhilippGackstatter]

Do you mean something like movdn.ASSET_SIZE? I think I haven't seen this done in the kernel / MASM code yet. We always (?) use immediate values when moving elements on the stack. I guess we could start using constants, but it would be a new pattern to my knowledge.

[bobbinth]

Same comment as above (also relevant in a few more places in this file).

[bobbinth]

Not for this PR, but I wonder if this should return [ASSET_KEY, ASSET_VALUE] - though, it depends on how we use this procedure.

[PhilippGackstatter]

I think the pattern of not returning outputs that are equal to inputs is generally cleaner (and better for generated code: #1717), since the caller can cheaply duplicate the values if they need it. For kernel-internal procedures like those in memory I think that makes sense as an optimization, but at the kernel procedure level, I'm not sure.