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(avm): Integrate public inputs in AVM recursive verifier #8846

Merged
merged 11 commits into from
Sep 28, 2024
Merged

feat(avm): Integrate public inputs in AVM recursive verifier #8846

merged 11 commits into from
Sep 28, 2024

Conversation

jeanmon
Copy link
Contributor

@jeanmon jeanmon commented Sep 27, 2024

Resolves #8714

@jeanmon jeanmon changed the title Integrate public inputs in AVM recursive verifier feat(avm): Integrate public inputs in AVM recursive verifier Sep 27, 2024
@jeanmon jeanmon marked this pull request as ready for review September 27, 2024 08:25
@jeanmon jeanmon force-pushed the jm/8714-avm-recursive-public-inputs branch from dfca084 to 71c6c5b Compare September 27, 2024 11:55
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 27, 2024 •
edited
Loading

Changes to public function bytecode sizes

Generated at commit: 302fee9e2cb6cb81457463e09431a1f6e7eaab66, compared to commit: 02821d0fb3000537aa8001a00d93c74af3003cc2

🧾 Summary (100% most significant diffs)

Program Bytecode size in bytes (+/-) %
Lending::public_dispatch +3,680 ❌ +4.48%
AvmTest::public_dispatch +119 ❌ +0.12%
CardGame::public_dispatch +20 ❌ +0.07%
Uniswap::public_dispatch +21 ❌ +0.06%
TokenBridge::public_dispatch +7 ❌ +0.02%
Token::public_dispatch +14 ❌ +0.02%
TokenBlacklist::public_dispatch +14 ❌ +0.01%
NFT::public_dispatch -35 ✅ -0.08%
Auth::public_dispatch -28 ✅ -0.08%
FPC::public_dispatch -28 ✅ -0.25%
Full diff report 👇
Program Bytecode size in bytes (+/-) %
Lending::public_dispatch 85,829 (+3,680) +4.48%
AvmTest::public_dispatch 97,635 (+119) +0.12%
CardGame::public_dispatch 29,782 (+20) +0.07%
Uniswap::public_dispatch 33,713 (+21) +0.06%
TokenBridge::public_dispatch 32,353 (+7) +0.02%
Token::public_dispatch 72,452 (+14) +0.02%
TokenBlacklist::public_dispatch 145,847 (+14) +0.01%
NFT::public_dispatch 43,299 (-35) -0.08%
Auth::public_dispatch 33,597 (-28) -0.08%
FPC::public_dispatch 11,379 (-28) -0.25%

@jeanmon jeanmon force-pushed the jm/8714-avm-recursive-public-inputs branch from aff7b91 to 627228f Compare September 27, 2024 12:54
@jeanmon jeanmon force-pushed the jm/8714-avm-recursive-public-inputs branch 2 times, most recently from 502446e to 165b793 Compare September 27, 2024 14:30
codygunton
codygunton approved these changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@codygunton codygunton left a comment

Choose a reason for hiding this comment

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

Looks good. I manually reconstructed the diff on evaluate_mle to see nothing significant changed across the move of the code. I make some small comments too.

barretenberg/cpp/src/barretenberg/dsl/acir_format/avm_recursion_constraint.cpp Outdated Show resolved Hide resolved
barretenberg/cpp/src/barretenberg/polynomials/polynomial.hpp Outdated Show resolved Hide resolved
barretenberg/cpp/src/barretenberg/stdlib/primitives/bool/bool.cpp
@@ -438,12 +438,6 @@ template <typename Builder> bool_t<Builder> bool_t<Builder>::implies(const bool_
return (!(*this) || other); // P => Q is equiv. to !P || Q (not(P) or Q).
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this go away because it's incorrect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment seemed to have been duplicated (maybe wrong merge/rebase resolution). The very same comment is on top of the next method and in this case the comment makes sense. Here, I did not see the point and I was misled until I realized that it is a duplicated comment.

@jeanmon jeanmon force-pushed the jm/8714-avm-recursive-public-inputs branch 2 times, most recently from e6bdbfb to 3c53781 Compare September 27, 2024 16:37
@jeanmon jeanmon force-pushed the jm/8714-avm-recursive-public-inputs branch from 3c53781 to 7b087d6 Compare September 28, 2024 09:18
@jeanmon jeanmon enabled auto-merge (squash) September 28, 2024 09:19
@jeanmon jeanmon merged commit 4354ae0 into master Sep 28, 2024
51 checks passed
@jeanmon jeanmon deleted the jm/8714-avm-recursive-public-inputs branch September 28, 2024 10:17
@AztecBot AztecBot mentioned this pull request Sep 28, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fcarreiro fcarreiro fcarreiro left review comments

@codygunton codygunton codygunton approved these changes

@IlyasRidhuan IlyasRidhuan Awaiting requested review from IlyasRidhuan IlyasRidhuan is a code owner

@Maddiaa0 Maddiaa0 Awaiting requested review from Maddiaa0 Maddiaa0 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Integrate public inputs to AVM recursive verifier
3 participants
@jeanmon @fcarreiro @codygunton