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

refactor: make limbs generic to accept limb size #365

Closed

Conversation

0xkanekiken
Copy link
Contributor

@0xkanekiken 0xkanekiken commented Mar 11, 2024

This PR tries to make the Limbs to be generic in the number of limbs size so that it could support points from different curves. At the moment the LIMB_SIZE is hardcoded to 32.

#[derive(Debug, Clone, AlignedBorrow)]
#[repr(C)]
pub struct FieldDenCols<T> {
/// The result of `a den b`, where a, b are field elements
pub result: Limbs<T>,
pub(crate) carry: Limbs<T>,
pub(crate) witness_low: [T; NUM_WITNESS_LIMBS],
pub(crate) witness_high: [T; NUM_WITNESS_LIMBS],
}

Major changes:

  • The number of witnesses is determined by 2 * NUM_LIMBS - 2, which in the above example, can be set to 2 * N - 2 where N is the const generic passed to the struct. Unfortunately, passing a generic to a constant operation is not supported in stable Rust, and requires the use of the nightly version. To circumvent this, introduced an additional const parameter for the number of witnesses. Need to figure out the best way to handle this.
  • Introduced a proc macroAlignedBorrowWithGenerics, to address the above issue. This implementation is not the cleanest and I am looking ways to modify AlignedBorrow to support use cases where the first generic is a type, and the rest are constants. At the very least, it should be capable of handling cases where the first generic is the type, and all subsequent generics are constants using one proc macro.
  • currently- I have disabled weierstrass related mods to check if it works with the edwards curve. I plan to enable them subsequently.

diff curves

Signed-off-by: 0xkanekiken <100861945+0xKanekiKen@users.noreply.github.com>
@0xkanekiken 0xkanekiken marked this pull request as draft March 11, 2024 16:11
let name = &ast.ident;

let methods = quote! {
impl<T, const N: usize, const M: usize> Borrow<#name<T, N, M>> for [T] {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there's some way to get all the generics from the ast, similar to the name parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Generic can be obtained from the derive macro token stream:
https://docs.rs/syn/latest/syn/struct.DeriveInput.html

@0xkanekiken I think your implementation could be changed a bit to support reading generic constants. There are ways to read the generics and add a new one (like T). Would be happy to chat about it more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see the commit diff. I've modified the AlignedBorrow macro to support constant generics.

Signed-off-by: 0xkanekiken <100861945+0xKanekiKen@users.noreply.github.com>
Signed-off-by: 0xkanekiken <100861945+0xKanekiKen@users.noreply.github.com>
}
})
.expect("Expected at least one type generic");

Copy link
Contributor

@tamirhemo tamirhemo Mar 11, 2024

Choose a reason for hiding this comment

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

Could you also add a panic! that would occur if a non-const generic is used other than T? that would be really good for our user experience which might not be clear right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see #368. Infact, I have modified the code to trigger a panic if the first generic is not a type, and for any subsequent generics, if they are not consts.

@brozorec brozorec mentioned this pull request Mar 13, 2024
@puma314
Copy link
Contributor

puma314 commented Mar 22, 2024

Closing in favor of this approach: #417

@puma314 puma314 closed this Mar 22, 2024
@0xkanekiken 0xkanekiken deleted the kaneki-limbs-refactor branch March 23, 2024 13:22
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.

3 participants