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

test(ivc): test of folding W #75

Merged
merged 21 commits into from
Jan 1, 2024
Merged

test(ivc): test of folding W #75

merged 21 commits into from
Jan 1, 2024

Conversation

cyphersnake
Copy link
Collaborator

@cyphersnake cyphersnake commented Dec 21, 2023

Part of #32
Test with NIFS & IVC folding of W

@cyphersnake cyphersnake self-assigned this Dec 21, 2023
@cyphersnake cyphersnake force-pushed the 32-step-circuit-ext-5 branch from 8cc3236 to 6c98135 Compare December 25, 2023 14:56
@cyphersnake cyphersnake force-pushed the 32-step-circuit-ext-5 branch from ed275f7 to 8812362 Compare December 26, 2023 14:12
@cyphersnake
Copy link
Collaborator Author

@chaosma I decomposed TableData::assembly into three functions and the question arose as to why the prepare part is not part of new at all. I.e. what does the expected flow of usage TableData look like?

@cyphersnake cyphersnake marked this pull request as ready for review December 26, 2023 14:39
@cyphersnake cyphersnake requested a review from chaosma December 26, 2023 15:07
@cyphersnake cyphersnake enabled auto-merge (rebase) December 26, 2023 15:07
}

// TODO Change design
pub(crate) fn postpone_assembly(&mut self) {
Copy link
Collaborator

@chaosma chaosma Dec 27, 2023

Choose a reason for hiding this comment

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

what's the purpose to split fn assembly into 3 small functions? Especially, do we need a postpone method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just haven't chosen the final design for using TableData in IVC yet, but it definitely won't be an assembly function, so I've summarised all the code that isn't about halo2::Circuit

@chaosma
Copy link
Collaborator

chaosma commented Dec 27, 2023

@chaosma I decomposed TableData::assembly into three functions and the question arose as to why the prepare part is not part of new at all. I.e. what does the expected flow of usage TableData look like?

good point. We can definitely move prepare part into the fn new. The reason why it is not in new: in the very beginning, my code doesn't have prepare part which causes error; then later the fix is into the assembly instead of new.

@cyphersnake
Copy link
Collaborator Author

@chaosma In that case, I'll mv prepare part to the new
What about postpone? What the idea of that part of code?

@cyphersnake
Copy link
Collaborator Author

cyphersnake commented Dec 27, 2023

@chaosma We still need the prepare_assembly function, because in fact the function should be called every time configure over cs is called

@cyphersnake Do you mean each time configure over cs, call prepare_assembly to reset the data inside TableData?
As for the postpone, I think it depending on whether it is necessary to separate from configuration in the design of IVC

@cyphersnake
Copy link
Collaborator Author

Added docs & decided not to touch TableData right now, but to work with it together with IVC & this place

What do you think?

@cyphersnake cyphersnake requested a review from chaosma December 27, 2023 12:42
let rW = ecc.scalar_mul(region, W2, r)?;
Ok(ecc.add(region, W1, &rW)?)
let res = ecc.add(region, W1, &rW)?;
debug!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

NBD: I usually will remove debug code after testing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not dbg! this is log::debug! expression. The lowest level of logs, even lower log::trace!

It's off on a real running system because of verbosity, but for debugging some cases it can stay on.

I left it here meaningfully

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just handy for projects under active development, to include the most detailed action log possible at any time, rather than adding debug messages every time.

Such more generalised code for debugging, which at the TRACE level and above does not give any overhead

.map(|(W_index, (W1, W2))| {
let rW = best_multiexp(&[*r], &[W2]).into();
let res = *W1 + rW;
debug!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

NBD: remove debug code after debugging

@cyphersnake
Copy link
Collaborator Author

@cyphersnake Do you mean each time configure over cs, call prepare_assembly to reset the data inside TableData?
As for the postpone, I think it depending on whether it is necessary to separate from configuration in the design of IVC

Now the real use case scenario is to call TableData::assembly around halo2::Circuit. However, the next step after testing the fold chip I will be testing the IVC itself. It will include the use of TableData and there it will be clear in what form we need it.

I can do well now and generalise the TableData API as much as possible, as another option is the possibility of multiple configurations and/or tracking the life stage of TableData (create -> configure -> assembly & only in this order), I'm just afraid it's redundant for our use-case

@chaosma
Copy link
Collaborator

chaosma commented Dec 30, 2023

@cyphersnake From our conversation, you decide not to touch the TableData code? In this case, are you planning to remove the prepare and postpone code?

@cyphersnake
Copy link
Collaborator Author

you decide not to touch the TableData code

@chaosma No, what is meant is not to finalize the design changes, but to leave the refactoring as it is already in use (I need the prepare function as the tests will break without it).

Alternatively, I can update the draft of the IVC API that uses TableData and get on with the final design right now. It's just that this will be ahead of debugging all the fold-chip tests

@cyphersnake cyphersnake merged commit 463f927 into main Jan 1, 2024
1 check passed
@cyphersnake cyphersnake deleted the 32-step-circuit-ext-5 branch January 1, 2024 13:39
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