Implement advice inputs extender #993

Fumuran · 2024-11-28T15:21:07Z

This PR implements a new function in TransactionKernel which extends the advice inputs with the provided account data and Merkle proofs.

Closes: #961

Fumuran · 2024-11-28T15:34:38Z

miden-lib/src/transaction/mod.rs

+        advice_inputs.extend_merkle_store(
+            merkle_path.inner_nodes(account_id.into(), account_header.hash())?,
+        );


The main question which I have is should we move this extension of the inputs with Merkle path out of this function. The reasons are:

merkle_path requires the miden-crypto to be imported.

obtaining the inner nodes requires the returning value to be a Result<> with the MerkleError as an error type, which then should be somehow converted to the ClientError (maybe it is not a problem, I'm not sure)

For the error, that would not be a problem. I think it should be fine to keep the merkle path inside the function. But more generally I wonder if those types should be re-exported from miden-objects instead.

igamigo

Looks good to me, left some mostly minor comments with some suggestions and questions. Also, should this be added to the changelog? Even though it's a simple change it is user-facing

igamigo · 2024-11-28T19:21:08Z

miden-lib/src/transaction/mod.rs

@@ -161,6 +160,50 @@ impl TransactionKernel {
            .expect("Invalid stack input")
    }

+    /// Extends the advice inputs with account data and Merkle proofs.
+    pub fn extend_advice_inputs_for_account(


The FPI tests could use this function for extending the advice inputs now, right? And so then it would be tested as well

I think we can use it for FPI tests, but in that case merkle_path should become an Option. If it is not a problem, I can adjust this function to work with FPI.

igamigo · 2024-11-28T19:29:48Z

miden-lib/src/transaction/mod.rs

+    /// Extends the advice inputs with account data and Merkle proofs.
+    pub fn extend_advice_inputs_for_account(


nit: It would b e nice to explain that merkle_path should be a path that corresponds to the note root for the account tree of the transaction block header, since it has a generic name for a parameter.

igamigo · 2024-11-28T19:31:42Z

miden-lib/src/transaction/mod.rs

+        advice_inputs.extend_merkle_store(
+            merkle_path.inner_nodes(account_id.into(), account_header.hash())?,
+        );


For the error, that would not be a problem. I think it should be fine to keep the merkle path inside the function. But more generally I wonder if those types should be re-exported from miden-objects instead.

igamigo · 2024-11-28T19:33:31Z

miden-lib/src/transaction/mod.rs

+                foreign_id_root,
+                [
+                    &foreign_id_and_nonce,
+                    vault_root.as_elements(),
+                    storage_root.as_elements(),
+                    code_root.as_elements(),
+                ]
+                .concat(),


Should this be AccountHeader::as_elements() as well? Not sure if that makes sense but it does seem more in line with the others since it does encode all the header information as well

bobbinth

Looks good! Thank you! I left one comment re clarifying one of the comments.

bobbinth · 2024-11-29T08:39:48Z

miden-lib/src/transaction/mod.rs

+    /// - merkle_path is the path which corresponds to the note root for the account tree of the
+    ///   transaction block header.
    pub fn extend_advice_inputs_for_account(


I didn't fully understand what merkle_path is supposed to represent. How is it related to note_root? As far as I remember, account tree contains just accounts and does not have note roots in it.

Also, would be good to explain why it can be optional.

Probably the definition from the miden-client should explain this variable better: Authentication path from the account_root of the block header to the account.

Ah I see my comment in this PR said "note root". I meant to write "account root", sorry.

igamigo

LGTM!

miden-lib/src/transaction/mod.rs

… merkle path of foreign account

bobbinth

Looks good! Thank you! I left a couple more small comments inline. After these are addressed, we can merge.

objects/src/accounts/storage/header.rs

miden-lib/src/transaction/mod.rs

bobbinth

Looks good! Thank you! I left a few more small comments inline. Once these are addressed, we can merge.

objects/src/accounts/storage/header.rs

objects/src/accounts/storage/mod.rs

bobbinth

Looks good! Thank you!

Fumuran added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Nov 28, 2024

Fumuran force-pushed the andrew-fpi-setup-improvements branch from b9d1dd8 to 00d6d63 Compare November 28, 2024 15:24

Fumuran commented Nov 28, 2024

View reviewed changes

Fumuran marked this pull request as ready for review November 28, 2024 15:34

Fumuran requested review from bobbinth and igamigo November 28, 2024 15:34

igamigo approved these changes Nov 28, 2024

View reviewed changes

bobbinth approved these changes Nov 29, 2024

View reviewed changes

igamigo approved these changes Nov 29, 2024

View reviewed changes

bobbinth reviewed Nov 29, 2024

View reviewed changes

miden-lib/src/transaction/mod.rs Outdated Show resolved Hide resolved

Fumuran added 3 commits November 30, 2024 02:17

feat: impl extend advice func for TXKernel

afadb73

refactor: add as_elements for AccountHeader, use new func in FPI tests

2cd0d2e

chore: update doc comment

a4cd34e

Fumuran force-pushed the andrew-fpi-setup-improvements branch from 333c39b to 20186fd Compare November 30, 2024 00:54

Fumuran requested a review from bobbinth November 30, 2024 00:55

refactor: make merkle path mandatory, use new function to provide the…

115648b

… merkle path of foreign account

Fumuran force-pushed the andrew-fpi-setup-improvements branch from 20186fd to 115648b Compare November 30, 2024 00:59

bobbinth approved these changes Nov 30, 2024

View reviewed changes

objects/src/accounts/storage/header.rs Outdated Show resolved Hide resolved

miden-lib/src/transaction/mod.rs Outdated Show resolved Hide resolved

refactor: update variable name

61cb5fe

Fumuran force-pushed the andrew-fpi-setup-improvements branch from 9f797b5 to baf1c10 Compare November 30, 2024 11:09

refactor: impl StorageSlotHeader, move as_elements there

d3a2db7

Fumuran force-pushed the andrew-fpi-setup-improvements branch from baf1c10 to d3a2db7 Compare November 30, 2024 11:14

Fumuran requested a review from bobbinth November 30, 2024 22:59

bobbinth approved these changes Dec 1, 2024

View reviewed changes

objects/src/accounts/storage/header.rs Outdated Show resolved Hide resolved

objects/src/accounts/storage/header.rs Outdated Show resolved Hide resolved

objects/src/accounts/storage/mod.rs Outdated Show resolved Hide resolved

refactor: move back the constant, add doc comments

68322b3

Fumuran force-pushed the andrew-fpi-setup-improvements branch from 5091ba2 to 68322b3 Compare December 1, 2024 22:24

bobbinth approved these changes Dec 1, 2024

View reviewed changes

Fumuran merged commit d3ddfad into next Dec 1, 2024
9 checks passed

Fumuran deleted the andrew-fpi-setup-improvements branch December 1, 2024 22:45

Fumuran mentioned this pull request Dec 1, 2024

FPI setup improvements #961

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement advice inputs extender #993

Implement advice inputs extender #993

Fumuran commented Nov 28, 2024

Fumuran Nov 28, 2024

igamigo Nov 28, 2024

igamigo left a comment •

edited

Loading

igamigo Nov 28, 2024

Fumuran Nov 28, 2024

igamigo Nov 28, 2024

igamigo Nov 28, 2024

igamigo Nov 28, 2024

Fumuran Nov 28, 2024

bobbinth left a comment

bobbinth Nov 29, 2024

Fumuran Nov 29, 2024

igamigo Nov 29, 2024

igamigo left a comment

bobbinth left a comment

bobbinth left a comment

bobbinth left a comment

		/// Extends the advice inputs with account data and Merkle proofs.
		pub fn extend_advice_inputs_for_account(

Implement advice inputs extender #993

Implement advice inputs extender #993

Conversation

Fumuran commented Nov 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igamigo left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

igamigo left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

igamigo left a comment •

edited

Loading