-
Notifications
You must be signed in to change notification settings - Fork 324
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
fix: memory fragmentation fixes to cut UltraHonk memory usage by 26% #11895
Changes from 10 commits
7bb2457
c08599d
1290a37
a846f7a
d20a679
f0c2eac
cb0e4d9
2a96e92
f2a8816
c6047ba
6e97025
111c657
80d0631
a3491d9
a5e7bf9
c3e82b2
888b998
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 |
---|---|---|
|
@@ -278,13 +278,16 @@ TEST_F(ECCVMTranscriptTests, ProverManifestConsistency) | |
|
||
// Automatically generate a transcript manifest by constructing a proof | ||
ECCVMProver prover(builder); | ||
prover.transcript->enable_manifest(); | ||
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. explicitly enable prover manifest for this test |
||
prover.ipa_transcript->enable_manifest(); | ||
ECCVMProof proof = prover.construct_proof(); | ||
|
||
// Check that the prover generated manifest agrees with the manifest hard coded in this suite | ||
auto manifest_expected = this->construct_eccvm_honk_manifest(); | ||
auto prover_manifest = prover.transcript->get_manifest(); | ||
|
||
// Note: a manifest can be printed using manifest.print() | ||
ASSERT(manifest_expected.size() > 0); | ||
for (size_t round = 0; round < manifest_expected.size(); ++round) { | ||
ASSERT_EQ(prover_manifest[round], manifest_expected[round]) << "Prover manifest discrepency in round " << round; | ||
} | ||
|
@@ -293,6 +296,7 @@ TEST_F(ECCVMTranscriptTests, ProverManifestConsistency) | |
auto prover_ipa_manifest = prover.ipa_transcript->get_manifest(); | ||
|
||
// Note: a manifest can be printed using manifest.print() | ||
ASSERT(ipa_manifest_expected.size() > 0); | ||
for (size_t round = 0; round < ipa_manifest_expected.size(); ++round) { | ||
ASSERT_EQ(prover_ipa_manifest[round], ipa_manifest_expected[round]) | ||
<< "IPA prover manifest discrepency in round " << round; | ||
|
@@ -311,6 +315,8 @@ TEST_F(ECCVMTranscriptTests, VerifierManifestConsistency) | |
|
||
// Automatically generate a transcript manifest in the prover by constructing a proof | ||
ECCVMProver prover(builder); | ||
prover.transcript->enable_manifest(); | ||
prover.ipa_transcript->enable_manifest(); | ||
ECCVMProof proof = prover.construct_proof(); | ||
|
||
// Automatically generate a transcript manifest in the verifier by verifying a proof | ||
|
@@ -325,10 +331,20 @@ TEST_F(ECCVMTranscriptTests, VerifierManifestConsistency) | |
// The last challenge generated by the ECCVM Prover is the translation univariate batching challenge and, on the | ||
// verifier side, is only generated in the translator verifier hence the ECCVM prover's manifest will have one extra | ||
// challenge | ||
ASSERT(prover_manifest.size() > 0); | ||
for (size_t round = 0; round < prover_manifest.size() - 1; ++round) { | ||
ASSERT_EQ(prover_manifest[round], verifier_manifest[round]) | ||
<< "Prover/Verifier manifest discrepency in round " << round; | ||
} | ||
|
||
// Check consistency of IPA transcripts | ||
auto prover_ipa_manifest = prover.ipa_transcript->get_manifest(); | ||
auto verifier_ipa_manifest = verifier.ipa_transcript->get_manifest(); | ||
ASSERT(prover_ipa_manifest.size() > 0); | ||
for (size_t round = 0; round < prover_ipa_manifest.size(); ++round) { | ||
ASSERT_EQ(prover_ipa_manifest[round], verifier_ipa_manifest[round]) | ||
<< "Prover/Verifier IPA manifest discrepency in round " << round; | ||
} | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,9 @@ bool ECCVMVerifier::verify_proof(const ECCVMProof& proof) | |
RelationParameters<FF> relation_parameters; | ||
transcript = std::make_shared<Transcript>(proof.pre_ipa_proof); | ||
ipa_transcript = std::make_shared<Transcript>(proof.ipa_proof); | ||
transcript->enable_manifest(); | ||
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. just enable the manifests in the native/recursive verifiers since we shouldn't be having memory problems in those scenarios 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. its a little ugly that we have to add this though 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. it doesnt seem too bad. I dont know, my sense was that the manifest was useful but if we find that it isnt maybe we'll eventually just kill it 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. yeah its not horrible since we don't really use the manifest outside of the tests. If we ever want to test the manifest more, it shouldn't be too bad to add a few more enable_manifest()s since I'm pretty I'm missing a few in some verifiers where the manifest isn't tested. |
||
ipa_transcript->enable_manifest(); | ||
|
||
VerifierCommitments commitments{ key }; | ||
CommitmentLabels commitment_labels; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,9 +55,11 @@ void TraceToPolynomials<Flavor>::add_memory_records_to_proving_key(TraceData& tr | |
ASSERT(proving_key.memory_read_records.empty() && proving_key.memory_write_records.empty()); | ||
|
||
// Update indices of RAM/ROM reads/writes based on where block containing these gates sits in the trace | ||
proving_key.memory_read_records.reserve(builder.memory_read_records.size()); | ||
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. reserve instead of just emplace_back since we know the size of this vector |
||
for (auto& index : builder.memory_read_records) { | ||
proving_key.memory_read_records.emplace_back(index + trace_data.ram_rom_offset); | ||
} | ||
proving_key.memory_write_records.reserve(builder.memory_write_records.size()); | ||
for (auto& index : builder.memory_write_records) { | ||
proving_key.memory_write_records.emplace_back(index + trace_data.ram_rom_offset); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,10 +59,10 @@ template <IsUltraFlavor Flavor> void OinkProver<Flavor>::prove() | |
// Generate relation separators alphas for sumcheck/combiner computation | ||
proving_key->alphas = generate_alphas_round(); | ||
|
||
#ifndef __wasm__ | ||
// #ifndef __wasm__ | ||
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. will delete |
||
// Free the commitment key | ||
proving_key->proving_key.commitment_key = nullptr; | ||
#endif | ||
// #endif | ||
} | ||
|
||
/** | ||
|
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.
new function to facilitate builder deallocation before proving