Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update mul_add result type #303

Merged
merged 3 commits into from
Oct 4, 2024
Merged

Update mul_add result type #303

merged 3 commits into from
Oct 4, 2024

Conversation

zemse
Copy link
Collaborator

@zemse zemse commented Oct 3, 2024

The current mul_add function does not include intermediate limb values returned from multiplication.

Also for assigning multiplication and addition result, it involves multiple lines. This PR replaces assignment in divu with assign_limb_with_carry and assign_limb_with_carry_auxiliary which simplifies the assignment code by abstracting assignment of limbs and carry using max carry value.

@zemse zemse requested review from KimiWu123 and hero78119 October 3, 2024 21:24
@@ -37,13 +37,13 @@ impl<E: ExtensionField, I: RIVInstruction> Instruction<E> for ShiftImmInstructio
circuit_builder: &mut CircuitBuilder<E>,
) -> Result<Self::InstructionConfig, ZKVMError> {
let mut imm = UInt::new(|| "imm", circuit_builder)?;
let mut rd_written = UInt::new_unchecked(|| "rd_written", circuit_builder)?;
let mut rd_written = UInt::new(|| "rd_written", circuit_builder)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Soundness fix and principle: we need to have range check on written register value

@hero78119
Copy link
Collaborator

Updated with

  • rebase to master
  • clean up shift_imm_circuit.rs to fully leverage mul_add
  • unify mul_add circuit/value return argument order

Copy link
Collaborator

@hero78119 hero78119 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
Updated with

  • rebase to master
  • clean up shift_imm_circuit.rs to fully leverage mul_add
  • unify mul_add circuit/value return argument order: (final_result, intermediate_mul)

@hero78119 hero78119 enabled auto-merge (squash) October 4, 2024 02:51
@hero78119 hero78119 merged commit 45a51f3 into scroll-tech:master Oct 4, 2024
6 checks passed
@zemse zemse deleted the mul-add branch October 8, 2024 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants