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

Include chunked phase2 script #439

Open
wants to merge 18 commits into
base: phase2
Choose a base branch
from
Open

Include chunked phase2 script #439

wants to merge 18 commits into from

Conversation

emmorais
Copy link

@emmorais emmorais commented Nov 4, 2021

Ongoing work. Currently getting the following error:

Running `/media/eduardo/DATA/eduardo/Aleo/code/aleo-setup/target/release/setup2 new --curve-type bw6 --chunk-size 131072 --batch-size 131072 --contribution-mode full --challenge-fname challenge --challenge-hash-fname challenge.verified.hash --phase1-fname ../../phase1-cli/scripts/combined --phase1-powers 18 --num-validators 1 --num-epochs 1`
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: ZexeSerializationError(IoError(Custom { kind: Other, error: "Invalid field element" }))', setup2/src/cli/new.rs:200:6
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

cargo $CARGO_VER build --release --bin setup2

phase2_new="cargo run --release --features cli -- new --curve-type $CURVE --chunk-size $CHUNK_SIZE --batch-size $BATCH --contribution-mode full"
phase2_chunked="cargo run --release --bin setup2 --features cli -- --curve-type $CURVE --chunk-size $CHUNK_SIZE --batch-size $BATCH --contribution-mode full --proving-system $PROVING_SYSTEM"

Choose a reason for hiding this comment

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

This should come with an argument for the binary, right? That is, one of new, contribute, etc?

Copy link

@niklaslong niklaslong Dec 6, 2021

Choose a reason for hiding this comment

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

Looks like we're still missing the seed and the proving_system in the related struct (edit: and possibly more judging by the rest of the script, and assuming it is correct).

Choose a reason for hiding this comment

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

The command is actually specified lower in the script.

Choose a reason for hiding this comment

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

@emmorais are these commands currently complete?

Comment on lines +26 to +27
phase2_1="cargo run --release --bin setup2 --features cli -- --curve-type $CURVE --batch-size $BATCH --contribution-mode chunked --chunk-size $CHUNK_SIZE --seed seed1 --proving-system $PROVING_SYSTEM"
phase2_2="cargo run --release --bin setup2 --features cli -- --curve-type $CURVE --batch-size $BATCH --contribution-mode chunked --chunk-size $CHUNK_SIZE --seed seed2 --proving-system $PROVING_SYSTEM"

Choose a reason for hiding this comment

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

Same for these two?

Choose a reason for hiding this comment

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

ditto

@Pratyush Pratyush force-pushed the phase2_chunked_script branch from 774524c to f7cd635 Compare December 21, 2021 22:20
UseCompression::Yes,
CheckForCorrectness::Full,
UseCompression::No,
CheckForCorrectness::No,
Copy link

Choose a reason for hiding this comment

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

if we've already checked the output of Phase1, then we don't need to check for correctness, which makes the preparation step faster. This should be re-enabled if we did not check that the output of Phase 1 is in the appropriate prime-order group.

Comment on lines +94 to +110
if input.len() > 10 {
let mut input = input
.par_iter()
.map(|(coeff, lag)| {
let ind = match *lag {
Index::Public(i) => i,
Index::Private(i) => num_inputs + i,
};
sum += coeffs[ind].mul(coeff).into_projective();
sum
},
)
.reduce(|| C::Projective::zero(), |a, b| a + b)
(coeff, ind)
})
.collect::<Vec<_>>();
input.sort_unstable_by(|a, b| a.1.cmp(&b.1));
let input = input
.into_par_iter()
.map(|(coeff, _)| coeff.to_repr())
.collect::<Vec<_>>();
snarkvm_algorithms::msm::variable_base::VariableBaseMSM::multi_scalar_mul(coeffs, &input)
Copy link

Choose a reason for hiding this comment

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

The dot product can be computed quickly using a variable base MSM. We do this if input.len() is large enough.

let mut points: Vec<_> = cfg_iter!(bases)
.map(|base| base.into_projective().mul(*coeff))
.map(|base| base.mul_bits(BitIteratorBE::new(coeff)))
Copy link

Choose a reason for hiding this comment

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

This change ensures that we use mixed addition, which makes the multiplication slightly faster.

let address = Address::try_from(private_key)?.to_string();

let message = format!("{} {}", method.to_lowercase(), path.to_lowercase());
let signature = hex::encode(&view_key.sign(message.as_bytes(), rng)?.to_bytes_le()?);
let signature = hex::encode(&private_key.sign(message.as_bytes(), rng)?.to_bytes_le()?);
Copy link

Choose a reason for hiding this comment

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

Q: is this change warranted? Not sure if this breaks things (did the address scheme change since the code was written?)

Copy link

Choose a reason for hiding this comment

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

Comment on lines +118 to +119
let private_key = PrivateKey::<Testnet2>::from_str(signing_key)?;
let signature = hex::encode(&private_key.sign(message.as_bytes(), rng)?.to_bytes_le()?);
Copy link

Choose a reason for hiding this comment

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

Ditto

@Pratyush
Copy link

Pratyush commented Jan 4, 2022

Update: in the latest commit, the error that Eduardo encountered is no longer present, so the new subcommand runs to completion. The remaining work involves implementing the rest of the commands specified in the script.

@Pratyush
Copy link

Pratyush commented Jan 6, 2022

In the latest commit, most tests are passing, except some tests in setup1-verifier. These tests are failing due to an inability to deserialize a PrivateKey from the hardcoded ViewKey bytes. We can no longer use the ViewKey because in the latest snarkvm, we use PrivateKeys to sign (whereas earlier we signed with ViewKeys).

To get the tests passing, I can sample a new PrivateKey and hardcode this in the tests, but overall I'm not sure if the new behaviour (i.e. switching away from ViewKey signing) is wanted.

@niklaslong
Copy link

This may need a rebase after #448 is merged.

@Pratyush
Copy link

Merged phase2 into this.

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