-
Notifications
You must be signed in to change notification settings - Fork 271
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: Single commitment key allocation in CIVC #9974
Changes from all commits
cbca890
4e33cfd
4b73307
b60289a
7abaea9
8b6cad4
fa7b6a0
c1c93bb
4e56cd2
a4d9bc7
dd14697
3e36b05
043f787
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 |
---|---|---|
|
@@ -176,10 +176,11 @@ void ClientIVC::accumulate(ClientCircuit& circuit, const std::shared_ptr<Verific | |
proving_key = std::make_shared<DeciderProvingKey>(circuit, trace_settings); | ||
trace_usage_tracker = ExecutionTraceUsageTracker(trace_settings); | ||
} else { | ||
proving_key = std::make_shared<DeciderProvingKey>( | ||
circuit, trace_settings, fold_output.accumulator->proving_key.commitment_key); | ||
proving_key = std::make_shared<DeciderProvingKey>(circuit, trace_settings); | ||
} | ||
|
||
proving_key->proving_key.commitment_key = bn254_commitment_key; | ||
|
||
vinfo("getting honk vk... precomputed?: ", precomputed_vk); | ||
// Update the accumulator trace usage based on the present circuit | ||
trace_usage_tracker.update(circuit); | ||
|
@@ -278,7 +279,7 @@ HonkProof ClientIVC::construct_and_prove_hiding_circuit() | |
MergeProof merge_proof = goblin.prove_merge(builder); | ||
merge_verification_queue.emplace_back(merge_proof); | ||
|
||
auto decider_pk = std::make_shared<DeciderProvingKey>(builder); | ||
auto decider_pk = std::make_shared<DeciderProvingKey>(builder, TraceSettings(), bn254_commitment_key); | ||
honk_vk = std::make_shared<VerificationKey>(decider_pk->proving_key); | ||
MegaProver prover(decider_pk); | ||
|
||
|
@@ -338,6 +339,7 @@ bool ClientIVC::verify(const Proof& proof) | |
HonkProof ClientIVC::decider_prove() const | ||
{ | ||
vinfo("prove decider..."); | ||
fold_output.accumulator->proving_key.commitment_key = bn254_commitment_key; | ||
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 didn't want to do this but IIRC a bigger refactor would be needed to construct the initial accumulator with this key (it starts out as a nullptr). |
||
MegaDeciderProver decider_prover(fold_output.accumulator); | ||
return decider_prover.construct_proof(); | ||
vinfo("finished decider proving."); | ||
|
@@ -352,11 +354,19 @@ HonkProof ClientIVC::decider_prove() const | |
bool ClientIVC::prove_and_verify() | ||
{ | ||
auto start = std::chrono::steady_clock::now(); | ||
auto proof = prove(); | ||
const auto proof = prove(); | ||
auto end = std::chrono::steady_clock::now(); | ||
auto diff = std::chrono::duration_cast<std::chrono::milliseconds>(end - start); | ||
vinfo("time to call ClientIVC::prove: ", diff.count(), " ms."); | ||
return verify(proof); | ||
|
||
start = end; | ||
const bool verified = verify(proof); | ||
end = std::chrono::steady_clock::now(); | ||
|
||
diff = std::chrono::duration_cast<std::chrono::milliseconds>(end - start); | ||
vinfo("time to verify ClientIVC proof: ", diff.count(), " ms."); | ||
|
||
return verified; | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,8 +98,6 @@ class ClientIVC { | |
using ProverFoldOutput = FoldingResult<Flavor>; | ||
|
||
public: | ||
GoblinProver goblin; | ||
|
||
ProverFoldOutput fold_output; // prover accumulator and fold proof | ||
|
||
std::shared_ptr<DeciderVerificationKey> verifier_accumulator; // verifier accumulator | ||
|
@@ -122,11 +120,19 @@ class ClientIVC { | |
// Setting auto_verify_mode = true will cause kernel completion logic to be added to kernels automatically | ||
bool auto_verify_mode; | ||
|
||
std::shared_ptr<typename MegaFlavor::CommitmentKey> bn254_commitment_key; | ||
|
||
GoblinProver goblin; | ||
|
||
bool initialized = false; // Is the IVC accumulator initialized | ||
|
||
ClientIVC(TraceSettings trace_settings = {}, bool auto_verify_mode = false) | ||
: trace_settings(trace_settings) | ||
, auto_verify_mode(auto_verify_mode) | ||
, bn254_commitment_key(trace_settings.structure.has_value() | ||
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. when would the structure not have value? 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. As things are set up we don't always use the structuring. This used to be encoded with an enum but now it's encoded with an optional. |
||
? std::make_shared<CommitmentKey<curve::BN254>>(trace_settings.dyadic_size()) | ||
: nullptr) | ||
, goblin(bn254_commitment_key) | ||
{} | ||
|
||
void instantiate_stdlib_verification_queue( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -236,7 +236,7 @@ WASM_EXPORT void acir_prove_and_verify_aztec_client(uint8_t const* acir_stack, | |
} | ||
// TODO(#7371) dedupe this with the rest of the similar code | ||
// TODO(https://github.com/AztecProtocol/barretenberg/issues/1101): remove use of auto_verify_mode | ||
ClientIVC ivc{ { E2E_FULL_TEST_STRUCTURE }, /*auto_verify_mode=*/true }; | ||
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. Bigger structure not necessary for any tests of this code. |
||
ClientIVC ivc{ { CLIENT_IVC_BENCH_STRUCTURE }, /*auto_verify_mode=*/true }; | ||
|
||
// Accumulate the entire program stack into the IVC | ||
// TODO(https://github.com/AztecProtocol/barretenberg/issues/1116): remove manual setting of is_kernel once databus | ||
|
@@ -267,6 +267,10 @@ WASM_EXPORT void acir_prove_and_verify_aztec_client(uint8_t const* acir_stack, | |
bool result = ivc.prove_and_verify(); | ||
info("verified?: ", result); | ||
|
||
end = std::chrono::steady_clock::now(); | ||
diff = std::chrono::duration_cast<std::chrono::milliseconds>(end - start); | ||
vinfo("time to construct, accumulate, prove and verify all circuits: ", diff.count()); | ||
|
||
*verified = result; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,6 +51,7 @@ class GoblinProver { | |
*/ | ||
|
||
std::shared_ptr<OpQueue> op_queue = std::make_shared<OpQueue>(); | ||
std::shared_ptr<CommitmentKey<curve::BN254>> commitment_key; | ||
|
||
MergeProof merge_proof; | ||
GoblinProof goblin_proof; | ||
|
@@ -70,11 +71,12 @@ class GoblinProver { | |
GoblinAccumulationOutput accumulator; // Used only for ACIR methods for now | ||
|
||
public: | ||
GoblinProver() | ||
GoblinProver(const std::shared_ptr<CommitmentKey<curve::BN254>>& bn254_commitment_key = nullptr) | ||
{ // Mocks the interaction of a first circuit with the op queue due to the inability to currently handle zero | ||
// commitments (https://github.com/AztecProtocol/barretenberg/issues/871) which would otherwise appear in the | ||
// first round of the merge protocol. To be removed once the issue has been resolved. | ||
GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit(op_queue); | ||
commitment_key = bn254_commitment_key ? bn254_commitment_key : nullptr; | ||
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, pointer/nullptr are truthy with values true/false, and I think this is clean and widely used. 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. why not just |
||
GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit(op_queue, commitment_key); | ||
} | ||
/** | ||
* @brief Construct a MegaHonk proof and a merge proof for the present circuit. | ||
|
@@ -160,7 +162,7 @@ class GoblinProver { | |
merge_proof_exists = true; | ||
} | ||
|
||
MergeProver merge_prover{ circuit_builder.op_queue }; | ||
MergeProver merge_prover{ circuit_builder.op_queue, commitment_key }; | ||
return merge_prover.construct_proof(); | ||
}; | ||
|
||
|
@@ -209,7 +211,7 @@ class GoblinProver { | |
|
||
auto translator_builder = | ||
std::make_unique<TranslatorBuilder>(translation_batching_challenge_v, evaluation_challenge_x, op_queue); | ||
translator_prover = std::make_unique<TranslatorProver>(*translator_builder, transcript); | ||
translator_prover = std::make_unique<TranslatorProver>(*translator_builder, transcript, commitment_key); | ||
} | ||
|
||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,8 +59,15 @@ template <typename T> struct MegaTraceBlockData { | |
}; | ||
} | ||
|
||
static uint32_t size() { return 0; } | ||
static uint32_t dyadic_size() { return 0; } | ||
size_t size() const | ||
requires std::same_as<T, uint32_t> | ||
{ | ||
size_t result{ 0 }; | ||
for (const auto& block_size : get()) { | ||
result += block_size; | ||
} | ||
return static_cast<size_t>(result); | ||
} | ||
|
||
bool operator==(const MegaTraceBlockData& other) const = default; | ||
}; | ||
|
@@ -72,6 +79,15 @@ struct TraceSettings { | |
// The size of the overflow block. Specified separately because it is allowed to be determined at runtime in the | ||
// context of VK computation | ||
uint32_t overflow_capacity = 0; | ||
|
||
size_t size() const { return structure->size() + static_cast<size_t>(overflow_capacity); } | ||
|
||
size_t dyadic_size() const | ||
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. Might also want a dyadic size on the structure but it wasn't needed. |
||
{ | ||
const size_t total_size = size(); | ||
const size_t lower_dyadic = 1 << numeric::get_msb(total_size); | ||
return total_size > lower_dyadic ? lower_dyadic << 1 : lower_dyadic; | ||
} | ||
}; | ||
|
||
class MegaTraceBlock : public ExecutionTraceBlock<fr, /*NUM_WIRES_ */ 4, /*NUM_SELECTORS_*/ 14> { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,8 +58,10 @@ template <IsUltraFlavor Flavor> void OinkProver<Flavor>::prove() | |
// Generate relation separators alphas for sumcheck/combiner computation | ||
proving_key->alphas = generate_alphas_round(); | ||
|
||
#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. what's the point of this? Seems like it won't be freed anyway if since ClientIVC is storing a shared_ptr to this. I guess for UH in wasm, it will no longer be freed then 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. You raise a good point--I wasn't thinking of UH WASM performance here. However, this optimization of freeing and reallocating the ck was only tested to work in x86, so you can think of this macro as almost being documentation of that fact. In the future we should have a PR that specifically tunes WASM memory performance of UH and yeah, perhaps an optimization at that point will require something different here. |
||
// Free the commitment key | ||
proving_key->proving_key.commitment_key = nullptr; | ||
#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.
A lot of the PR is just threading the CIVC class-level ck through.