-
Notifications
You must be signed in to change notification settings - Fork 195
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
feat: 20-30% cost reduction in recursive ipa algorithm #9420
base: master
Are you sure you want to change the base?
Conversation
removed unneccessary uses of `pow` operator in ipa - in the recursive setting these were stdlib::bigfield::pow calls and very expensive removed the number of distinct multiplication calls in ipa::reduce_verify_internal cycle_scalar::cycle_scalar(stdlib::bigfield) constructor now more optimally constructs a cycle_scalar out of a bigfield element. New method leverages the fact that `scalar.lo` and `scalar.hi` are implicitly range-constrained to remove reundant bigfield constructor calls and arithmetic calls, and the process of performing a scalar multiplication applies a modular reduction to the imput, which makes the explicit call to `validate_scalar_is_in_field` unneccessary
@@ -84,7 +84,7 @@ template <typename RecursiveFlavor> class ECCVMRecursiveTests : public ::testing | |||
OuterBuilder outer_circuit; | |||
RecursiveVerifier verifier{ &outer_circuit, verification_key }; | |||
verifier.verify_proof(proof); | |||
info("Recursive Verifier: num gates = ", outer_circuit.num_gates); | |||
info("Recursive Verifier: num gates = ", outer_circuit.get_estimated_num_finalized_gates()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you print out that this number is an estimate? or finalize the circuit and print out the actual finalized gate count
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's quite expensive to actually finalise the circuit - is it important for this number to be 100% accurate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be too expensive right? Its fine to just keep it as is but say that its num estimated finalized gates instead of just num gates though.
field_t hi = field_t::from_witness(ctx, hi_v); | ||
|
||
uint256_t hi_max = (scalar.binary_basis_limbs[0].maximum_value >> BigScalarField::NUM_LIMB_BITS); | ||
const size_t hi_bits = hi_max.get_msb() + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
causing a error:
fatal error: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long long') to 'const size_t' (aka 'const unsigned long') [-Wshorten-64-to-32] 737 | const size_t hi_bits = hi_max.get_msb() + 1;
Can add a static_cast here to fix it, but I would also recommend not using size_t.
eccvm_recursive_verifier_test measurements (size-512 eccvm recursive verification)
Old: 876,214
New: 678,751
The relative performance delta should be much greater for large eccvm instances as this PR removes an nlogn algorithm.
This PR resolves issue #857 and issue #1023 (single batch mul in IPA)
Re: #1023. The code still performs 2 batch muls, but all additional * operator calls have been combined into the batch muls.
It is not worth combining both batch muls, as it would require a multiplication operation on a large number of scalar multipliers. In the recursive setting the scalars are bigfield elements - the extra bigfield::operator* cost is not worth combining both batch_mul calls.
Additional improvements:
removed unneccessary uses of
pow
operator in ipa - in the recursive setting these were stdlib::bigfield::pow calls and very expensiveremoved the number of distinct multiplication calls in ipa::reduce_verify_internal
cycle_scalar::cycle_scalar(stdlib::bigfield) constructor now more optimally constructs a cycle_scalar out of a bigfield element. New method leverages the fact that
scalar.lo
andscalar.hi
are implicitly range-constrained to remove reundant bigfield constructor calls and arithmetic calls, and the process of performing a scalar multiplication applies a modular reduction to the imput, which makes the explicit call tovalidate_scalar_is_in_field
unneccessary