Catch usize conversion and Err more consistently #1499

vicsn · 2023-05-02T13:44:00Z

Motivation

See for a motivation this comment.

However, my initial intuition was wrong, we can't reduce developer confusion with a "use usize as little as possible" because Rust's library uses usize all over the place. We instead increase clarity and safety by being more rigorous about our usage of usize. See the CONTRIBUTING doc for more details.

Test Plan

Given that the main concern for this is consensus issues, I suggest to take extra care to stress test snark{VM,OS} on both 32-bit and 64-bit machines, generating test input from the different machine or constructing it manually. Let me know if you want me to help setting that up already, otherwise I'll make a TODO for once we start testing AleoBFT more extensively.

Pratyush · 2023-05-02T13:50:16Z

algorithms/src/snark/marlin/ahp/ahp.rs

+        let mut z_b_s = BTreeMap::new();
+        for (circuit_id, circuit_state) in state.circuit_specific_states.iter() {
+            let mut z_b_i = Vec::with_capacity(usize::try_from(circuit_state.batch_size)?);
+            for i in 0..circuit_state.batch_size {
+                let z_b = witness_label(*circuit_id, "z_b", usize::try_from(i)?);
+                z_b_i.push(LinearCombination::new(z_b.clone(), [(F::one(), z_b)]));
+            }
+            z_b_s.insert(circuit_id, z_b_i);
+        }


I think we can use Itertools::try_collect here, instead of rewriting as a for loop

I don't want to return the Result, I have to unwrap the result in order to use z_b on line 246. I got it to compile if we return a Result anyway for all paths, but I don't think we'd be happy with this (the compiler asks for those type annotations:

let z_b_s = state .circuit_specific_states .iter() .map(|(&circuit_id, circuit_state)| { let z_b_i = (0..circuit_state.batch_size) .map(|i| { let z_b = witness_label(circuit_id, "z_b", usize::try_from(i)?); Ok::<LinearCombination<F>, std::num::TryFromIntError>(LinearCombination::new(z_b.clone(), [(F::one(), z_b)])) }).collect::<Result<Vec<_>,_>>(); Ok::<(CircuitId, Vec<LinearCombination<F>>), std::num::TryFromIntError>((circuit_id, z_b_i?)) }) .collect::<Result<BTreeMap<_, _>,_>>()?;

Meanwhile, online fora on related issues keep suggesting to just use a forloop.

Got it down to 4916a3c but it's still more code than the for-loop

Pratyush · 2023-05-02T13:51:39Z

algorithms/src/snark/marlin/ahp/ahp.rs

            })
-            .collect::<BTreeMap<_, _>>();
+            .collect::<Result<Vec<_>, _>>()?;


Ditto here, I think we can use try_collect to still collect into a BTreeMap

Once we do that, the code that uses z_b_s_at_beta should go back to its old state.

I can't use try_collect because I need to annotate the types, but I can use ? within the map now and collect: 4916a3c

Pratyush · 2023-05-02T13:53:43Z

algorithms/src/snark/marlin/ahp/prover/round_functions/first.rs

@@ -74,13 +74,14 @@ impl<F: PrimeField, MM: MarlinMode> AHPForR1CS<F, MM> {
    #[allow(clippy::type_complexity)]
    pub fn prover_first_round<'a, R: RngCore>(
        mut state: prover::State<'a, F, MM>,
+        batch_sizes: impl Iterator<Item = (&'a CircuitId, &'a usize)>,


Why are we passing this separately? Isn't it included in state?

It is in state, but we created this iterator already earlier so I thought why not reuse it for line 138 below.

Why are we passing this in if it's in state?

See the comment right above yours... But since you both think it's weird, removed it: 7f2a6c8

Pratyush · 2023-05-02T13:54:23Z

algorithms/src/snark/marlin/ahp/prover/round_functions/first.rs

@@ -94,7 +95,7 @@ impl<F: PrimeField, MM: MarlinMode> AHPForR1CS<F, MM> {
            let c_domain = circuit_state.constraint_domain;
            let i_domain = circuit_state.input_domain;

-            let mut circuit_r_b_s = Vec::with_capacity(batch_size);
+            let mut circuit_r_b_s = Vec::with_capacity(z_a.len());


Why did this change? We do still have batch_size in scope

algorithms/src/snark/marlin/ahp/prover/round_functions/first.rs

Pratyush · 2023-05-02T13:55:51Z

algorithms/src/snark/marlin/ahp/prover/round_functions/first.rs

        rng: &mut R,
    ) -> Result<prover::State<'a, F, MM>, AHPError> {
        let round_time = start_timer!(|| "AHP::Prover::FirstRound");
        let mut r_b_s = Vec::with_capacity(state.circuit_specific_states.len());
-        let mut job_pool = snarkvm_utilities::ExecutionPool::with_capacity(3 * state.total_instances);
+        let mut job_pool = snarkvm_utilities::ExecutionPool::with_capacity(usize::try_from(3 * state.total_instances)?);


If the following works, I think we should switch to it, instead of using the more verbose usize::try_from

Suggested change

let mut job_pool = snarkvm_utilities::ExecutionPool::with_capacity(usize::try_from(3 * state.total_instances)?);

let mut job_pool = snarkvm_utilities::ExecutionPool::with_capacity((3 * state.total_instances).try_into()?);

Pratyush · 2023-05-02T13:57:10Z

algorithms/src/snark/marlin/ahp/prover/state.rs

@@ -114,13 +114,13 @@ impl<'a, F: PrimeField, MM: MarlinMode> State<'a, F, MM> {

                let first_padded_public_inputs = &variable_assignments[0].0;
                let input_domain = EvaluationDomain::new(first_padded_public_inputs.len()).unwrap();
-                let batch_size = variable_assignments.len();
+                let batch_size = u32::try_from(variable_assignments.len())?;


Same, let's change this to try_into()?

Pratyush · 2023-05-02T14:01:31Z

algorithms/src/snark/marlin/ahp/prover/state.rs

-                let mut x_polys = Vec::with_capacity(batch_size);
-                let mut padded_public_variables = Vec::with_capacity(batch_size);
-                let mut private_variables = Vec::with_capacity(batch_size);
+                let mut z_as = Vec::with_capacity(variable_assignments.len());


Maybe we should make a batch_size_usize local variable and use that here?

Indeed would be easier to read 5ebe7b2

Pratyush · 2023-05-02T14:02:07Z

algorithms/src/snark/marlin/ahp/verifier/messages.rs

+        let mut batch_sizes = BTreeMap::new();
+        for (circuit_id, batch_size) in
+            state.circuit_specific_states.iter().map(|(circuit_id, s)| (circuit_id, s.batch_size))
+        {
+            batch_sizes.insert(*circuit_id, usize::try_from(batch_size)?);
+        }


Same, try_collect and try_into.

CONTRIBUTING.md

ljedrz · 2023-05-05T10:09:18Z

algorithms/src/fft/polynomial/sparse.rs


 use std::{collections::BTreeMap, fmt};

 /// Stores a sparse polynomial in coefficient form.
-#[derive(Clone, PartialEq, Eq, Hash, Default, CanonicalSerialize, CanonicalDeserialize)]
+#[derive(Clone, PartialEq, Eq, Hash, Default)]


side note: it might be a good idea to check if any other objects can be "trimmed" in a similar fashion for faster compilation

Added a thumbsup! rust-lang/rust#50927

algorithms/src/snark/marlin/data_structures/proof.rs

ljedrz

LGTM 👍

ljedrz · 2023-05-05T10:27:52Z

If we're concerned with verbosity, we could always introduce helper macros in snarkvm_utilities that would substitute calls like

usize::try_from(x)?

with sth like

to_usize!(x)

Such calls are cheap and they provide us with much more cross-platform safety, so even if it's an annoyance, it is IMO well worth it.

vicsn · 2023-05-05T16:20:17Z

For the record, I cannot reproduce this CI error locally: thread 'package::run::tests::test_run_with_import' panicked at 'called `Result::unwrap()` on an `Err` value: marlin: PolynomialCommitmentError(AnyhowError(Expected size of 12582920, found size of 12075008))', vm/package/run.rs:145:44

ljedrz · 2023-05-05T16:22:08Z

For the record, I cannot reproduce this CI error locally: thread 'package::run::tests::test_run_with_import' panicked at 'called Result::unwrap() on an Err value: marlin: PolynomialCommitmentError(AnyhowError(Expected size of 12582920, found size of 12075008))', vm/package/run.rs:145:44

some CI tests checking object sizes have been flaky recently

algorithms/src/snark/marlin/ahp/prover/state.rs

howardwu · 2023-05-05T22:50:06Z

@vicsn the CI failures on synthesizer are concerning. Our serialization sizes appear to have genuinely changed. Those failures aren't flaky. (only the ParameterError ones are flaky, which are not present in synthesizer).

vicsn · 2023-05-07T22:16:20Z

@vicsn the CI failures on synthesizer are concerning. Our serialization sizes appear to have genuinely changed. Those failures aren't flaky. (only the ParameterError ones are flaky, which are not present in synthesizer).

I should have tackled this right away! The failing tests can be explained by the fact that we moved from usize (which was 64 bit on everyone's machine so far) to u32. So we can either (1) change the code to use u64 or (2) resample parameters yet again and update the tests. As a batch size of u32 gives us enough space I vote for this option.

In more detail:

The block:: tests fail because they can't deserialize Proof - which is hardcoded. Resampling genesis resolved the issue for me.
The vm:: tests fail because they are 8 bytes (for two proof instances) or 4 bytes (for one proof instance) less than expected. 1690b3b

howardwu · 2023-05-14T00:25:09Z

@vicsn can you try adding the following line to the top of snarkvm-algorithms/src/lib.rs and see if the compiler warns?

#![warn(clippy::cast_possible_truncation)]

We use this flag in circuit and console to ensure there are no unsafe casts.

vicsn · 2023-05-15T06:41:13Z

warn(clippy::cast_possible_truncation)]

Added it. For the adjustments in the last commits I:

let structs use fixed-size types instead of usize to make it clear to readers what size to expect for operations
if the callchain doesn't use Results or requires a lot of casting, see if we can avoid the overhead of try_from and wrap the function in #[allow(...)]. I added comments about why casting is safe, if you think it is too ugly we can remove those but at least it can help your review. A big downside of this approach is that it can give a false sense of security in the future - just like all uses of #[allow(...)] in the codebase. It would be cool if #[allow(...)] codeblocks would have to be manually approved/enabled in CI whenever there is a commit which changes something in the respective codeblock. Perhaps for now it suffices to add a line to the PR template or CONTRIBUTING: "- [ ] I checked whether the functions I changed are still allowed to have it's #[allow(...)]". @ljedrz shall I add that?

ljedrz · 2023-05-17T07:36:22Z

@vicsn I think it's always better to have more comments to avoid any misunderstanding/misuse 👍. As for future-proofing, I think the safest course of action is to have more comprehensive tests, including extremes in terms of the number of items, but having some extra information in CONTRIBUTING is also valuable.

What we could also do is add assertions in some of the places (the ones where we know the value is smaller than some other value) where we perform the "currently valid" casts - simple comparisons shouldn't be noticeable at all perf-wise.

ljedrz · 2023-05-17T07:51:16Z

algorithms/src/snark/marlin/data_structures/proof.rs

+        let mut batch_sizes = Vec::with_capacity(batch_sizes_in.len());
+        for (z_b_evals, batch_size) in evaluations.z_b_evals.iter().zip(batch_sizes_in.values()) {


nice allocation removal 👌

ljedrz

The further changes look fine to me 👍.

vicsn · 2023-05-19T21:28:32Z

@howardwu if you like the PR's logic, I suggest we merge and I will tackle this piece by piece in the coming weeks: https://github.com/AleoHQ/snarkVM/issues/1567

howardwu · 2023-05-25T21:51:24Z

algorithms/src/polycommit/sonic_pc/data_structures.rs

        })
    }
 }

 impl<E: PairingEngine> ToBytes for CommitterKey<E> {
+    // Values are safe to cast to u32 because they are read as u32.
+    #[allow(clippy::cast_possible_truncation)]


This should be fixed, not allowed. See https://github.com/AleoHQ/snarkVM/blob/testnet3/synthesizer/coinbase/src/helpers/coinbase_solution/bytes.rs#L37 for how to proceed

Fixed: f339c4c

howardwu · 2023-05-25T21:51:30Z

algorithms/src/polycommit/kzg10/data_structures.rs

@@ -131,6 +131,8 @@ impl<E: PairingEngine> FromBytes for UniversalParams<E> {
    }
 }

+// It is safe to cast `supported_degree_bounds.len()` and individual bounds to a `u32` because they are read and used as u32
+#[allow(clippy::cast_possible_truncation)]


This should be fixed, not allowed. See https://github.com/AleoHQ/snarkVM/blob/testnet3/synthesizer/coinbase/src/helpers/coinbase_solution/bytes.rs#L37 for how to proceed

Fixed: f339c4c

howardwu · 2023-05-25T21:51:40Z

algorithms/src/msm/variable_base/standard.rs

 fn update_buckets<G: AffineCurve>(
    base: &G,
    mut scalar: <G::ScalarField as PrimeField>::BigInteger,
    w_start: usize,
    c: usize,
    buckets: &mut [G::Projective],
 ) {
+    assert!(w_start <= u32::MAX as usize);


Why are we using assert?

Just for extra clarity, removed the assert now

howardwu · 2023-05-25T21:51:42Z

algorithms/src/msm/variable_base/batched.rs

 fn batched_window<G: AffineCurve>(
    bases: &[G],
    scalars: &[<G::ScalarField as PrimeField>::BigInteger],
    w_start: usize,
    c: usize,
 ) -> (G::Projective, usize) {
+    assert!(w_start <= u32::MAX as usize);


Why are we using assert?

Just for extra clarity, we'd already panic if the conversion would fail so I now removed the asserts.

howardwu · 2023-05-25T21:51:48Z

algorithms/src/msm/variable_base/batched.rs

 pub(super) fn batch_add<G: AffineCurve>(
    num_buckets: usize,
    bases: &[G],
    bucket_positions: &mut [BucketPosition],
 ) -> Vec<G> {
    // assert_eq!(bases.len(), bucket_positions.len());
    assert!(!bases.is_empty());
+    assert!(num_buckets <= u32::MAX as usize);


Why are we using assert?

So instead of panicking on a failed cast, we'd panic somewhat more clearly on this assert.

But anyway, gave it more thought, removed the assert and now passing a u32 to this function so we only have to reason about safe casting one level higher.

algorithms/src/lib.rs

howardwu · 2023-05-25T21:52:05Z

algorithms/src/crypto_hash/poseidon.rs

    /// Obtain the limbs directly from a big int
    pub fn get_limbs_representations_from_big_integer<TargetField: PrimeField>(
        elem: &<TargetField as PrimeField>::BigInteger,
        optimization_type: OptimizationType,
    ) -> SmallVec<[F; 10]> {
        let params = get_params(TargetField::size_in_bits(), F::size_in_bits(), optimization_type);
+        assert!(params.bits_per_limb <= u32::MAX as usize);


Why are we using assert?

In this case I think it's fine to error out, or we can have params.bits_per_limb be a u32. Generally when we have a malformed config it's better to assert and fail quickly. This avoid polluting function signatures with Results

Catch error in map

- fixes PR comments - Introduces try_write_as

…d witness_labels using u32

vicsn requested review from Pratyush and howardwu May 2, 2023 13:44

Pratyush reviewed May 2, 2023

View reviewed changes

algorithms/src/snark/marlin/ahp/prover/round_functions/first.rs Show resolved Hide resolved

Pratyush reviewed May 2, 2023

View reviewed changes

howardwu requested a review from ljedrz May 5, 2023 01:49

ljedrz reviewed May 5, 2023

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

ljedrz reviewed May 5, 2023

View reviewed changes

algorithms/src/snark/marlin/data_structures/proof.rs Show resolved Hide resolved

ljedrz approved these changes May 5, 2023

View reviewed changes

howardwu reviewed May 5, 2023

View reviewed changes

algorithms/src/snark/marlin/ahp/prover/state.rs Show resolved Hide resolved

Base automatically changed from staging to testnet3 May 10, 2023 19:02

vicsn force-pushed the catch_usize_and_err branch from 1690b3b to 8fb4a79 Compare May 12, 2023 10:20

vicsn requested a review from ljedrz May 15, 2023 06:41

ljedrz reviewed May 17, 2023

View reviewed changes

ljedrz approved these changes May 17, 2023

View reviewed changes

vicsn force-pushed the catch_usize_and_err branch from a1b5201 to 0c8a7f6 Compare May 17, 2023 15:18

vicsn mentioned this pull request May 17, 2023

Multi circuit proof integration #1510

Closed

vicsn mentioned this pull request May 24, 2023

Prevent addition overflow #1584

Closed

howardwu changed the base branch from testnet3 to staging May 25, 2023 21:38

howardwu reviewed May 25, 2023

View reviewed changes

algorithms/src/lib.rs Show resolved Hide resolved

howardwu reviewed May 25, 2023

View reviewed changes

vicsn force-pushed the catch_usize_and_err branch 2 times, most recently from d56a16c to d33b4e2 Compare June 6, 2023 16:43

vicsn requested a review from howardwu June 6, 2023 16:45

Base automatically changed from staging to testnet3 June 6, 2023 16:57

vicsn requested a review from ljedrz June 6, 2023 17:04

vicsn added 10 commits June 6, 2023 19:22

Catch Err from evals.get_lc_eval

4052d3f

Enforce a batch_size of u32

0c6fd62

Catch error in map

Add cast_possible_truncation to algorithms

f0bb49d

Update tx sizes

779c86f

Add warn(clippy::cast_possible_truncation)]

bbd3193

Assert that we don't cast values which are too big

6077b88

TO SQUASH

291b731

- fixes PR comments - Introduces try_write_as

Don't pass batch_sizes separate from state, and define batch_sizes an…

1a8acc1

…d witness_labels using u32

Use try_write_as in synthesizer

a794c2a

Update tests

5239635

vicsn force-pushed the catch_usize_and_err branch from 2500e08 to 5239635 Compare June 6, 2023 17:41

Return the right Result

eeaaa4d

vicsn mentioned this pull request Sep 13, 2023

Prevent addition overflow #1983

Merged

	let mut job_pool = snarkvm_utilities::ExecutionPool::with_capacity(usize::try_from(3 * state.total_instances)?);
	let mut job_pool = snarkvm_utilities::ExecutionPool::with_capacity((3 * state.total_instances).try_into()?);

		let mut batch_sizes = Vec::with_capacity(batch_sizes_in.len());
		for (z_b_evals, batch_size) in evaluations.z_b_evals.iter().zip(batch_sizes_in.values()) {

Catch usize conversion and Err more consistently #1499

Are you sure you want to change the base?

Catch usize conversion and Err more consistently #1499

Conversation

vicsn commented May 2, 2023 • edited Loading

Motivation

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljedrz left a comment

Choose a reason for hiding this comment

ljedrz commented May 5, 2023

vicsn commented May 5, 2023

ljedrz commented May 5, 2023

howardwu commented May 5, 2023

vicsn commented May 7, 2023

howardwu commented May 14, 2023

vicsn commented May 15, 2023 • edited Loading

ljedrz commented May 17, 2023 • edited Loading

Choose a reason for hiding this comment

ljedrz left a comment

Choose a reason for hiding this comment

vicsn commented May 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicsn commented May 2, 2023 •

edited

Loading

vicsn commented May 15, 2023 •

edited

Loading

ljedrz commented May 17, 2023 •

edited

Loading