sxt-proof-of-sql icon indicating copy to clipboard operation
sxt-proof-of-sql copied to clipboard

Make opaque `Scalar` conversions explicit rather than using `From`/`Into` trait.

Open JayWhite2357 opened this issue 1 year ago • 10 comments

Background and Motivation

Currently, the Scalar trait requires various conversions using the From and Into traits. Some of these are confusing, while others are clear. In general, the rule should be that for types with a natural embedding into the associated Scalar field, Scalar should implement From. In all other cases, the conversion should either be auto-implemented, or a trait method, where appropriate.

NOTE: The following issue also necessitates changes to Scalar, so be aware of potential merge conflicts: https://github.com/spaceandtimelabs/sxt-proof-of-sql/issues/234

Changes Required

Each of these should be a separate PR.

  • [ ] Remove Into<[u64; 4]>, From<[u64; 4]>, and RefInto<[u64; 4]> bounds.
    • Replace these with the trait methods fn from_limbs(val: [u64; 4]) -> Self and fn to_limbs(&self) -> [u64; 4]
    • These three conversions are the base conversions that are used for many other conversions. A Scalar is a field element (number mod some prime) with slightly less than 256 bits. Usually, Scalars are internally stored in Montgomery form, so these conversions are non-free. However, they are needed in some situations.
  • [ ] Remove From<&'a String>, From<String>, and From<&'a str> bounds. These three implementations ultimately rely on the conversions to and from limbs. As a result, these should have blanket/default implementations.
    • Replace these with fn from_str_via_hash(val: &str) -> Self and add a default implementation.
    • While it would be nice to impl<S: Scalar> From<String> for S, this is not allowed by the Rust type system. Solutions other than the default implementation of a trait method are welcome.
    • This conversion requires that the string is hashed in order to convert it to 256 bits which are ultimately converted into the Scalar itself. This is different from a parsing method.
  • [ ] Remove the VarInt bound and replace it with a blanket impl<S: Scalar> VarInt for S implementation. The existing (and requested) implementation relies on the limb conversions only.

JayWhite2357 avatar Oct 07 '24 09:10 JayWhite2357

/bounty $100

JayWhite2357 avatar Oct 07 '24 09:10 JayWhite2357

💎 $100 bounty • Space and Time

Steps to solve:

  1. Start working: (Optional) Comment /attempt #228 with your implementation plan. Note: we will only assign an issue if you include an implementation plan with a time estimate. Additionally, to be assigned an issue, you must have previously contributed to the project. You can still work on an issue and submit a PR without being assigned.
  2. Submit work: Create a pull request including /claim #228 in the PR body to claim the bounty
  3. Receive payment: 100% of the bounty is received 2-5 days post-reward. Make sure you are eligible for payouts

Thank you for contributing to spaceandtimelabs/sxt-proof-of-sql!

Add a bountyShare on socials

Attempt Started (GMT+0) Solution
🟢 @b4s36t4 Oct 9, 2024, 5:27:42 AM WIP
🟢 @varshith257 Oct 13, 2024, 7:36:07 AM WIP
🟢 @maujim Nov 4, 2024, 5:20:18 AM #335

algora-pbc[bot] avatar Oct 07 '24 09:10 algora-pbc[bot]

/attempt #228

Algora profile Completed bounties Tech Active attempts Options
@varshith257 15 bounties from 7 projects
Go, TypeScript,
Scala & more
﹟232
Cancel attempt

varshith257 avatar Oct 13 '24 07:10 varshith257

/attempt #228

Options

maujim avatar Nov 04 '24 05:11 maujim

@JayWhite2357 can you please take a look at the liked PR?

maujim avatar Nov 14 '24 04:11 maujim

@JayWhite2357 can you please take a look at the liked PR?

@maujim Sorry for the delay on this, the PR is still open and needs conflict resolution, I can review ASAP if you still want to try to get it in. If you still want to try this I think its possible to resolve the formatter error. This issue might need to be resolved in order to accompany #229

Let us know when you can

Dustin-Ray avatar Jan 20 '25 23:01 Dustin-Ray

@Dustin-Ray I'm on macos / arm and I'm getting two compilation errors on main related to my machine. Given this, I don't think I'll be diving back into this one.

error: failed to run custom build command for `blitzar-sys v1.99.0`
note: To improve backtraces for build dependencies, set the CARGO_PROFILE_DEV_BUILD_OVERRIDE_DEBUG=true environment variable to enable debug information generation.

Caused by:
  process didn't exit successfully: `/Users/mukund/dev/sxt-proof-of-sql/target/debug/build/blitzar-sys-a863c08174abcbdf/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at /Users/mukund/.config/cargo/registry/src/index.crates.io-6f17d22bba15001f/blitzar-sys-1.99.0/build.rs:14:9:
  Unsupported OS. Only Linux is supported.
  stack backtrace:
     0: rust_begin_unwind
               at /rustc/9fc6b43126469e3858e2fe86cafb4f0fd5068869/library/std/src/panicking.rs:665:5
     1: core::panicking::panic_fmt
               at /rustc/9fc6b43126469e3858e2fe86cafb4f0fd5068869/library/core/src/panicking.rs:76:14
     2: build_script_build::main
     3: core::ops::function::FnOnce::call_once
  note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
warning: build failed, waiting for other jobs to finish...
error: failed to run custom build command for `halo2curves v0.8.0`
note: To improve backtraces for build dependencies, set the CARGO_PROFILE_DEV_BUILD_OVERRIDE_DEBUG=true environment variable to enable debug information generation.

Caused by:
  process didn't exit successfully: `/Users/mukund/dev/sxt-proof-of-sql/target/debug/build/halo2curves-0db72ce93f44cb27/build-script-build` (exit status: 1)
  --- stderr
  Currently feature `asm` can only be enabled on x86_64 arch.

maujim avatar Jan 23 '25 03:01 maujim

@maujim I think You can fix this error by turning off blitzar using this env variable:

export BLITZAR_BACKEND=cpu

If you're ok passing on this issue I can go ahead and manually take over, thanks for the effort so far we really appreciate the help.

Dustin-Ray avatar Jan 23 '25 16:01 Dustin-Ray

I tried a fresh git clone && cargo build and I get the halo2curves issue before the blitzar one.

Took a quick look and it seems that the arm feature is enabled by default via the nova-snark dependency. https://github.com/microsoft/Nova/blob/main/Cargo.toml#L70

So I think its best for someone else to take over for now

maujim avatar Jan 23 '25 17:01 maujim

I'd like to pick up this issue. I'm currently reading about Montgomery scalars. What exactly is the correct conversion to and from Scalar <--> [u64; 4] ?

matt-user avatar Apr 01 '25 15:04 matt-user