Skip to content

feat(noir): separate arguments of inputs - #456

Merged
sirasistant merged 4 commits into
masterfrom
arv/separate_args
May 5, 2023
Merged

sirasistant merged 4 commits into
masterfrom
arv/separate_args

Conversation

@sirasistant

Copy link
Copy Markdown
Contributor

Description

Closes #455

Extracts the arguments from the inputs struct in noir contracts. For now they have to be manually pushed to the context.
Updates the postprocessor to omit the Inputs & CallContext, allowing to avoid writing ABI comments in the noir code.

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

@sirasistant sirasistant self-assigned this May 4, 2023

@spalladino spalladino 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.

Looks good! Great that we have the open keyword now.

_call_context: pub CallContext,
amount: pub Field,
recipient: pub Point,
open fn mint(

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.

Whoa!


/// ABI value type "secret"
/// ABI value params [{"name":"input","type":{"kind":"field"},"visibility":"public"}]
/// ABI value return [{"kind":"field"}]

@spalladino spalladino May 4, 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.

Shouldn't we keep the return annotation at least? Can the client consume this value?

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.

Users currently cannot consume the return value for secret functions! The idea is to phase out the ABI manual comments for the introduction of the aztec noir compiler 😄

@sirasistant
sirasistant merged commit 7b5f2bf into master May 5, 2023
@sirasistant
sirasistant deleted the arv/separate_args branch May 5, 2023 07:16
ludamad added a commit that referenced this pull request Jul 14, 2023
codygunton pushed a commit that referenced this pull request Jan 23, 2024
rangozd pushed a commit to rangozd/aztec-packages that referenced this pull request Aug 5, 2026
* fix 1: pin honk_key_gen Solidity VK emission to flavor entity count

Audit finding AztecProtocol#1. The Solidity VK generator hand-wrote the precomputed-G1
emission list with no link to the flavor, so adding/removing a precomputed
entity would silently drift the generated on-chain VK out of sync with the
C++ layout. Drive the emission count from a (commitment, name) array and
static_assert it against the flavor's size(); add a test asserting the
generator emits exactly size() G1 points.

* fix 3: set beta_quartic in RelationParameters::compute_beta_powers

Audit finding AztecProtocol#3. compute_beta_powers() set beta..beta_cube but left
beta_quartic at 0, so the test-only get_random() built
eccvm_set_permutation_delta with a zero domain-separation tag instead of the
production FIRST_TERM_TAG * beta^4. Set beta_quartic = beta_cube * beta so
test-infra parameters match the eccvm relation semantics.

* fix 4: reject wrong-size field input in VK deserialization

Audit finding AztecProtocol#4. NativeVerificationKey_::from_field_elements and the
StdlibVerificationKey_ span constructor consumed only the fields they needed
and silently ignored trailing ones, so an oversize (malformed) VK deserialized
without error. Assert the input field count matches the VK layout exactly on
both paths so a malformed VK is rejected rather than masked.

* fix 6: add [[nodiscard]] to create_recursion_constraints

Audit finding AztecProtocol#6. create_recursion_constraints returns the aggregated pairing
points and IPA claim the caller must deferred-accumulate to complete recursive
verification, but unlike its inner helper create_honk_recursion_constraints it
carried no [[nodiscard]] -- a caller could silently drop the result and skip
accumulation with no diagnostic. Add the attribute, matching the inner helper.

* fix 7: assert that hasZK and masking layout are consistent.

* fix: extend gemini-masking-layout assert to remaining ZK flavors

Finding AztecProtocol#7's gemini_masking_layout_consistent<> static_assert was only on
Ultra/UltraZK/Mega/MegaZKFlavor. UltraKeccakZKFlavor, UltraZKRecursiveFlavor and
MegaZKRecursiveFlavor hand-declare both the masking flag and the AllValues layout,
so they can drift independently the same way -- MegaZKRecursiveFlavor most acutely,
since its HasGeminiMasking=false is decoupled from HasZK=true. Add the assert to
all three; all are currently consistent, so this is pure hardening.
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.

Add arguments as public parameters in noir functions and in the function push them onto the parameters field in the ABI

2 participants