Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce create_account kernel section #183

Merged
merged 1 commit into from
Aug 8, 2023
Merged

Conversation

frisitano
Copy link
Contributor

@frisitano frisitano commented Aug 1, 2023

Tests failing due to - #185. Rebasing will fix tests once merged.

Introduces account creation logic in the kernel.

We identify a new account by checking if the nonce is equal to zero. If the account is new then we enforce the condition that the nonce must be incremented in the transaction, this condition is enforced in the epilogue. We now apply two permutations of the hashing function to compute the seed digest and as such the work has doubled. To account for the doubling of work I have reduced the required trailing bits by one.

closes: #87, #84

@frisitano frisitano force-pushed the frisitano-create-account branch 6 times, most recently from 472a907 to 33d6420 Compare August 3, 2023 14:23
@frisitano frisitano marked this pull request as ready for review August 3, 2023 15:00
@frisitano frisitano requested a review from bobbinth August 3, 2023 15:01
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This is not a full review yet, but I left a few comments inline.

Also, more generally, having seen the code, I'm wondering about two things:

  1. Whether we can actually get away without a special flag for new account in the inputs.
  2. Whether we indeed need to perform inclusion/exclusion proofs against the account DB.

For the first one, the reasoning is as follows:

The block producer would need to verify whether the input state of the account for the transaction is actually the latest state according to the account DB. Since the root of the account DB is constantly changing, we can't really delegate this check to the user (or we could, but this would make things way more complicated for the block producer).

For existing accounts this is not a problem: the block producer takes the input account hash and compares it against what it has in the database (this would also be done in the block kernel). However, for new accounts this would be a problem as the initial hash of the account state would not match what is in the database (i.e., [ZERO; 4]). So, the block producer needs to be able to differentiate between new and existing accounts to perform the check. There are two ways we can communicate this info to the block producer:

  1. We could still use the nonce, but then the block producer would need to know the nonce not only for new accounts but also for existing ones. Also, we'd need to change how we compute hash of the account so that we don't leak any other info (i.e., the hash could be computed as hash(nonce, hash(account_id, vault_root, storage_root, code_root)).
  2. We could provide the is_new_account flag as an input to the transaction kernel.

It seems to me that the second option is easier and preferable because it does not leak account nonce to the block producer all the time.

Regarding the second point:

Since the block producer will need to check account state hash against the account DB anyway, I'm not sure we need to do this check as the part of the transaction kernel. This would simplify the code, but also would mean that the user can update block_ref input without the need to update the Merkle authentication path for the account.

@frisitano
Copy link
Contributor Author

frisitano commented Aug 4, 2023

  1. We could still use the nonce, but then the block producer would need to know the nonce not only for new accounts but also for existing ones. Also, we'd need to change how we compute hash of the account so that we don't leak any other info (i.e., the hash could be computed as hash(nonce, hash(account_id, vault_root, storage_root, code_root)).
  2. We could provide the is_new_account flag as an input to the transaction kernel.

good catch! I think there may be a third option we can consider here. Set the initial account hash public input to [ZERO; WORD_SIZE] for new accounts. What do you think is preferable?

Since the block producer will need to check account state hash against the account DB anyway, I'm not sure we need to do this check as the part of the transaction kernel. This would simplify the code, but also would mean that the user can update block_ref input without the need to update the Merkle authentication path for the account.

Good point, I will remove the account db lookups.

@bobbinth
Copy link
Contributor

bobbinth commented Aug 4, 2023

I think there may be a third option we can consider here. Set the initial account hash public input to [ZERO; WORD_SIZE] for new accounts. What do you think is preferable?

And then the actual account state would be provided non-deterministically (and verified against the ID provided via stack inputs)? I think this would be preferable - though, we should think through potential attack vectors here (I'm not seeing any, but it's also pretty late for me :) )

@frisitano
Copy link
Contributor Author

frisitano commented Aug 4, 2023

I think there may be a third option we can consider here. Set the initial account hash public input to [ZERO; WORD_SIZE] for new accounts. What do you think is preferable?

And then the actual account state would be provided non-deterministically (and verified against the ID provided via stack inputs)? I think this would be preferable - though, we should think through potential attack vectors here (I'm not seeing any, but it's also pretty late for me :) )

Yes exactly. We would have a conditional branch based on the initial account hash. If the initial account hash is an empty word then we verify the account data satisfies new account requirements - if it is not empty then we apply the checks we currently have in place for existing accounts. I don't see any attack vectors but I will give it some more thought. More specifically we would add the conditional branching here (I think we can remove the hashing for new accounts).

@frisitano frisitano force-pushed the frisitano-create-account branch from 33d6420 to ed9f0c2 Compare August 4, 2023 10:16
@frisitano frisitano force-pushed the frisitano-create-account branch 5 times, most recently from 68c4037 to 8c95e7d Compare August 4, 2023 15:12
@frisitano frisitano requested a review from bobbinth August 4, 2023 21:08
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you! I left a few small comments inline.

@frisitano frisitano force-pushed the frisitano-create-account branch 2 times, most recently from f8e8e61 to 82d68d6 Compare August 7, 2023 14:46
@frisitano frisitano requested a review from bobbinth August 7, 2023 16:46
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good! Thank you! We should probably update Cargo.toml files to but back to the next branch of Miden VM, and then we can merge.

Comment on lines 19 to 29
assembly = { package = "miden-assembly", git = "https://github.com/0xPolygonMiden/miden-vm.git", branch = "next", default-features = false }
assembly = { package = "miden-assembly", git = "https://github.com/0xPolygonMiden/miden-vm.git", branch = "frisitano-u32-trailing-zeros", default-features = false }

[dev-dependencies]
crypto = { package = "miden-crypto", git = "https://github.com/0xPolygonMiden/crypto.git", branch = "next", default-features = false }
miden-objects = { package = "miden-objects", path = "../objects", features = ["testing"], default-features = false }
miden-stdlib = { package = "miden-stdlib", git = "https://github.com/0xPolygonMiden/miden-vm.git", branch = "next", default-features = false }
processor = { package = "miden-processor", git = "https://github.com/0xPolygonMiden/miden-vm.git", branch = "next", features = ["internals"], default-features = false }
vm-core = { package = "miden-core", git = "https://github.com/0xPolygonMiden/miden-vm.git", branch = "next", default-features = false }
miden-stdlib = { package = "miden-stdlib", git = "https://github.com/0xPolygonMiden/miden-vm.git", branch = "frisitano-u32-trailing-zeros", default-features = false }
processor = { package = "miden-processor", git = "https://github.com/0xPolygonMiden/miden-vm.git", branch = "frisitano-u32-trailing-zeros", features = ["internals"], default-features = false }
vm-core = { package = "miden-core", git = "https://github.com/0xPolygonMiden/miden-vm.git", branch = "frisitano-u32-trailing-zeros", default-features = false }

[build-dependencies]
assembly = { package = "miden-assembly", git = "https://github.com/0xPolygonMiden/miden-vm.git", branch = "next" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably revert these changes back to the next branch (here and in other Cargo.toml` files).

@frisitano frisitano force-pushed the frisitano-create-account branch from 82d68d6 to 8b78656 Compare August 8, 2023 07:25
@frisitano frisitano merged commit 273d655 into main Aug 8, 2023
@frisitano frisitano deleted the frisitano-create-account branch August 8, 2023 07:43
daedlock pushed a commit to keomprotocol/miden-base that referenced this pull request Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating new accounts
2 participants