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 Rust APIs #292

Merged
merged 78 commits into from
Dec 12, 2023
Merged

Conversation

DmytroTym
Copy link
Contributor

No description provided.

ImmanuelSegol and others added 30 commits October 3, 2023 15:22
* refactor

* refactor

* revert

* refactor: clang format

* Update icicle/appUtils/msm/msm.cu
…fc7ed6068e8eab9d81a8d6002949524597a19' into develop/vhnat/feat-233-update-api-fix-rust-build
…lop/vhnat/feat-233-update-api-fix-rust-build
Hotfix to slowdown in go when more than one curve is imported.
Add concurrency groups at workflow level for CI. Remove dev CI since we no longer use dev branch. Resolves #180
Resolves #191 and #113

---------

Co-authored-by: DmytroTym <dmytrotym1@gmail.com>
Co-authored-by: ImmanuelSegol <3ditds@gmail.com>
DmytroTym and others added 15 commits November 20, 2023 22:50
Update bug issue template to apply correct label
* Update rust bindings to support large_bucket_factor parameter
* Special treatment of ones in MSM removed

---------
Co-authored-by: DmytroTym <dmytrotym1@gmail.com>
Docs: Emphasize enabling tests in cmake build process
Out implementation of Barrett modular multiplication improved by utilising Karatsuba multiplication and more careful optimisations of lsb and msb multipliers in reduction stage
@DmytroTym
Copy link
Contributor Author

This is an early version of refactored NTT on CUDA and Rust sides. Some things I would like to note:

  • Twiddles. They are generated by InitDomain function from one element. The details of how they are created and stored are purposefully hidden (because their implementation will likely change in the near future with our new NTT version). Domain class which stores twiddles is also a singleton which can only be initialised once (I assume that twiddles of a curve don't change during program execution), though I guess the way it's implemented is not thread-safe, need to fix that. It's currently not possible to initialise default icicle twiddles, I thought that maybe it's more of an error-prone distraction than a convenience though let me know if you disagree.
  • As discussed offline, NTT is now out-of-place which helps caller manage data allocations and IMO makes for more intuitive calls when inputs/outputs are on different devices.

Everything is super raw for now and there are several obvious things to do (which are in progress rn) like creating more tests, merging in main and adding the rest of the curves to Rust. There are even several cases where NTT is still incorrect which I'll solve as tests are added for these cases. That said, there are several problems I want to solve but am not sure how:

  • Making Rust calls more safe. Using MSMs as an example, current call accepts slices which might be either device or host slices. And values like cfg.are_points_on_device and cfg.are_scalars_on_device and set independently which might lead to failures inside CUDA. I guess one solution would be to explicitly mark where a slice lives, something like
pub enum HostOrDevice<T> {
    Host(T),
    Device(T),
}

pub fn msm<'a>(
    scalars: &HostOrDevice<[ScalarField]>,
    ...
)
  • Using notation from the previous point, I guess it's more idiomatic for Rust to return the value wrapped into Result, so the return type should not be CudaResult<()> but rather CudaResult<HostOrDevice<[G1Projective]>>.
  • About errors: for now all the functions in Rust return Result<T, CudaError> where CudaError is simply a wrapper around cudaError_t. But we're not limited to just CUDA errors. What if for example the number of points and scalars in MSM aren't equal, of maybe the size of NTT is too large for a given curve? I don't think actual error handling is in the scope of this PR but at least modelling the types would be nice (I guess it needs to be done on CUDA side?).
  • I certainly don't like how currently functions like msm and ntt are just floating around in Rust unconnected to anything generic. We just won't be able to write anything that accepts curve as a generic parameter this way. But I'm not sure about the design, also I don't think this can be done in C++ and then somehow mapped onto Rust? I guess we need Rust-native code with traits and all that.
  • What do you think about tests in curve crates, how do we minimise the amount of copy-paste? I would go with some mix of generic functions and macros. Something like: first write a function (generic over field) that checks a number of, say, NTT-related conditions inside core crate, then unify all the NTT tests into one macro and then just invoke this macro from your curve crate. This way most of the logic would be outside of macros, plus generic tests would serve as tiny PoCs of viability of the generic design mentioned in the previous point.

@vhnatyk @jeremyfelder @ChickenLover wdyt?

@ChickenLover
Copy link
Contributor

@DmytroTym
Twiddles: for halo2 integrations it is a default to have 2 different omegas in a domain. So when we did the integrations, we had to create 2 sets of twiddles separately

@DmytroTym
Copy link
Contributor Author

DmytroTym commented Dec 9, 2023

@ChickenLover oh...
These are not domains of different sizes, not cosets, but two completely distinct subgroups?

@ChickenLover
Copy link
Contributor

@ChickenLover oh... These are not domains of different size, not cosets, but two completely distinct subgroups?

never mind. There are two subgroups, but one of them is itself a subgroup of a bigger one, so I guess it's ok

@ChickenLover
Copy link
Contributor

        let mut extended_omega = F::ROOT_OF_UNITY;

        // Get extended_omega, the 2^{extended_k}'th root of unity
        // The loop computes extended_omega = omega^{2 ^ (S - extended_k)}
        // Notice that extended_omega ^ {2 ^ extended_k} = omega ^ {2^S} = 1.
        for _ in extended_k..F::S {
            extended_omega = extended_omega.square();
        }
        let extended_omega = extended_omega;
        let mut extended_omega_inv = extended_omega; // Inversion computed later

        // Get omega, the 2^{k}'th root of unity (i.e. n'th root of unity)
        // The loop computes omega = extended_omega ^ {2 ^ (extended_k - k)}
        //           = (omega^{2 ^ (S - extended_k)})  ^ {2 ^ (extended_k - k)}
        //           = omega ^ {2 ^ (S - k)}.
        // Notice that omega ^ {2^k} = omega ^ {2^S} = 1.
        let mut omega = extended_omega;
        for _ in k..extended_k {
            omega = omega.square();
        }

This is how they are computed in the halo2 code

@DmytroTym
Copy link
Contributor Author

DmytroTym commented Dec 9, 2023

@ChickenLover yup, that's supported, you just need to pass omega of larger order, so extended_omega into InitDomain

@ChickenLover
Copy link
Contributor

@DmytroTym
Regarding HostOrDevice enum - I think this is a must. One should have explicit control over data movement and it should be visible in code. We can also detect some errors at the compilation phase if we do that.

Regarding return types - I think that hiding as much CUDA specific details is always better for us, despite situations like unexpected CUDA errors or CUDA related configuration. Following this logic, maybe it would be a good idea to have our own structs for the Result and Error type.

For the error handling - we definitely want to design error classes on the C++ side. We can do something as simple as error_code + string message and, if needed, wrap them with the wrapper language native structs.

Regarding floating msm and ntt functions: maybe we can define MSM and NTT as traits and implement them for different curves with macros?

Regarding tests - I really like what you described. If you'll make a draft PR that implements that idea - that would be nice

@DmytroTym DmytroTym marked this pull request as ready for review December 12, 2023 17:23
@DmytroTym DmytroTym merged commit 629d8cb into feat/233-update-api Dec 12, 2023
@DmytroTym DmytroTym deleted the develop/dima/feat-233-update-rust-apis branch December 12, 2023 17:31
@DmytroTym DmytroTym mentioned this pull request Dec 12, 2023
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.

9 participants