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

Add ChunkedPippenger to variable-base MSM #364

Merged
merged 18 commits into from
Dec 16, 2021

Conversation

huyuncong
Copy link
Member

@huyuncong huyuncong commented Dec 10, 2021

Description

ChunkedPippenger is a core algorithm for streaming MSM.
We could add streaming MSM on top of it after the stream utils are imported.
The test of this algorithm is added into tests macros in test-templates.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

@weikengchen
Copy link
Member

Interesting: internal compiler error: unexpected panic
This is often due to the nightly being unstable.

The other bug: https://github.com/arkworks-rs/algebra/runs/4478158408?check_suite_focus=true
This could be fixed by using the full name of the struct.

@@ -3,6 +3,8 @@ mod variable_base;
pub use fixed_base::*;
pub use variable_base::*;

pub mod stream_pippenger;
Copy link
Member

Choose a reason for hiding this comment

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

I would move this into variable_base, since it's a variable-base MSM

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the core internal algorithm of streaming MSM. But we do have another streaming MSM function for public use that has not been imported yet because it relies on streaming utilities. The plan is to first decide which repo to put streaming utilities and then import streaming MSM into variable-base MSM.

Copy link
Member

Choose a reason for hiding this comment

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

Pratyush, I think here involves a decision, that is where to put this newer stream variable-base MSM---it clearly belongs to variable base. Does it replace the original variable base algorithm? If not, how should we name then?

Copy link
Member

Choose a reason for hiding this comment

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

I mean that we add this algorithm as another file in the variable_base module, instead of making it a top-level member of the msm module.

@Pratyush
Copy link
Member

A micro-optimization that you might want to consider is to build up a buffer of results, and then sum that buffer up in parallel.

@huyuncong
Copy link
Member Author

Interesting: internal compiler error: unexpected panic This is often due to the nightly being unstable.

It seems the compiler error is not caused by the imported code. Any idea on fixing that?

@weikengchen
Copy link
Member

We can either wait for one more day or ignore that. Sometimes, nightly is just unstable.

@mmaker
Copy link
Member

mmaker commented Dec 11, 2021

A micro-optimization that you might want to consider is to build up a buffer of results, and then sum that buffer up in parallel.

We can't. The chunk size max_msm_buffer is a constant and, if we want to keep log space, we can't store N/max_msm_buffer elements. We can, however, build them up in chunks again, but that seems.. overkill? After all it's just group addition. What would you expect from this?

As another note: when/if you merge, can you please credit me as co-author of the commitment? This code comes from mmaker/gemini. I can push an empty commitment here if this makes it easier for you.

@Pratyush
Copy link
Member

You can build up a max_msm_buffer-sized buffer of results, and then sum that. This would at most double your memory consumption. However that might still be out of your memory budget, and the time savings might not be sufficient to warrant it...

Re: the authorship, we'll merge it via squash-ing the PR, so if you add an empty commit then you should be automatically credited with co-authorship in the squash commit.

@mmaker
Copy link
Member

mmaker commented Dec 13, 2021

Yah, that's what I meant; I'd stick with this "naïve" version for now.
Added a marker, thanks!

@weikengchen
Copy link
Member

weikengchen commented Dec 14, 2021

It seems that pub mod variable_base; can be changed back to mod variable_base;. So I changed that.

@weikengchen weikengchen changed the title add ChunkedPippenger to msm and add tests for it Add ChunkedPippenger to variable-base MSM Dec 14, 2021
@weikengchen
Copy link
Member

I will wait for Pratyush to do a final pass.

@weikengchen weikengchen merged commit 030cab7 into arkworks-rs:master Dec 16, 2021
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.

5 participants