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

absorb instances in folding #2198

Merged
merged 19 commits into from
May 17, 2024
Merged

absorb instances in folding #2198

merged 19 commits into from
May 17, 2024

Conversation

fabrizio-m
Copy link
Contributor

No description provided.

@dannywillems dannywillems self-requested a review May 8, 2024 20:11
let extended_commitments = self.extended.iter().flat_map(|commit| {
assert_eq!(commit.elems.len(), 1);
let point = commit.elems[0];
//this may need change if we need to support the infinity point
Copy link
Member

Choose a reason for hiding this comment

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

Why this case? Can we not simply use absorb_commitment, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could, but I was not sure about its implementation, and is one extra thing to duplicate in IVC, but I have no issue with changing it if you think is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

another option is to change the signature to allow only one field, and leave it up to the trait implementation to decompose and represent the points in the native field.

@@ -237,6 +237,10 @@ impl<'a, CF: FoldingConfig> FoldingScheme<'a, CF> {
fq_sponge.absorb_g(&error_commitments[0].elems);
fq_sponge.absorb_g(&error_commitments[1].elems);

let to_absorb = env.to_absorb();
Copy link
Member

@dannywillems dannywillems May 8, 2024

Choose a reason for hiding this comment

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

Can you indicate the order of the absorption in the documentation, please? We need to be consistent for the IVC circuit, and it is good to document it.
We should have a test checking the order is in line with the documentation.

Copy link
Member

Choose a reason for hiding this comment

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

Note that the typed transcript could be used, see #2094.
The idea is that every "round" (compute_error, compute_extended, etc) sends a message type, that can be absorb.
Each function returns a type "Message", and the sponge gets to absorb a heterogeneous list of "absorbable messages".
The heterogeneous list can be implemented as Nil, Cons constructors.

But I suppose it will be for the future. For now, we can keep it simple, and write in comments the order.

@dannywillems dannywillems marked this pull request as draft May 9, 2024 11:46
Copy link
Member

@dannywillems dannywillems left a comment

Choose a reason for hiding this comment

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

Converting to draft + see comments for the requests regarding changes.
I'm not sure zero_vec/zero_commitments should be parts of the code, and I suppose it makes the code fails.

@dannywillems
Copy link
Member

Any update @fabrizio-m?

@fabrizio-m fabrizio-m marked this pull request as ready for review May 16, 2024 15:16
@@ -82,6 +95,16 @@ impl<G: CommitmentCurve, I: Instance<G>> ExtendedInstance<G, I> {
extended: vec![],
}
}
/// wraps the inner absorb, adding its own commitments
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// wraps the inner absorb, adding its own commitments
/// Return the elements to be absorbed by the sponge
///
/// The commitments to the additional columns created by quadriticization
/// are appended to the existing commitments of the initial instance
/// to be absorbed. The scalars are unchanged.

Co-authored-by: Danny Willems <be.danny.willems@gmail.com>
Copy link
Member

@dannywillems dannywillems left a comment

Choose a reason for hiding this comment

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

Let's have a test checking how many scalars and commitments are absorbed. Let's not care about the actual values for now.

Copy link
Member

@dannywillems dannywillems left a comment

Choose a reason for hiding this comment

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

I will accept for now, for the interface, and the test.
If the number of values to be absorbed is wrong, I suggest fixing after when we add the IVC library.

@dannywillems dannywillems merged commit dc43c7d into master May 17, 2024
6 checks passed
@dannywillems dannywillems deleted the fabrizio-m/absorb-in-folding branch May 17, 2024 17:15
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