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

Prover: implements the top-level of the limitless prover #352

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

AlexandreBelling
Copy link
Contributor

This PR implements issue(s) #

Checklist

  • I wrote new tests for my new core changes.
  • I have successfully ran tests, style checker and build against my new changes locally.
  • I have informed the team of any breaking changes if there are any.

@AlexandreBelling AlexandreBelling added the Prover Tag to use for all work impacting the prover label Nov 28, 2024
@AlexandreBelling AlexandreBelling self-assigned this Nov 28, 2024
@codecov-commenter
Copy link

codecov-commenter commented Nov 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.18%. Comparing base (c356dc4) to head (54f9d33).
Report is 32 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main     #352   +/-   ##
=========================================
  Coverage     70.17%   70.18%           
  Complexity     1070     1070           
=========================================
  Files           306      306           
  Lines         12337    12338    +1     
  Branches       1179     1179           
=========================================
+ Hits           8658     8659    +1     
  Misses         3200     3200           
  Partials        479      479           
Flag Coverage Δ *Carryforward flag
hardhat 98.70% <ø> (+<0.01%) ⬆️
kotlin 67.86% <ø> (ø) Carriedforward from a8ff658

*This pull request uses carry forward flags. Click here to find out more.

see 1 file with indirect coverage changes

@Soleimani193 Soleimani193 force-pushed the prover/limitless-top-level branch from 19846ef to 16fbcd9 Compare December 2, 2024 08:49
// Analyze is responsible for letting the module discoverer compute how to
// group best the columns into modules.
Analyze(comp *wizard.CompiledIOP)
ModuleList(comp *wizard.CompiledIOP) []string
Copy link
Contributor

Choose a reason for hiding this comment

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

[]string -> []moduleName

Analyze(comp *wizard.CompiledIOP)
ModuleList(comp *wizard.CompiledIOP) []string
FindModule(col ifaces.Column) moduleName
FindNbSegments(moduleName string, SegParam map[string]int) int
Copy link
Contributor

@arijitdutta67 arijitdutta67 Dec 9, 2024

Choose a reason for hiding this comment

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

A comment describing SegParam is helpful. Considering it gives the max segment size for a given module, consider map[string]int -> map[moduleName]int.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is that a member of the module discovery interface? And what are the parameters? From the code I understand that the function returns Segparams[moduleName], you don't need a function to do that

@@ -108,6 +114,10 @@ func CompileLogDerivative(comp *wizard.CompiledIOP) {
va.Name = zC.Name
}

if comp.HasSeed {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand what comp.HasSeed has to do with how the lookup compiler works. The only important thing is how the coin are sampled but then this logic should be implemented in the coin package.

}
})
// assign the public input
run.AssignColumn(z.PI.GetColID(), sv.NewRegular([]field.Element{zSum}))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be in a dedicated logDerivative compiler object.

// Filter LPP queries
q := comp.QueriesNoParams.Data(qName)

switch v := q.(type) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since you have a switch, it would be simpler to have one function for global, one for local and one for lookup, and one for projection and one for permutation. That way you don't have to do a switch in a switch, it's nicer to read that way.

You may want to move the if disc.QueryIsInModule(v, moduleName) our of the switch as it happens in all situations

* commit all ideas
* added aggregator
* initial framework added
* added grand product query
* Added initial framework of the grand prod query
* module discoverer is added
* added test for AddGdProduct
* Prover action added
* coin in grand product test, deriveName for GP added
* Test with fragment added
* added documentation

---------

Signed-off-by: Arijit Dutta <[email protected]>
Co-authored-by: AlexandreBelling <[email protected]>
Co-authored-by: Soleimani193 <[email protected]>
* initial framework for lookup
* added logderivative_sum query
* added documentation
* getting the multiplicity table from compild iop
* added preparation for lookup
* added IntoLogDerivationSum during the preparation
* added the check for LogDerivativeSum query
* added the test for LogDerivativeSum
* moving two functions from wizardutils to the column package
* adding module discoverer
* added more methodes to module discoverer
* test for distributing LogDerivSum passes without compiling the shares of LogDerivSum
* test for distribution of the expressions passes

---------

Co-authored-by: arijitdutta67 <[email protected]>
Co-authored-by: AlexandreBelling <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Prover Tag to use for all work impacting the prover
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants