Skip to content

fix: add missing ecc doubling gate into ultra plonk and ultra honk #2610

Merged
zac-williamson merged 16 commits into
masterfrom
zw/add_double_gate_to_ultra_plonk
Oct 3, 2023
Merged

fix: add missing ecc doubling gate into ultra plonk and ultra honk #2610
zac-williamson merged 16 commits into
masterfrom
zw/add_double_gate_to_ultra_plonk

Conversation

@zac-williamson

@zac-williamson zac-williamson commented Oct 1, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR #1945 added a new selector into the Ultra arithmetisation (elliptic curve point doubling). However this change was not propagated to the polynomial relations evaluated by the UltraPlonk and UltraHonk Prover/Verifier algorithms.

This PR fixes this, as well as upgrades the BaseUltraVerifier.sol contract to use the new gate.

Checklist:

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

  • If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
  • I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
  • Every change is related to the PR description.
  • I have linked this pull request to relevant issues (if any exist).

@zac-williamson zac-williamson changed the title bug fix: add missing ecc doubling gate into ultra plonk and ultra honk fix: add missing ecc doubling gate into ultra plonk and ultra honk Oct 1, 2023
kevaundray
kevaundray reviewed Oct 3, 2023
View reviewed changes
Comment thread barretenberg/cpp/src/barretenberg/proof_system/relations/ultra_relation_consistency.test.cpp Outdated
FF& q_arith = std::get<6>(_data);
FF& q_sort = std::get<7>(_data);
FF& q_elliptic = std::get<8>(_data);
FF& q_double = std::get<9>(_data);

@kevaundray kevaundray Oct 3, 2023

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.

why is this here and why does it have the same index as q_aux?

@zac-williamson zac-williamson Oct 3, 2023

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.

whoops I need to remove this

kevaundray
kevaundray reviewed Oct 3, 2023
View reviewed changes
Comment thread barretenberg/cpp/src/barretenberg/solidity_helpers/circuits/recursive_circuit.hpp

return (result == pairing_target_field::one());
}
return true;

@kevaundray kevaundray Oct 3, 2023

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.

To confirm; if builder.contains_recursive_proof and there are only 15 proof elements for example, this will return true -- is that what we want?

@zac-williamson zac-williamson Oct 3, 2023

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.

I think this needs a separate issue - I copied this code from stdlib_recursion so it exists elsewhere

@zac-williamson zac-williamson Oct 3, 2023

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.

actually I'll just fix. If it's not 16 that's a problem

Maddiaa0
Maddiaa0 reviewed Oct 3, 2023
View reviewed changes

@Maddiaa0 Maddiaa0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ive only checked the solidity for equiv with elliptic relation and it looks good, have minor nits about updating / adding new relation formulas in the sol.

Comment thread barretenberg/sol/src/ultra/BaseUltraVerifier.sol Outdated
error EC_SCALAR_MUL_FAILURE();
error PROOF_FAILURE();

bytes4 internal constant ERR_S = 0xf7b44074;

@Maddiaa0 Maddiaa0 Oct 2, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can just emit the message

@zac-williamson zac-williamson Oct 3, 2023

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. I removed this code as it was just debug code that shouldn't have been there anyways

Comment thread barretenberg/sol/src/ultra/BaseUltraVerifier.sol Outdated
* y_double_identity = x_1_sqr_mul_3 * (x_1 - x_3) - (y_1 + y_1) * (y_1 + y_3);
*/
{
let x1_sqr := mulmod(mload(X1_EVAL_LOC), mload(X1_EVAL_LOC), p)

@Maddiaa0 Maddiaa0 Oct 2, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can save some gas throughout by duping on stack instead of mloading twice

@zac-williamson zac-williamson Oct 3, 2023

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.

yup. I was focusing on just getting a relatively clear implementation down. We can make a separate issue to optimise if this is something we want to tackle

@zac-williamson zac-williamson Oct 3, 2023

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.

Added an issue for this #2636

Comment thread barretenberg/sol/src/ultra/BaseUltraVerifier.sol Outdated
batch_evaluation := addmod(batch_evaluation, mulmod(mload(C_V13_LOC), mload(QARITH_EVAL_LOC), p), p)
batch_evaluation := addmod(batch_evaluation, mulmod(mload(C_V14_LOC), mload(QSORT_EVAL_LOC), p), p)
batch_evaluation := addmod(batch_evaluation, mulmod(mload(C_V15_LOC), mload(QELLIPTIC_EVAL_LOC), p), p)
batch_evaluation := addmod(batch_evaluation, mulmod(mload(C_V30_LOC), mload(QDOUBLE_EVAL_LOC), p), p)

@Maddiaa0 Maddiaa0 Oct 2, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: using chall 30 here sticks out, could 16 be used and the rest bumped

@zac-williamson zac-williamson Oct 3, 2023

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.

yeah it was odd. Later changes removed the need for this entirely

Comment thread barretenberg/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.cpp
w_2_value + q_m_value * w_2_shifted_value,
w_3_value + q_c_value * w_3_shifted_value,
q_3_value))) {
#ifndef FUZZING

@Maddiaa0 Maddiaa0 Oct 2, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

was the removal of all of these for debugging or intentional?

@zac-williamson zac-williamson Oct 3, 2023

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.

accidental. I've added back in

Comment thread barretenberg/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.hpp Outdated
template <typename CircuitBuilder> bool is_same_state(const CircuitBuilder& builder)
{
if (!(public_inputs == builder.public_inputs)) {
std::cout << "pubinp" << std::endl;

@Maddiaa0 Maddiaa0 Oct 2, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Debugging info?

@zac-williamson zac-williamson Oct 3, 2023

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 for spotting, removed

Comment thread barretenberg/sol/src/ultra/BaseUltraVerifier.sol
* sign_term += sign_term
* sign_term *= q_sign
*/
let x_diff := addmod(mload(X2_EVAL_LOC), sub(p, mload(X1_EVAL_LOC)), p)

@Maddiaa0 Maddiaa0 Oct 3, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

    // q_elliptic * (x3 + x2 + x1)(x2 - x1)(x2 - x1) - y2^2 - y1^2 + 2(y2y1)*q_sign = 0

@zac-williamson zac-williamson Oct 3, 2023

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.

added

Comment thread barretenberg/sol/src/ultra/BaseUltraVerifier.sol
mload(C_ALPHA_BASE_LOC),
p
)

@Maddiaa0 Maddiaa0 Oct 3, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

    // q_elliptic * (x3 + x2 + x1)(x2 - x1)(x2 - x1) - y2^2 - y1^2 + 2(y2y1)*q_sign = 0

@zac-williamson zac-williamson Oct 3, 2023

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.

added

Comment thread barretenberg/sol/src/ultra/BaseUltraVerifier.sol
* x_double_identity = (x_3 + x_1 + x_1) * y_1_sqr_mul_4 - x_1_pow_4_mul_9;
* y_double_identity = x_1_sqr_mul_3 * (x_1 - x_3) - (y_1 + y_1) * (y_1 + y_3);
*/
let x1_sqr := mulmod(mload(X1_EVAL_LOC), mload(X1_EVAL_LOC), p)

@Maddiaa0 Maddiaa0 Oct 3, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

    // (x3 + x1 + x1) (4y1*y1) - 9 * x1 * x1 * x1 * x1 = 0

@zac-williamson zac-williamson Oct 3, 2023

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.

added

Comment thread barretenberg/sol/src/ultra/BaseUltraVerifier.sol
p
)
leftovers :=
let y_double_identity :=

@Maddiaa0 Maddiaa0 Oct 3, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

    // (y1 + y1) (2y1) - (3 * x1 * x1)(x1 - x3) = 0

@zac-williamson zac-williamson Oct 3, 2023

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.

added

Comment thread barretenberg/sol/src/ultra/BaseUltraVerifier.sol Outdated
*/
let x1_sqr := mulmod(mload(X1_EVAL_LOC), mload(X1_EVAL_LOC), p)
let y1_sqr := mulmod(mload(Y1_EVAL_LOC), mload(Y1_EVAL_LOC), p)
let x_pow_4 := mulmod(addmod(y1_sqr, 17, p), mload(X1_EVAL_LOC), p)

@Maddiaa0 Maddiaa0 Oct 3, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

b here could probably be a constant, rather than a magic 17

@zac-williamson zac-williamson Oct 3, 2023

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.

good point. I've added 17 as a constant

kevaundray
kevaundray approved these changes Oct 3, 2023
View reviewed changes

@kevaundray kevaundray left a comment

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.

Left some nits, but overall looks fine to me

@zac-williamson zac-williamson merged commit 7cb7c58 into master Oct 3, 2023
@zac-williamson zac-williamson deleted the zw/add_double_gate_to_ultra_plonk branch October 3, 2023 15:43
@AztecBot AztecBot mentioned this pull request Oct 3, 2023
Merged
@vezenovm vezenovm mentioned this pull request Oct 3, 2023
Merged
5 tasks
kevaundray pushed a commit that referenced this pull request Oct 3, 2023
@AztecBot
chore(master): Release 0.8.1 (#2640)
0731311 
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.8.1</summary>

##
[0.8.1](aztec-packages-v0.8.0...aztec-packages-v0.8.1)
(2023-10-03)


### Bug Fixes

* Add missing ecc doubling gate into ultra plonk and ultra honk
([#2610](#2610))
([7cb7c58](7cb7c58))
* Benchmark script fixes for master branch
([#2638](#2638))
([0a161a4](0a161a4))
* Redirect sunset instructions
([#2646](#2646))
([9253442](9253442))
* Remove -u from build_wasm script so that we can skip the build when
SKIP_CPP_BUILD is unset
([#2649](#2649))
([84b8ff4](84b8ff4))


### Miscellaneous

* **benchmark:** Measure block sync time
([#2637](#2637))
([d11343f](d11343f))
* Update acir_tests script to point to master
([#2650](#2650))
([51d1e79](51d1e79))
</details>

<details><summary>barretenberg.js: 0.8.1</summary>

##
[0.8.1](barretenberg.js-v0.8.0...barretenberg.js-v0.8.1)
(2023-10-03)


### Bug Fixes

* Remove -u from build_wasm script so that we can skip the build when
SKIP_CPP_BUILD is unset
([#2649](#2649))
([84b8ff4](84b8ff4))
</details>

<details><summary>barretenberg: 0.8.1</summary>

##
[0.8.1](barretenberg-v0.8.0...barretenberg-v0.8.1)
(2023-10-03)


### Bug Fixes

* Add missing ecc doubling gate into ultra plonk and ultra honk
([#2610](#2610))
([7cb7c58](7cb7c58))


### Miscellaneous

* Update acir_tests script to point to master
([#2650](#2650))
([51d1e79](51d1e79))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
AztecBot added a commit to AztecProtocol/barretenberg that referenced this pull request Oct 5, 2023
@AztecBot
chore(master): Release 0.8.1 (#2640)
dc6de42 
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.8.1</summary>

##
[0.8.1](AztecProtocol/aztec-packages@aztec-packages-v0.8.0...aztec-packages-v0.8.1)
(2023-10-03)


### Bug Fixes

* Add missing ecc doubling gate into ultra plonk and ultra honk
([#2610](AztecProtocol/aztec-packages#2610))
([7cb7c58](AztecProtocol/aztec-packages@7cb7c58))
* Benchmark script fixes for master branch
([#2638](AztecProtocol/aztec-packages#2638))
([0a161a4](AztecProtocol/aztec-packages@0a161a4))
* Redirect sunset instructions
([#2646](AztecProtocol/aztec-packages#2646))
([9253442](AztecProtocol/aztec-packages@9253442))
* Remove -u from build_wasm script so that we can skip the build when
SKIP_CPP_BUILD is unset
([#2649](AztecProtocol/aztec-packages#2649))
([84b8ff4](AztecProtocol/aztec-packages@84b8ff4))


### Miscellaneous

* **benchmark:** Measure block sync time
([#2637](AztecProtocol/aztec-packages#2637))
([d11343f](AztecProtocol/aztec-packages@d11343f))
* Update acir_tests script to point to master
([#2650](AztecProtocol/aztec-packages#2650))
([51d1e79](AztecProtocol/aztec-packages@51d1e79))
</details>

<details><summary>barretenberg.js: 0.8.1</summary>

##
[0.8.1](AztecProtocol/aztec-packages@barretenberg.js-v0.8.0...barretenberg.js-v0.8.1)
(2023-10-03)


### Bug Fixes

* Remove -u from build_wasm script so that we can skip the build when
SKIP_CPP_BUILD is unset
([#2649](AztecProtocol/aztec-packages#2649))
([84b8ff4](AztecProtocol/aztec-packages@84b8ff4))
</details>

<details><summary>barretenberg: 0.8.1</summary>

##
[0.8.1](AztecProtocol/aztec-packages@barretenberg-v0.8.0...barretenberg-v0.8.1)
(2023-10-03)


### Bug Fixes

* Add missing ecc doubling gate into ultra plonk and ultra honk
([#2610](AztecProtocol/aztec-packages#2610))
([7cb7c58](AztecProtocol/aztec-packages@7cb7c58))


### Miscellaneous

* Update acir_tests script to point to master
([#2650](AztecProtocol/aztec-packages#2650))
([51d1e79](AztecProtocol/aztec-packages@51d1e79))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Maddiaa0 pushed a commit that referenced this pull request Oct 6, 2023
@zac-williamson @vezenovm
@kevaundray @Maddiaa0
fix: add missing ecc doubling gate into ultra plonk and ultra honk (#…
7bcbfa7 
…2610)

PR #1945 added a new selector into the Ultra arithmetisation (elliptic
curve point doubling). However this change was not propagated to the
polynomial relations evaluated by the UltraPlonk and UltraHonk
Prover/Verifier algorithms.

This PR fixes this, as well as upgrades the BaseUltraVerifier.sol
contract to use the new gate.

# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [x] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [x] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [x] Every change is related to the PR description.
- [x] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).

---------

Co-authored-by: vezenovm <mvezenov@gmail.com>
Co-authored-by: kevaundray <kevtheappdev@gmail.com>
Maddiaa0 pushed a commit that referenced this pull request Oct 6, 2023
@AztecBot @Maddiaa0
chore(master): Release 0.8.1 (#2640)
1501345 
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.8.1</summary>

##
[0.8.1](aztec-packages-v0.8.0...aztec-packages-v0.8.1)
(2023-10-03)


### Bug Fixes

* Add missing ecc doubling gate into ultra plonk and ultra honk
([#2610](#2610))
([7cb7c58](7cb7c58))
* Benchmark script fixes for master branch
([#2638](#2638))
([0a161a4](0a161a4))
* Redirect sunset instructions
([#2646](#2646))
([9253442](9253442))
* Remove -u from build_wasm script so that we can skip the build when
SKIP_CPP_BUILD is unset
([#2649](#2649))
([84b8ff4](84b8ff4))


### Miscellaneous

* **benchmark:** Measure block sync time
([#2637](#2637))
([d11343f](d11343f))
* Update acir_tests script to point to master
([#2650](#2650))
([51d1e79](51d1e79))
</details>

<details><summary>barretenberg.js: 0.8.1</summary>

##
[0.8.1](barretenberg.js-v0.8.0...barretenberg.js-v0.8.1)
(2023-10-03)


### Bug Fixes

* Remove -u from build_wasm script so that we can skip the build when
SKIP_CPP_BUILD is unset
([#2649](#2649))
([84b8ff4](84b8ff4))
</details>

<details><summary>barretenberg: 0.8.1</summary>

##
[0.8.1](barretenberg-v0.8.0...barretenberg-v0.8.1)
(2023-10-03)


### Bug Fixes

* Add missing ecc doubling gate into ultra plonk and ultra honk
([#2610](#2610))
([7cb7c58](7cb7c58))


### Miscellaneous

* Update acir_tests script to point to master
([#2650](#2650))
([51d1e79](51d1e79))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Maddiaa0 Maddiaa0 Maddiaa0 left review comments

+1 more reviewer

@kevaundray kevaundray kevaundray approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

No open projects
A3
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zac-williamson @kevaundray @Maddiaa0 @vezenovm