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

On duplication in LEM integration #639

Closed
huitseeker opened this issue Aug 29, 2023 · 2 comments
Closed

On duplication in LEM integration #639

huitseeker opened this issue Aug 29, 2023 · 2 comments

Comments

@huitseeker
Copy link
Contributor

huitseeker commented Aug 29, 2023

I think there are two factors of duplication in #629:

Misuse of genericity

This is essentially explained in a PR comment. That is, the types that depend on a generic C: Coprocessor<F> are redefined in #629 because the LEM logic cannot for now accomodate any coprocessor other than DummyCoproc<F>. However, for any GenericType<Foo, Bar>, there is nothing in Rust that prevents you from defining methods for type GenericType<Foo, usize>.

I have a branch that starts working out the deduplication in https://github.com/huitseeker/lurk-rs/tree/lem-integration-experiment

The reason I think it's important to resolve this is that the methods we'd define today on Foo<F, DummyCoproc<F>> will eventually be able to be defined on Foo<F, C: Coproc<F>>, which is going to be much easier to adapt to if they're not distinct types.

Not abstracting the Multiframe, and failing to define provers in terms of that abstraction

The MultiFrame is the fundamental type that's defined starkly differently in LEM and Lurk:

This struct appears in the APIs of the prover:
https://github.com/lurk-lab/lurk-rs/blob/9aa8b75247066b70972f2806a1537c359f47c2c6/src/proof/mod.rs#L20
https://github.com/lurk-lab/lurk-rs/blob/9aa8b75247066b70972f2806a1537c359f47c2c6/src/proof/mod.rs#L33-L34
https://github.com/lurk-lab/lurk-rs/blob/9aa8b75247066b70972f2806a1537c359f47c2c6/src/proof/mod.rs#L107-L110
https://github.com/lurk-lab/lurk-rs/blob/9aa8b75247066b70972f2806a1537c359f47c2c6/src/proof/nova.rs#L121
https://github.com/lurk-lab/lurk-rs/blob/9aa8b75247066b70972f2806a1537c359f47c2c6/src/proof/nova.rs#L131-L141
https://github.com/lurk-lab/lurk-rs/blob/9aa8b75247066b70972f2806a1537c359f47c2c6/src/proof/nova.rs#L172-L181
https://github.com/lurk-lab/lurk-rs/blob/9aa8b75247066b70972f2806a1537c359f47c2c6/src/proof/nova.rs#L421-L428

morally speaking, the only thing we need (that is, the only thing Nova requires) in the position of this MultiFrame in the prover is some instance of StepCircuit. I suspect we could implement:

@arthurpaulino arthurpaulino linked a pull request Aug 31, 2023 that will close this issue
@huitseeker
Copy link
Contributor Author

Summarizing the story:

The present should close as soon as #717 and #718 are merged, because while we are indeed taking some duplication with those PRs, this is better than the alternative (worked out in #729) due to a lower complexity.

The missing pieces are NIVC support from #677 and #725, which should also resolve soon.

@huitseeker
Copy link
Contributor Author

Closed with the merge of #717, #718.

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 a pull request may close this issue.

1 participant