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.

Sign up for GitHub

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.3 #1693

Merged
merged 5 commits into from
Jul 9, 2024
Merged

bump curve25519-dalek from 3.2.1 to 4.1.3 #1693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1a946c5
Bump curve25519-dalek from 3.2.1 to 4.1.3
dependabot[bot] Mar 26, 2024
6185e9d
hack spl test
yihau Jun 19, 2024
1e0b6d0
ignore some spl downstream tests
yihau Jun 21, 2024
6f9c204
use into_option
yihau Jun 24, 2024
1982c90
hack spl test 2
yihau Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .github/scripts/downstream-project-spl-common.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,11 @@ fi

# anza migration stopgap. can be removed when agave is fully recommended for public usage.
sed -i 's/solana-geyser-plugin-interface/agave-geyser-plugin-interface/g' ./Cargo.toml

# should be removed when spl bump their curve25519-dalek
sed -i "s/^curve25519-dalek =.*/curve25519-dalek = \"4.1.3\"/" token/client/Cargo.toml
sed -i "s/^curve25519-dalek =.*/curve25519-dalek = \"4.1.3\"/" token/confidential-transfer/proof-generation/Cargo.toml

# ignore these tests temporarily. see: https://github.com/anza-xyz/agave/pull/1693#issuecomment-2182615788
sed -i 's/\([ \t]*\)async_trial!(confidential_transfer,/\1\/\/ async_trial!(confidential_transfer,/' token/cli/tests/command.rs
sed -i '/async fn confidential_transfer_transfer_with_fee_and_split_proof_context_in_parallel(/i #[ignore]' token/program-2022-test/tests/confidential_transfer.rs
Comment on lines +30 to +37
Copy link

Choose a reason for hiding this comment

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

I'm a bit worried about this.
My understanding is that the whole purpose of the downstream tests it to make sure we are not breaking things downstream.
If we need to patch the downstream source for things to work, we failed.

Or am I missing something here?

Copy link

Choose a reason for hiding this comment

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

Yeah this is obviously not ideal, but it seems that the dalek issue seems to be quite specific to the SPL confidential transfer logic, so I think it makes sense to defer the issue to downstream on the SPL side. Many people have been waiting for the dalek version bump and I think it is not ideal to delay the upgrade any more for a specific application.

With solana-zk-token-sdk being replace with solana-zk-sdk, the token-2022 tests in SPL will also need restructuring and the issue could be addressed then.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this one is unavoidable for this upgrade. will need to upgrade upstream first then downstream

# should be removed when spl bump their curve25519-dalek
sed -i "s/^curve25519-dalek =.*/curve25519-dalek = \"4.1.3\"/" token/client/Cargo.toml
sed -i "s/^curve25519-dalek =.*/curve25519-dalek = \"4.1.3\"/" token/confidential-transfer/proof-generation/Cargo.toml

the controversial part should be

# ignore these tests temporarily. see: https://github.com/anza-xyz/agave/pull/1693#issuecomment-2182615788
sed -i 's/\([ \t]*\)async_trial!(confidential_transfer,/\1\/\/ async_trial!(confidential_transfer,/' token/cli/tests/command.rs
sed -i '/async fn confidential_transfer_transfer_with_fee_and_split_proof_context_in_parallel(/i #[ignore]' token/program-2022-test/tests/confidential_transfer.rs

if I don't miss anything, this upgrade should only affect zk features. (I did some benches for confirmation earlier days)
#1693 (comment)
#1693 (comment)

we also haven't published this feature and program to any of network, so I guess maybe we should be fine to do this one 🤔

cc @joncinque

88 changes: 52 additions & 36 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

35 changes: 1 addition & 34 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ criterion-stats = "0.3.0"
crossbeam-channel = "0.5.13"
csv = "1.3.0"
ctrlc = "3.4.4"
curve25519-dalek = "3.2.1"
curve25519-dalek = { version = "4.1.3", features = ["digest", "rand_core"] }
dashmap = "5.5.3"
derivation-path = { version = "0.2.0", default-features = false }
derivative = "2.2.0"
Expand Down Expand Up @@ -507,39 +507,6 @@ solana-program = { path = "sdk/program" }
solana-zk-sdk = { path = "zk-sdk" }
solana-zk-token-sdk = { path = "zk-token-sdk" }

# Our dependency tree has `curve25519-dalek` v3.2.1. They have removed the
# constraint in the next major release. The commit that removes the `zeroize`
# constraint was added to multiple release branches, but not to the 3.2 branch.
#
# `curve25519-dalek` maintainers are saying they do not want to invest any more
# time in the 3.2 release:
#
# https://github.com/dalek-cryptography/curve25519-dalek/issues/452#issuecomment-1749809428
#
# So we have to fork and create our own release, based on v3.2.1, with the
# commit that removed `zeroize` constraint on the `main` branch cherry-picked on
# top.
#
# `curve25519-dalek` v3.2.1 release:
#
# https://github.com/dalek-cryptography/curve25519-dalek/releases/tag/3.2.1
#
# Corresponds to commit
#
# https://github.com/dalek-cryptography/curve25519-dalek/commit/29e5c29b0e5c6821e4586af58b0d0891dd2ec639
#
# Comparison with `b500cdc2a920cd5bff9e2dd974d7b97349d61464`:
#
# https://github.com/dalek-cryptography/curve25519-dalek/compare/3.2.1...solana-labs:curve25519-dalek:b500cdc2a920cd5bff9e2dd974d7b97349d61464
#
# Or, using the branch name instead of the hash:
#
# https://github.com/dalek-cryptography/curve25519-dalek/compare/3.2.1...solana-labs:curve25519-dalek:3.2.1-unpin-zeroize
#
[patch.crates-io.curve25519-dalek]
git = "https://github.com/anza-xyz/curve25519-dalek.git"
rev = "b500cdc2a920cd5bff9e2dd974d7b97349d61464"

# Solana RPC nodes experience stalls when running with `tokio` containing this
# commit:
# https://github.com/tokio-rs/tokio/commit/4eed411519783ef6f58cbf74f886f91142b5cfa6
Expand Down
12 changes: 8 additions & 4 deletions curves/curve25519/src/edwards.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,10 @@ mod target_arch {
type Error = Curve25519Error;

fn try_from(pod: &PodEdwardsPoint) -> Result<Self, Self::Error> {
CompressedEdwardsY::from_slice(&pod.0)
let Ok(compressed_edwards_y) = CompressedEdwardsY::from_slice(&pod.0) else {
return Err(Curve25519Error::PodConversion);
};
compressed_edwards_y
.decompress()
.ok_or(Curve25519Error::PodConversion)
}
Expand All @@ -73,9 +76,10 @@ mod target_arch {
type Point = Self;

fn validate_point(&self) -> bool {
CompressedEdwardsY::from_slice(&self.0)
.decompress()
.is_some()
let Ok(compressed_edwards_y) = CompressedEdwardsY::from_slice(&self.0) else {
return false;
};
compressed_edwards_y.decompress().is_some()
}
}

Expand Down
12 changes: 8 additions & 4 deletions curves/curve25519/src/ristretto.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,10 @@ mod target_arch {
type Error = Curve25519Error;

fn try_from(pod: &PodRistrettoPoint) -> Result<Self, Self::Error> {
CompressedRistretto::from_slice(&pod.0)
let Ok(compressed_ristretto) = CompressedRistretto::from_slice(&pod.0) else {
return Err(Curve25519Error::PodConversion);
};
compressed_ristretto
.decompress()
.ok_or(Curve25519Error::PodConversion)
}
Expand All @@ -73,9 +76,10 @@ mod target_arch {
type Point = Self;

fn validate_point(&self) -> bool {
CompressedRistretto::from_slice(&self.0)
.decompress()
.is_some()
let Ok(compressed_ristretto) = CompressedRistretto::from_slice(&self.0) else {
return false;
};
compressed_ristretto.decompress().is_some()
}
}

Expand Down
8 changes: 6 additions & 2 deletions curves/curve25519/src/scalar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ mod target_arch {
type Error = Curve25519Error;

fn try_from(pod: &PodScalar) -> Result<Self, Self::Error> {
Scalar::from_canonical_bytes(pod.0).ok_or(Curve25519Error::PodConversion)
Scalar::from_canonical_bytes(pod.0)
.into_option()
.ok_or(Curve25519Error::PodConversion)
}
}

Expand All @@ -32,7 +34,9 @@ mod target_arch {
type Error = Curve25519Error;

fn try_from(pod: PodScalar) -> Result<Self, Self::Error> {
Scalar::from_canonical_bytes(pod.0).ok_or(Curve25519Error::PodConversion)
Scalar::from_canonical_bytes(pod.0)
.into_option()
.ok_or(Curve25519Error::PodConversion)
}
}
}
4 changes: 2 additions & 2 deletions perf/src/sigverify.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1280,7 +1280,7 @@ mod tests {
for _ in 0..1_000_000 {
thread_rng().fill(&mut input);
let ans = get_checked_scalar(&input);
let ref_ans = Scalar::from_canonical_bytes(input);
let ref_ans = Scalar::from_canonical_bytes(input).into_option();
if let Some(ref_ans) = ref_ans {
passed += 1;
assert_eq!(ans.unwrap(), ref_ans.to_bytes());
Expand Down Expand Up @@ -1315,7 +1315,7 @@ mod tests {
for _ in 0..1_000_000 {
thread_rng().fill(&mut input);
let ans = check_packed_ge_small_order(&input);
let ref_ge = CompressedEdwardsY::from_slice(&input);
let ref_ge = CompressedEdwardsY::from_slice(&input).unwrap();
if let Some(ref_element) = ref_ge.decompress() {
if ref_element.is_small_order() {
assert!(!ans);
Expand Down
Loading