Skip to content

chore: turn on masking in ultra and mega zk + oink clean-up #11693

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.

Sign up for GitHub

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

Merged
iakovenkos merged 5 commits into from
Feb 4, 2025
Merged

chore: turn on masking in ultra and mega zk + oink clean-up #11693

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
45688cd
ultra mega zk masking + oink clean-up
iakovenkos Feb 3, 2025
a0b2950
tests adjusted
iakovenkos Feb 3, 2025
252f83a
resolved review comments
iakovenkos Feb 4, 2025
d311330
removed unused variable
iakovenkos Feb 4, 2025
991a7eb
expanded the comment on not masking ecc op wires
iakovenkos Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions barretenberg/cpp/src/barretenberg/commitment_schemes/commitment_key.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,26 @@ template <class Curve> class CommitmentKey {

return result;
}

enum class CommitType { Default, Structured, Sparse, StructuredNonZeroComplement };

Commitment commit_with_type(PolynomialSpan<const Fr> poly,
CommitType type,
const std::vector<std::pair<size_t, size_t>>& active_ranges = {},
size_t final_active_wire_idx = 0)
{
switch (type) {
case CommitType::Structured:
return commit_structured(poly, active_ranges, final_active_wire_idx);
case CommitType::Sparse:
return commit_sparse(poly);
case CommitType::StructuredNonZeroComplement:
return commit_structured_with_nonzero_complement(poly, active_ranges, final_active_wire_idx);
case CommitType::Default:
default:
return commit(poly);
}
}
};

} // namespace bb
17 changes: 16 additions & 1 deletion barretenberg/cpp/src/barretenberg/ultra_honk/mega_honk.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,11 @@ TYPED_TEST(MegaHonkTests, Basic)
TYPED_TEST(MegaHonkTests, BasicStructured)
{
using Flavor = TypeParam;

// MegaZKFlavor is only used to prove the Hiding Circuit.

@ledwards2225 ledwards2225 Feb 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand why we're skipping these specific tests. Is it because the mechanism doesn't work in the structured trace case? Can you make this a little more explicit?

@iakovenkos iakovenkos Feb 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, agree that the original comment was misleading

I'm planning to switch the hiding circuit from Mega to MegaZK in a follow-up, will investigate how much time we spend on commitments and whether we could use structured polys there.

if constexpr (std::is_same_v<Flavor, MegaZKFlavor>) {
GTEST_SKIP() << "Skipping 'BasicStructured' test for MegaZKFlavor.";
}
typename Flavor::CircuitBuilder builder;
using Prover = UltraProver_<Flavor>;
using Verifier = UltraVerifier_<Flavor>;
Expand Down Expand Up @@ -143,6 +148,11 @@ TYPED_TEST(MegaHonkTests, BasicStructured)
TYPED_TEST(MegaHonkTests, DynamicVirtualSizeIncrease)
{
using Flavor = TypeParam;

// MegaZKFlavor is only used to prove the Hiding Circuit.
if constexpr (std::is_same_v<Flavor, MegaZKFlavor>) {
GTEST_SKIP() << "Skipping 'DynamicVirtualSizeIncrease' test for MegaZKFlavor.";
}
typename Flavor::CircuitBuilder builder;
using Prover = UltraProver_<Flavor>;
using Verifier = UltraVerifier_<Flavor>;
Expand Down Expand Up @@ -382,6 +392,11 @@ TYPED_TEST(MegaHonkTests, StructuredTraceOverflow)
TYPED_TEST(MegaHonkTests, PolySwap)
{
using Flavor = TypeParam;

// MegaZKFlavor is only used to prove the Hiding Circuit.
if constexpr (std::is_same_v<Flavor, MegaZKFlavor>) {
GTEST_SKIP() << "Skipping 'PolySwap' test for MegaZKFlavor.";
}
using Builder = Flavor::CircuitBuilder;

TraceSettings trace_settings{ SMALL_TEST_STRUCTURE_FOR_OVERFLOWS };
Expand Down Expand Up @@ -421,4 +436,4 @@ TYPED_TEST(MegaHonkTests, PolySwap)
auto proof = prover.construct_proof();
EXPECT_FALSE(verifier.verify_proof(proof));
}
}
}
104 changes: 31 additions & 73 deletions barretenberg/cpp/src/barretenberg/ultra_honk/oink_prover.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,52 +99,29 @@ template <IsUltraFlavor Flavor> void OinkProver<Flavor>::execute_wire_commitment
// We only commit to the fourth wire polynomial after adding memory recordss
{
PROFILE_THIS_NAME("COMMIT::wires");
if (proving_key->get_is_structured()) {
witness_commitments.w_l = proving_key->proving_key.commitment_key->commit_structured(
proving_key->proving_key.polynomials.w_l, proving_key->proving_key.active_region_data.get_ranges());
witness_commitments.w_r = proving_key->proving_key.commitment_key->commit_structured(
proving_key->proving_key.polynomials.w_r, proving_key->proving_key.active_region_data.get_ranges());
witness_commitments.w_o = proving_key->proving_key.commitment_key->commit_structured(
proving_key->proving_key.polynomials.w_o, proving_key->proving_key.active_region_data.get_ranges());
} else {
witness_commitments.w_l =
proving_key->proving_key.commitment_key->commit(proving_key->proving_key.polynomials.w_l);
witness_commitments.w_r =
proving_key->proving_key.commitment_key->commit(proving_key->proving_key.polynomials.w_r);
witness_commitments.w_o =
proving_key->proving_key.commitment_key->commit(proving_key->proving_key.polynomials.w_o);
}
}

auto wire_comms = witness_commitments.get_wires();
auto wire_labels = commitment_labels.get_wires();
for (size_t idx = 0; idx < 3; ++idx) {
transcript->send_to_verifier(domain_separator + wire_labels[idx], wire_comms[idx]);
commit_to_witness_polynomial(proving_key->proving_key.polynomials.w_l, commitment_labels.w_l);
commit_to_witness_polynomial(proving_key->proving_key.polynomials.w_r, commitment_labels.w_r);
commit_to_witness_polynomial(proving_key->proving_key.polynomials.w_o, commitment_labels.w_o);

@ledwards2225 ledwards2225 Feb 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this will never use the structured commit - am I missing something?

@iakovenkos iakovenkos Feb 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, fixed!

}

if constexpr (IsMegaFlavor<Flavor>) {

// Commit to Goblin ECC op wires
for (auto [commitment, polynomial, label] : zip_view(witness_commitments.get_ecc_op_wires(),
proving_key->proving_key.polynomials.get_ecc_op_wires(),
commitment_labels.get_ecc_op_wires())) {
// Commit to Goblin ECC op wires. Currently, they are not masked in MegaZKFlavor
for (auto [polynomial, label] :
zip_view(proving_key->proving_key.polynomials.get_ecc_op_wires(), commitment_labels.get_ecc_op_wires())) {
{
PROFILE_THIS_NAME("COMMIT::ecc_op_wires");
commitment = proving_key->proving_key.commitment_key->commit(polynomial);
}
transcript->send_to_verifier(domain_separator + label, commitment);
commit_to_witness_polynomial(polynomial, label);
};
}

// Commit to DataBus related polynomials
for (auto [commitment, polynomial, label] :
zip_view(witness_commitments.get_databus_entities(),
proving_key->proving_key.polynomials.get_databus_entities(),
commitment_labels.get_databus_entities())) {
for (auto [polynomial, label] : zip_view(proving_key->proving_key.polynomials.get_databus_entities(),
commitment_labels.get_databus_entities())) {
{
PROFILE_THIS_NAME("COMMIT::databus");
commitment = proving_key->proving_key.commitment_key->commit(polynomial);
commit_to_witness_polynomial(polynomial, label);
}
transcript->send_to_verifier(domain_separator + label, commitment);
}
}
}
Expand All @@ -168,27 +145,19 @@ template <IsUltraFlavor Flavor> void OinkProver<Flavor>::execute_sorted_list_acc
// Commit to lookup argument polynomials and the finalized (i.e. with memory records) fourth wire polynomial
{
PROFILE_THIS_NAME("COMMIT::lookup_counts_tags");
witness_commitments.lookup_read_counts =
proving_key->proving_key.commitment_key->commit(proving_key->proving_key.polynomials.lookup_read_counts);
witness_commitments.lookup_read_tags =
proving_key->proving_key.commitment_key->commit(proving_key->proving_key.polynomials.lookup_read_tags);
commit_to_witness_polynomial(proving_key->proving_key.polynomials.lookup_read_counts,

@ledwards2225 ledwards2225 Feb 4, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please turn on the use of commit_sparse here? Its not being used in master either but it definitely should be used

@iakovenkos iakovenkos Feb 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, I'm seeing -70ms in ClientIVCBench

commitment_labels.lookup_read_counts);

commit_to_witness_polynomial(proving_key->proving_key.polynomials.lookup_read_tags,
commitment_labels.lookup_read_tags);
}
{
PROFILE_THIS_NAME("COMMIT::wires");
if (proving_key->get_is_structured()) {
witness_commitments.w_4 = proving_key->proving_key.commitment_key->commit_structured(
proving_key->proving_key.polynomials.w_4, proving_key->proving_key.active_region_data.get_ranges());
} else {
witness_commitments.w_4 =
proving_key->proving_key.commitment_key->commit(proving_key->proving_key.polynomials.w_4);
}
auto commit_type = (proving_key->get_is_structured()) ? CommitmentKey::CommitType::Structured
: CommitmentKey::CommitType::Default;
commit_to_witness_polynomial(
proving_key->proving_key.polynomials.w_4, domain_separator + commitment_labels.w_4, commit_type);
}

transcript->send_to_verifier(domain_separator + commitment_labels.lookup_read_counts,
witness_commitments.lookup_read_counts);
transcript->send_to_verifier(domain_separator + commitment_labels.lookup_read_tags,
witness_commitments.lookup_read_tags);
transcript->send_to_verifier(domain_separator + commitment_labels.w_4, witness_commitments.w_4);
}

/**
Expand All @@ -208,24 +177,20 @@ template <IsUltraFlavor Flavor> void OinkProver<Flavor>::execute_log_derivative_

{
PROFILE_THIS_NAME("COMMIT::lookup_inverses");
witness_commitments.lookup_inverses = proving_key->proving_key.commitment_key->commit_sparse(
proving_key->proving_key.polynomials.lookup_inverses);
commit_to_witness_polynomial(proving_key->proving_key.polynomials.lookup_inverses,
commitment_labels.lookup_inverses,
CommitmentKey::CommitType::Sparse);
}
transcript->send_to_verifier(domain_separator + commitment_labels.lookup_inverses,
witness_commitments.lookup_inverses);

// If Mega, commit to the databus inverse polynomials and send
if constexpr (IsMegaFlavor<Flavor>) {
for (auto [commitment, polynomial, label] :
zip_view(witness_commitments.get_databus_inverses(),
proving_key->proving_key.polynomials.get_databus_inverses(),
commitment_labels.get_databus_inverses())) {
for (auto [polynomial, label] : zip_view(proving_key->proving_key.polynomials.get_databus_inverses(),
commitment_labels.get_databus_inverses())) {
{
PROFILE_THIS_NAME("COMMIT::databus_inverses");
commitment = proving_key->proving_key.commitment_key->commit_sparse(polynomial);
commit_to_witness_polynomial(polynomial, label, CommitmentKey::CommitType::Sparse);
}
transcript->send_to_verifier(domain_separator + label, commitment);
}
};
}
}

Expand All @@ -243,18 +208,11 @@ template <IsUltraFlavor Flavor> void OinkProver<Flavor>::execute_grand_product_c

{
PROFILE_THIS_NAME("COMMIT::z_perm");
if (proving_key->get_is_structured()) {
witness_commitments.z_perm =
proving_key->proving_key.commitment_key->commit_structured_with_nonzero_complement(
proving_key->proving_key.polynomials.z_perm,
proving_key->proving_key.active_region_data.get_ranges(),
proving_key->final_active_wire_idx + 1);
} else {
witness_commitments.z_perm =
proving_key->proving_key.commitment_key->commit(proving_key->proving_key.polynomials.z_perm);
}
auto commit_type = (proving_key->get_is_structured()) ? CommitmentKey::CommitType::StructuredNonZeroComplement
: CommitmentKey::CommitType::Default;
commit_to_witness_polynomial(
proving_key->proving_key.polynomials.z_perm, commitment_labels.z_perm, commit_type);
}
transcript->send_to_verifier(domain_separator + commitment_labels.z_perm, witness_commitments.z_perm);
}

template <IsUltraFlavor Flavor> typename Flavor::RelationSeparator OinkProver<Flavor>::generate_alphas_round()
Expand Down
35 changes: 35 additions & 0 deletions barretenberg/cpp/src/barretenberg/ultra_honk/oink_prover.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,41 @@ template <IsUltraFlavor Flavor> class OinkProver {
void execute_log_derivative_inverse_round();
void execute_grand_product_computation_round();
RelationSeparator generate_alphas_round();

private:
static void mask_witness_polynomial(Polynomial<FF>& polynomial)
{
const size_t circuit_size = polynomial.virtual_size();

@ledwards2225 ledwards2225 Feb 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constexpr check here is redundant with the one in commit_to_witness_polynomial. If you want to ensure its only used for ZK flavors you could use a concept requires

@iakovenkos iakovenkos Feb 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

if constexpr (Flavor::HasZK) {
for (size_t idx = 1; idx < MASKING_OFFSET; idx++) {
polynomial.at(circuit_size - idx) = FF::random_element();
}
}
}

/**
* @brief A uniform method to mask, commit, and send the corresponding commitment to the verifier.
*
* @param polynomial
* @param label
* @param type
*/
void commit_to_witness_polynomial(Polynomial<FF>& polynomial,

@ledwards2225 ledwards2225 Feb 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this is a bit of a nitpick but I don't love a method with a simple name that does more than it says, i.e. this doesn't just commit, it potentially adds blinding and adds the commit to the proof. I don't feel strongly though. Either way, could you move the definitions of any new methods to the .cpp file?

@iakovenkos iakovenkos Feb 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will move to cpp.

I see your point, the justification I had for myself is that commit and send_to_verifier are basically inseparable + when I added branching on ZK in each method, they became unreadable

const std::string& label,
CommitmentKey::CommitType type = CommitmentKey::CommitType::Default)
{
// Mask if needed
if constexpr (Flavor::HasZK) {
mask_witness_polynomial(polynomial);
};

typename Flavor::Commitment commitment;

commitment = proving_key->proving_key.commitment_key->commit_with_type(
polynomial, type, proving_key->proving_key.active_region_data.get_ranges());
// Send the commitment to the verifier
transcript->send_to_verifier(domain_separator + label, commitment);
}
};

using MegaOinkProver = OinkProver<MegaFlavor>;
Expand Down