Move note inputs number to the note data section by Fumuran · Pull Request #1327 · 0xMiden/protocol

Fumuran · 2025-05-06T12:48:18Z

This PR changes the way we handle note inputs. The number of input values is removed from the input values vector provided to the advice inputs, it is stored in the memory (in the notes data section) instead.

Closes: #1267

Fumuran · 2025-05-06T18:25:20Z

crates/miden-lib/asm/kernels/transaction/api.masm

 export.note_get_inputs_commitment
+    exec.memory::get_input_note_num_inputs
+    # => [num_inputs, pad(16)]
+
    exec.note::get_note_inputs_commitment
-    # => [NOTE_INPUTS_COMMITMENT, pad(16)]
+    # => [NOTE_INPUTS_COMMITMENT, num_inputs, pad(16)]

    # truncate the stack
-    swapw dropw
-    # => [NOTE_INPUTS_COMMITMENT, pad(12)]
+    swapdw dropw dropw
+    # => [NOTE_INPUTS_COMMITMENT, num_inputs, pad(11)]
 end


Although my implementation works, I'm not sure which approach would be the best to get the number of note inputs from memory.

The only way to do so from the miden lib is to call some kernel procedure. So we have two options, which are both not great IMO:

Create a new kernel procedure which will return the number of inputs (which is overkill).

Return the number of inputs alongside with some other data, i.e. change some existing kernel procedure to return it as well (which is not a good UX, it could be quite confusing).

For now I implemented the second approach.

I think the approach you've taken is fine. Returning it as part of the commitment is reasonable and, as you've mentioned, we don't have to add a new kernel procedure.

PhilippGackstatter

Looks good to me!

PhilippGackstatter · 2025-05-07T13:42:20Z

crates/miden-lib/asm/kernels/transaction/lib/memory.masm

    mem_storew
 end

+#! Returns the inputs number of the note currently being processed.


Suggested change

#! Returns the inputs number of the note currently being processed.

#! Returns the number of inputs of the note currently being processed.

Nit: I find this slightly clearer.

PhilippGackstatter · 2025-05-07T13:53:29Z

crates/miden-lib/asm/miden/note.masm

-#! - inputs_len, the note's input count.
-#! - INPUTS, the data corresponding to the note's inputs.
+#! - INPUTS_COMMITMENT is the sequential hash of the padded note's inputs.
+#! - INPUTS is the the data corresponding to the note's inputs.


Suggested change

#! - INPUTS is the the data corresponding to the note's inputs.

#! - INPUTS is the data corresponding to the note's inputs.

PhilippGackstatter · 2025-05-07T13:58:43Z

crates/miden-lib/asm/miden/note.masm

+    adv_push.1 dup.5
+    gte assert.err=ERR_NOTE_TOO_FEW_ELEMENTS_FOR_NOTE_INPUTS


Why gte rather than eq? In what scenarios would more elements be appropriate/valid?

The resulting note inputs vector could contain padded values (ZEROs that we are pushing there before hashing), so the length value we are getting from the adv.push_mapvaln could be bigger than the actual number of inputs or they could be equal, if the real number of values divides by 8. But it never should be less — that would be an error.

Can we round up to the next multiple of 8 so we can use eq? If we can do this, then we don't have to think through whether using gte would be an attack vector somehow.

Yes, makes sense. I'll update the code to round up the real number of values.

PhilippGackstatter · 2025-05-07T14:19:14Z

crates/miden-testing/src/kernel_tests/tx/test_note.rs

+#[test]
+fn test_public_key_as_note_input() {


Can you add a comment what the purpose of the test is? Both for future reference and because this tests a very specific issue which is not obvious.

PhilippGackstatter · 2025-05-07T14:21:06Z

crates/miden-testing/src/kernel_tests/tx/test_note.rs

+    let recipient = NoteRecipient::new(
+        serial_num,
+        note_script,
+        NoteInputs::new(vec![ZERO, ONE, Felt::new(2), Felt::new(3)]).unwrap(),


The point of the test is that this word is the same as mock_public_key, right? Could we make a single variable out of this (which has that value and is a Word) and then add a comment to explain that this is used both as pub key as well as note input? Just to make it easier to understand.

bobbinth

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

bobbinth · 2025-05-10T22:23:33Z

crates/miden-objects/src/note/inputs.rs

    pub fn format_for_advice(&self) -> Vec<Felt> {
-        // NOTE: keep map in sync with the `note::get_inputs` API procedure
-        let mut padded = pad_inputs(&self.values);
-        padded.insert(0, self.num_values().into());
-        padded
+        pad_inputs(&self.values)
    }


Not for this PR, but currently the logic of how a given objects is injected into the advice input can be located in a number of places (e.g., here and also in miden-lib). And it is not easy to tell why certain logic "lives" together with the type (like here) or why it "lives" in the transaction advice input injector module of miden-lib. I wonder if there is a good way to come up with a clear methodology for this.

I agree. I think for me, the clearest way would be if everything was contained within miden-lib as part of extend_advice_inputs.
The other approach is to move it to the individual types. That doesn't make much sense to me, because this format is specific to the transaction kernel / advice provider so it being "closer" to that, i.e. in miden-lib, seems best. Also, in the future, we might have multiple ways to add a given type to the advice provider when we introduce the batch or block kernels.

bobbinth · 2025-05-10T22:26:32Z

crates/miden-lib/src/transaction/inputs.rs

+        // NOTE: keep in sync with the `prologue::process_note_inputs_length` kernel procedure
+        note_data.push(recipient.inputs().num_values().into());
+
        // NOTE: keep in sync with the `prologue::process_note_assets` kernel procedure
        note_data.push((assets.num_assets() as u32).into());
        note_data.extend(assets.to_padded_assets());


What's the main reason why for assets, we push all assets into note_data (i.e., on line 269), but for note inputs we push just the number of inputs?

That's just a matter of decision: previously none of the note input values were stored in the memory, so for now I decided to store only their number to make less changes to the memory layout of the input notes. But if you think that it will be better to store the values themselves too — I can do so.

Created an issue: #1355

bobbinth · 2025-05-10T22:28:33Z

crates/miden-lib/src/transaction/memory.rs

+// - NUM_INPUTS is encoded as [num_inputs, 0, 0, 0].
+// - NUM_ASSETS is encoded as [num_assets, 0, 0, 0].


What's the reason for using two words here? Could this not be:

[num_inputs, num_assets, 0, 0]

But also, if having two words simplifies things, it's fine to keep.

I thought about it, but decided that it could be confusing. But if we decide to move the note inputs from the advice provider to the memory, that would make more sense: each word would contain the number of corresponding values.

bobbinth · 2025-05-10T22:29:39Z

crates/miden-lib/build.rs

 /// The docs.rs build pipeline has a read-only filesystem, so we have to avoid writing to `src`,
 /// otherwise the docs will fail to build there. Note that writing to `OUT_DIR` is fine.
-const BUILD_GENERATED_FILES_IN_SRC: bool = option_env!("BUILD_GENERATED_FILES_IN_SRC").is_some();
+const BUILD_GENERATED_FILES_IN_SRC: bool = true; // option_env!("BUILD_GENERATED_FILES_IN_SRC").is_some();


Should this change be reverted?

refactor: move note inputs number to the note data section

a23cd8a

Fumuran changed the title ~~refactor: move note inputs number to the note data section~~ Move note inputs number to the note data section May 6, 2025

Fumuran added the no docs label May 6, 2025

Fumuran added 2 commits May 6, 2025 21:08

test: add test to check the collision

c52b279

chore: remove commented code

a5b8237

Fumuran commented May 6, 2025

View reviewed changes

Fumuran marked this pull request as ready for review May 6, 2025 18:40

Fumuran requested review from PhilippGackstatter and bobbinth May 6, 2025 18:40

PhilippGackstatter approved these changes May 7, 2025

View reviewed changes

refactor: update docs and comments, improve test

f673cfc

bobbinth approved these changes May 10, 2025

View reviewed changes

Fumuran added 3 commits May 12, 2025 15:19

Merge branch 'next' into andrew-refactor-note-inputs

1bcb2de

refactor: round up number of note inputs

5600f7b

Merge branch 'next' into andrew-refactor-note-inputs

7106211

Fumuran mentioned this pull request May 12, 2025

Investigate moving note inputs to the memory #1355

Closed

Fumuran merged commit 68b7cc7 into next May 12, 2025
16 checks passed

Fumuran deleted the andrew-refactor-note-inputs branch May 12, 2025 18:00

tomyrd mentioned this pull request May 14, 2025

fix: remove padding from output note inputs #1360

Merged

	#! Returns the inputs number of the note currently being processed.
	#! Returns the number of inputs of the note currently being processed.

	#! - INPUTS is the the data corresponding to the note's inputs.
	#! - INPUTS is the data corresponding to the note's inputs.

		adv_push.1 dup.5
		gte assert.err=ERR_NOTE_TOO_FEW_ELEMENTS_FOR_NOTE_INPUTS

		// - NUM_INPUTS is encoded as [num_inputs, 0, 0, 0].
		// - NUM_ASSETS is encoded as [num_assets, 0, 0, 0].

		#[test]
		fn test_public_key_as_note_input() {

Conversation

Fumuran commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PhilippGackstatter May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fumuran commented May 6, 2025 •

edited

Loading

PhilippGackstatter May 7, 2025 •

edited

Loading