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

[WIP] Mandatory degree #2097

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

[WIP] Mandatory degree #2097

wants to merge 2 commits into from

Conversation

georgwiese
Copy link
Collaborator

@georgwiese georgwiese commented Nov 15, 2024

Idea: After machine instantiation the type of the degree changes from MachineDegree (optional min / max) to NamspaceDegree (mandatory min / max).

Requires more changes in the test: in many tests, we rely on a to-be-removed linker feature (if the main degree is static, it is used for all sub-machines). Instead, we should always set the degree range at instantiation time.

@@ -22,16 +22,16 @@ mod utils {

machine Main with degree: 8 {
// use a machine from another module by relative path
my_module::Other a;
my_module::Other a(8, 8);
Copy link
Member

Choose a reason for hiding this comment

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

This started as a one-day-before-the-realease-hack and now it's mandatory?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my understanding, the hacky part is the syntax around how degrees are passed to sub-machines. I don't think that we want to pass degree ranges is controversial?

Copy link
Member

Choose a reason for hiding this comment

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

For me it is, shouldn't it somehow at least have a default? What do I care about the degree of a sub-machine, unless I already know that I will call it every single row like the memory machine?

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 think it's hard to pick a good default heuristically. It depends on the block size and how often we expect to call the machine, and both the user would know better than us.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Basically,

  • The status quo (pre Remove env var MAX_DEGREE_LOG #2092) is that the default is min_degree: 2**5, max_degree: 2**MAX_DEGREE_LOG, where MAX_DEGREE_LOG is an environment variable. But using env variables is weird.
  • With Remove env var MAX_DEGREE_LOG #2092, we fail if the degree range is unknown after the linker. This is already kind of making it mandatory to set the range, except that we have a strange rule in the linker: If the degree of the main machine is static, use that degree for all sub-machines (even if it's already been set differently). That rule is also weird and should be removed IMO. It also doesn't have any effect if the main machine is dynamically sized, which should be the case in most real-world applications.

Copy link
Member

Choose a reason for hiding this comment

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

The problem I see with having default ones is that only very large defaults are safe enough, but with that you make the life of any user that doesn't need it worse because fixed columns gen and setup time will be massive

Copy link
Member

Choose a reason for hiding this comment

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

Why not generate the fixed columns only once they are needed? Because of the verification key?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, exactly, we only know what's needed at runtime, but at that time, we already need to have committed to the fixed columns in all possible sizes.

Base automatically changed from test-degree to main November 15, 2024 20:01
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