Skip to content

fix: skip default pp in sol aggregation#20521

Open
iakovenkos wants to merge 3 commits intomerge-train/barretenbergfrom
si/fix-pp-in-sol
Open

fix: skip default pp in sol aggregation#20521
iakovenkos wants to merge 3 commits intomerge-train/barretenbergfrom
si/fix-pp-in-sol

Conversation

@iakovenkos
Copy link
Contributor

after switching from default on-curve pp (P_0, P_1) to (0,0) sol would fail at on-curve validation step for these values, skip aggregation step if default value is provided

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mostly the changes from regeneration

Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont like that we can get so out of date

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we uncommit these?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The smallest move, if we do want to manually check these diffs, is to add a test that it gets generated the same way

Fr recursionSeparator = generateRecursionSeparator(proof.pairingPointObject, P_0_agg, P_1_agg);
(Honk.G1Point memory P_0_other, Honk.G1Point memory P_1_other) =
convertPairingPointsToG1(proof.pairingPointObject);
// Aggregate pairing points (skip if default/infinity — no recursive verification occurred)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New logic here

Fr recursionSeparator = generateRecursionSeparator(proof.pairingPointObject, pair.P_0, pair.P_1);
(Honk.G1Point memory P_0_other, Honk.G1Point memory P_1_other) =
convertPairingPointsToG1(proof.pairingPointObject);
// Aggregate pairing points (skip if default/infinity — no recursive verification occurred)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New logic here

[[maybe_unused]] const auto clear_tag = OriginTag::constant(); /* A tag representing a constant value */ \
[[maybe_unused]] const auto constant_tag = OriginTag::constant(); /* Alias for clear_tag */ \
const auto submitted_value_origin_tag = OriginTag( \
[[maybe_unused]] const auto submitted_value_origin_tag = OriginTag( \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ludamad thx for the suggestion!

@iakovenkos iakovenkos marked this pull request as ready for review February 14, 2026 20:35
@iakovenkos iakovenkos self-assigned this Feb 14, 2026
@iakovenkos iakovenkos added the ci-full Run all master checks. label Feb 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-full Run all master checks.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants