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

Poseidon2 half output #2514

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Poseidon2 half output #2514

wants to merge 10 commits into from

Conversation

lvella
Copy link
Member

@lvella lvella commented Mar 3, 2025

Of all uses we have for Poseidon2 (Merkle Tree, Sponge absorption, Sponge squeeze), none use the full 8 field elements of the output. Sponge absorption uses only the final 4, and Merkle Tree and Sponge squeeze uses only the initial 4.

This change introduces a switch for the user to select what parts of the final state to output (first half, second half, all of it or none of it).

@lvella lvella force-pushed the poseidon2_half_output branch from be59c9f to 6707734 Compare March 4, 2025 13:21
@lvella lvella changed the base branch from main to fix_else_if March 4, 2025 13:43
Base automatically changed from fix_else_if to main March 4, 2025 14:41
@lvella lvella marked this pull request as ready for review March 4, 2025 23:07
@leonardoalt leonardoalt requested a review from georgwiese March 8, 2025 13:36
@@ -39,20 +39,16 @@ machine Main with degree: main_degree {

function main {
// Store 8 field elements sequentially in memory
mstore 100, 0;
mstore 100, 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was there a reason for this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

If there was, I forgot. It feels like it had something to do with the removal of the 0x0000000100000002 case below.

Comment on lines +35 to +40
pub enum Poseidon2OutputHalf {
//None = 0,
FirstHalf = 1,
SecondHalf = 2,
//Full = 3,
}
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
pub enum Poseidon2OutputHalf {
//None = 0,
FirstHalf = 1,
SecondHalf = 2,
//Full = 3,
}
pub enum Poseidon2OutputHalf {
FirstHalf = 1,
SecondHalf = 2,
}

Copy link
Member Author

Choose a reason for hiding this comment

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

These comments act as a sort of documentation... do you really want them gone?

let output_halves;
let output_first_half = new_bool();
let output_second_half = new_bool();
output_halves = output_first_half + 2 * output_second_half;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need support for both and none? I think it's only used in the test?

If we only supported exactly one half, we could:

  • Receive output_second_half directly as input (saves a column)
  • Define let output_first_half = 1 - output_second_half (saves another column)
  • Remove 4 links to memory

Copy link
Member Author

@lvella lvella Mar 11, 2025

Choose a reason for hiding this comment

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

I tried doing this way, but I couldn't because the index of output[index] must be constant, so all 8 links are needed regardless...

Now thinking better, I could create a new array[4], and assign the values depending on the output_first_half, but then the column balance would be +3.

Would it be worth?

As for the need of the full state, I know of a theoretical one: the original form of sponge construction (not the one used in plonky3) requires the full state. But yeah, we currently don't implement that, and the only reason I can think of for ever needing it is for compatibility with some existing system.

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 this would work:

let output_first_half = 1 - output_second_half;
let output_0 = output_first_half * output[0] + output_second_half * output[4];
link if is_used ~> mem.mstore(output_addr + 0, output_time_step, output_0);

which would not introduce any extra columns, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarding whether we'll need the full state, I don't know :)

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.

3 participants