Skip to content
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

Jump to bottom

feat: run solidity tests for all acir artifacts #3161

Merged
merged 22 commits into from
Nov 7, 2023
Merged

feat: run solidity tests for all acir artifacts #3161

merged 22 commits into from
Nov 7, 2023

Conversation

Maddiaa0
Copy link
Member

@Maddiaa0 Maddiaa0 commented Oct 31, 2023 •
edited
Loading

fixes: #3048

This pr:

  • Adds secp256k1 circuit and solidity verifier to the tests
  • Runs a solidity verifier test on ALL acir artifacts
    • Generate a proof
    • Generate a vk
    • Generate sol verification key
    • We create anvil on a random port
    • Compile the contract using solcjs to orchestrate compilation
    • Deploy the contract
    • Test the contract with the created prood

This pr builds on #3215 which fixes verifier issues ( in a blanket manner ) not directly addressing the dummy constraints issue that appears to be causing it.

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).

Maddiaa0
Maddiaa0 commented Oct 31, 2023
View reviewed changes
barretenberg/sol/src/ultra/instance/Add2UltraVerifier.sol
@@ -11,6 +11,6 @@ contract Add2UltraVerifier is BASE {
}

function loadVerificationKey(uint256 vk, uint256 _omegaInverseLoc) internal pure virtual override(BASE) {
VK.loadVerificationKey(vk, _omegaInverseLoc);
VK.loadVerificationKey(vk, _omegaInverseLoc);
Copy link
Member Author

Choose a reason for hiding this comment

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

Any changes here are due to new forge fmt rules

@Maddiaa0 Maddiaa0 marked this pull request as draft October 31, 2023 17:56
@AztecBot
Copy link
Collaborator

AztecBot commented Oct 31, 2023 •
edited
Loading

Benchmark results

No metrics with a significant change found.

Detailed results

All benchmarks are run on txs on the Benchmarking contract on the repository. Each tx consists of a batch call to create_note and increment_balance, which guarantees that each tx has a private call, a nested private call, a public call, and a nested public call, as well as an emitted private note, an unencrypted log, and public storage read and write.

This benchmark source data is available in JSON format on S3 here.

Values are compared against data from master at commit d707d4e3 and shown if the difference exceeds 1%.

L2 block published to L1

Each column represents the number of txs on an L2 block published to L1.

Metric 8 txs 32 txs 128 txs
l1_rollup_calldata_size_in_bytes 45,444 179,588 716,132
l1_rollup_calldata_gas 222,996 867,668 3,449,384
l1_rollup_execution_gas 842,083 3,594,776 22,204,753
l2_block_processing_time_in_ms 1,986 7,510 (-1%) 29,696 (-1%)
note_successful_decrypting_time_in_ms 293 (-1%) 864 (-3%) 3,126 (-3%)
note_trial_decrypting_time_in_ms 95.0 (-5%) 72.0 (-26%) 133 (-1%)
l2_block_building_time_in_ms 13,296 52,344 (-1%) 208,670 (-1%)
l2_block_rollup_simulation_time_in_ms 11,951 47,044 (-1%) 187,634 (-1%)
l2_block_public_tx_process_time_in_ms 1,306 5,163 (-2%) 20,549 (-2%)

L2 chain processing

Each column represents the number of blocks on the L2 chain where each block has 16 txs.

Metric 5 blocks 10 blocks
node_history_sync_time_in_ms 21,656 (-1%) 42,516
note_history_successful_decrypting_time_in_ms 2,010 (-1%) 3,931 (-1%)
note_history_trial_decrypting_time_in_ms 121 (+1%) 146 (+2%)
node_database_size_in_bytes 1,632,032 1,099,958
pxe_database_size_in_bytes 29,748 59,307

Circuits stats

Stats on running time and I/O sizes collected for every circuit run across all benchmarks.

Circuit circuit_simulation_time_in_ms circuit_input_size_in_bytes circuit_output_size_in_bytes
private-kernel-init 773 61,697 18,905
private-kernel-ordering 123 24,297 8,153
base-rollup 1,773 656,311 814
root-rollup 79.4 (+3%) 4,072 1,097
private-kernel-inner 789 81,568 18,905
public-kernel-private-input 41.5 (-1%) 41,519 18,841
public-kernel-non-first-iteration 26.2 (-1%) 41,497 18,841
merge-rollup 0.920 (-1%) 2,592 873

Miscellaneous

Transaction sizes based on how many contracts are deployed in the tx.

Metric 0 deployed contracts 1 deployed contracts
tx_size_in_bytes 8,787 27,547

@Maddiaa0 Maddiaa0 changed the title feat: ecdsa sig verification in aztec packages feat: ecdsa sig verification solidity test in aztec packages Oct 31, 2023
@socket-security Socket Security
Copy link

socket-security bot commented Nov 3, 2023 •
edited
Loading

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
ethers 6.8.1 network, filesystem +4 13.5 MB ricmoo
solc 0.8.22 filesystem, shell +4 8.81 MB cameel

@Maddiaa0 Maddiaa0 mentioned this pull request Nov 3, 2023
Closed
@Maddiaa0 Maddiaa0 changed the title feat: ecdsa sig verification solidity test in aztec packages feat: run solidity tests for all acir artifacts Nov 4, 2023
@Maddiaa0 Maddiaa0 marked this pull request as ready for review November 4, 2023 02:35
@Maddiaa0 Maddiaa0 enabled auto-merge (squash) November 4, 2023 02:38
LHerskind
LHerskind reviewed Nov 4, 2023
View reviewed changes
Copy link
Contributor

@LHerskind LHerskind left a comment

Choose a reason for hiding this comment

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

Mostly nits related to formatting.

Some of the tests seems to be failing though.

barretenberg/acir_tests/sol-test/src/index.js Outdated
const readPublicInputs = (proofAsFields) => {
const publicInputs = [];
// A proof with no public inputs is 93 fields long
const numPublicInputs = proofAsFields.length - 93;
Copy link
Contributor

Choose a reason for hiding this comment

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

Make a constant for the 93, someday we will have forgotten and try use it for a different scheme or something. Also just easier to follow.

barretenberg/acir_tests/sol-test/src/index.js Outdated
const launchAnvil = async (port) => {
const handle = spawn("anvil", ["-p", port]);

// wait until the anvil instance is ready on port 8545
Copy link
Contributor

Choose a reason for hiding this comment

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

Not on port 8545, but on port instead right?

barretenberg/acir_tests/sol-test/src/index.js
handle.stderr.on("data", (data) => {
const str = data.toString();
if (str.includes("error binding")) {
reject("we go again baby")
Copy link
Contributor

Choose a reason for hiding this comment

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

I like. Skibbidy.

barretenberg/acir_tests/sol-test/src/index.js Outdated

/**
*
* @param {number} numPublicInputs
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems stale as you compute it from the number of fields

barretenberg/acir_tests/sol-test/src/index.js Outdated
*
* @param {number} numPublicInputs
* @param {Array<String>} proofAsFields
* @returns {Array<Tuple<Array<String>,number>}
Copy link
Contributor

Choose a reason for hiding this comment

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

Also looks a little different from what I see below.

barretenberg/cpp/src/barretenberg/solidity_helpers/circuits/ecdsa_circuit.hpp
}

// This is the message that we would like to confirm
std::string message_string = "goblin";
Copy link
Contributor

Choose a reason for hiding this comment

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

barretenberg/cpp/src/barretenberg/solidity_helpers/circuits/ecdsa_circuit.hpp Show resolved Hide resolved
barretenberg/cpp/src/barretenberg/solidity_helpers/key_gen.cpp Outdated
info("Only blake, add2 and recursive circuits are supported at the moment");
return 1;
}
info("Only blake, add2 and recursive circuits are supported at the moment");
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably change this to just be "unsupported circuit" so we don't have to keep extending it with specific names.

barretenberg/sol/src/ultra/BaseUltraVerifier.sol
addmod(y1y2, y1y2, p),
p
),
addmod(sub(p, addmod(y2_sqr, y1_sqr, p)), addmod(y1y2, y1y2, p), p),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a forge fmt or how come formatting changed in here? 👀

barretenberg/sol/test/ultra/ECDSA.t.sol Outdated
verifier = IVerifier(address(new EcdsaUltraVerifier()));
fuzzer = fuzzer.with_circuit_flavour(DifferentialFuzzer.CircuitFlavour.Ecdsa);

// Does the noir code do this?
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this question be an issue or?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, i was thinking this could be a debugging issue will remove

@Maddiaa0
Copy link
Member Author

Maddiaa0 commented Nov 4, 2023

Mostly nits related to formatting.

Some of the tests seems to be failing though.

I've got some concurrency issues to sort out there

@Maddiaa0 Maddiaa0 requested a review from LHerskind November 6, 2023 14:53
LHerskind
LHerskind approved these changes Nov 7, 2023
View reviewed changes
Copy link
Contributor

@LHerskind LHerskind left a comment

Choose a reason for hiding this comment

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

Good job @Maddiaa0 👍

@Maddiaa0 Maddiaa0 merged commit d09f667 into master Nov 7, 2023
2 checks passed
@Maddiaa0 Maddiaa0 deleted the md/ecdsa branch November 7, 2023 15:40
@AztecBot AztecBot mentioned this pull request Nov 7, 2023
Merged
rahul-kothari pushed a commit that referenced this pull request Nov 7, 2023
@AztecBot
chore(master): Release 0.14.2 (#3266)
2da5feb 
🤖 I have created a release *beep* *boop*
---


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

##
[0.14.2](aztec-packages-v0.14.1...aztec-packages-v0.14.2)
(2023-11-07)


### Features

* Load private tests and docs
([#3243](#3243))
([f3d8aae](f3d8aae)),
closes
[#1285](#1285)
* Run solidity tests for all acir artifacts
([#3161](#3161))
([d09f667](d09f667))


### Bug Fixes

* Wait for accounts to catch up with notes when deployed
([#2834](#2834))
([a8f3119](a8f3119))


### Miscellaneous

* Add noir-protocol-circuits to deploy_npm
([#3268](#3268))
([1a22cae](1a22cae))
* Aztec-cli better volume mounting strategy
([#3138](#3138))
([d40460e](d40460e))
* Disable circuits tasks
([#3253](#3253))
([e8945f8](e8945f8))
</details>

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

##
[0.14.2](barretenberg.js-v0.14.1...barretenberg.js-v0.14.2)
(2023-11-07)


### Features

* Run solidity tests for all acir artifacts
([#3161](#3161))
([d09f667](d09f667))
</details>

<details><summary>barretenberg: 0.14.2</summary>

##
[0.14.2](barretenberg-v0.14.1...barretenberg-v0.14.2)
(2023-11-07)


### Features

* Run solidity tests for all acir artifacts
([#3161](#3161))
([d09f667](d09f667))
</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 Nov 8, 2023
@AztecBot
chore(master): Release 0.14.2 (#3266)
c223a22 
🤖 I have created a release *beep* *boop*
---


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

##
[0.14.2](AztecProtocol/aztec-packages@aztec-packages-v0.14.1...aztec-packages-v0.14.2)
(2023-11-07)


### Features

* Load private tests and docs
([#3243](AztecProtocol/aztec-packages#3243))
([f3d8aae](AztecProtocol/aztec-packages@f3d8aae)),
closes
[#1285](AztecProtocol/aztec-packages#1285)
* Run solidity tests for all acir artifacts
([#3161](AztecProtocol/aztec-packages#3161))
([d09f667](AztecProtocol/aztec-packages@d09f667))


### Bug Fixes

* Wait for accounts to catch up with notes when deployed
([#2834](AztecProtocol/aztec-packages#2834))
([a8f3119](AztecProtocol/aztec-packages@a8f3119))


### Miscellaneous

* Add noir-protocol-circuits to deploy_npm
([#3268](AztecProtocol/aztec-packages#3268))
([1a22cae](AztecProtocol/aztec-packages@1a22cae))
* Aztec-cli better volume mounting strategy
([#3138](AztecProtocol/aztec-packages#3138))
([d40460e](AztecProtocol/aztec-packages@d40460e))
* Disable circuits tasks
([#3253](AztecProtocol/aztec-packages#3253))
([e8945f8](AztecProtocol/aztec-packages@e8945f8))
</details>

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

##
[0.14.2](AztecProtocol/aztec-packages@barretenberg.js-v0.14.1...barretenberg.js-v0.14.2)
(2023-11-07)


### Features

* Run solidity tests for all acir artifacts
([#3161](AztecProtocol/aztec-packages#3161))
([d09f667](AztecProtocol/aztec-packages@d09f667))
</details>

<details><summary>barretenberg: 0.14.2</summary>

##
[0.14.2](AztecProtocol/aztec-packages@barretenberg-v0.14.1...barretenberg-v0.14.2)
(2023-11-07)


### Features

* Run solidity tests for all acir artifacts
([#3161](AztecProtocol/aztec-packages#3161))
([d09f667](AztecProtocol/aztec-packages@d09f667))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
@Savio-Sou Savio-Sou mentioned this pull request Nov 22, 2023
Closed
@Savio-Sou
Copy link
Member

Also resolved #3127?

@Maddiaa0
Copy link
Member Author

Maddiaa0 commented Nov 23, 2023 •
edited
Loading

Yep; Ill close it out,

image

Running through bb.js is fine, which leads me to believe it is probably the wrapper that is causing issues now, but ill have to look into it soon. I dont think i have the capacity right now

@Maddiaa0 Maddiaa0 mentioned this pull request Nov 23, 2023
Closed
@Savio-Sou Savio-Sou mentioned this pull request Nov 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LHerskind LHerskind LHerskind approved these changes

@charlielye charlielye Awaiting requested review from charlielye

Assignees
No one assigned
Labels
None yet
Projects
A3
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add ECDSA secp256k1 verification Solidity tests
4 participants
@Maddiaa0 @AztecBot @Savio-Sou @LHerskind