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.

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: Support contract package type for the info command #2088

Closed
wants to merge 9 commits into from

Conversation

Ethan-000
Copy link
Contributor

@Ethan-000 Ethan-000 commented Jul 29, 2023

Description

Problem*

Resolves #1970

Summary*

fn main(x : Field, y : pub Field) {
    assert(x * 2 == y * 3);
}

contract Foo {
    fn double(x: Field) -> pub Field { x * 2 }
    fn triple(x: Field) -> pub Field { x * 3 }
    internal fn quadruple(x: Field) -> pub Field { x * 4 }
    open internal fn skibbidy(x: Field) -> pub Field { x * 5 }
}

run nargo info --contracts in the contracts test in test_data now returns

1

run nargo info --contract Foo returns

2

run nargo info --contract F returns

3

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

This does not look very good imho so this issue is opened to address this and others #2089

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@Ethan-000 Ethan-000 changed the title feat: Addnargo info --contracts command to count contract gates feat: Add nargo info --contracts command to count contract gates Jul 29, 2023
@Ethan-000 Ethan-000 changed the title feat: Add nargo info --contracts command to count contract gates feat: Add nargo info --contracts and nargo info --contract <CONTRACT>command to count contract gates Jul 29, 2023
@iAmMichaelConnor
Copy link
Collaborator

Thanks so much for this!

If I may, it'd be even sexier if the data could be tabulated, e.g. using the prettytable crate (or something).

A more advanced question: for an ultraplonk backend, tables are used in some computations (e.g. hashing). Now the 'circuit size' has three interesting pieces of information:

  • the number of gates/constraints (as is shown in your lovely examples above)
  • the total number of 'rows' in all plookup tables
  • the next largest power of two, which is greater than max(# gates, # table rows).

As an extreme example, a circuit might only have 10 gates, but could include a table which is 10,000 rows. Then the proving time will be greatly affected by these 10,000 rows. The important metric for proving time is then: 2^14 = 16,384.
I.e. 2.pow(ceiling(log_2(max(# gates, # rows)))

@iAmMichaelConnor
Copy link
Collaborator

Whoops, you beat me to it, re formatting :) #2089

@Ethan-000
Copy link
Contributor Author

Thanks so much for this!

If I may, it'd be even sexier if the data could be tabulated, e.g. using the prettytable crate (or something).

A more advanced question: for an ultraplonk backend, tables are used in some computations (e.g. hashing). Now the 'circuit size' has three interesting pieces of information:

* the number of gates/constraints (as is shown in your lovely examples above)

* the total number of 'rows' in all plookup tables

* the next largest power of two, which is greater than `max(# gates, # table rows)`.

As an extreme example, a circuit might only have 10 gates, but could include a table which is 10,000 rows. Then the proving time will be greatly affected by these 10,000 rows. The important metric for proving time is then: 2^14 = 16,384. I.e. 2.pow(ceiling(log_2(max(# gates, # rows)))

ahh thanks for suggestion 🙂 will checkout this crate I think a table should be better formatting than printing

about getting the circuit size. maybe this is the correct function to use https://github.com/AztecProtocol/barretenberg/blob/b22b8d5f42ddfae140068c3ce8b3053d4c8d1874/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.hpp#L849-L861
I don't know if the current Noir backend use it though. probably should check this

@iAmMichaelConnor
Copy link
Collaborator

@codygunton might be able to point you to the relevant functions. That one looks like it might be correct, for giving you the value max(# gates, # table rows).

As for whether the Noir backend uses this (and is 'allowed' to make a call to get this information, without sacrificing 'backend agnosticism'), someone on the Noir team will need to help you there :)

@vezenovm
Copy link
Contributor

vezenovm commented Jul 31, 2023

As for whether the Noir backend uses this (and is 'allowed' to make a call to get this information, without sacrificing 'backend agnosticism'), someone on the Noir team will need to help you there :)

So nargo info fetches the exact number of gates/constraints, using get_num_gates in the composer. We expose get_total_circuit_size (which gives max(# gates, # table rows)) to compute the subgroup size of the proving key and circuit. It sounds like it would more useful for you to have the total circuit size number when working with UP. @iAmMichaelConnor

@iAmMichaelConnor
Copy link
Collaborator

I'd be in favour of providing all the numbers. As much useful info as we can be given :)

@phated
Copy link
Contributor

phated commented Jul 31, 2023

Hey @Ethan-000, this should probably be implemented on top of #2075 because it will make contracts work with each command. I'm taking a look at it this week.

@phated phated marked this pull request as draft July 31, 2023 15:06
@Ethan-000
Copy link
Contributor Author

Hey @Ethan-000, this should probably be implemented on top of #2075 because it will make contracts work with each command. I'm taking a look at it this week.

sounds good :)

@kevaundray
Copy link
Contributor

@phated whats the status #2075 ? If it is not completed, then I'd opt for reviewing and merging this in and when that PR lands, it rebases off of this

@phated
Copy link
Contributor

phated commented Aug 7, 2023

whats the status #2075 ? If it is not completed, then I'd opt for reviewing and merging this in and when that PR lands, it rebases off of this

I'm trying to answer some design questions right now, but hoping to have something up by end of tomorrow. We could merge this, but then there'd be a "breaking" change that only affects people that built a compiler between this PR and my PR (since mine will be removing both flags) and it seems like unnecessary/confusing churn.

@phated
Copy link
Contributor

phated commented Aug 8, 2023

I created #2204 to unblock this without churn.

@TomAFrench
Copy link
Member

We should also add support for contracts in nargo check. We can't produce a Prover.toml file but we can check that the Noir is syntactically correct.

@kevaundray
Copy link
Contributor

@Ethan-000 #2204 is getting merged, can you rebase and mark as ready when you are ready for a review

@phated phated changed the title feat: Add nargo info --contracts and nargo info --contract <CONTRACT>command to count contract gates feat: Support contract package type for the info command Aug 8, 2023
@phated
Copy link
Contributor

phated commented Aug 8, 2023

Updated the title to reflect the implementation that needs to happen 🙇

@Ethan-000
Copy link
Contributor Author

reopened #2249

@Ethan-000 Ethan-000 closed this Aug 10, 2023
@Ethan-000 Ethan-000 deleted the e/contract_info branch August 27, 2023 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make nargo gates work with contract functions
6 participants