-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[Perf] Only calculate the hash of circuit commitments once for the VK #2964
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
base: staging
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ | |
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| use crate::{polycommit::sonic_pc, snark::varuna::ahp::indexer::*}; | ||
| use crate::{AlgebraicSponge, polycommit::sonic_pc, snark::varuna::ahp::indexer::*}; | ||
| use snarkvm_curves::PairingEngine; | ||
| use snarkvm_utilities::{FromBytes, FromBytesDeserializer, ToBytes, ToBytesSerializer, into_io_error, serialize::*}; | ||
|
|
||
|
|
@@ -25,17 +25,33 @@ use std::{ | |
| io::{self, Read, Write}, | ||
| str::FromStr, | ||
| string::String, | ||
| sync::OnceLock, | ||
| }; | ||
|
|
||
| /// Verification key for a specific index (i.e., R1CS matrices). | ||
| #[derive(Debug, Clone, PartialEq, Eq, CanonicalSerialize, CanonicalDeserialize)] | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct CircuitVerifyingKey<E: PairingEngine> { | ||
| /// Stores information about the size of the circuit, as well as its defined | ||
| /// field. | ||
| pub circuit_info: CircuitInfo, | ||
| /// Commitments to the indexed polynomials. | ||
| pub circuit_commitments: Vec<sonic_pc::Commitment<E>>, | ||
| pub id: CircuitId, | ||
| pub circuit_commitments_hash: OnceLock<E::Fq>, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is expensive enough that we should store it to disk - and it would be great if we can get rid of the OnceLock (which obfuscates when initialization happens, making performance analysis harder). O:) You correctly observed that we don't have to transmit it over the wire though.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue was that at the moment of creation, the Storing to the disk would probably work around this, but retrieving it would be expensive, perhaps to the point of offsetting any performance gains from caching it, unless the hashing is really computationally expensive.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
To be clear, we would retrieve it from disk only when we retrieve the VK from disk. And the hashes are very expensive. However, a big downside I do see is the sheer amount of work to adjust the database logic. So for the first version you can also compute it during construction.
Looks to me it's always available in N::varuna_fs_parameters() ? Or is there a scoping issue? Have fun with that :")
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's correct; however, there is no notion of the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For my future self reading this: we should just store to disk, but we can write out that logic when we have a definite timeline for landing this feature. |
||
| } | ||
|
|
||
| impl<E: PairingEngine> CircuitVerifyingKey<E> { | ||
| pub fn get_or_calculate_circuit_commitments_hash<FS: AlgebraicSponge<E::Fq, 2>>( | ||
| &self, | ||
| fs_parameters: &FS::Parameters, | ||
| ) -> &E::Fq { | ||
| self.circuit_commitments_hash.get_or_init(|| { | ||
| let mut sponge = FS::new_with_parameters(fs_parameters); | ||
| sponge.absorb_native_field_elements(&self.circuit_commitments); | ||
|
|
||
| sponge.squeeze_native_field_elements(1)[0] | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| impl<E: PairingEngine> FromBytes for CircuitVerifyingKey<E> { | ||
|
|
@@ -99,6 +115,58 @@ impl<'de, E: PairingEngine> Deserialize<'de> for CircuitVerifyingKey<E> { | |
| } | ||
| } | ||
|
|
||
| impl<E: PairingEngine> CanonicalSerialize for CircuitVerifyingKey<E> { | ||
| fn serialize_with_mode<W: Write>(&self, mut writer: W, compress: Compress) -> Result<(), SerializationError> { | ||
| self.circuit_info.serialize_with_mode(&mut writer, compress)?; | ||
| self.circuit_commitments.serialize_with_mode(&mut writer, compress)?; | ||
| self.id.serialize_with_mode(&mut writer, compress)?; | ||
| // The hash is omitted. | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn serialized_size(&self, compress: Compress) -> usize { | ||
| self.circuit_info.serialized_size(compress) | ||
| + self.circuit_commitments.serialized_size(compress) | ||
| + self.id.serialized_size(compress) | ||
| // The hash is omitted. | ||
| } | ||
| } | ||
|
|
||
| impl<E: PairingEngine> CanonicalDeserialize for CircuitVerifyingKey<E> { | ||
| fn deserialize_with_mode<R: Read>( | ||
| mut reader: R, | ||
| compress: Compress, | ||
| validate: Validate, | ||
| ) -> Result<Self, SerializationError> { | ||
| let circuit_info = CanonicalDeserialize::deserialize_with_mode(&mut reader, compress, validate)?; | ||
| let circuit_commitments = CanonicalDeserialize::deserialize_with_mode(&mut reader, compress, validate)?; | ||
| let id = CanonicalDeserialize::deserialize_with_mode(&mut reader, compress, validate)?; | ||
| Ok(Self { circuit_info, circuit_commitments, id, circuit_commitments_hash: Default::default() }) | ||
| } | ||
| } | ||
|
|
||
| impl<E: PairingEngine> Valid for CircuitVerifyingKey<E> { | ||
| fn check(&self) -> Result<(), SerializationError> { | ||
| Valid::check(&self.circuit_info)?; | ||
| Valid::check(&self.circuit_commitments)?; | ||
| Valid::check(&self.id)?; | ||
| // The hash is omitted. | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn batch_check<'a>(batch: impl Iterator<Item = &'a Self> + Send) -> Result<(), SerializationError> | ||
| where | ||
| Self: 'a, | ||
| { | ||
| let batch: Vec<_> = batch.collect(); | ||
| Valid::batch_check(batch.iter().map(|v| &v.circuit_info))?; | ||
| Valid::batch_check(batch.iter().map(|v| &v.circuit_commitments))?; | ||
| Valid::batch_check(batch.iter().map(|v| &v.id))?; | ||
| // The hash is omitted. | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| impl<E: PairingEngine> Ord for CircuitVerifyingKey<E> { | ||
| fn cmp(&self, other: &Self) -> Ordering { | ||
| self.id.cmp(&other.id) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -120,6 +120,7 @@ impl<E: PairingEngine, FS: AlgebraicSponge<E::Fq, 2>, SM: SNARKMode> VarunaSNARK | |
| let circuit_verifying_key = CircuitVerifyingKey { | ||
| circuit_info: indexed_circuit.index_info, | ||
| circuit_commitments, | ||
| circuit_commitments_hash: Default::default(), | ||
| id: indexed_circuit.id, | ||
| }; | ||
| let circuit_proving_key = CircuitProvingKey { | ||
|
|
@@ -134,10 +135,10 @@ impl<E: PairingEngine, FS: AlgebraicSponge<E::Fq, 2>, SM: SNARKMode> VarunaSNARK | |
| Ok(circuit_keys) | ||
| } | ||
|
|
||
| fn init_sponge<'a>( | ||
| fn init_sponge( | ||
| fs_parameters: &FS::Parameters, | ||
| inputs_and_batch_sizes: &BTreeMap<CircuitId, (usize, &[Vec<E::Fr>])>, | ||
| circuit_commitments: impl Iterator<Item = &'a [crate::polycommit::sonic_pc::Commitment<E>]>, | ||
| circuit_commitments_hashes: Vec<E::Fq>, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you make a 3rd VarunaVersion, and guard the changes by it to preserve backwards compatibility? You can peek at where we use VarunaVersion::V2 for inspiration. |
||
| ) -> FS { | ||
| let mut sponge = FS::new_with_parameters(fs_parameters); | ||
| sponge.absorb_bytes(Self::PROTOCOL_NAME); | ||
|
|
@@ -147,9 +148,7 @@ impl<E: PairingEngine, FS: AlgebraicSponge<E::Fq, 2>, SM: SNARKMode> VarunaSNARK | |
| sponge.absorb_nonnative_field_elements(input.iter().copied()); | ||
| } | ||
| } | ||
| for circuit_specific_commitments in circuit_commitments { | ||
| sponge.absorb_native_field_elements(circuit_specific_commitments); | ||
| } | ||
| sponge.absorb_native_field_elements(&circuit_commitments_hashes); | ||
| sponge | ||
| } | ||
|
|
||
|
|
@@ -393,10 +392,13 @@ where | |
|
|
||
| let committer_key = CommitterUnionKey::union(keys_to_constraints.keys().map(|pk| pk.committer_key.deref())); | ||
|
|
||
| let circuit_commitments = | ||
| keys_to_constraints.keys().map(|pk| pk.circuit_verifying_key.circuit_commitments.as_slice()); | ||
| let circuit_commitments_hashes = keys_to_constraints | ||
| .keys() | ||
| .map(|pk| pk.circuit_verifying_key.get_or_calculate_circuit_commitments_hash::<FS>(fs_parameters)) | ||
| .copied() | ||
| .collect(); | ||
|
|
||
| let mut sponge = Self::init_sponge(fs_parameters, &inputs_and_batch_sizes, circuit_commitments.clone()); | ||
| let mut sponge = Self::init_sponge(fs_parameters, &inputs_and_batch_sizes, circuit_commitments_hashes); | ||
|
|
||
| // -------------------------------------------------------------------- | ||
| // First round | ||
|
|
@@ -860,7 +862,12 @@ where | |
| let fifth_commitments = [LabeledCommitment::new_with_info(&fifth_round_info["h_2"], comms.h_2)]; | ||
|
|
||
| let circuit_commitments = keys_to_inputs.keys().map(|vk| vk.circuit_commitments.as_slice()); | ||
| let mut sponge = Self::init_sponge(fs_parameters, &inputs_and_batch_sizes, circuit_commitments.clone()); | ||
| let circuit_commitments_hashes = keys_to_inputs | ||
| .keys() | ||
| .map(|vk| vk.get_or_calculate_circuit_commitments_hash::<FS>(fs_parameters)) | ||
| .copied() | ||
| .collect(); | ||
| let mut sponge = Self::init_sponge(fs_parameters, &inputs_and_batch_sizes, circuit_commitments_hashes); | ||
|
|
||
| // -------------------------------------------------------------------- | ||
| // First round | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
note: this is needed, because the
CircuitVerifyingKeyhas interior mutability now; however, in its case it is perfectly fine, as the new member has no impact on theOrdimpl (which only considers theid), and the fact that the key implements it is the reason why the warning is raised; see the correspondingclippylint link.