Extend AccountId to two Felts and refactor creation process

[PhilippGackstatter]

What has changed

AccountId previously fit into a felt or ~64 bits. Now its representation is two felts:

pub struct AccountId {
    first_felt: Felt,
    second_felt: Felt,
}

In Rust, the first and second felts are always called as such, typically accessed with id.first_felt() and id.second_felt().
In MASM, the first felt is the hi felt and the second felt is the lo felt and its typical stack representation is [account_id_hi, account_id_lo].

The layout of an ID is:

1st felt: [random (56 bits) | storage mode (2 bits) | type (2 bits) | version (4 bits)]
2nd felt: [block_epoch (16 bits) | random (40 bits) | 8 zero bits]

See the AccountId documentation for details on the layout.

There is a type AccountIdPrefix representing the validated first felt of an AccountId which is primarily used for non-fungible assets and for asset deserialization. Ideally we would make that private, but for now it must be public for the constructors of non fungible assets.

Notable Changes

Here is a quick overview of what changes with this PR. Layouts are in "memory order", that is, stack order would be the reverse of that.

• Fungible Asset representation:
  • previously: [amount, 0, 0, faucet_id]
  • now: [amount, 0, faucet_id_lo, faucet_id_hi]
• Non-Fungible Asset representation:
  • previously: [hash_0, faucet_id, hash_2, hash_3]
  • now: [hash0, hash1, hash2, faucet_id_hi]
• NoteMetadata
  • previously: [tag, sender_acct_id, encoded_type_and_ex_hint, aux]
  • now: [sender_hi, sender_lo_type_and_hint_tag, note_tag_hint_payload, aux]
  • The NoteMetadata AfterBlock can only take values less than u32::MAX so that when encoded together with the note tag in a felt (i.e. note_tag_hint_payload above) felt validity is guaranteed.
• The serialization of AccountIdPrefix is compatible with AccountId to allow for deserialization of a prefix from a serialized AccountId. This is used for the next point.
• AccountIdPrefix serialization is such that the first byte of the serialized layout contains the metadata. This is used in the asset deserialization to read the type and based on that deserialize the bytes as a fungible or non-fungible asset.
• The NoteTag::from_account_id effectively takes the most significant bits of id_first_felt << 1. This means the high zero bit is ignored with the rationale being that it would not add any value for matching an account ID against a note tag, since all ID's high bits are zero. Let me know if this doesn't make sense.
• The previous account creation tests were replaced because they now require a MockChain due to the dependence of the account ID on an anchor block.
• The layout of account IDs in various keys, stacks and hashes across Rust and Masm is not entirely consistent I think because it's not always quite clear to me whether a layout needs to be reversed or not. What is also unclear to me is in what order IDs should be layed out within layouts like [account_nonce, 0, account_id_hi, account_id_lo]. So anything related to that would be particularly helpful to have reviewed.
• I snuck in some small improvements to the debugging experience but I'd like to revisit this topic in a future PR.

This PR does not change anything about the account tree, e.g. the accounts: SimpleSmt<ACCOUNT_TREE_DEPTH> field in MockChain.

I think a rough sensible review order would be AccountId + AccountIdPrefix followed by the changes to assets, all in Rust.
Then moving over to MASM.

Open Questions

• Should we enforce MIN_ACCOUNT_ONES in both first and second felt? For now only the first felt is enforced with the only rationale being that the first felt is used as the key in the asset SMT for fungible assets. But if we treat this as an implementation detail then it might make sense to also enforce something similar on the second felt?
• Do we need a serialization format for NoteMetadata that is different from the Word encoding? We had a different one before this PR, but now the serialization format and word encoding are identical.

Follow-Up Tasks

There are still some TODOs in the code and those could mostly be addressed in a follow-up PR so this PR could would not necessarily be blocked by that (checked boxes are included in this PR).

• Introduce AccountIdError. There are now new error conditions for IDs and a separate error might be cleaner.
  • With the introduction of this error it might be possible to make AccountId::new_dummy a const fn and then we could possibly replace the somewhat duplicate functionality of miden_objects::testing::account_id::account_id.
• Quite a bit of Rust documentation needs to be updated.
• Implement Display for AccountType and use it in error messages.
• Remove unnecessary code in generate_account_seed. See also: Extend AccountId to two Felts and refactor creation process #982 (comment)
• Remove build.rs constants patching for POW which we no longer have.
• Move type, storage mode, version out into their own modules to reduce size of account_id.rs. See also Extend AccountId to two Felts and refactor creation process #982 (comment).
• Move newly introduced duplicate procedures into the new util library (after Remove duplicate procedures from miden-lib #1002).
• Go through error messages and double check if they are up to date (e.g. ones previously mentioning "account id" should now likely mention first or second felt for accuracy).
• validate_fungible_asset_origin validates the account ID and then calls validate_fungible_asset which validates it again check if we can remove some redundancy here.
• Deal with NoteExecutionHint::AfterBlock being constructable by users but it now cannot contain u32::MAX and we might want to prevent that.
• Update and test build_swap_tag.
• Add lots of new tests for changed things: version and epoch in ID, test new and changed MASM procedures more thoroughly.
  • See TODO in create_account_invalid_seed.
• Think about account ID encoding for network identifier and error detection (e.g. bech32, base58, etc.).
• Add AccountId version wrapper (see Extend AccountId to two Felts and refactor creation process #982 (comment)) and consider making ID V1 validation a separate procedure in MASM.
• Consider taking required parameters in AccountBuilder::new, i.e.: Extend AccountId to two Felts and refactor creation process #982 (comment).
• Consistently capitalize "account id" as "account ID".
• With epoch taken as a concept of the protocol, we should probably change NoteExecutionHint::OnBlockSlot to not use epoch, as it is a somewhat different concept. See Extend AccountId to two Felts and refactor creation process #982 (comment) for details.
• Make u128::to_be_bytes and <[u8; 15]> encodings consistent (see Extend AccountId to two Felts and refactor creation process #982 (comment)).
  • This would be fundamentally at odds with current asset serialization where the first byte is assumed to be the account ID metadata. Since we can't change u128::to_be_bytes the metadata would not be in the first byte.
  • One approach may be to implement a special account ID serialization for assets where the first byte is the metadata byte, while the above formats for account IDs are the same.
• Check if we can properly build pre-genesis accounts: Extend AccountId to two Felts and refactor creation process #982 (comment)
  • Is the name correct here? Wouldn't these be just "genesis" accounts?
• Rename AccountId::new_dummy -> AccountId::dummy (Extend AccountId to two Felts and refactor creation process #982 (comment))
• Support dummy AccountId creation ideally with being able to specify the type and storage mode, but also not necessarily having to do so. Check if adding rng support is easily possible. There is some variability here, but not so much to justify a complete builder.
  • A macro supporting some optional arguments might be great for usability in test code, e.g.:
    • let dummy: AccountId = account_id_dummy!(&some_rng);
    • let dummy: AccountId = account_id_dummy!([0xff; 15]);
    • let dummy: AccountId = account_id_dummy!([0xff; 15], AccountType::FungibleFaucet, AccountStorageMode::Public);
    • See also: Extend AccountId to two Felts and refactor creation process #982 (comment)
• Use more precise truncation strategy for less subtleness: Extend AccountId to two Felts and refactor creation process #982 (comment)
• Consider adding faucet_id_prefix to Assets and rename Asset::faucet_id to Asset::faucet_id_prefix (Extend AccountId to two Felts and refactor creation process #982 (comment), Extend AccountId to two Felts and refactor creation process #982 (comment))
• Consider creating benchmarks for single and concurrent account id computation (Extend AccountId to two Felts and refactor creation process #982 (comment))
• Encapsulate behavior like is account a faucet check behind utility methods (Extend AccountId to two Felts and refactor creation process #982 (comment))
• Validate metadata in validate_id MASM procedure (Extend AccountId to two Felts and refactor creation process #982 (comment))
• Differentiate between "pure" and "instance" account procedures (Extend AccountId to two Felts and refactor creation process #982 (comment))

Why it changed

See #140, although I'd like to ideally add a summary here.

closes #140

[Fumuran]

Looks greaat!
I looked through the miden-lib, looks good to me, I left just one nit inline.

[PhilippGackstatter]

I think I've addressed all the minor changes as well as the removal of the top zero bit and changing the non-fungible asset layout to avoid having to set the fungible bit.

Other outstanding issues have been added to the follow-up tasks in the PR description or need feedback in the comments. I think they could pretty much all be resolved after this PR as well if we like to merge it, so I don't think the PR needs to necessarily be blocked by these.

[Fumuran]

LGTM, great work!

[Mirko-von-Leipzig]

Some drive-by comments :) Feel free to ignore.

[Mirko-von-Leipzig]

This seems largely redundant? Are the function's not enough, or are these required for generics somewhere?

[PhilippGackstatter]

I think I added these for consistency. I believe there were some existing From impls which I just extended, but they are redundant, yeah. The same goes for the From impls on AccountType then. I'll see if I can remove them in a follow-up PR.

[Mirko-von-Leipzig]

nit: probably just dummy? We don't use new_empty for example.

[Mirko-von-Leipzig]

Could we get rng support? In most node tests (e.g. 0xPolygonMiden/miden-node#591) we really just want some number of unique account IDs.

Sometimes one would also like to specify the account type and storage mode I suppose, but in general just having a quick way to create a random account ID without any additional fuss would make test code easier to grok and create.

[tomyrd]

I couldn't do a full code review, just some things I noticed as I worked with this branch.
I've been using these changes for the last couple of days to migrate the node/client repos and the documentation/explanations seems really clear ❤️ Specifically on the base rust types. The current implementation seems to work correctly on both, I'll try to mantain both PRs updated as more commits are made here.

Additionally I had a question regarding the enforcement of prefix collision. The entity responsible for this check would be the node, right?

[tomyrd]

Based on this comment. I used this as the account id anchor for genesis accounts. Would it be a good idea to have a separate constructor? Or maybe mention it in the docs.

[bobbinth]

I added AccountIdAnchor::PRE_GENESIS constant which could be used instead.

[bobbinth]

Looks good! Thank you! I left some more comments inline - but these can be addressed in follow-up PRs.

[bobbinth]

I added AccountIdAnchor::PRE_GENESIS constant which could be used instead.

[bobbinth]

Not related to this PR, but how is "global" account ID different from "native" account ID? cc @Fumuran.

[PhilippGackstatter]

As far as I can tell, there is no difference.

The global ID is stored as part of process_global_inputs so that it can later be retrieved to run this check:

miden-base/miden-lib/asm/kernels/transaction/lib/prologue.masm

Lines 468 to 474 in 487e8b9

	# assert the account id matches the account id in global inputs
	exec.memory::get_global_acct_id
	exec.memory::get_account_id
	exec.account::is_id_eq assert.err=ERR_PROLOGUE_MISMATCH_OF_ACCOUNT_IDS_FROM_GLOBAL_INPUTS_AND_ADVICE_PROVIDER
	# => [ACCT_HASH]

(where get_account_id returns the ID of the current account, which at this point is the native account).

We could get rid of this notion of global ID and storing it in memory if we pass it via the stack in prologue::prepare_transaction.

[bobbinth]

Related to one of the previous comments: I wonder if we should come up with a way to differentiate between "pure" procedures and procedures which work in the context of the currently active account. For example, account::get_id assumes that we are getting ID of the currently active account, but account::is_fungible_faucet is a "pure" procedure unrelated to the currently active account.

One potential approach could be to move "pure" procedures into a separate module. For example, something like account::get_id but account_utils::is_fungible_faucet. But there are probably other approaches to this as well.

[bobbinth]

I think naming can be improved here (i.e., we are not validating a full ID but rather some properties of an ID).

[PhilippGackstatter]

This is an interesting point, actually.
This procedure should probably also validate account type and storage mode as it is used in validate_fungible_asset, validate_non_fungible_asset and validate_new_account. I'm not sure if we actually ever validate in the kernel that storage mode is not set to 0b11, for example, but we should.
If we add this, then this function is properly named as it validates everything we can validate about IDs.