Skip to content

Conversation

@kayabaNerve
Copy link
Contributor

@kayabaNerve kayabaNerve commented Sep 3, 2025

The underlying field elements are unsafe for public consumption as they have undefined arithmetic after a certain amount of uses. This trait solves the problem as following:

  • Defining a ff::Field wrapper which reduces after every operation
  • Defining a bespoke LazyField trait which tracks consumed capacity using typenum
  • Definining a wrapper so any existing ff::Field may satisfy LazyField (actually allowing the LazyField trait to be considered for usage)
  • Defining a marker trait for any lazy field with a certain amount of capacity, so code generic to the field may still reduce how often they perform modular reductions
  • Implementing LazyField for FieldElement

Supersedes #787 if and only if this path is preferred. Resolves #389. This new type also could be used internally, ensuring there's no human-error in the existing uses of FieldElement, yet that would be a much more intensive PR best left to later.

The underlying field elements are unsafe for public consumption as they have
undefined arithmetic after a certain amount of uses. This trait solves the
problem as following:
- Defining a `ff::Field` wrapper which reduces after _every operation_
- Defining a bespoke `LazyField` trait which tracks consumed capacity using
  `typenum`
- Definining a wrapper so any existing `ff::Field` may satisfy `LazyField`
  (actually allowing the `LazyField` trait to be considered for usage)
- Defining a marker trait for any lazy field with a certain amount of capacity,
  so code generic to the field may still reduce how often they perform modular
  reductions
- Implementing `LazyField` for `FieldElement`
@kayabaNerve
Copy link
Contributor Author

kayabaNerve commented Sep 3, 2025

cc @tarcieri

My next step would be to work on tests if this is eligible for consideration. In reality, the lazy traits should be upstreamed somewhere, but it's probably best to resolve their API first here (with context of usage).

Unfortunately, due to the 32-bit backends, we only get three additions guaranteed (not eight). That may be without issue, as curve formula generally do an add and then a multiply (so only requiring a single addition), but the gap between 32-bit and 64-bit platforms is still notable.

Help is requested regarding the implementation of Reducible for FieldElement. This PR pushes Rust's type system's bounds evaluation, and for some reason it can't handle, in this context:

type X = Y;
fn z() -> Y;

where z is inherited from a trait defined as fn z() -> Self::X. As it fails to realize Self::X == Y, I had to use a unsafe pointer dereference to coerce the type (as it's immediately obvious the types are the same and therefore this isn't actually a cast).

Also, I'm unsure why the CI is failing. It's expecting a std feature and not finding it, yet I didn't touch the features except to add expose-field?

…`LazyField` trait

Adds a simple wrapper which stops dependents from accessing the underlying type.
All always return reduced outputs for all four backends present within
curve25519-dalek. This may be an unnecessary design choice of the backends,
offering potential future improvements, yet it's one we can take advantage of
here.
@tarcieri
Copy link
Contributor

tarcieri commented Sep 3, 2025

Maybe just call it hazmat? I know expose-field matches e.g. k256 but I've considered renaming that. Perhaps it should also be placed under a hazmat module.

@kayabaNerve the only build error I'm seeing is:

  error: associated function `from_bytes_wide` is never used
     --> curve25519-dalek/src/field.rs:104:19
      |
  101 | impl FieldElement {
      | ----------------- associated function in this implementation
  ...
  104 |     pub(crate) fn from_bytes_wide(bytes: &[u8; 64]) -> Self {
      |                   ^^^^^^^^^^^^^^^
      |
      = note: `-D dead-code` implied by `-D warnings`
      = help: to override `-D warnings` add `#[allow(dead_code)]`
  
  error: could not compile `curve25519-dalek` (lib) due to 1 previous error

As it fails to realize Self::X == Y, I had to use a unsafe pointer dereference

Can't you just bound appropriately, e.g. where Self: SomeTrait<X = Y>?

@kayabaNerve
Copy link
Contributor Author

I didn't realize expose-field was in k256 😅 I named this myself. I'm fine using hazmat.

The library compiles for me, with --all-features --all-targets. The CI is failing re: no-std for some reason. If you look at the implementation of Reducible, you'll see an unsafe pointer dereference which is obviously safe, as it's just a Copy that's opaque to Rust. If you try to remove it, as it shouldn't be needed, then Rust will fail to compile the library as the type system borks out (hence why I used unsafe to perform 'type coercion'. Because Rust can't tell it's the same type). I attempted adding bounds to resolve it, yet the type system seems to refuse to acknowledge an immediately-prior declared type is in fact the immediately-prior declared type in whatever archaic context I constructed.

I'll take a look at the from_bytes_wide error though.

@kayabaNerve
Copy link
Contributor Author

And somehow, CI passes now. Amazing. I have no idea what's different.

Renamed to hazmat, added test which verifies these constraints do actually produce a usable result.

kayabaNerve added a commit to kayabaNerve/curve25519-dalek that referenced this pull request Sep 3, 2025
kayabaNerve added a commit to kayabaNerve/curve25519-dalek that referenced this pull request Sep 3, 2025
@kayabaNerve
Copy link
Contributor Author

kayabaNerve commented Sep 3, 2025

Fun. The latest commit edited the README and caused a spurious test failure... I'll revisit the bounds on 32-bit platforms. We may only get a single addition :/

Comment on lines 89 to 129
#[test]
fn lazy_add_then_mul() {
use crate::hazmat::FieldElement;
use core::marker::PhantomData;
use ff::Field;
use rand_core::{OsRng, TryRngCore};

let mut rng = OsRng.unwrap_err();

let a = FieldElement::random(&mut rng);
let b = FieldElement::random(&mut rng);
let c = FieldElement::random(&mut rng);
let d = FieldElement::random(&mut rng);
let e = FieldElement::random(&mut rng);
let f = FieldElement::random(&mut rng);
let expected = (a + b + c) * (d + e + f);

assert_eq!(
LazyField::add(a, &b)
.add(&c)
.mul(&LazyField::add(d, &e).add(&f)),
expected
);
assert_eq!(add_triple_then_mul(a, b, c, d, e, f), expected);

let a = EagerField(a, PhantomData::<typenum::U1>);
let b = EagerField(b, PhantomData::<typenum::U1>);
let c = EagerField(c, PhantomData::<typenum::U1>);
let d = EagerField(d, PhantomData::<typenum::U1>);
let e = EagerField(e, PhantomData::<typenum::U1>);
let f = EagerField(f, PhantomData::<typenum::U1>);

assert_eq!(
LazyField::add(a, &b)
.add(&c)
.mul(&LazyField::add(d, &e).add(&f))
.0,
expected
);
assert_eq!(add_triple_then_mul(a, b, c, d, e, f).0, expected);
}
Copy link
Contributor

@tarcieri tarcieri Sep 3, 2025

Choose a reason for hiding this comment

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

If you used proptests for these, we could get the information needed to reproduce CI failures like: https://github.com/dalek-cryptography/curve25519-dalek/actions/runs/17444578081/job/49535734542?pr=816#step:8:205

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will look to set that up next.

@kayabaNerve
Copy link
Contributor Author

The docstring says the values are allowed to grow by up to 2^1.75, which should mean the current bound of 3 is sufficient to prevent an overflow. I'm unsure why a panic was observed accordingly. The bounds stated in the u32 backend may be incorrect. I've decreased the bound to 2, or just a single addition, for 32-bit backends and increased the runs per unit tests.

@kayabaNerve
Copy link
Contributor Author

Found it. I assumed from_bytes_wide would return a reduced result, as from_bytes does. It does not. This caused field elements to start at 2, where of course, any 2 plus any 2 would be 4 (> the bound of 3).

Restores bound of `3` for 32-bit platforms.
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.

Exposing the field element API

2 participants