refactor: rvr extensions remove code duplication

[mansur20478]

Refactor rvr algebra extension to remove code duplication. Also removes code duplication between Algebra/ECC/Pairing extensions for common computation.

Changes:

1. Create common pad_modulus function for padding in algebra extension.
2. Create common FieldArithInstr IR for modular and fp2 in algebra extension. Add ArithKind trait for modular and fp2 to implement.
3. Create common FieldSetupInstr IR for modular and fp2 in algebra extension. Add SetupKind trait; C function name derived from kind prefix.
4. Create common FieldIsEqInstr IR for modular in algebra extension. Add IsEqKind trait; not implemented for fp2.
5. Create common macro for unknown field in FFI.
6. Reduce code duplication for known field arithmetics in algebra extension.

resolves INT-7215

[mansur20478]

@claude please, review this pr, the original goal was to:
"""
Currently there's some duplicate code in the Algebra extension for multiple field types, and also between Algebra/ECC/Pairing extensions for common computation. Refactor these parts while maintaining modularity.
"""

Please, be prudent, and evaluate the correctness of the code that is in this pr, the quality and overall

[claude]

Claude finished @mansur20478's task in 3m 47s —— View job

---

PR Review: Refactor RVR Extensions — Remove Code Duplication

• Read all changed files and understand the refactoring
• Compare with base branch to verify correctness
• Evaluate code quality and duplication reduction
• Check for potential issues (correctness, naming, modularity)
• Write final review

---

Summary

This PR consolidates duplicate code across the modular/fp2 algebra and pairing extensions. The changes are well-structured into 7 atomic commits, each addressing a distinct deduplication axis. Net result: 368 additions, 507 deletions (−139 lines).

Correctness Verdict: Looks Good

I traced all refactored code paths against the base branch and the behavior is preserved:

1. pad_modulus extraction — Identical logic extracted from both modular::make_modulus_info and fp2::make_modulus_info. Both callers now use the shared version.

2. FieldArithInstr<K> / ArithKind trait — The generic emit_c in common.rs:89-107 correctly parameterizes on K::c_prefix() and K::known_suffix():

• For ModArithKind: c_prefix()="mod", known_suffix()=Some(field.c_suffix()) → generates rvr_ext_mod_{op}_{suffix} (matches original ModArithInstr::emit_c)
• For Fp2ArithKind: c_prefix()="fp2", known_suffix()=field.fp2_c_suffix() → generates rvr_ext_fp2_{op}_{suffix} (matches original Fp2ArithInstr::emit_c)

3. FieldSetupInstr — The opname() uses &self.c_fn["rvr_ext_".len()..] to slice the C function name. With "rvr_ext_mod_setup" this yields "mod_setup" and with "rvr_ext_fp2_setup" it yields "fp2_setup" — both matching the original hardcoded values. The emit_c delegates to the stored c_fn name directly.

4. KnownFieldArith trait + blanket impl — Clean extraction. The add/sub/mul/div/is_eq implementations via Field trait operators are identical to what was repeated in every FieldArith impl for known fields (both modular and fp2).

5. known_field_op_fn! / unknown_field_op_fn! macros — The shared macros in ffi/common are strictly more general than the per-crate versions they replace: known_field_op_fn! adds a $wrapper parameter (the per-crate macros had the wrapper hardcoded), and unknown_field_op_fn! adds a $field_ty parameter.

6. read_pairing_points refactoring — The shared function correctly parameterizes on Fq, Fq2, fq_bytes, read_fq, and make_fq2. The BN254 call passes bn256::Fq2::new and the BLS12-381 call passes |c0, c1| bls12_381::Fq2 { c0, c1 } — matching the original struct construction patterns exactly.

Suggestions

Minor:

1. KnownField visibility widened to pub (lib.rs:65). This was pub(crate) before and is only used within the crate. The widening is forced by ArithKind::known_suffix(field: KnownField) being in a pub trait. Consider whether ArithKind could be pub(crate) instead — it's only implemented and used within this crate. If external consumers shouldn't implement ArithKind, narrowing the trait visibility would let KnownField stay pub(crate). Fix this →

2. FieldSetupInstr::opname uses string slicing (common.rs:64): &self.c_fn["rvr_ext_".len()..]. This is correct but fragile — if someone passes a c_fn that doesn't start with "rvr_ext_", it silently returns a wrong substring (or panics if shorter). A debug_assert! or strip_prefix with a panic message would make the contract explicit. Low risk since all call sites are within this crate.

3. Commit message typos — "cmmon" (commit 7b16496), "isntead" and "seperate" (846f02a), "trates" (0a3598a). Worth squash-fixing before merge if the team cares about git log readability.

4. ModIsEqInstr not unified — This instruction has a different signature (returns a value via extern_call_expr rather than writing through a register pointer), so it's correctly left as a separate type. Just noting this is intentional, not an oversight.

Overall

Clean, well-scoped refactoring. The ArithKind trait is a good abstraction boundary — it captures exactly the two axes of variation (C prefix and known-field suffix lookup) without over-engineering. The KnownFieldArith blanket impl in the FFI layer is similarly well-targeted. No correctness issues found. The benchmark numbers show no regression.

Verdict: Approve (modulo the minor suggestions above).

---

[github-actions]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

[mansur20478]

@claude Is it good idea to unify IR for modular and fp2? what are the pros and the cons of the solution?

[claude]

Claude finished @mansur20478's task in 1m 21s —— View job

---

Analysis: Should the IR for Modular and Fp2 be Unified?

• Read the unified FieldArithInstr and FieldSetupInstr in common.rs
• Read the ArithKind trait and its implementations
• Read the original modular and fp2 IR code (before and after)
• Read FFI layer to understand the full picture
• Analyze the design tradeoffs
• Write up pros and cons

---

Short answer: yes, for this codebase, it's a good design decision. The two IR node families (modular and fp2) had nearly identical structure — the only real differences were a string prefix ("mod" vs "fp2") and how known-field suffixes are resolved. The ArithKind trait captures exactly those two axes of variation. Here's the detailed breakdown:

Pros

1. The duplication was real, not superficial.
Before this PR, ModArithInstr and Fp2ArithInstr were independent structs with identical field layouts (op, rd_reg, rs1_reg, rs2_reg, num_limbs, modulus) and near-identical emit_c implementations. The only textual differences were "mod" vs "fp2" in C function names and how KnownField maps to a suffix (field.c_suffix() vs field.fp2_c_suffix()). This is exactly the kind of duplication worth eliminating — mechanical copies that diverge silently.

2. The ArithKind trait is minimal and well-targeted.
It has only three methods: opname(), c_prefix(), and known_suffix(). This captures exactly the variation between modular and fp2, nothing more. There's no over-abstraction here — no builder pattern, no dynamic dispatch, no configuration object. The trait is pub(crate), so it's a private implementation detail.

3. Bugs fixed once instead of twice.
Any future change to how emit_c formats the C function call, handles the known-field vs unknown-modulus branching, or escapes the modulus bytes now lives in one place (common.rs:147-180). Previously, a bugfix in the modular path could easily miss the fp2 path (or vice versa).

4. FieldSetupInstr is even cleaner.
The setup instructions for both modular and fp2 had identical logic — the only difference was the C function name string ("rvr_ext_mod_setup" vs "rvr_ext_fp2_setup"). Parameterizing on a c_fn: &'static str is simpler than maintaining two separate structs. Both ModSetupInstr and Fp2SetupInstr are now type aliases for the same FieldSetupInstr.

5. The FFI macros (known_field_op_fn! / unknown_field_op_fn!) follow the same pattern.
Previously, each FFI crate (modular/fp2) had its own copy of these macros with a hardcoded wrapper type. Now the wrapper and field type are parameters, so the macros live in ffi/common and are reused by both staticlibs. This also enables the pairing extension to reuse them in the future.

Cons

1. Indirection cost: reading the code requires a hop through ArithKind.
When a developer sees FieldArithInstr<ModArithKind>, they need to look up ModArithKind to understand what C functions it generates. Before the refactor, ModArithInstr::emit_c was self-contained — you could read the IR node and see exactly what C it emitted. The indirection is shallow (trait impl is ~10 lines in the same crate), but it's real.

2. FieldSetupInstr::opname uses string slicing — &self.c_fn["rvr_ext_".len()..].
This is a fragile implicit contract. If someone constructs a FieldSetupInstr with a c_fn that doesn't start with "rvr_ext_", it silently returns a wrong substring or panics if shorter. A separate ModSetupInstr / Fp2SetupInstr struct with hardcoded opname() values would have been statically correct. The risk is low (all call sites are in-crate and pass the right strings), but it trades compile-time safety for runtime convention.

3. FieldIsEqInstr<K> is only used as ModIsEqInstr today.
Fp2 doesn't have an IS_EQ instruction, so FieldIsEqInstr is generic over ArithKind but only ever instantiated with ModArithKind. The generic parameter is speculative — it assumes a future Fp2IsEqInstr might appear. If that never happens, the generic is dead weight. That said, the cost is minimal (a PhantomData and a trait bound), and making it generic is consistent with FieldArithInstr.

4. The type aliases can hide what's happening.
pub type ModSetupInstr = FieldSetupInstr means ModSetupInstr and Fp2SetupInstr are the same type at the Rust level. If someone accidentally uses a Fp2SetupInstr where a ModSetupInstr is expected (or vice versa), the type system won't catch it. The FieldArithInstr<K> approach is better here — the phantom type parameter makes ModArithInstr and Fp2ArithInstr distinct types — but setup instructions don't carry this distinction.

5. Adding a third field type with a different emit_c structure would require rethinking the abstraction.
The current design assumes all field arithmetic IR nodes share the same emit_c shape: rvr_ext_{prefix}_{op}[_{suffix}]. If a future extension needs a different code-gen pattern (e.g., extra parameters, different call conventions), it can't reuse FieldArithInstr<K> and would need its own struct anyway. However, this is speculative — the current design handles the two existing cases well, and YAGNI applies.

Verdict

The pros clearly outweigh the cons. The two most legitimate concerns (string slicing in opname, and setup type aliases being interchangeable) are low-risk given that all construction sites are within the same crate. The refactoring eliminated ~140 net lines of genuine mechanical duplication while keeping the abstraction narrow and crate-private. The ArithKind trait is a textbook example of a well-scoped parameterization — it captures exactly the axes of variation and nothing else.

---

View job

[mansur20478]

@claude please, perform review of this PR again

[claude]

Claude finished @mansur20478's task in 3m 59s —— View job

---

PR Review: Refactor RVR Extensions — Remove Code Duplication

• Read all changed files and the full diff
• Compare with base branch to verify correctness
• Evaluate code quality, abstractions, and edge cases
• Check for potential issues (correctness, naming, modularity, safety)
• Check CI status
• Write final review

---

Net result: 470 additions, 549 deletions (−79 lines). Well-structured across 9 atomic commits, each addressing a distinct deduplication axis.

Correctness: Looks Good

I traced all refactored code paths against the base branch (3282efa) and the behavior is preserved:

1. pad_modulus extraction — Identical logic extracted from both modular::make_modulus_info and fp2::make_modulus_info. Both callers now use the shared version. The modular variant still takes &BigUint (since it needs the reference for find_non_qr afterward), while fp2 takes owned BigUint — pad_modulus correctly takes &BigUint to serve both.

2. FieldArithInstr<K> / ArithKind trait — The generic emit_c in common.rs:167-200 correctly parameterizes on K::c_prefix() and K::known_suffix():

   • ModArithKind: c_prefix()="mod", known_suffix()=Some(field.c_suffix()) → rvr_ext_mod_{op}_{suffix} ✓
   • Fp2ArithKind: c_prefix()="fp2", known_suffix()=field.fp2_c_suffix() → rvr_ext_fp2_{op}_{suffix} ✓

3. FieldSetupInstr<K> — emit_c generates rvr_ext_{prefix}_setup using K::c_prefix(). For mod: "rvr_ext_mod_setup", for fp2: "rvr_ext_fp2_setup" — both matching the originals. The previous review's concern about fragile string slicing (opname) is now resolved: opname() is provided directly by the SetupKind trait ("mod_setup" / "fp2_setup"), no string slicing involved.

4. FieldIsEqInstr<K> — The emit_c for IS_EQ correctly handles both the known-field fast path (extern_call_expr returning uint32_t) and the unknown-modulus fallback. Only instantiated for ModArithKind as expected.

5. KnownFieldArith blanket impl — Clean extraction. The add/sub/mul/div/is_eq implementations via Field trait operators are identical to what was repeated in every FieldArith impl for known fields.

6. known_field_op_fn! / unknown_field_op_fn! macros — The shared macros in ffi/common correctly add $wrapper and $field_ty parameters. The macro bodies are exact substitutions of the per-crate versions they replace.

7. read_pairing_points refactoring — Correctly parameterizes on Fq, Fq2, fq_bytes, read_fq, and make_fq2. BN254 passes bn256::Fq2::new and BLS12-381 passes |c0, c1| bls12_381::Fq2 { c0, c1 } — matching the original construction patterns.

CI Status

Check	Status
Extension Tests	✅ Pass
Extension Tests (CUDA)	✅ Pass
Lint Workspace	❌ Fail — cargo +nightly fmt
Pull Request	⏳ Queued

The lint failure must be fixed before merge. The fmt check found formatting issues across all 8 changed files. Running cargo +nightly fmt --all should resolve all of them. The issues are all nightly-rustfmt style violations (e.g., single-line bodies that should be multi-line, import grouping, argument placement).

Issues Found

1. Formatting (blocking) — CI lint is failing. Run cargo +nightly fmt --all and push the result. Example violations:

• common.rs:58: constructor body on one line exceeds width limit
• modular/ffi/src/lib.rs:51: single-line read_elem/write_elem bodies need multi-line formatting
• common/ffi/src/lib.rs:71-77: blanket impl arithmetic methods need multi-line formatting
• Various import groupings across all files

2. Trait hierarchy is slightly over-engineered (non-blocking) — There are now four traits (FieldKind, ArithKind, SetupKind, IsEqKind) where only one type (ModArithKind) implements all four and another (Fp2ArithKind) implements three. The hierarchy adds a small conceptual overhead. However, since they're all pub(crate) and the decomposition is clean, this is fine — it does prevent accidentally implementing IsEqKind for Fp2 which has no IS_EQ instruction.

3. FieldIsEqInstr<K> only used as ModIsEqInstr (non-blocking) — The generic over IsEqKind is speculative since fp2 has no IS_EQ. The cost is minimal (a PhantomData + trait bound), and it's consistent with the other instruction families. Acceptable.

4. Visibility narrowing is correct (observation) — ModArithInstr, ModIsEqInstr, ModSetupInstr, Fp2ArithInstr, Fp2SetupInstr were previously pub re-exported from lib.rs. They're now pub(crate) type aliases. I confirmed no external crate references these types — only the two extension files within this crate use them. The public API correctly only exports ModularRvrExtension, Fp2RvrExtension, HintNonQrInstr, and HintSqrtInstr. KnownField stays pub(crate) as before.

Suggestions

1. Fix formatting — cargo +nightly fmt --all and push. This is the only blocker.

2. Consider making FieldKind a single trait with associated constants for the Kind variants — Instead of 4 separate traits, you could have marker traits like HasArith, HasSetup, HasIsEq as supertrait bounds on FieldKind itself. But this is a style preference, not a correctness issue. The current approach is fine.

3. Commit message typos (minor) — "isntead" and "seperate" (846f02a), "trates" (0a3598a), "cmmon" (7b16496). Worth squash-fixing before merge if git log readability matters.

Overall

Clean, well-scoped refactoring that eliminates genuine mechanical duplication while keeping the abstraction narrow and crate-private. The FieldKind trait hierarchy captures exactly the axes of variation (C prefix, known-field suffix lookup, opname) without over-engineering. Benchmark numbers show no regression. The only blocking issue is running cargo +nightly fmt --all to fix formatting.

Verdict: Approve after formatting fix.

---

View job

[github-actions]

group	app.proof_time_ms	app.cycles	leaf.proof_time_ms
fibonacci	1,035	4,000,051	389
keccak	15,531	14,365,133	2,975
sha2_bench	8,266	11,167,961	1,016
regex	1,172	4,090,656	359
ecrecover	437	112,210	285
pairing	600	592,827	298
kitchen_sink	3,873	1,979,971	855

Note: cells_used metrics omitted because CUDA tracegen does not expose unpadded trace heights.

Commit: 9099816

Benchmark Workflow

[mansur20478]

@claude please compare that there are no differences compared with before the refactoring, meaning that behavior and output of functions is the same as it was before the refactoring

[claude]

Claude finished @mansur20478's task in 4m 16s —— View job

---

Behavioral Equivalence Verification

I read every line of the 8 changed files and compared each refactored function against its original implementation in the base branch (3282efa). Below is the detailed trace for each change.

• Gather the full diff and read all changed files (old vs new)
• Verify pad_modulus extraction (modular + fp2)
• Verify FieldArithInstr<K> / ArithKind — emit_c output matches original ModArithInstr and Fp2ArithInstr
• Verify FieldSetupInstr<K> / SetupKind — emit_c output matches original ModSetupInstr and Fp2SetupInstr
• Verify FieldIsEqInstr<K> / IsEqKind — emit_c output matches original ModIsEqInstr
• Verify FFI macros (known_field_op_fn! / unknown_field_op_fn!) produce identical C function signatures
• Verify KnownFieldArith blanket impl matches per-type impls
• Verify read_pairing_points shared function matches BN254 + BLS12-381 originals
• Final summary

---

1. pad_modulus extraction

Original (modular) — modular.rs:make_modulus_info lines 39–51:

let bytes = modulus.bits().div_ceil(8) as usize;
assert!(bytes <= 48, "modulus exceeds maximum supported size of 384 bits");
let num_limbs = if bytes <= 32 { 32u32 } else { 48u32 };
let mut modulus_bytes = modulus.to_bytes_le();
modulus_bytes.resize(num_limbs as usize, 0);
// then uses modulus_bytes, num_limbs

Original (fp2) — fp2.rs:make_modulus_info lines 24–35:

let bytes = modulus.bits().div_ceil(8) as usize;
assert!(bytes <= 48, "modulus exceeds maximum supported size of 384 bits");
let num_limbs = if bytes <= 32 { 32u32 } else { 48u32 };
let mut modulus_bytes = modulus.to_bytes_le();
modulus_bytes.resize(num_limbs as usize, 0);

New shared — lib.rs:pad_modulus lines 23–33:

fn pad_modulus(modulus: &BigUint) -> (Vec<u8>, u32) {
    let bytes = modulus.bits().div_ceil(8) as usize;
    assert!(bytes <= 48, "modulus exceeds maximum supported size of 384 bits");
    let num_limbs = if bytes <= 32 { 32u32 } else { 48u32 };
    let mut modulus_bytes = modulus.to_bytes_le();
    modulus_bytes.resize(num_limbs as usize, 0);
    (modulus_bytes, num_limbs)
}

Callers:

• modular.rs:41: let (modulus_bytes, num_limbs) = pad_modulus(modulus); — then computes non_qr exactly as before.
• fp2.rs:28: let (modulus_bytes, num_limbs) = pad_modulus(&modulus); — identical to original.

Verdict: Identical. The logic byte-for-byte matches both original inline implementations.

---

2. FieldArithInstr<K> / ArithKind — emit_c output

Original ModArithInstr::emit_c (modular.rs:75–92):

let rd = ctx.read_reg(self.rd_reg);
let rs1 = ctx.read_reg(self.rs1_reg);
let rs2 = ctx.read_reg(self.rs2_reg);
let op_name = self.op.c_name();
if let Some(field) = detect_known_field(&self.modulus) {
    let suffix = field.c_suffix();
    let name = format!("rvr_ext_mod_{op_name}_{suffix}");
    ctx.extern_call(&name, &["state", &rd, &rs1, &rs2]);
} else {
    let mod_literal = format_c_byte_array(&self.modulus);
    ctx.write_line("{");
    ctx.write_line(&format!("static const uint8_t mod_[] = {mod_literal};"));
    let name = format!("rvr_ext_mod_{op_name}");
    let num_limbs = format!("{}u", self.num_limbs);
    ctx.extern_call(&name, &["state", &rd, &rs1, &rs2, &num_limbs, "mod_"]);
    ctx.write_line("}");
}

New generic FieldArithInstr<K>::emit_c (common.rs:187–206), with K = ModArithKind:

• K::c_prefix() → "mod"
• K::known_suffix(field) → Some(field.c_suffix())

Substituting:

let prefix = "mod";
let known_suffix = detect_known_field(&self.modulus).and_then(|f| Some(f.c_suffix()));
// known_suffix = detect_known_field(&self.modulus).map(|f| f.c_suffix())
if let Some(suffix) = known_suffix {
    let name = format!("rvr_ext_mod_{op_name}_{suffix}");
    ctx.extern_call(&name, &["state", &rd, &rs1, &rs2]);
} else {
    // ... format!("rvr_ext_mod_{op_name}") ...
}

Identical output — the known-field branch produces rvr_ext_mod_{op}_{suffix}, the unknown branch produces rvr_ext_mod_{op} with num_limbs and mod_ arguments. Same as original.

Original Fp2ArithInstr::emit_c (fp2.rs:66–83):

• Known field branch: detect_known_field(&self.modulus).and_then(|f| f.fp2_c_suffix()) → "rvr_ext_fp2_{op_name}_{suffix}"
• Unknown branch: "rvr_ext_fp2_{op_name}"

New generic with K = Fp2ArithKind:

• K::c_prefix() → "fp2"
• K::known_suffix(field) → field.fp2_c_suffix()
• Known: detect_known_field(&self.modulus).and_then(|f| f.fp2_c_suffix()) → format!("rvr_ext_fp2_{op_name}_{suffix}") — same.
• Unknown: format!("rvr_ext_fp2_{op_name}") — same.

Verdict: Identical. Both ModArithInstr and Fp2ArithInstr produce exactly the same C code as before.

opname() values:

• Old ModArithInstr::opname() → "mod_arith". New ModArithKind::opname() → "mod_arith". ✅
• Old Fp2ArithInstr::opname() → "fp2_arith". New Fp2ArithKind::opname() → "fp2_arith". ✅

---

3. FieldSetupInstr<K> / SetupKind — emit_c output

Original ModSetupInstr::emit_c (modular.rs:145–153):

let name = "rvr_ext_mod_setup";
ctx.extern_call("rvr_ext_mod_setup", &["state", &rd, &rs1, &rs2, &num_limbs]);

New generic with K = ModArithKind:

• K::c_prefix() → "mod"
• format!("rvr_ext_{}_setup", "mod") → "rvr_ext_mod_setup" ✅

Original Fp2SetupInstr::emit_c (fp2.rs:109–115):

ctx.extern_call("rvr_ext_fp2_setup", &["state", &rd, &rs1, &rs2, &num_limbs]);

New generic with K = Fp2ArithKind:

• K::c_prefix() → "fp2"
• format!("rvr_ext_{}_setup", "fp2") → "rvr_ext_fp2_setup" ✅

opname() values:

• Old ModSetupInstr::opname() → "mod_setup". New ModArithKind SetupKind::opname() → "mod_setup". ✅
• Old Fp2SetupInstr::opname() → "fp2_setup". New Fp2ArithKind SetupKind::opname() → "fp2_setup". ✅

Verdict: Identical.

---

4. FieldIsEqInstr<K> / IsEqKind — emit_c output

Original ModIsEqInstr::emit_c (modular.rs:107–131):

if let Some(field) = detect_known_field(&self.modulus) {
    let suffix = field.c_suffix();
    let name = format!("rvr_ext_mod_iseq_{suffix}");
    let val = ctx.extern_call_expr("uint32_t", &name, &["state", &rs1, &rs2]);
    ctx.write_reg(self.rd_reg, &val);
} else {
    // format!("rvr_ext_mod_iseq") with num_limbs + mod_
}

New generic with K = ModArithKind (common.rs:143–180):

• K::c_prefix() → "mod"
• K::known_suffix(field) → Some(field.c_suffix())
• Known: format!("rvr_ext_mod_iseq_{suffix}") ✅
• Unknown: format!("rvr_ext_mod_iseq") ✅
• Both use extern_call_expr("uint32_t", ...) and ctx.write_reg(self.rd_reg, &val) ✅

opname(): Old → "mod_iseq". New ModArithKind IsEqKind::opname() → "mod_iseq". ✅

Verdict: Identical.

---

5. FFI macros — known_field_op_fn! / unknown_field_op_fn!

Original field_op_fn! in modular (modular.rs:153–162):

macro_rules! field_op_fn {
    ($name:ident, $field:ty, $op:ident) => {
        #[no_mangle]
        pub unsafe extern "C" fn $name(state: *mut c_void, rd: u32, rs1: u32, rs2: u32) {
            let f = KnownPrimeField::<$field>(PhantomData);
            exec_op(&f, state, rd, rs1, rs2, |f, a, b| f.$op(a, b));
        }
    };
}

New shared known_field_op_fn! (common/src/lib.rs:242–257):

macro_rules! known_field_op_fn {
    ($name:ident, $wrapper:ident, $field:ty, $op:ident) => {
        #[no_mangle]
        pub unsafe extern "C" fn $name(state: *mut c_void, rd: u32, rs1: u32, rs2: u32) {
            let f = $wrapper::<$field>(::std::marker::PhantomData);
            $crate::exec_op(&f, state, rd, rs1, rs2, |f, a, b| f.$op(a, b));
        }
    };
}

The only difference is $wrapper is now a parameter instead of hardcoded KnownPrimeField. Call sites pass KnownPrimeField for modular and KnownComplexField for fp2, which matches the original hardcoded values exactly.

Modular call sites (modular/src/lib.rs:127–130):

known_field_op_fn!([<rvr_ext_mod_add_ $suffix>], KnownPrimeField, $field, add);

Expands to same as old field_op_fn!([<rvr_ext_mod_add_ $suffix>], $field, add) with KnownPrimeField hardcoded. ✅

Fp2 call sites (fp2/src/lib.rs:126–129):

known_field_op_fn!([<rvr_ext_fp2_add_ $suffix>], KnownComplexField, $field, add);

Same as old field_op_fn!([<rvr_ext_fp2_add_ $suffix>], $field, add) with KnownComplexField hardcoded. ✅

Original unknown_field_op_fn! in modular (modular.rs:168–183):

macro_rules! unknown_field_op_fn {
    ($name:ident, $op:ident) => {
        // ...
        let f = UnknownPrimeField { modulus, num_limbs };
        exec_op(&f, state, rd_ptr, rs1_ptr, rs2_ptr, |f, a, b| f.$op(a, b));
    };
}

New shared unknown_field_op_fn! (common/src/lib.rs:264–286):

macro_rules! unknown_field_op_fn {
    ($name:ident, $field_ty:ident, $op:ident) => {
        // ...
        let f = $field_ty { modulus, num_limbs };
        $crate::exec_op(&f, state, rd_ptr, rs1_ptr, rs2_ptr, |f, a, b| f.$op(a, b));
    };
}

Same pattern — $field_ty replaces hardcoded UnknownPrimeField. Call sites pass the correct types:

• unknown_field_op_fn!(rvr_ext_mod_add, UnknownPrimeField, add) ✅
• unknown_field_op_fn!(rvr_ext_fp2_add, UnknownComplexField, add) ✅

Verdict: Identical. The generated C ABI function signatures, bodies, and symbol names are all identical.

---

6. KnownFieldArith blanket impl — arithmetic ops

Original per-type impls (in both modular and fp2 FFI crates) all had identical arithmetic:

fn add(&self, a: Self::Elem, b: Self::Elem) -> Self::Elem { a + b }
fn sub(&self, a: Self::Elem, b: Self::Elem) -> Self::Elem { a - b }
fn mul(&self, a: Self::Elem, b: Self::Elem) -> Self::Elem { a * b }
fn div(&self, a: Self::Elem, b: Self::Elem) -> Self::Elem { a * b.invert().unwrap() }
fn is_eq(&self, a: &Self::Elem, b: &Self::Elem) -> bool { a == b }

This was repeated 3 times:

1. KnownPrimeField<F: PrimeField<Repr=[u8;32]>> — modular FFI
2. KnownPrimeField<Bls12381Fq> — modular FFI
3. KnownComplexField<bn256::Fq2> — fp2 FFI
4. KnownComplexField<blstrs::Fp2> — fp2 FFI

New blanket impl (common/src/lib.rs:57–90):

impl<T: KnownFieldArith> FieldArith for T {
    fn add(&self, a: Self::Elem, b: Self::Elem) -> Self::Elem { a + b }
    fn sub(&self, a: Self::Elem, b: Self::Elem) -> Self::Elem { a - b }
    fn mul(&self, a: Self::Elem, b: Self::Elem) -> Self::Elem { a * b }
    fn div(&self, a: Self::Elem, b: Self::Elem) -> Self::Elem { a * b.invert().unwrap() }
    fn is_eq(&self, a: &Self::Elem, b: &Self::Elem) -> bool { a == b }
}

Each type now only implements KnownFieldArith with read_elem/write_elem, and gets the arithmetic ops for free.

I verified each read_elem/write_elem in the new KnownFieldArith impls matches the original FieldArith impls exactly (same function calls, same arguments, same return types):

• KnownPrimeField<F: PrimeField<Repr=[u8;32]>> — read_field_256/write_field_256 ✅
• KnownPrimeField<Bls12381Fq> — read_bls12_381_fq/write_bls12_381_fq ✅
• KnownComplexField<bn256::Fq2> — Fq2::new(read_field_256(state, ptr), read_field_256(state, ptr + FIELD_256_BYTES)) / writes both c0, c1 ✅
• KnownComplexField<blstrs::Fp2> — Fp2::new(read_bls12_381_fq(state, ptr), read_bls12_381_fq(state, ptr + BLS12_381_ELEM_BYTES)) / writes c0(), c1() ✅

The UnknownPrimeField and UnknownComplexField structs still implement FieldArith directly (not KnownFieldArith), and their impls are byte-for-byte identical to the originals. ✅

Verdict: Identical. All generated code paths produce the same results.

---

7. read_pairing_points shared function

Original hint_bn254 (pairing/rvr/ffi/src/lib.rs:95–128 in base):

let p_ptr = rd_mem_u64_wrapper(state, rs1_val);
let p_len = rd_mem_u64_wrapper(state, rs1_val + SLICE_LEN_OFFSET);
let q_ptr = rd_mem_u64_wrapper(state, rs2_val);
let q_len = rd_mem_u64_wrapper(state, rs2_val + SLICE_LEN_OFFSET);
if p_len != q_len { return None; }

let p: Vec<_> = (0..p_len).map(|i| {
    let base = point_base(p_ptr, i, G1_AFFINE_COORDS * BN254_FQ_BYTES)?;
    Some(AffinePoint::new(read_bn254_fq(state, base)?, read_bn254_fq(state, base + BN254_FQ_BYTES)?))
}).collect::<Option<_>>()?;

let q: Vec<_> = (0..q_len).map(|i| {
    let base = point_base(q_ptr, i, G2_AFFINE_COORDS * BN254_FQ_BYTES)?;
    let x = bn256::Fq2::new(read_bn254_fq(state, base)?, read_bn254_fq(state, base + BN254_FQ_BYTES)?);
    let y = bn256::Fq2::new(read_bn254_fq(state, base + 2 * BN254_FQ_BYTES)?, read_bn254_fq(state, base + 3 * BN254_FQ_BYTES)?);
    Some(AffinePoint::new(x, y))
}).collect::<Option<_>>()?;

New read_pairing_points + BN254 call (pairing/rvr/ffi/src/lib.rs:86–138):

unsafe fn read_pairing_points<Fq: Field, Fq2: Field>(
    state, rs1_val, rs2_val, fq_bytes,
    read_fq: impl Fn(*mut c_void, u32) -> Option<Fq>,
    make_fq2: impl Fn(Fq, Fq) -> Fq2,
) -> Option<(Vec<AffinePoint<Fq>>, Vec<AffinePoint<Fq2>>)> {
    // same p_ptr/p_len/q_ptr/q_len logic
    // same p construction with read_fq(state, base)
    // q: make_fq2(read_fq(state, base)?, read_fq(state, base + fq_bytes)?)
}

// BN254 call:
read_pairing_points(state, rs1_val, rs2_val, BN254_FQ_BYTES,
    |s, ptr| unsafe { read_bn254_fq(s, ptr) },
    bn256::Fq2::new,
)

Substituting the closures:

• read_fq → read_bn254_fq ✅
• make_fq2(a, b) → bn256::Fq2::new(a, b) ✅
• fq_bytes → BN254_FQ_BYTES ✅

The P loop builds AffinePoint::new(read_fq(state, base)?, read_fq(state, base + fq_bytes)?) — same as original. The Q loop builds make_fq2(read_fq(state, base)?, read_fq(state, base + fq_bytes)?) — which expands to bn256::Fq2::new(read_bn254_fq(state, base)?, read_bn254_fq(state, base + BN254_FQ_BYTES)?) — same as original. ✅

Original hint_bls12_381 (pairing/rvr/ffi/src/lib.rs:133–164 in base):
The BLS12-381 variant used struct literal construction:

let x = bls12_381::Fq2 { c0: read_bls12_381_fq(state, base)?, c1: read_bls12_381_fq(state, base + BLS12_381_FQ_BYTES)? };

New BLS12-381 call:

read_pairing_points(state, rs1_val, rs2_val, BLS12_381_FQ_BYTES,
    |s, ptr| unsafe { read_bls12_381_fq(s, ptr) },
    |c0, c1| bls12_381::Fq2 { c0, c1 },
)

Substituting: make_fq2(c0, c1) → bls12_381::Fq2 { c0, c1 } — exactly matches the original struct literal pattern. ✅

The rest of both hint_bn254 and hint_bls12_381 (miller loop, final_exp_hint, serialization) is unchanged. ✅

Verdict: Identical.

---

8. try_lift_* functions (modular and fp2 lifters)

The only change in the lifter functions is how IR nodes are constructed — using ::new(...) constructors instead of direct struct literals. I verified each call site:

Modular (8 match arms — ADD, SUB, SETUP_ADDSUB, MUL, DIV, SETUP_MULDIV, IS_EQ, SETUP_ISEQ):

Old	New	Same fields?
ModArithInstr { op: ModOp::Add, rd_reg, rs1_reg, rs2_reg, num_limbs: info.num_limbs, modulus: info.modulus_bytes.clone() }	ModArithInstr::new(ModOp::Add, rd_reg, rs1_reg, rs2_reg, info.num_limbs, info.modulus_bytes.clone())	✅ (constructor sets _kind: PhantomData)
ModSetupInstr { rd_reg, rs1_reg, rs2_reg, num_limbs: info.num_limbs }	ModSetupInstr::new(rd_reg, rs1_reg, rs2_reg, info.num_limbs)	✅
ModIsEqInstr { rd_reg, rs1_reg, rs2_reg, num_limbs: info.num_limbs, modulus: info.modulus_bytes.clone() }	ModIsEqInstr::new(rd_reg, rs1_reg, rs2_reg, info.num_limbs, info.modulus_bytes.clone())	✅

All remaining logic (opcode dispatch, phantom handling, extension trait methods) is byte-for-byte unchanged. ✅

Fp2 (6 match arms — ADD, SUB, SETUP_ADDSUB, MUL, DIV, SETUP_MULDIV):
Same pattern — Fp2ArithInstr { ... } → Fp2ArithInstr::new(...), Fp2SetupInstr { ... } → Fp2SetupInstr::new(...). All fields are passed in the same order. ✅

---

9. Unchanged code (verified not touched)

The following are byte-for-byte identical between old and new:

• HintNonQrInstr struct + ExtInstr impl ✅
• HintSqrtInstr struct + ExtInstr impl ✅
• ModularRvrExtension struct + RvrExtension impl (c_headers, c_sources, staticlib_files, extra_c_sources, extra_c_include_files, extra_cflags) ✅
• Fp2RvrExtension struct + RvrExtension impl ✅
• ModOp enum + c_name() ✅
• KnownField enum + c_suffix() + fp2_c_suffix() ✅
• KNOWN_FIELDS static table ✅
• detect_known_field() ✅
• format_c_byte_array() ✅
• UnknownPrimeField FieldArith impl ✅
• UnknownComplexField FieldArith impl ✅
• exec_iseq() helper ✅
• rvr_ext_mod_iseq (unknown fallback) ✅
• rvr_ext_mod_setup ✅
• rvr_ext_fp2_setup ✅
• mod_sqrt_impl + rvr_ext_algebra_hint_sqrt ✅
• All pairing FFI entry points (rvr_ext_pairing_hint_final_exp_bn254, rvr_ext_pairing_hint_final_exp_bls12_381) ✅

---

Final verdict

No behavioral differences found. Every refactored function produces the same output as its original implementation. Specifically:

1. All generated C code (function names, arguments, control flow) is identical
2. All FFI symbol names and signatures are identical
3. All arithmetic operations produce the same results
4. All I/O (read_elem/write_elem) paths are identical
5. All opcode dispatch logic is identical
6. All constructor call sites pass the same values in the same order, with PhantomData being the only added (zero-size) field

The refactoring is purely structural — introducing type-level parameterization (ArithKind, SetupKind, IsEqKind) and a blanket impl (KnownFieldArith) to eliminate mechanical duplication, without changing any observable behavior.

[gdmlcjs]

LGTM!