Update recursive verifier to use Horner eval ops instead of `rcomb_*` #1665

Al-Kindi-0 · 2025-02-17T16:18:14Z

As a side effect, we now have a very accurate formula to estimate the cycle count of the recursive verifier as a function of its parameters.
Putting in draft as some older PRs need to be merged before this one.

plafer

Very cool!

Left some questions and nits

plafer · 2025-03-03T13:29:10Z

core/Cargo.toml

 memchr = { version = "2.7", default-features = false }
-miden-crypto = { version = "0.13", default-features = false }
+#miden-crypto = { version = "0.13", default-features = false }
+miden-crypto = {git = "https://github.com/0xPolygonMiden/crypto", branch = "al-update-winter", default-features = false }


Let's remember to update this before we merge

@Al-Kindi-0 we can revert this now

EDIT: or are we waiting for miden-crypto v0.14.0?

stdlib/tests/crypto/falcon.rs

stdlib/asm/crypto/stark/constants.masm

stdlib/asm/crypto/stark/deep_queries.masm

stdlib/asm/crypto/stark/verifier.masm

stdlib/asm/crypto/stark/deep_queries.masm

stdlib/asm/crypto/stark/constants.masm

bobbinth · 2025-03-05T18:32:05Z

@Al-Kindi-0 - should this PR be based on #1658? Basically, the sequence would be:

Release miden-crypto v0.14.0
Merge #1658.
Merge this PR.

Right?

bobbinth · 2025-03-05T18:39:03Z

stdlib/tests/crypto/stark/verifier_recursive/mod.rs

+    let mut main_and_aux_frame_states = ood_main_trace_frame.current().to_vec();
+    main_and_aux_frame_states.extend_from_slice(
+        ood_aux_trace_frame
+            .as_ref()
+            .expect("execution trace should have an auxiliary segment")
+            .current(),
+    );
+    main_and_aux_frame_states.extend_from_slice(ood_main_trace_frame.next());
+    main_and_aux_frame_states.extend_from_slice(
+        ood_aux_trace_frame
+            .as_ref()
+            .expect("execution trace should have an auxiliary segment")
+            .next(),
+    );


Could we add a small comment here explaining what arrangement we are trying to achieve? I think it is something like:

[main_current_elements, aux_current_elements, main_next_elements, aux_next_elements]

That's correct, will add a comment explaining that.

Al-Kindi-0 · 2025-03-05T18:54:55Z

@Al-Kindi-0 - should this PR be based on #1658? Basically, the sequence would be:
* Release `miden-crypto` v0.14.0

* Merge [#1658](https://github.com/0xPolygonMiden/miden-vm/pull/1658).

* Merge this PR.
Right?

That is correct, we should make a new release of the crypto crate with the new Winterfell release, update the Miden VM PR migrating to the new Winterfell release i.e., #1658, and then we can merge the current PR.

bobbinth

Looks good! Thank you! Not a very deep review from me, but I left some comments inline (mostly doc-related).

stdlib/asm/crypto/stark/constants.masm

stdlib/asm/crypto/stark/random_coin.masm

stdlib/asm/crypto/stark/verifier.masm

stdlib/asm/crypto/stark/ood_frames.masm

stdlib/asm/crypto/stark/deep_queries.masm

bobbinth

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

bobbinth · 2025-03-09T07:28:16Z

stdlib/asm/crypto/stark/constants.masm

+# OOD_TRACE_CURRENT_PTR: (71 + 7) * 2 * 2 = 312
 # OOD_CONSTRAINT_EVALS_PTR: 8 * 2 = 16


Shouldn't this say:

# OOD_TRACE_CURRENT_PTR: (71 + 7) * 2 = 156 # OOD_TRACE_NEXT_PTR: (71 + 7) * 2 = 156 # OOD_CONSTRAINT_EVALS_PTR: 8 * 2 = 16

Missed updating these, thanks, fixed now

bobbinth · 2025-03-09T07:37:46Z

stdlib/asm/crypto/stark/verifier.masm

@@ -141,7 +140,7 @@ export.verify
    #       VI) Evaluate the constraints over the OOD frame and assert equality with H(z)
    #==============================================================================================

-    # TODO: Compare with the evaluation of the constraints on the EvaluationFrame
+    # Compare with the evaluation of the constraints on the trace columns OOD evaluation frame. 


Shouldn't the TDDO stay here?

We usually remove todos when we have open issues tracking them. Re-added it now.

stdlib/asm/crypto/stark/random_coin.masm

bobbinth · 2025-03-09T07:42:13Z

stdlib/asm/crypto/stark/random_coin.masm

-    push.0x020407
+    ## field extension degree || FRI folding factor || FRI remainder polynomial max degree || blowup factor
+    push.0x02040708
+    # => [PROOF_OPTIONS, MODULUS1, MODULUS0, TRACE_INFO, trace_length, num_queries, blowup, grinding, ...]


MODULUS1 and MODULUS0 are single elements, right? If so, these should be modulus1, modulus0.

good point, fixed

plafer

LGTM! (assuming we merge after we update the Cargo.toml with the new miden-crypto release)

Al-Kindi-0 marked this pull request as draft February 17, 2025 16:18

This was referenced Feb 21, 2025

Optimize FRI remainder polynomial check #1670

Draft

Finalize the design of the ACE chiplet #1637

Open

Al-Kindi-0 force-pushed the al-update-recursive-verifier-after-horner-eval branch from c294a7d to 03bc93e Compare March 3, 2025 08:09

Al-Kindi-0 marked this pull request as ready for review March 3, 2025 08:10

Al-Kindi-0 requested review from bobbinth and plafer March 3, 2025 08:10

plafer reviewed Mar 3, 2025

View reviewed changes

plafer mentioned this pull request Mar 4, 2025

Format MASM #1678

Open

bobbinth reviewed Mar 5, 2025

View reviewed changes

bobbinth reviewed Mar 6, 2025

View reviewed changes

bobbinth approved these changes Mar 9, 2025

View reviewed changes

Al-Kindi-0 force-pushed the al-update-recursive-verifier-after-horner-eval branch 2 times, most recently from a547563 to 34d4a9d Compare March 9, 2025 12:22

Al-Kindi-0 added 8 commits March 9, 2025 13:23

feat: add horner_eval_ext, update tests and docs

41c79e1

feat: updated recursive verifier to use horner_eval_*

999ca7c

doc: update comments

eaf88fa

fix: add changelog

c1bf6be

feat: update compute_Hz to use horner_eval_ext

de1e661

address feedback

81f0f30

address feedback

17c5c49

address feedback

5a81c5f

Al-Kindi-0 force-pushed the al-update-recursive-verifier-after-horner-eval branch from 34d4a9d to 5a81c5f Compare March 9, 2025 12:24

plafer mentioned this pull request Mar 10, 2025

Consider having the number of columns as a variable instead of being harcoded #1690

Open

plafer approved these changes Mar 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update recursive verifier to use Horner eval ops instead of `rcomb_*` #1665

Update recursive verifier to use Horner eval ops instead of `rcomb_*` #1665

Al-Kindi-0 commented Feb 17, 2025

plafer left a comment

plafer Mar 3, 2025

plafer Mar 10, 2025 •

edited

Loading

bobbinth commented Mar 5, 2025

bobbinth Mar 5, 2025

Al-Kindi-0 Mar 5, 2025

Al-Kindi-0 commented Mar 5, 2025

bobbinth left a comment

bobbinth left a comment

bobbinth Mar 9, 2025

Al-Kindi-0 Mar 9, 2025

bobbinth Mar 9, 2025

Al-Kindi-0 Mar 9, 2025

bobbinth Mar 9, 2025

Al-Kindi-0 Mar 9, 2025

plafer left a comment

		# OOD_TRACE_CURRENT_PTR: (71 + 7) * 2 * 2 = 312
		# OOD_CONSTRAINT_EVALS_PTR: 8 * 2 = 16

Update recursive verifier to use Horner eval ops instead of rcomb_* #1665

Are you sure you want to change the base?

Update recursive verifier to use Horner eval ops instead of rcomb_* #1665

Conversation

Al-Kindi-0 commented Feb 17, 2025

plafer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plafer Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

bobbinth commented Mar 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Al-Kindi-0 commented Mar 5, 2025

bobbinth left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plafer left a comment

Choose a reason for hiding this comment

Update recursive verifier to use Horner eval ops instead of `rcomb_*` #1665

Update recursive verifier to use Horner eval ops instead of `rcomb_*` #1665

plafer Mar 10, 2025 •

edited

Loading