Hash arbitrary number of elements in `masm` #750

Fumuran · 2024-06-12T20:49:23Z

This PR implements compute_inputs_hash function in note.masm which allows to compute RPO hash of arbitrary number of Felt elements.

TODO:

Implement compute_inputs_hash
Update changelog
Update docs

bobbinth

Thank you! Not a full review, but I left a couple of comments inline.

miden-lib/asm/miden/note.masm

partylikeits1983 · 2024-06-17T21:38:02Z

@Overcastan Is it easy to get the note serial number or serial number hash easily?

Once it is possible to hash an arbitrary number of elements in masm, and get the serial number, you can use this procedure: #757

If you can only get the hash, that's fine too, you'd just modify this procedure a bit.

Fumuran · 2024-06-18T11:44:00Z

@partylikeits1983 I thought that note serial number is kind of user input, isn't it? So we can add it as an input to the procedure, but in that case versatility of this procedure will decrease. But I don't know the exact use case for it, so if at the moment of this procedure call we always have the note serial number, it should be fine to add it as an input.

partylikeits1983 · 2024-06-21T21:11:13Z

Should we add a test for this?

Fumuran · 2024-06-21T23:34:18Z

I was thinking about that, but there is a problem: ideally we want to have something similar to tests in native.rs, but to do so in miden-base we essentially need to copy the whole test-utils crate.
Probably it makes sense to create a different PR just to implement testing for modules here in base, since we need to implement quite a few testing utilities.

miden-lib/asm/miden/note.masm

bobbinth · 2024-06-22T08:25:11Z

I was thinking about that, but there is a problem: ideally we want to have something similar to tests in native.rs, but to do so in miden-base we essentially need to copy the whole test-utils crate.
Probably it makes sense to create a different PR just to implement testing for modules here in base, since we need to implement quite a few testing utilities.

I think once we finalize the procedure here, we should basically copy it (with some minor modifications) to the native hash module (which will be renamed to rpo in the future) and test it there. Once we move miden-base to the next version of Miden VM, we'll just start using the procedure from the rpo module.

Fumuran · 2024-06-24T22:44:45Z

I implemented this function using cdrop instructions, but I'm not sure that it will be better in performance and more easy to understand. I think I could try to optimize it by dividing into two cases: we need to drop more than 4 elements and less than 4 elements, which will make this function faster, although even more complicated.

miden-lib/asm/miden/note.masm

Fumuran · 2024-06-25T15:41:58Z

I thought that if we are going to check all 7 top stack elements by hand anyway, we can simplify the cdrop implementation even further, removing latch variable and cdrop instruction. This implementation uses ifs, so I'm not sure that it will be faster, but it is more simple for sure.

bobbinth · 2024-06-25T17:24:41Z

I thought that if we are going to check all 7 top stack elements by hand anyway, we can simplify the cdrop implementation even further, removing latch variable and cdrop instruction. This implementation uses ifs, so I'm not sure that it will be faster, but it is more simple for sure.

Using if's will introduce quite a bit of overhead here - so, we should probably go back to using cdrop's.

bobbinth

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

Before merging this PR, let's make a similar PR in miden-base to update the rpo module (the PR would need to made against Al's branch). That one is already using a new padding rule - and so, I think, the code should be a bit simpler.

In that PR, let's rename hash_memory into hash_memory_words and name the function you wrote in this PR hash_memory.

miden-lib/asm/miden/note.masm

Fumuran · 2024-06-26T17:44:58Z

Since hash_memory is called from (yet) non-existent module rpo, tests are keep failing.

bobbinth · 2024-06-28T09:22:07Z

Since hash_memory is called from (yet) non-existent module rpo, tests are keep failing.

We won't be able to update miden-base to the latest miden-vm for some time - but we need to merge this PR soon. Could you bring back the previous implementation (the one on which the miden-vm PR is based on). For the implementation in this PR, we should still use the old padding rule.

bobbinth

Looks good! Thank you. I left some more comments inline.

Also, it would be good to test it somehow - even if manually (i.e., we don't need to create a lot of code for this, I just want to make sure similar tests as we did have in miden-vm work with this too).

miden-lib/asm/miden/note.masm

bobbinth

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

bobbinth · 2024-07-02T00:13:18Z

miden-lib/asm/miden/note.masm

+    # => [num_inputs/8, num_inputs%8, inputs_ptr]
+
+    # get the end_addr for hash_memory procedure (end address for pairs of words)


nit: this should say "hash_memory_even" procedure.

bobbinth · 2024-07-02T00:14:45Z

miden-tx/src/tests/kernel_tests/test_note.rs

+    let code = "
+        use.miden::note
+
+        begin
+            push.1.2.3.4.1000 mem_storew dropw
+            push.5.6.7.8.1001 mem_storew dropw
+            push.9.10.11.12.1002 mem_storew dropw
+            push.13.14.15.16.1003 mem_storew dropw
+
+            push.5.1000
+            exec.note::compute_inputs_hash
+
+            push.8.1000
+            exec.note::compute_inputs_hash
+
+            push.15.1000
+            exec.note::compute_inputs_hash
+
+            push.0.1000
+            exec.note::compute_inputs_hash
+        end
+    ";


Let's add comments to explain what is being hashed here.

bobbinth · 2024-07-02T00:15:26Z

miden-tx/src/tests/kernel_tests/test_note.rs

+    let process = tx_context.execute_code(code).unwrap();
+    let expected_stack = [
+        Felt::new(0),
+        Felt::new(0),
+        Felt::new(0),
+        Felt::new(0),
+        Felt::new(10300020282439016154),
+        Felt::new(3516596904277416676),
+        Felt::new(11018788508269249672),
+        Felt::new(7921509648524809116),
+        Felt::new(13608701685256682132),
+        Felt::new(16013969809933496273),
+        Felt::new(15720844923951376941),
+        Felt::new(15975159621759139720),
+        Felt::new(12095223039215569196),
+        Felt::new(16902760742589336416),
+        Felt::new(12194156716918087419),
+        Felt::new(2777745863384272413),
+    ];
+    assert_eq!(process.get_stack_state()[0..16], expected_stack);


Is it possible to compute these dynamically? Or at least write comments about how these were computed?

bobbinth

All looks good! Thank you!

Fumuran requested a review from bobbinth June 12, 2024 20:55

Fumuran linked an issue Jun 12, 2024 that may be closed by this pull request

Using hmerge to hash 9 or more stack elements #736

Open

bobbinth reviewed Jun 13, 2024

View reviewed changes

miden-lib/asm/miden/note.masm Outdated Show resolved Hide resolved

miden-lib/asm/miden/note.masm Show resolved Hide resolved

Fumuran force-pushed the andrew-hash-more-than-8 branch from 4161d6c to 12e7cd5 Compare June 19, 2024 21:49

Fumuran marked this pull request as ready for review June 19, 2024 21:53

Fumuran requested a review from bobbinth June 19, 2024 22:12

Fumuran force-pushed the andrew-hash-more-than-8 branch from 12e7cd5 to aa7e423 Compare June 20, 2024 12:20

bobbinth reviewed Jun 22, 2024

View reviewed changes

miden-lib/asm/miden/note.masm Show resolved Hide resolved

bobbinth reviewed Jun 24, 2024

View reviewed changes

miden-lib/asm/miden/note.masm Outdated Show resolved Hide resolved

Fumuran force-pushed the andrew-hash-more-than-8 branch from 68c78e4 to 55ebae4 Compare June 25, 2024 15:38

bobbinth reviewed Jun 26, 2024

View reviewed changes

Fumuran force-pushed the andrew-hash-more-than-8 branch 2 times, most recently from 80103e5 to 74d3199 Compare June 26, 2024 17:36

Fumuran force-pushed the andrew-hash-more-than-8 branch from a2ff613 to 00e05b2 Compare June 28, 2024 15:38

Fumuran added 6 commits June 29, 2024 00:38

feat: implement compute_inputs_hash

6095aad

refactor: make double word hashing check, add 4 element hashing case

2b6bf89

feat: implement compute_inputs_hash with cdrop

91a605c

refactor: get rid of the excess if's

efaf04e

feat: implement compute_inputs_hash without cdrop

b19129c

refactor: remove excess implementations, update cdrop implementation

ed12a9b

Fumuran added 2 commits June 29, 2024 00:39

refactor: move hash_memory to the miden-vm

3c880f3

refactor: apply the version of the procedure from the miden-vm

5066552

Fumuran force-pushed the andrew-hash-more-than-8 branch from 00e05b2 to 26a010f Compare June 28, 2024 21:40

fix: fix the stack cleaning

58eb753

Fumuran force-pushed the andrew-hash-more-than-8 branch from 26a010f to 58eb753 Compare June 28, 2024 21:44

Fumuran requested a review from bobbinth June 29, 2024 08:22

bobbinth requested changes Jun 30, 2024

View reviewed changes

miden-lib/asm/miden/note.masm Outdated Show resolved Hide resolved

miden-lib/asm/miden/note.masm Outdated Show resolved Hide resolved

miden-lib/asm/miden/note.masm Outdated Show resolved Hide resolved

refactor: update values naming, improve hashing logic, add test

426d073

Fumuran requested a review from bobbinth July 1, 2024 14:35

bobbinth approved these changes Jul 2, 2024

View reviewed changes

refactor: fix comment, improve test

557f7a3

Fumuran requested a review from bobbinth July 2, 2024 17:55

bobbinth approved these changes Jul 2, 2024

View reviewed changes

bobbinth merged commit 89437d1 into next Jul 2, 2024
12 checks passed

bobbinth deleted the andrew-hash-more-than-8 branch July 2, 2024 21:11

bobbinth pushed a commit that referenced this pull request Jul 4, 2024

feat: add note::compute_inputs_hash procedure to tx kernel (#750)

70fc00e

bobbinth mentioned this pull request Jul 4, 2024

Using hmerge to hash 9 or more stack elements #736

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hash arbitrary number of elements in `masm` #750

Hash arbitrary number of elements in `masm` #750

Fumuran commented Jun 12, 2024 •

edited

Loading

bobbinth left a comment

partylikeits1983 commented Jun 17, 2024 •

edited

Loading

Fumuran commented Jun 18, 2024

partylikeits1983 commented Jun 21, 2024

Fumuran commented Jun 21, 2024

bobbinth commented Jun 22, 2024

Fumuran commented Jun 24, 2024

Fumuran commented Jun 25, 2024 •

edited

Loading

bobbinth commented Jun 25, 2024

bobbinth left a comment

Fumuran commented Jun 26, 2024

bobbinth commented Jun 28, 2024

bobbinth left a comment

bobbinth left a comment

bobbinth Jul 2, 2024

bobbinth Jul 2, 2024

bobbinth Jul 2, 2024

bobbinth left a comment

		# => [num_inputs/8, num_inputs%8, inputs_ptr]

		# get the end_addr for hash_memory procedure (end address for pairs of words)

Hash arbitrary number of elements in masm #750

Hash arbitrary number of elements in masm #750

Conversation

Fumuran commented Jun 12, 2024 • edited Loading

bobbinth left a comment

Choose a reason for hiding this comment

partylikeits1983 commented Jun 17, 2024 • edited Loading

Fumuran commented Jun 18, 2024

partylikeits1983 commented Jun 21, 2024

Fumuran commented Jun 21, 2024

bobbinth commented Jun 22, 2024

Fumuran commented Jun 24, 2024

Fumuran commented Jun 25, 2024 • edited Loading

bobbinth commented Jun 25, 2024

bobbinth left a comment

Choose a reason for hiding this comment

Fumuran commented Jun 26, 2024

bobbinth commented Jun 28, 2024

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth Jul 2, 2024

Choose a reason for hiding this comment

bobbinth Jul 2, 2024

Choose a reason for hiding this comment

bobbinth Jul 2, 2024

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Hash arbitrary number of elements in `masm` #750

Hash arbitrary number of elements in `masm` #750

Fumuran commented Jun 12, 2024 •

edited

Loading

partylikeits1983 commented Jun 17, 2024 •

edited

Loading

Fumuran commented Jun 25, 2024 •

edited

Loading