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

Implement MemoSet. #1004

Merged
merged 24 commits into from
Jan 11, 2024
Merged

Implement MemoSet. #1004

merged 24 commits into from
Jan 11, 2024

Conversation

porcuquine
Copy link
Contributor

This PR implements a MemoSet proof-of-concept, sufficient to implement mutually recursive coprocessors ('coroutines') using Lurk calling conventions. It does not integrate this ability into the Lurk reduction, nor provide concrete coprocessors sufficient to package this work into existing NIVC proofs.

The transcript is produced in the most straightforward way that can work, using literal Lurk data. It is worth noting that this dramatically simplified the already difficult task of getting an evaluation-time and proof-time transcript to match. There are undoubtedly optimization opportunities via reducing hashing costs. Because of the debugging benefit of the current approach, it is likely worth refactoring to something less legible only carefully. Ideally, it would be a simple configuration change to recover legible hashing for debugging purposes.

Please reserve most discussion of this PR until after the holidays, since I intend not to engage in the meantime, and this will make that easier.

@porcuquine porcuquine force-pushed the memoset branch 10 times, most recently from d8e4937 to 8965f96 Compare December 23, 2023 08:34
Copy link
Contributor

@arthurpaulino arthurpaulino left a comment

Choose a reason for hiding this comment

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

I'm looking into it. The first thing I want to leave as a reminder is that we need better documentation for this.

src/lem/store.rs Outdated Show resolved Hide resolved
src/coprocessor/memoset/mod.rs Outdated Show resolved Hide resolved
src/circuit/gadgets/pointer.rs Show resolved Hide resolved
src/coprocessor/memoset/multiset.rs Outdated Show resolved Hide resolved
src/coprocessor/memoset/mod.rs Outdated Show resolved Hide resolved
src/coprocessor/memoset/mod.rs Outdated Show resolved Hide resolved
@porcuquine porcuquine force-pushed the memoset branch 2 times, most recently from ca59e25 to 32ed056 Compare January 8, 2024 19:02
@porcuquine porcuquine marked this pull request as ready for review January 10, 2024 00:35
@porcuquine porcuquine requested review from a team as code owners January 10, 2024 00:35
Copy link
Contributor

@gabriel-barrett gabriel-barrett left a comment

Choose a reason for hiding this comment

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

Very good work! I think you've come up with a very reasonable initial framework for dealing with the memoset

mpenciak
mpenciak previously approved these changes Jan 10, 2024
@porcuquine porcuquine enabled auto-merge January 11, 2024 20:01
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Overall, this looks great, if a bit sparsely documented (in places I commented on)!
Thanks a lot!

src/coprocessor/memoset/mod.rs Show resolved Hide resolved
//! Whenever a computation being proved needs the result of a query, the prover non-deterministically supplies the
//! correct response. The resulting key-value pair is then added to a multiset representing deferred proofs. The
//! dependent proof now must not be accepted until every element in the deferred-proof multiset has been proved.
//!
Copy link
Contributor

Choose a reason for hiding this comment

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

While this module documentation is useful to document the building blocks, I would love to see some use-case level doc: what can I find in this module, what can I use it for, and what can I not use it for?

The core piece of information that we may want to volunteer at the top-level, is that this particular coprocessor takes assumptions about Lurk data structures (and their hash-consing), because it is meant to represent Lurk evaluation proofs. That's not a thing that all coprocessors do (e.g sha256) and laying that out in a couple sentences at the top should help the reader interpret the meaning of a struct named "Transcript" at the top of the file (i.e. this is not a garden-variety RO).

}
}

fn build_transcript(&self, s: &Store<F>) -> (Transcript<F>, Vec<Ptr>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have access to std::slice::group_by in a future version of Rust, and already have those through itertools. Does that change some of the algorithmic costs incurred by .sort() here?

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 think it helps with this. We can avoid the sort by just assembling insertions as a hash table of vectors, though.

//! `Scope`. This allows us to accumulate queries and the subqueries on which they depend, along with the memoized query
//! results computed 'naturally' during evaluation. We then separate and sort in an order matching that which the NIVC
//! prover will follow when provably maintaining the multiset accumulator and Fiat-Shamir transcript in the circuit.

Copy link
Contributor

Choose a reason for hiding this comment

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

We may also want to add some context for how this is in the coprocessor module but does not implement the Coprocessor traits. This is fine for now, but a TODO to find a "home" for this module would help.

@@ -0,0 +1,850 @@
//! The `memoset` module implements a `MemoSet`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: you've made the public API of this module quite sparse by making few members public, which is great.

It also means that if you were to annotate the top of this module with #![deny(missing_docs)] the amount of work you'd have to do to fix would be small.

@porcuquine porcuquine added this pull request to the merge queue Jan 11, 2024
@huitseeker huitseeker removed this pull request from the merge queue due to a manual request Jan 11, 2024
@porcuquine porcuquine added this pull request to the merge queue Jan 11, 2024
github-actions bot pushed a commit that referenced this pull request Jan 11, 2024
github-actions bot pushed a commit that referenced this pull request Jan 11, 2024
@porcuquine porcuquine mentioned this pull request Jan 11, 2024
13 tasks
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 11, 2024
@porcuquine porcuquine added this pull request to the merge queue Jan 11, 2024
github-actions bot pushed a commit that referenced this pull request Jan 11, 2024
Merged via the queue into main with commit d9a54a6 Jan 11, 2024
12 checks passed
@porcuquine porcuquine deleted the memoset branch January 11, 2024 22:57
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.

6 participants