feat(avm): full poseidon2 #9141
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @IlyasRidhuan and the rest of your teammates on |
IlyasRidhuan
requested review from
jeanmon,
fcarreiro and
Maddiaa0
as code owners
IlyasRidhuan
force-pushed
the
ir/10-08-feat_avm_full_poseidon2
branch
from
32f661e
to
bcb9217
Compare
IlyasRidhuan
force-pushed
the
ir/10-08-feat_avm_full_poseidon2
branch
from
bcb9217
to
787b8ae
Compare
jeanmon
approved these changes
Oct 11, 2024
| @@ -44,4 +44,4 @@ typename Poseidon2<Params>::FF Poseidon2<Params>::hash_buffer(const std::vector< | |||
| } | |||
|
|
|||
| template class Poseidon2<Poseidon2Bn254ScalarFieldParams>; | |||
| } // namespace bb::crypto | |||
There was a problem hiding this comment.
Nitpick: convention that we keep an empty line before end of namespace
|
|
||
| // If poseidon perm is active, it must be either a mem op or immediate but not both | ||
| sel_poseidon_perm * (1 - sel_poseidon_perm_mem_op + sel_poseidon_perm_immediate) = 0; | ||
| // If inactive the mem op or immediate selectors must be 0 |
There was a problem hiding this comment.
"... the mem op or immediate" ... I think you meant "... the mem op and immediate"
| pol commit sel_poseidon_perm_immediate; | ||
| sel_poseidon_perm_immediate * (1 - sel_poseidon_perm_immediate) = 0; | ||
|
|
||
| // If poseidon perm is active, it must be either a mem op or immediate but not both |
There was a problem hiding this comment.
You can actually replace both relations by a single one:
sel_poseidon_perm = sel_poseidon_perm_mem_op + sel_poseidon_perm_immediate
Then later #[skippable_if] becomes simply:
sel_poseidon_perm = 0
| // Selector for if values are read from memory or already loaded in registers | ||
| pol commit sel_poseidon_perm_mem_op; | ||
| sel_poseidon_perm_mem_op * (1 - sel_poseidon_perm_mem_op) = 0; | ||
| pol commit sel_poseidon_perm_immediate; |
There was a problem hiding this comment.
As there is no use of sel_poseidon_perm_immediate in this file, I would mention that the selector is used in poseidon2_full.pil
| @@ -9,9 +9,20 @@ namespace poseidon2(256); | |||
| // Selector is boolean | |||
| sel_poseidon_perm * (1 - sel_poseidon_perm) = 0; | |||
|
|
|||
| // Selector for if values are read from memory or already loaded in registers | |||
There was a problem hiding this comment.
Could you elaborate a bit more? This sentence is not super clear.
|
|
||
| return current_state; | ||
| std::array<FF, 4> AvmPoseidon2TraceBuilder::poseidon2_permutation(std::array<FF, 4> const& input, |
There was a problem hiding this comment.
nitpick: we try to establish the style "const type" rather than "type const"
| { | ||
| using Poseidon2 = crypto::Poseidon2<crypto::Poseidon2Bn254ScalarFieldParams>; | ||
| FF output = Poseidon2::hash(input); | ||
| // 64 rounds of hashing should be enough (1 << 6 == 64) per full hash |
There was a problem hiding this comment.
Maybe let us introduce a constant for this 64 and some explanations (related to sub clock, etc...)
| const FF iv = (static_cast<uint256_t>(input.size()) << 64); | ||
| std::array<FF, 4> input_array = { 0, 0, 0, iv }; | ||
|
|
||
| size_t padded_size = input.size() % 3 == 0 ? input.size() : input.size() + (3 - input.size() % 3); |
There was a problem hiding this comment.
This should do the job: const size_t padded_size = 3 * ((input.size()+2) / 3)
|
|
||
| for (const auto& src : poseidon2_hash_trace) { | ||
| size_t padded_size = | ||
| src.input_length % 3 == 0 ? src.input_length : src.input_length + (3 - src.input_length % 3); |
| num_rounds_rem--; | ||
| } | ||
| // Careful - we assume here that the permutation events are in order | ||
| std::advance(perm_event, 1); |
There was a problem hiding this comment.
Isn't this equivalent to perm_event++ ?
IlyasRidhuan
force-pushed
the
ir/10-08-feat_avm_full_poseidon2
branch
from
787b8ae
to
a421532
Compare
Changes to public function bytecode sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
Changes to circuit sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Poseidon2 implementation for internal use by the avm in bytecode hashing / address derivation etc