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

Start VM with initialized Sponge state #296

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jan-ferdinand
Copy link
Member

Previously, it was necessary to initialize Triton VM's Sponge state manually before any other Sponge instruction could be used. Now, instructions sponge_absorb, sponge_absorb_mem, and sponge_squeeze can be executed without first executing instruction sponge_init.

This is a breaking change because the VMState's field sponge is both public and changes type from Option<Tip5> to Tip5.

Specific reasons I have flagged you for review:

Apart from these specific questions, I'm happy to receive general ∧/∨ other feedback, of course.

@jan-ferdinand jan-ferdinand changed the title refactor!: Start VM with initialized Sponge state Start VM with initialized Sponge state May 29, 2024
@jan-ferdinand jan-ferdinand force-pushed the start_with_init_sponge branch 3 times, most recently from 09521e3 to 9a79cfd Compare May 29, 2024 10:05
Previously, it was necessary to initialize Triton VM's Sponge state
manually before any other Sponge instruction could be used. Now,
instructions `sponge_absorb`, `sponge_absorb_mem`, and `sponge_squeeze`
can be executed without first executing instruction `sponge_init`.
Copy link
Collaborator

@aszepieniec aszepieniec left a comment

Choose a reason for hiding this comment

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

I cannot say I managed to wrap my head around it, sorry. Maybe I need to sit on it for a while. Let me try again tomorrow.

@@ -435,6 +436,7 @@ impl ExtHashTable {
let if_ci_is_sponge_init_then_round_number_is_0 =
if_ci_is_sponge_init_then_.clone() * round_number.clone();

// the capacity being zero is guaranteed by the evaluation argument
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// the capacity being zero is guaranteed by the evaluation argument
// the capacity-elements being zero is guaranteed by the evaluation argument

Comment on lines +792 to +800
// If the next round number is 0, then
// - the next mode is either `Hash` or `Pad`, or
// - the next instruction is `sponge_init`, or
// - the capacity does not change, or
// - the current mode is `ProgramHashing` and (!) the next mode is `sponge`.
//
// The “and” marked with an exclamation mark requires above constraint to be
// expressed through two polynomials.
let capacity_doesnt_change_at_section_start_when_program_hashing_or_ =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think section_start is a poor choice of words. What is a section? Are there at most four sections, one for each mode? I gather (perhaps incorrectly) that section means the same as round. Why not use that word instead? Or even better: new_round.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it does mean "contiguous region of constant mode" I would suggest using instead mode_switch instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This (pair of) constraints is very confusing. Is it because it is trying to achieve several features at once? If so, consider separating it into more constraints. If mode switches from A to B then enforce C; if mode switches from X to Y then enforce Z; etc.

capacity_doesnt_change_at_section_start_when_program_hashing_or_
* Self::select_mode(circuit_builder, &mode_next, HashTableMode::Sponge);

let randomized_sum_of_state_differences =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider renaming to state_doesnt_change.

Self::round_number_deselector(circuit_builder, &round_number_next, 0)
* Self::select_mode(circuit_builder, &mode, HashTableMode::ProgramHashing)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would expect there to be a deselector for mode Sponge. Can't you transition from something other than ProgramHashing to Sponge?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The four separate modes are program_hashing, sponge, hash, and pad, and they evolve in that order.

Got it!

@@ -189,8 +157,12 @@ define `state_i_hi_limbs_minus_2_pow_32 := 2^32 - 1 - 2^16·state_i_highest_lk_i
1. If the `round_no` in the next row is 0
and the `Mode` in the next row is either `program_hashing` or `sponge`
and the instruction in the next row is either `sponge_absorb` or `sponge_init`,
then the capacity's state registers don't change.
1. If the `round_no` in the next row is 0 and the current instruction in the next row is `sponge_squeeze`, then none of the state registers change.
then the capacity's state registers don't change,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The capacity elements do not change if the next instruction is sponge_init? That doesn't sound right. I would expect them to be set to zero.

then the capacity's state registers don't change,
or the `Mode` in the current row is `program_hashing` and the `Mode` in the next row is `sponge`.
1. If the `round_no` in the next row is 0
and the current instruction in the next row is `sponge_squeeze`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

"current instruction in the next row" is confusing; consider abbreviating to "CI".

1. If the `round_no` in the next row is 0
and the current instruction in the next row is `sponge_squeeze`,
then none of the state registers change,
or the `Mode` in the current row is `program_hashing`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

So if the current mode is program_hashing and the next instruction is sponge_squeeze then the state registers can change? Very counter-intuitive.

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