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: add blake2b compression precompile #321

Closed
wants to merge 16 commits into from

Conversation

0xkanekiken
Copy link
Contributor

@0xkanekiken 0xkanekiken commented Feb 27, 2024

The implementation is based on reference #180 and partially addresses issue #231. In the Blake2b hash function, the state and message elements are represented as u64 values, whereas the sp1 standard supports u32 words. As a result, each u64 is implemented using two u32 limbs, with appropriate handling for this format.

Furthermore, two operations for u64 data types have been introduced:

  • Fixed right shift for u64.
  • Addition for u64.

The XOR operation for u64 can be derived from the XOR operations of individual u32 limbs.

To-Do:

  • Add tests.
  • Integrate with sp1.

Signed-off-by: 0xKanekiKen <100861945+0xKanekiKen@users.noreply.github.com>
Signed-off-by: 0xKanekiKen <100861945+0xKanekiKen@users.noreply.github.com>
Signed-off-by: 0xkanekiken <100861945+0xKanekiKen@users.noreply.github.com>
Signed-off-by: 0xkanekiken <100861945+0xKanekiKen@users.noreply.github.com>
Signed-off-by: 0xkanekiken <100861945+0xKanekiKen@users.noreply.github.com>
Signed-off-by: 0xkanekiken <100861945+0xKanekiKen@users.noreply.github.com>
Signed-off-by: 0xkanekiken <100861945+0xKanekiKen@users.noreply.github.com>
Signed-off-by: 0xkanekiken <100861945+0xKanekiKen@users.noreply.github.com>
@0xkanekiken 0xkanekiken changed the title Add blake2b compression precompile feat: add blake2b compression precompile Feb 28, 2024
Signed-off-by: 0xkanekiken <100861945+0xKanekiKen@users.noreply.github.com>
Signed-off-by: 0xkanekiken <100861945+0xKanekiKen@users.noreply.github.com>
Signed-off-by: 0xkanekiken <100861945+0xKanekiKen@users.noreply.github.com>
@0xkanekiken
Copy link
Contributor Author

encountered the following verification error and need to figure out how to resolve it

image

@kevjue
Copy link
Contributor

kevjue commented Feb 29, 2024

@0xkanekiken - I see that your PR is modeled off of the Blake3 precompile. I recently fixed a bug in that precompile which I believe also affects this PR.

Signed-off-by: 0xkanekiken <100861945+0xKanekiKen@users.noreply.github.com>
@0xkanekiken
Copy link
Contributor Author

@kevjue- thank you. I had the same question when I was taking a look at the constraints of blake3. However, I noticed that the round_index was updated to ROUND_COUNT, which seemed to make the constraint valid. In fact, the tests for blake3 were also passing.

pad_rows(&mut rows, || {
let mut row = [F::zero(); NUM_BLAKE3_COMPRESS_INNER_COLS];
let cols: &mut Blake3CompressInnerCols<F> = row.as_mut_slice().borrow_mut();
// Put this value in this padded row to avoid failing the constraint.
cols.round_index = F::from_canonical_usize(ROUND_COUNT);
row
});

I attempted to incorporate the fix you introduced, but I am still encountering the NonZeroCumulativeSum error. Is there a more systematic approach to evaluating all the AIR and determining whether it satisfies the constraints or not?

@kevjue
Copy link
Contributor

kevjue commented Feb 29, 2024

@0xkanekiken - Ya, the fix PR removed the need for setting round_index. That was there to deal with the very last real row of blake3 where it expected the next (which would be a padded row) row to have a value of round_index + 1, which would be ROUND_COUNT.

Regarding the NonZeroCumulativeSum issue, that means that something is inconsistent with your interactions (a term we use for communication between chips).

You can try adding this function (

pub fn debug_interactions_with_all_chips<SC: StarkGenericConfig<Val = BabyBear>>(
) in your test case which will output any inconsistencies. Here is an example of how to use it:
fn test_memory_lookup_interactions() {
.

I actually suspect that all your constraints in your blake2b air related to the blake logic are fine. You normally encounter the NonZeroCumulativeSum after the constraints for the AIR are checked. You may want to check your memory constraints as that uses interactions.

Signed-off-by: 0xkanekiken <100861945+0xKanekiKen@users.noreply.github.com>
@0xkanekiken
Copy link
Contributor Author

Thank you for all the guidance and support! Indeed, the problem stemmed from how I defined the memory access AIR constraints. I've addressed this issue in 54a416e. Consequently, the tests are passing, and I'm able to generate the proof. It's ready for a review now.

@0xkanekiken 0xkanekiken marked this pull request as ready for review February 29, 2024 23:37
@kevjue
Copy link
Contributor

kevjue commented Mar 1, 2024

Thanks for the PR @0xkanekiken ! In general, it looks great! But we'll take a closer look at it soon.

core/src/operations/addu64.rs Outdated Show resolved Hide resolved
}

// The expected output state is the result of compress_inner.
let output_state: [u8; 128] = [
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be possible to import a pure rust implementation of blake2b in this program and then get the output_state from calling that so that we have a ground truth reference for the code?

core/src/syscall/precompiles/blake2b/compress/trace.rs Outdated Show resolved Hide resolved
core/src/syscall/precompiles/blake2b/compress/mod.rs Outdated Show resolved Hide resolved
core/src/syscall/precompiles/blake2b/compress/mod.rs Outdated Show resolved Hide resolved
core/src/operations/addu64.rs Outdated Show resolved Hide resolved
core/src/operations/addu64.rs Outdated Show resolved Hide resolved
core/src/operations/addu64.rs Outdated Show resolved Hide resolved
core/src/operations/fixed_rotate_rightu64.rs Outdated Show resolved Hide resolved
impl Blake2bCompressInnerChip {
/// Constrains the given index is correct for the given selector. The `selector` is an
/// `n`-dimensional boolean array whose `i`-th element is true if and only if the index is `i`.
fn constrain_index_selector<AB: SP1AirBuilder>(
Copy link
Contributor

Choose a reason for hiding this comment

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

do we not already have a generic thing of doing this? this seems like a really good utility to have in airbuilder in general tbh, so maybe we should move it to a move general crate with all the existing air builder methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I address this issue in the current PR, or should it be handled in a separate PR by creating an issue?

@0xkanekiken 0xkanekiken force-pushed the kaneki-add-blake3b branch 2 times, most recently from 02bbcb0 to dae5554 Compare March 5, 2024 21:25
Signed-off-by: 0xkanekiken <100861945+0xKanekiKen@users.noreply.github.com>
@puma314
Copy link
Contributor

puma314 commented Jun 11, 2024

At some point, we should merge a PR like this for the blake2b precompile if there is further demand, but for now it's not high priority and this PR is quite out of date so closing for now.

@puma314 puma314 closed this Jun 11, 2024
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.

4 participants