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(avm): poseidon2 in vm2 #11597

Merged
merged 1 commit into from
Feb 14, 2025
Merged

Conversation

IlyasRidhuan
Copy link
Contributor

Please read contributing guidelines and remove this line.

Copy link
Contributor Author

IlyasRidhuan commented Jan 29, 2025

@IlyasRidhuan IlyasRidhuan force-pushed the ir/01-10-feat_avm_constrained_ec_add branch 2 times, most recently from 5a378e0 to d916f01 Compare January 30, 2025 19:01
@IlyasRidhuan IlyasRidhuan force-pushed the ir/01-10-feat_avm_constrained_ec_add branch from d916f01 to c176d6c Compare February 10, 2025 15:07
@IlyasRidhuan IlyasRidhuan force-pushed the ir/01-28-feat_poseidon2_in_vm2 branch 2 times, most recently from 038aa43 to 9e80f24 Compare February 10, 2025 16:23
@IlyasRidhuan IlyasRidhuan force-pushed the ir/01-10-feat_avm_constrained_ec_add branch 2 times, most recently from 35fce8a to b103a20 Compare February 12, 2025 14:03
@IlyasRidhuan IlyasRidhuan force-pushed the ir/01-28-feat_poseidon2_in_vm2 branch from 9e80f24 to 8ea0328 Compare February 12, 2025 14:10
@IlyasRidhuan IlyasRidhuan marked this pull request as ready for review February 12, 2025 14:26
@IlyasRidhuan IlyasRidhuan force-pushed the ir/01-10-feat_avm_constrained_ec_add branch from b103a20 to 7883080 Compare February 12, 2025 15:59
Copy link
Contributor

@fcarreiro fcarreiro left a comment

Choose a reason for hiding this comment

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

Will leave some more useful comments tomorrow!

// Alpha (exponentiation): 5
namespace poseidon2_params;
// Internal Matrix Diagonal (for partial rounds)
pol MU_0 = 7626475329478847982857743246276194948757851985510858890691733676098590062311;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Jean was suggesting we put these (well, for ECC) in the aztec constants so that you can use them both in PIL and C++? no strong preference from my side but worth considering

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm i think this is fine as is. They're already available in cpp through bb in nicer hex form anyways, while in PIL they need to be in this decimal format.

@IlyasRidhuan IlyasRidhuan force-pushed the ir/01-10-feat_avm_constrained_ec_add branch from 7883080 to 73f9803 Compare February 12, 2025 22:57
@IlyasRidhuan IlyasRidhuan force-pushed the ir/01-28-feat_poseidon2_in_vm2 branch 2 times, most recently from 0b4fd1c to 09bac87 Compare February 13, 2025 09:07
@IlyasRidhuan IlyasRidhuan force-pushed the ir/01-10-feat_avm_constrained_ec_add branch from 73f9803 to dece8ee Compare February 13, 2025 09:46
@IlyasRidhuan IlyasRidhuan force-pushed the ir/01-28-feat_poseidon2_in_vm2 branch from 09bac87 to 7bd3cc6 Compare February 13, 2025 09:46
Base automatically changed from ir/01-10-feat_avm_constrained_ec_add to master February 13, 2025 10:32
@IlyasRidhuan IlyasRidhuan force-pushed the ir/01-28-feat_poseidon2_in_vm2 branch from 7bd3cc6 to 2df2297 Compare February 13, 2025 16:56
Copy link
Contributor

@fcarreiro fcarreiro left a comment

Choose a reason for hiding this comment

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

Looking good in general! Leaving the constraints to Jean.

} // namespace bb::avm2::simulation
Copy link
Contributor

Choose a reason for hiding this comment

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

[Re: line +181]

nit: revert?

See this comment inline on Graphite.


std::vector<Poseidon2PermutationEvent> event_results = perm_event_emitter.dump_events();

EXPECT_EQ(result, expected);
Copy link
Contributor

Choose a reason for hiding this comment

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

You should still use a container matcher. If you want to reuse your expected variable, you can use ElementsAreArray(). Same in the other EXPECT_EQ against expected. (and in other tests).

@IlyasRidhuan IlyasRidhuan force-pushed the ir/01-28-feat_poseidon2_in_vm2 branch from 2df2297 to 3b7e83a Compare February 13, 2025 21:21
@IlyasRidhuan IlyasRidhuan force-pushed the ir/01-28-feat_poseidon2_in_vm2 branch from 3b7e83a to ee53456 Compare February 14, 2025 11:19
@IlyasRidhuan IlyasRidhuan merged commit 2c199d8 into master Feb 14, 2025
53 checks passed
@IlyasRidhuan IlyasRidhuan deleted the ir/01-28-feat_poseidon2_in_vm2 branch February 14, 2025 12:43
@fcarreiro fcarreiro changed the title feat: poseidon2 in vm2 feat(avm): poseidon2 in vm2 Feb 14, 2025
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.

2 participants