Nova vairants (Ova) NIFS abstraction #165
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CPerezz
reviewed
Oct 3, 2024
|
This is awesome. Will definitely be helpful! |
arnaucube
force-pushed
the
nova-vairants-nifs-abstraction
branch
from
4a91528 to
e939418
Compare
CPerezz
approved these changes
Oct 3, 2024
|
Don't merge yet, I'll do some updates. |
arnaucube
force-pushed
the
nova-vairants-nifs-abstraction
branch
4 times, most recently
from
b43000b to
b45e6ea
Compare
CPerezz
approved these changes
Oct 3, 2024
…defining a common interface between the three Nova variants The recent Ova NIFS PR #163 (#163) and Mova NIFS PR #161 (#161) PRs add Nova NIFS variants implementations which differ from Nova in the logic done for the `E` error terms of the instances. The current Ova implementation (#163) is based on the existing Nova NIFS code base and adds the modifications to the `E` logic on top of it, and thus duplicating the code. Similarly for the Mova NIFS impl. The rest of the Mova & Ova schemes logic that is not yet implemented is pretty similar to Nova one (ie. the IVC logic, the circuits and the Decider), so ideally that can be done reusing most of the already existing Nova code without duplicating it. This PR is a first step in that direction for the existing Ova NIFS code. This commit adds the NIFS trait abstraction with the idea of allowing to reduce the amount of duplicated code for the Ova's NIFS impl on top of the Nova's code.
This is done from the existing Ova implementation at
`folding/ova/{mod.rs,nofs.rs}`, but removing when possible code that is not
needed or duplicated from the Nova logic.
This commit combined with the other ones (add nifs abstraction & port Ova to the nifs abstraction) allows to effectively get rid of ~400 lines of code that were duplicated in the Ova NIFS impl from the Nova impl.
arnaucube
force-pushed
the
nova-vairants-nifs-abstraction
branch
from
b45e6ea to
2e1039e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The recent Ova NIFS PR #163 and Mova NIFS PR #161 PRs add Nova NIFS variants implementations which differ from Nova in the logic done for the
Eerror terms of the instances.The current Ova implementation is based on the existing Nova NIFS code base and adds the modifications to the
Elogic on top of it, and thus duplicating the code. Similarly for the Mova NIFS impl.The rest of the Mova & Ova schemes logic that is not yet implemented is pretty similar to Nova one (ie. the IVC logic, the circuits and the Decider), so ideally that can be done reusing most of the already existing Nova code without duplicating it. This PR is a first step in that direction for the existing Ova NIFS code.
This PR:
NIFStrait abstraction (based on the needs for Nova, Mova, Ova), defining a common interface between the three Nova variantsI'll track the Mova PR progress, and maybe will also port the Mova NIFS impl into this new abstraction already in this PR, but if Mova PR takes some day I think we can merge this one and I can port that Mova impl into this new abstraction once that one is ready.