Skip to content

refactor(field): consolidate Goldilocks hybrid into canonical, fix ty… #1172

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.

Sign up for GitHub

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

Open
diegokingston wants to merge 2 commits into
base: main
Choose a base branch
from
Open

refactor(field): consolidate Goldilocks hybrid into canonical, fix ty… #1172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c36311c
refactor(field): consolidate Goldilocks hybrid into canonical, fix ty…
diegokingston Feb 12, 2026
29b7ca6
add tests for byte conversion
nicole-graus Feb 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
195 changes: 4 additions & 191 deletions crates/math/benches/criterion_goldilocks_comparison.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,19 @@
//! Comparison benchmarks for Goldilocks field implementations
//!
//! Compares:
//! - Classic Goldilocks64Field
//! - Hybrid Goldilocks64HybridField
//! - Goldilocks64Field (base + extensions)
//! - Montgomery U64GoldilocksPrimeField
//!
//! And Degree2 extensions for Classic and Hybrid

use criterion::{black_box, criterion_group, criterion_main, BenchmarkId, Criterion, Throughput};
use rand::rngs::StdRng;
use rand::{Rng, SeedableRng};

use lambdaworks_math::field::element::FieldElement;

// Classic
use lambdaworks_math::field::fields::u64_goldilocks_field::{
Degree2GoldilocksExtensionField, Degree3GoldilocksExtensionField, Goldilocks64Field,
};

// Hybrid
use lambdaworks_math::field::fields::u64_goldilocks_hybrid_field::{
Degree3GoldilocksHybridExtensionField, Goldilocks64HybridExtensionField,
Goldilocks64HybridField,
};

// Montgomery
use lambdaworks_math::field::fields::fft_friendly::u64_goldilocks::U64GoldilocksPrimeField;

Expand Down Expand Up @@ -94,66 +84,6 @@ fn bench_classic_base(c: &mut Criterion) {
group.finish();
}

fn bench_hybrid_base(c: &mut Criterion) {
let mut group = c.benchmark_group("Goldilocks Hybrid Base");
type F = Goldilocks64HybridField;
type FE = FieldElement<F>;

let mut rng = StdRng::seed_from_u64(SEED);

for size in SIZES {
let values: Vec<FE> = (0..size).map(|_| FE::from(rng.gen::<u64>())).collect();
group.throughput(Throughput::Elements(size as u64));

group.bench_with_input(BenchmarkId::new("mul", size), &values, |b, vals| {
b.iter(|| {
let mut acc = vals[0];
for v in &vals[1..] {
acc *= v;
}
black_box(acc)
})
});

group.bench_with_input(BenchmarkId::new("add", size), &values, |b, vals| {
b.iter(|| {
let mut acc = vals[0];
for v in &vals[1..] {
acc = acc + v;
}
black_box(acc)
})
});

group.bench_with_input(BenchmarkId::new("sub", size), &values, |b, vals| {
b.iter(|| {
let mut acc = vals[0];
for v in &vals[1..] {
acc = acc - v;
}
black_box(acc)
})
});

group.bench_with_input(BenchmarkId::new("square", size), &values, |b, vals| {
b.iter(|| {
for v in vals {
black_box(v.square());
}
})
});

group.bench_with_input(BenchmarkId::new("inv", size), &values, |b, vals| {
b.iter(|| {
for v in vals {
black_box(v.inv().unwrap());
}
})
});
}
group.finish();
}

fn bench_montgomery_base(c: &mut Criterion) {
let mut group = c.benchmark_group("Goldilocks Montgomery Base");
type F = U64GoldilocksPrimeField;
Expand Down Expand Up @@ -276,64 +206,6 @@ fn bench_classic_ext2(c: &mut Criterion) {
group.finish();
}

fn bench_hybrid_ext2(c: &mut Criterion) {
let mut group = c.benchmark_group("Goldilocks Hybrid Ext2");
type F = Goldilocks64HybridExtensionField;
type FE = FieldElement<F>;
type BaseFE = FieldElement<Goldilocks64HybridField>;

let mut rng = StdRng::seed_from_u64(SEED);

for size in SIZES {
let values: Vec<FE> = (0..size)
.map(|_| {
FE::new([
BaseFE::from(rng.gen::<u64>()),
BaseFE::from(rng.gen::<u64>()),
])
})
.collect();
group.throughput(Throughput::Elements(size as u64));

group.bench_with_input(BenchmarkId::new("mul", size), &values, |b, vals| {
b.iter(|| {
let mut acc = vals[0];
for v in &vals[1..] {
acc *= v;
}
black_box(acc)
})
});

group.bench_with_input(BenchmarkId::new("add", size), &values, |b, vals| {
b.iter(|| {
let mut acc = vals[0];
for v in &vals[1..] {
acc = acc + v;
}
black_box(acc)
})
});

group.bench_with_input(BenchmarkId::new("square", size), &values, |b, vals| {
b.iter(|| {
for v in vals {
black_box(v.square());
}
})
});

group.bench_with_input(BenchmarkId::new("inv", size), &values, |b, vals| {
b.iter(|| {
for v in vals {
black_box(v.inv().unwrap());
}
})
});
}
group.finish();
}

// ============================================
// EXTENSION FIELD BENCHMARKS (Degree 3)
// ============================================
Expand Down Expand Up @@ -397,85 +269,26 @@ fn bench_classic_ext3(c: &mut Criterion) {
group.finish();
}

fn bench_hybrid_ext3(c: &mut Criterion) {
let mut group = c.benchmark_group("Goldilocks Hybrid Ext3");
type F = Degree3GoldilocksHybridExtensionField;
type FE = FieldElement<F>;
type BaseFE = FieldElement<Goldilocks64HybridField>;

let mut rng = StdRng::seed_from_u64(SEED);

for size in SIZES {
let values: Vec<FE> = (0..size)
.map(|_| {
FE::new([
BaseFE::from(rng.gen::<u64>()),
BaseFE::from(rng.gen::<u64>()),
BaseFE::from(rng.gen::<u64>()),
])
})
.collect();
group.throughput(Throughput::Elements(size as u64));

group.bench_with_input(BenchmarkId::new("mul", size), &values, |b, vals| {
b.iter(|| {
let mut acc = vals[0];
for v in &vals[1..] {
acc *= v;
}
black_box(acc)
})
});

group.bench_with_input(BenchmarkId::new("add", size), &values, |b, vals| {
b.iter(|| {
let mut acc = vals[0];
for v in &vals[1..] {
acc = acc + v;
}
black_box(acc)
})
});

group.bench_with_input(BenchmarkId::new("square", size), &values, |b, vals| {
b.iter(|| {
for v in vals {
black_box(v.square());
}
})
});

group.bench_with_input(BenchmarkId::new("inv", size), &values, |b, vals| {
b.iter(|| {
for v in vals {
black_box(v.inv().unwrap());
}
})
});
}
group.finish();
}

// ============================================
// CRITERION SETUP
// ============================================

criterion_group!(
name = base_fields;
config = Criterion::default().sample_size(10);
targets = bench_classic_base, bench_hybrid_base, bench_montgomery_base
targets = bench_classic_base, bench_montgomery_base
);

criterion_group!(
name = ext2_fields;
config = Criterion::default().sample_size(10);
targets = bench_classic_ext2, bench_hybrid_ext2
targets = bench_classic_ext2
);

criterion_group!(
name = ext3_fields;
config = Criterion::default().sample_size(10);
targets = bench_classic_ext3, bench_hybrid_ext3
targets = bench_classic_ext3
);

criterion_main!(base_fields, ext3_fields);
Copy link

@github-actions github-actions bot Feb 12, 2026

Choose a reason for hiding this comment

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

Correctness

  • Inversions: There are uses of v.inv().unwrap() without handling potential failures. If the inversion can return None (e.g., trying to invert zero), it must be handled or ensured that inputs are non-zero.

Security

  • Use of unwrap: The use of unwrap() can lead to panics, which might expose information on inputs if invocations fail under specific conditions (e.g., input secrecy concerns).
  • Timing side-channels: No mention of whether mathematical operations are performed in constant-time. Operations involving secret inputs should be constant-time to avoid leaking information.

Performance

  • Redundant Code Removal: The removal of benchmarks for Goldilocks64HybridField and its extensions is noted, but ensure these are no longer relevant. If they aren't actively used, removing them enhances performance by simplifying the code base.

Bugs & Errors

  • Potential Panics: As noted, unwrap() can panic. If these are in critical paths, they must be replaced with graceful error handling.

Code Simplicity

  • Duplicated Code: The removed sections had duplicated logic for different types (e.g., benchmarks for Hybrid fields). If the types are capable of being handled generically, consider consolidating instead of removing.

Ensure that all necessary test coverage is maintained with the removal of these hybrid benchmarks if these types are still in use elsewhere.

Loading
Loading