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

Bump curve25519-dalek from 3.2.1 to 4.1.2 #513

Conversation

andreisilviudragnea
Copy link

@andreisilviudragnea andreisilviudragnea commented Mar 31, 2024

Problem

Dependabot tried to bump rand to 0.8.5 in #36, but failed to do so.

Summary of Changes

I updated the necessary dependencies together for rand update to succeed:

  • curve25519-dalek
  • sha3
  • rand

Fixes #36

@mergify mergify bot requested a review from a team March 31, 2024 17:30
@andreisilviudragnea andreisilviudragnea force-pushed the bump-curve25519-dalek branch 2 times, most recently from 7c2ed90 to d0f663d Compare March 31, 2024 18:03
@samkim-crypto samkim-crypto added the CI Pull Request is ready to enter CI label Apr 23, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Apr 23, 2024
@andreisilviudragnea andreisilviudragnea requested review from a team as code owners April 24, 2024 09:01
@samkim-crypto samkim-crypto self-requested a review May 1, 2024 00:25
@samkim-crypto samkim-crypto added the CI Pull Request is ready to enter CI label May 1, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label May 1, 2024
Copy link

@samkim-crypto samkim-crypto left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Everything looks good to me overall! Let's just fix the CI issues 🙏 . Can you rebase?

Comment on lines +41 to +43
.map(|slice| {
CompressedRistretto::from_slice(slice).expect("Input slice should have a length of 32")
})

Choose a reason for hiding this comment

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

Suggested change
.map(|slice| {
CompressedRistretto::from_slice(slice).expect("Input slice should have a length of 32")
})
.and_then(|slice| CompressedRistretto::from_slice(slice).ok())

Since we are returning None if the input is wrong length, it seems like we can just add .ok() here and remove the line above that does the length check.

@samkim-crypto
Copy link

@andreisilviudragnea, I was wondering if you still had plans to update this PR? It is completely fine if not since we have another community PR that does the same: #946. If you are still willing to work on this PR, please let me know by tomorrow 🙏 since we do need to bump the dalek crate.

@andreisilviudragnea
Copy link
Author

I can update it in a few minutes if it's ok

@andreisilviudragnea andreisilviudragnea force-pushed the bump-curve25519-dalek branch 2 times, most recently from 0498cfe to 2e91035 Compare May 7, 2024 00:57
@samkim-crypto samkim-crypto added the CI Pull Request is ready to enter CI label May 7, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label May 7, 2024
@yihau yihau added the CI Pull Request is ready to enter CI label May 7, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label May 7, 2024
@samkim-crypto samkim-crypto added the CI Pull Request is ready to enter CI label May 7, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label May 7, 2024
@andreisilviudragnea
Copy link
Author

andreisilviudragnea commented May 7, 2024

@samkim-crypto There are a few problems to fix on CI, but let's handle them one by one.

First of all, all errors in CI related to error[E0635]: unknown feature stdarch_x86_avx512 are caused by the fact that the anzaxyz/ci:rust_1.76.0_nightly-2024-01-05 image is too old and does not include the feature stdarch_x86_avx512 (I got lost in Rust repository Git history. The commit which includes the stdarch_x86_avx512 feature should be 18de239e: add stdarch_x86_avx512 feature flag for AVX-512-supporting architectures 23.02.2024, 05:26 AquaEBM).

So the first step is to create a CI image with a newer Rust Nightly version than 23.02.2024.

I see @brooksprumo last updated the CI image in solana-labs#34673, so they should know how to update it to a newer Rust Nightly than 23.02.2024.

@yihau
Copy link
Member

yihau commented May 29, 2024

@andreisilviudragnea sorry for the late. we have merged the bumping PR! you can rebase and continue with this one!

@samkim-crypto samkim-crypto added the CI Pull Request is ready to enter CI label May 30, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label May 30, 2024
@andreisilviudragnea andreisilviudragnea force-pushed the bump-curve25519-dalek branch 2 times, most recently from e02c90c to 45307f3 Compare June 1, 2024 08:51
@yihau yihau added the CI Pull Request is ready to enter CI label Jun 3, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Jun 3, 2024
@yihau
Copy link
Member

yihau commented Jun 3, 2024

looks like we run into some other issues 🫠 If you don't mind, I can take this one over. I will make sure to credit you and keep your name!

@andreisilviudragnea
Copy link
Author

@yihau Hello, ok, thank you, as I am quite crowded these days.

@ItsFunny
Copy link

ItsFunny commented Jun 5, 2024

when will this be merged ?

@andreisilviudragnea andreisilviudragnea force-pushed the bump-curve25519-dalek branch 2 times, most recently from 3c1d750 to d7b2313 Compare June 7, 2024 10:06
@andreisilviudragnea
Copy link
Author

@yihau I wanna try to fix this this weekend.

@yihau yihau added the CI Pull Request is ready to enter CI label Jun 7, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Jun 7, 2024
@andreisilviudragnea
Copy link
Author

andreisilviudragnea commented Jun 8, 2024

Hello. First of all, I don't know why the latest CI trigger did not run the Downstream project - SPL workflow. But looking at a previous run on this branch, the error:

error[E0277]: cannot multiply `&spl_token_2022::solana_zk_token_sdk::encryption::pedersen::PedersenCommitment` by `Scalar`
   --> token/client/src/proof_generation.rs:376:82
    |
376 |         fee_commitment * one_in_basis_points_scalar - transfer_amount_commitment * fee_rate_scalar;
    |                                                                                  ^ no implementation for `&spl_token_2022::solana_zk_token_sdk::encryption::pedersen::PedersenCommitment * Scalar`
    |
    = help: the trait `Mul<Scalar>` is not implemented for `&spl_token_2022::solana_zk_token_sdk::encryption::pedersen::PedersenCommitment`
    = help: the following other types implement trait `Mul<Rhs>`:
              <spl_token_2022::solana_zk_token_sdk::encryption::pedersen::PedersenCommitment as Mul<curve25519_dalek::scalar::Scalar>>
              <spl_token_2022::solana_zk_token_sdk::encryption::pedersen::PedersenCommitment as Mul<&'b curve25519_dalek::scalar::Scalar>>
              <&'a spl_token_2022::solana_zk_token_sdk::encryption::pedersen::PedersenCommitment as Mul<&'b curve25519_dalek::scalar::Scalar>>
              <&'a spl_token_2022::solana_zk_token_sdk::encryption::pedersen::PedersenCommitment as Mul<curve25519_dalek::scalar::Scalar>>

comes from the fact that both solana-zk-token-sdk (from this repo) and spl-token-client (from solana-program-library repo) crates depend on curve25519-dalek v3.2.1 and both need to be updated at the same time.

solana-program-library repo started depending on curve25519-dalek v3.2.1 since solana-labs/solana-program-library#5258.

I suggest re-exporting curve25519-dalek from solana-zk-token-sdk crate, in order to fix the CI and remove the dependency of solana-program-library on a specific curve25519-dalek version. I created a PR for this: #1657.

After merging #1657, I will create a PR in solana-program-library repository to use the new re-exported module and remove the curve25519-dalek version from Cargo.toml file and re-try running the CI on this PR again.

@samkim-crypto

@yihau
Copy link
Member

yihau commented Jun 10, 2024

we also need to fix this one: https://buildkite.com/anza/agave/builds/5647
Screenshot 2024-06-10 at 12 42 21

@yihau
Copy link
Member

yihau commented Jun 11, 2024

@andreisilviudragnea sorry, I think I will try to split/clean this commit bd41d6a into smaller pieces. it do too many things together.

btw, I will commit them to here. I want your name can be kept in the git history!


updated: I think I will keep you code but overwrite with mine.

@yihau
Copy link
Member

yihau commented Jun 11, 2024

moved to #1693

@andreisilviudragnea
Copy link
Author

Superseded by #1693.

@andreisilviudragnea andreisilviudragnea deleted the bump-curve25519-dalek branch August 14, 2024 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants