Skip to content

Upgrade to v0.4.0#94

Open
TakodaS wants to merge 40 commits intoarkworks-rs:masterfrom
TakodaS:master
Open

Upgrade to v0.4.0#94
TakodaS wants to merge 40 commits intoarkworks-rs:masterfrom
TakodaS:master

Conversation

@TakodaS
Copy link

@TakodaS TakodaS commented Sep 7, 2023

Description

Updated arkworks-rs dependencies to v0.4.0.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests (Not needed as no new functionality was added).
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@TakodaS TakodaS marked this pull request as draft September 7, 2023 14:41
@TakodaS
Copy link
Author

TakodaS commented Sep 7, 2023

@Pratyush Should I bother implementing CryptographicSponge for the Fiat-Shamir hash function found in rng or should I rip this all out in favour of Poseidon?

Edit: I see something similar is already in PR. Maybe I can commandeer that PR. I will go talk to @vlopes11.

@TakodaS TakodaS marked this pull request as ready for review September 7, 2023 16:15
@TakodaS TakodaS changed the title Draft: Upgrade to v0.4.0 Upgrade to v0.4.0 Sep 7, 2023
@TakodaS
Copy link
Author

TakodaS commented Sep 12, 2023

@Pratyush Should I bother implementing CryptographicSponge for the Fiat-Shamir hash function found in rng or should I rip this all out in favour of Poseidon?

Edit: I see something similar is already in PR. Maybe I can commandeer that PR. I will go talk to @vlopes11.

Implementing FS hashing using the PoseidonSponge, temporarily with the same default initialization as used in the tests for crypto-primitives.

rayon = { version = "1", optional = true }
digest = { version = "0.9" }
derivative = { version = "2", features = ["use_core"] }
itertools = "0.11.0"
Copy link
Author

Choose a reason for hiding this comment

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

I will probably have to remove this to pass the no-std test.

#[derive(Clone)]
pub struct SimplePoseidonRng<F: PrimeField>(PoseidonSponge<F>);

impl<F: PrimeField> RngCore for SimplePoseidonRng<F> {
Copy link
Author

Choose a reason for hiding this comment

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

RngCore is required for backwards compatibility with generation of random field elements in ark-ff and ark-poly. Changing these is a much larger piece of work than exposing the sponge construction through RngCore and beyond the scope of this PR imo.

@TakodaS TakodaS requested a review from Pratyush September 28, 2023 11:05
Comment on lines +67 to +89
/// Instantiate Poseidon sponge with default parameters
impl<F: PrimeField> Default for SimplePoseidonRng<F> {
fn default() -> Self {
// let default =
// Self(PoseidonSponge::new(&poseidon_parameters_for_test()))
let (alpha, rate, full_rounds, partial_rounds) = (17, 2, 8, 29);
let (ark, mds) = find_poseidon_ark_and_mds(
F::MODULUS_BIT_SIZE as u64,
rate,
full_rounds,
partial_rounds,
0,
);
let config = PoseidonConfig {
full_rounds: full_rounds as usize,
partial_rounds: partial_rounds as usize,
alpha: alpha as u64,
ark,
mds,
rate,
capacity: 1,
};
SimplePoseidonRng(PoseidonSponge::new(&config))
Copy link
Author

Choose a reason for hiding this comment

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

I hope these are good defaults.

src/lib.rs Outdated
impl<F: PrimeField, PC: PolynomialCommitment<F, DensePolynomial<F>>, FS: FiatShamirRng>
Marlin<F, PC, FS>
impl<
F: PrimeField + Absorb,
Copy link
Author

Choose a reason for hiding this comment

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

The addition of the Absorb trait bound is very problematic. Although it will improve efficiency of absorption, it makes the implementation far less generic. I am not certain how to improve this, considering that specialization of traits is an unstable feature.

Copy link
Author

@TakodaS TakodaS Oct 9, 2023

Choose a reason for hiding this comment

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

I resolved this by implementing the fast_prove and fast_verify methods that may be invoked when the PrimeField also satisfies the Absorb trait bound. I think this is a good trade off, since Absorb is not nearly as well implemented as Canonical(De)Serialize and it may be a pain for people to upgrade to the new implementation. Over time, I think the old method can be deprecated in favor of the new, but I think a prerequisite is implementing a derive Absorb macro.

Obviously there is a lot of code duplication, this can be refactored if @Pratyush approves the design philosophy here.

@autquis
Copy link

autquis commented Feb 13, 2024

Any updates on this? I am ready to take any necessary steps to make it ready to merge. @Pratyush @mmagician

Comment on lines +380 to +385
let (eta_a, eta_b, eta_c) = rng
.squeeze_field_elements(3)
.iter()
.map(|x: &F| x.to_owned())
.collect_tuple()
.unwrap();
Copy link
Member

@Pratyush Pratyush Feb 27, 2024

Choose a reason for hiding this comment

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

Suggested change
let (eta_a, eta_b, eta_c) = rng
.squeeze_field_elements(3)
.iter()
.map(|x: &F| x.to_owned())
.collect_tuple()
.unwrap();
let [eta_a, eta_b, eta_c] = rng
.squeeze_field_elements(3)[..3]
else { unreachable!("should have three elements") };

Comment on lines +317 to +322
let (f1, f2, f3) = rng
.squeeze_field_elements(3)
.iter()
.map(|x: &F| x.to_owned())
.collect_tuple()
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

This is not secure; we cannot use the sponge for randomness for zero knowledge. The old version should work fine.

Comment on lines +62 to +67
let (eta_a, eta_b, eta_c) = rng
.squeeze_field_elements(3)
.iter()
.map(|x: &F| x.to_owned())
.collect_tuple()
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let (eta_a, eta_b, eta_c) = rng
.squeeze_field_elements(3)
.iter()
.map(|x: &F| x.to_owned())
.collect_tuple()
.unwrap();
let [eta_a, eta_b, eta_c, ..] = rng
.squeeze_field_elements(3)[..3]
else { unreachable!("should be of size 3) };

Comment on lines +200 to +210
let fcinput = first_comms
.iter()
.map(|p| p.commitment().clone())
.collect::<Vec<_>>();

fs_rng.absorb(&to_bytes![first_comms, prover_first_msg].unwrap());
match prover_first_msg {
ProverMsg::FieldElements(ref elems) => {
absorb!(&mut fs_rng, &to_bytes(&fcinput), &to_bytes(elems));
}
ProverMsg::EmptyMessage => fs_rng.absorb(&to_bytes(&fcinput)),
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's wrap this into a helper function.

Comment on lines +231 to +240
let scinput = second_comms
.iter()
.map(|p| p.commitment().clone())
.collect::<Vec<_>>();
match prover_second_msg {
ProverMsg::FieldElements(ref elems) => {
absorb!(&mut fs_rng, &to_bytes(&scinput), &to_bytes(elems));
}
ProverMsg::EmptyMessage => fs_rng.absorb(&to_bytes(&scinput)),
}
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Comment on lines +260 to +269
let tcinput = third_comms
.iter()
.map(|p| p.commitment().clone())
.collect::<Vec<_>>();
match prover_third_msg {
ProverMsg::FieldElements(ref elems) => {
absorb!(&mut fs_rng, &to_bytes(&tcinput), &to_bytes(elems));
}
ProverMsg::EmptyMessage => fs_rng.absorb(&to_bytes(&tcinput)),
}
Copy link
Member

Choose a reason for hiding this comment

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

Same.

Comment on lines +396 to +403
match &proof.prover_messages[0] {
ProverMsg::FieldElements(ref elems) => {
absorb!(&mut fs_rng, &to_bytes(first_comms), &to_bytes(elems));
}
ProverMsg::EmptyMessage => fs_rng.absorb(&to_bytes(first_comms)),
}
let (_, verifier_state) =
AHPForR1CS::verifier_first_round(index_vk.index_info, &mut fs_rng)?;
Copy link
Member

Choose a reason for hiding this comment

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

Let's use the helper function here also.

{
/// Create a zkSNARK asserting that the constraint system is satisfied.
/// Uses fast absorption of field elements into sponge
pub fn fast_prove<C: ConstraintSynthesizer<F>, R: RngCore + CryptographicSponge>(
Copy link
Member

Choose a reason for hiding this comment

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

What is this new function?

Copy link
Author

Choose a reason for hiding this comment

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

Comment on lines +141 to +146
let (a, b) = rng
.squeeze_field_elements(2)
.iter()
.map(|x: &Fr| x.to_owned())
.collect_tuple()
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Let's create a helper squeeze macro.

@TakodaS
Copy link
Author

TakodaS commented Mar 6, 2024

Any updates on this? I am ready to take any necessary steps to make it ready to merge. @Pratyush @mmagician

Go right ahead! I guess you can make a new pull request and I will close this one?

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