Made initial hash optional (`None` for new accounts) #529

polydez · 2024-03-13T17:45:42Z

Made initial_account_hash in ProvenTransaction, ProvenTransactionBuilder and TransactionId constructor optional.

I also considered to make hash in Account optional, but finally decided to leave it without changes.

Resolves #522

bobbinth

Thank you! Looks good! I left a couple of comments inline - the main one is for testing one of the things which I think might have previously been a bug.

Also, let's update the changelog.

bobbinth · 2024-03-13T23:41:09Z

objects/src/transaction/transaction_id.rs

@@ -89,7 +91,7 @@ impl From<&ExecutedTransaction> for TransactionId {
        let input_notes_hash = tx.input_notes().commitment();
        let output_notes_hash = tx.output_notes().commitment();
        Self::new(
-            tx.initial_account().hash(),
+            tx.initial_account().proof_init_hash(),


I guess this was an error before - right? I'm thinking we should create a test to make sure that transaction ID remains the same as a transaction moves from executed to proven (or maybe we can integrate into existing tests?).

No, this is not a bugfix, just because of changing in input argument it was more convenient to use proof_init_hash. If it's confusing, it might be better to revert to account's hash usage here and implement additional logic for handling new accounts here.

Hmmm - I still think it was a bug previously. The test you've added is good - but it only tests the case of existing accounts. This case would have worked fine before, I think.

The case which I think would have failed before is when a new account is involved in a transaction. Could we test this as well?

@bobbinth I'll implement this test and will fix the bug, thank you!

Just to clarify: I think the bug is already fixed - we just need to confirm this via a test.

@bobbinth no, actually no tests failed in next branch after adding this check into prove_and_verify_transaction function.

hmmm - are we sure then that we are testing executing transactions against new accounts? Is there any test that you see that does that?

(it is of course possible that I'm wrong and there was no bug to begin with, but I'd like to be sure).

I tried to analyze our tests and now I can say we don't have any tests in miden-base which produce new account in transaction.

Could you add one such test? This could be something like consuming a note for a new account.

You could do it in a different PR based on this branch, and then, once it is done, we can merge both these PRs.

@bobbinth, I created an issue for implementing of this test: #529 (comment)
Can we merge this PR then? You would probably want to take a final look.

objects/src/accounts/mod.rs

miden-lib/src/transaction/inputs.rs

CHANGELOG.md

bobbinth · 2024-03-14T04:30:08Z

objects/src/transaction/transaction_id.rs

@@ -89,7 +91,7 @@ impl From<&ExecutedTransaction> for TransactionId {
        let input_notes_hash = tx.input_notes().commitment();
        let output_notes_hash = tx.output_notes().commitment();
        Self::new(
-            tx.initial_account().hash(),
+            tx.initial_account().proof_init_hash(),


Hmmm - I still think it was a bug previously. The test you've added is good - but it only tests the case of existing accounts. This case would have worked fine before, I think.

The case which I think would have failed before is when a new account is involved in a transaction. Could we test this as well?

hackaugusto

lgtm, thanks!

edit: ofcourse, don't merge it before finishing @bobbinth 's requests :)

fix: use `core::ops::Not` instead of `std::ops::Not` fix: address review comments refactor: rename function docs: update changelog tests: add checking proven transaction id for cases when account creation was involved

bobbinth

Thank you! Looks good! I added a couple of comments inline - but looking this over now, I'm not sure this is as much of an improvement as I originally thought. What worries me a bit is the need for debug_assert_ne!(initial_account_hash, Some(Digest::default())) in a couple of places as now we can have potentially redundant representation of initial hash (both None and Digest::default()` semantically mean the same thing).

So, I'm on the fence but leaning a bit toward not merging this. What do you think?

(regardless of the above, I would still rename Account::init_proof_hash() into just Account::init_hash()).

bobbinth · 2024-04-30T07:16:54Z

objects/src/accounts/mod.rs

+    /// Returns hash of the given account as used for the initial account state hash in transaction
+    /// proofs.
+    ///
+    /// For existing accounts, this is exactly the same as [Account::hash()], however, for new
+    /// accounts this value is set to `None`. This is because when a transaction is executed
+    /// against a new account, public input for the initial account state is set to [ZERO; 4] to
+    /// distinguish new accounts from existing accounts, and `None` returned from this method.
+    /// indicates that `[ZERO; 4]` should be used as a public input. The actual hash of the initial
+    /// account state (and the initial state itself), are provided to the VM via the advice provider.
+    pub fn init_account_hash(&self) -> Option<Digest> {
+        self.is_new().not().then(|| self.hash())
+    }


I would move this method back where it was (i.e., line 93 above). Regarding naming, I'd probably just call it init_hash() (I think the account part is not needed as it is a method on Account struct anyway).

bobbinth · 2024-04-30T07:18:14Z

objects/src/transaction/transaction_id.rs

+        let initial_account_hash =
+            tx.initial_account().is_new().not().then(|| tx.initial_account().hash());
        Self::new(
-            tx.initial_account().proof_init_hash(),
+            initial_account_hash,


We could have probably kept this as tx.initial_account().init_hash().

bobbinth · 2024-05-03T22:27:27Z

@polydez - I'm thinking we should revert most of the changes here except for renaming Account::proof_init_hash() into Account::init_hash().

# Conflicts: # CHANGELOG.md

bobbinth

Looks good! Thank you!

* feat: optional initial hash for new accounts fix: use `core::ops::Not` instead of `std::ops::Not` fix: address review comments refactor: rename function docs: update changelog tests: add checking proven transaction id for cases when account creation was involved * refactor: move `init_account_hash` function back to `Account` * fix: no-std build * fix: address review comments

polydez requested review from hackaugusto and bobbinth March 13, 2024 17:45

polydez self-assigned this Mar 13, 2024

polydez linked an issue Mar 13, 2024 that may be closed by this pull request

Make ProvenTransaction::initial_account_hash optional #522

Closed

bobbinth reviewed Mar 13, 2024

View reviewed changes

polydez changed the title ~~Optional initial hash for new accounts~~ Made initial hash optional (None for new accounts) Mar 14, 2024

polydez requested a review from bobbinth March 14, 2024 03:53

bobbinth reviewed Mar 14, 2024

View reviewed changes

hackaugusto approved these changes Mar 14, 2024

View reviewed changes

polydez requested a review from bobbinth March 15, 2024 04:49

bobbinth mentioned this pull request Apr 9, 2024

feat: add creation of onchain notes #587

Merged

mFragaBA added a commit that referenced this pull request Apr 9, 2024

try replicating changes from first two commits from #529

1056c38

igamigo mentioned this pull request Apr 9, 2024

fix: Account details propagation #588

Merged

bobbinth force-pushed the next branch from 9d914fe to 0ddc139 Compare April 26, 2024 10:05

polydez added 2 commits April 26, 2024 18:52

feat: optional initial hash for new accounts

00fe609

fix: use `core::ops::Not` instead of `std::ops::Not` fix: address review comments refactor: rename function docs: update changelog tests: add checking proven transaction id for cases when account creation was involved

refactor: move init_account_hash function back to Account

1a7452a

polydez force-pushed the polydez-opt-initial-hash branch from c5d581d to 1a7452a Compare April 26, 2024 14:16

fix: no-std build

0e52caa

polydez mentioned this pull request Apr 29, 2024

Implement test for producing and proving of transaction for new account #635

Closed

bobbinth reviewed Apr 30, 2024

View reviewed changes

polydez added 2 commits May 6, 2024 11:51

Merge branch 'next' into polydez-opt-initial-hash

432800d

# Conflicts: # CHANGELOG.md

fix: address review comments

049c4b6

polydez requested a review from bobbinth May 6, 2024 06:57

bobbinth approved these changes May 6, 2024

View reviewed changes

polydez mentioned this pull request May 6, 2024

Make ProvenTransaction::initial_account_hash optional #522

Closed

polydez merged commit 5f59653 into next May 6, 2024
9 checks passed

polydez deleted the polydez-opt-initial-hash branch May 6, 2024 07:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made initial hash optional (`None` for new accounts) #529

Made initial hash optional (`None` for new accounts) #529

polydez commented Mar 13, 2024

bobbinth left a comment

bobbinth Mar 13, 2024

polydez Mar 14, 2024

bobbinth Mar 14, 2024

polydez Mar 14, 2024

bobbinth Mar 14, 2024

polydez Mar 19, 2024 •

edited

Loading

bobbinth Mar 19, 2024

polydez Mar 21, 2024

bobbinth Mar 21, 2024

polydez Apr 29, 2024 •

edited

Loading

bobbinth Mar 14, 2024

hackaugusto left a comment •

edited

Loading

bobbinth left a comment

bobbinth Apr 30, 2024

bobbinth Apr 30, 2024

bobbinth commented May 3, 2024

bobbinth left a comment

Made initial hash optional (None for new accounts) #529

Made initial hash optional (None for new accounts) #529

Conversation

polydez commented Mar 13, 2024

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polydez Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polydez Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackaugusto left a comment • edited Loading

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth commented May 3, 2024

bobbinth left a comment

Choose a reason for hiding this comment

Made initial hash optional (`None` for new accounts) #529

Made initial hash optional (`None` for new accounts) #529

polydez Mar 19, 2024 •

edited

Loading

polydez Apr 29, 2024 •

edited

Loading

hackaugusto left a comment •

edited

Loading