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

feat: implement LogUp scheme #420

Merged
merged 31 commits into from
Jun 13, 2024
Merged

feat: implement LogUp scheme #420

merged 31 commits into from
Jun 13, 2024

Conversation

Insun35
Copy link
Contributor

@Insun35 Insun35 commented May 20, 2024

Implement logarithmic derivative lookup scheme.
Logarithmic derivatives translate products of linear factors into sums of their reciprocals, turning zeroes into simple poles of same multiplicity. It reduces computation cost using grand sum argument instead of grand product argument.
See Paper 2022/1530

Referenced Scroll Halo2 implementation.
We named Logup not MVLookup because this implementation uses univariate polynomial commitment scheme.

@Insun35 Insun35 force-pushed the feat/implement-logup-scheme branch 3 times, most recently from feb2a27 to e0f3423 Compare May 20, 2024 12:44
@Insun35 Insun35 marked this pull request as draft May 21, 2024 05:06
@Insun35 Insun35 force-pushed the feat/implement-logup-scheme branch 3 times, most recently from 35d5ddf to 8a314d3 Compare May 26, 2024 09:04
@Insun35 Insun35 force-pushed the feat/implement-logup-scheme branch 4 times, most recently from d144562 to 6f6be38 Compare May 30, 2024 04:23
@Insun35 Insun35 marked this pull request as ready for review May 30, 2024 04:35
@Insun35 Insun35 force-pushed the feat/implement-logup-scheme branch 3 times, most recently from 71cf1d6 to ca0d474 Compare May 30, 2024 06:55
tachyon/zk/lookup/lookup_argument.h Outdated Show resolved Hide resolved
tachyon/zk/lookup/lookup_argument.h Outdated Show resolved Hide resolved
tachyon/zk/lookup/logup/scheme.h Outdated Show resolved Hide resolved
@chokobole
Copy link
Contributor

Please remove this statement from the commit body of feat(zk): add constraint_system lookup logic for LookupsMap

Also add kInvalid type to avoid using Lookup() and LookupAny()
without setting a lookup type.

@chokobole
Copy link
Contributor

chokobole commented May 30, 2024

Please add links to the rust reference code to every single commit!

@chokobole
Copy link
Contributor

You need to add license of Geometry Research. Does Scroll take this code from here, right?

@chokobole
Copy link
Contributor

chokobole commented May 30, 2024

Please change the type of fix(zk): fix wrong iterator access, feat(zk): add circuit test flags for LogUp and feat(zk): implement MultiLookupCircuit to test

tachyon/zk/lookup/lookup_argument.h Show resolved Hide resolved
tachyon/zk/lookup/lookup_argument.h Show resolved Hide resolved
tachyon/zk/lookup/lookup_argument.h Outdated Show resolved Hide resolved
tachyon/zk/plonk/constraint_system/constraint_system.h Outdated Show resolved Hide resolved
tachyon/zk/plonk/constraint_system/constraint_system.h Outdated Show resolved Hide resolved
tachyon/zk/plonk/examples/multi_lookup_circuit.h Outdated Show resolved Hide resolved
tachyon/zk/plonk/examples/multi_lookup_circuit.h Outdated Show resolved Hide resolved
tachyon/zk/plonk/examples/multi_lookup_circuit.h Outdated Show resolved Hide resolved
tachyon/zk/plonk/examples/multi_lookup_circuit.h Outdated Show resolved Hide resolved
tachyon/zk/plonk/examples/multi_lookup_circuit.h Outdated Show resolved Hide resolved
@Insun35 Insun35 force-pushed the feat/implement-logup-scheme branch 4 times, most recently from aae25a7 to b3d6f86 Compare June 4, 2024 04:35
@TomTaehoonKim
Copy link
Contributor

@Insun35 Could you fix typo in 98aa037's commit body? "Constraint system select" -> "Constraint system selects"

@Insun35 Insun35 force-pushed the feat/implement-logup-scheme branch from 8cb3ce2 to ea1ae1e Compare June 13, 2024 10:07
Add `btree_map_stringifier` and `lookup_tracker_stringifier`.
@Insun35 Insun35 force-pushed the feat/implement-logup-scheme branch from ea1ae1e to 7147174 Compare June 13, 2024 10:18
tachyon/base/containers/container_util_unittest.cc Outdated Show resolved Hide resolved
tachyon/base/containers/container_util_unittest.cc Outdated Show resolved Hide resolved
tachyon/zk/plonk/halo2/proof_reader.h Outdated Show resolved Hide resolved
tachyon/zk/plonk/halo2/proof_reader.h Outdated Show resolved Hide resolved
tachyon/zk/plonk/halo2/proof_reader.h Outdated Show resolved Hide resolved
Copy link
Contributor

@TomTaehoonKim TomTaehoonKim left a comment

Choose a reason for hiding this comment

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

LGTM

@Insun35 Insun35 force-pushed the feat/implement-logup-scheme branch from 7147174 to 844a9de Compare June 13, 2024 11:26
`MultiLookupCircuit` to be introduced in the following commit requires 2^8
`kMaxExtendedDomainSize`.
@Insun35 Insun35 force-pushed the feat/implement-logup-scheme branch from 844a9de to 8ad250a Compare June 13, 2024 11:28
Copy link
Contributor

@chokobole chokobole left a comment

Choose a reason for hiding this comment

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

LGTM

@chokobole chokobole merged commit 4b86393 into main Jun 13, 2024
3 checks passed
@chokobole chokobole deleted the feat/implement-logup-scheme branch June 13, 2024 12:29
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