Feat: septic curve based continuation #1061
Merged
Conversation
This file contains hidden or 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
kunxian-xia
commented
Sep 10, 2025
kunxian-xia
commented
Sep 22, 2025
21 tasks
hero78119
reviewed
Oct 28, 2025
Collaborator
hero78119
left a comment
hero78119
left a comment
There was a problem hiding this comment.
1st quick review pass, without reviewing global chip in detail
| /// 2. The curve's order is a large prime number of 31x7 bits | ||
| #[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)] | ||
| pub struct SepticPoint<F> { | ||
| pub x: SepticExtension<F>, |
Collaborator
There was a problem hiding this comment.
a tiny question: it seems SepticExtension extension field is also a field, so in future both could be define separately?, e.g.
pub struct SepticPoint<F, E<BaseField = F>> {
pub x: E,
pub y: E,
}
Collaborator
Author
There was a problem hiding this comment.
yeah, this should work.
hero78119
reviewed
Oct 29, 2025
| assert_eq!(xs.len(), 7); | ||
| assert_eq!(ys.len(), 7); | ||
| assert_eq!(slopes.len(), 7); | ||
| assert_eq!(final_sum.len(), 7 * 2); |
Collaborator
There was a problem hiding this comment.
nitpicks: replace with SEPTIC_EXTENSION_DEGREE
| .map(|i| cb.create_witin(|| format!("slope{}", i))) | ||
| .collect(); | ||
| let addr = cb.create_witin(|| "addr"); | ||
| let is_ram_register = cb.create_witin(|| "is_ram_register"); |
Collaborator
There was a problem hiding this comment.
it seems there missing assert_bit on is_ram_register
| 1 - is_global_write.expr(), | ||
| local_clk.expr(), | ||
| )?; | ||
| // TODO: enforce shard = shard_id in the public values |
Collaborator
There was a problem hiding this comment.
there is api cb.query_shard_id() and we need below constrian
- if is_global_write == 0, then assert_lt(global_clk, cb.query_shard_id())
- if is_global_write == 1, then assert_eq(global_clk, cb.query_shard_id())
hero78119
reviewed
Oct 29, 2025
| let addr = cb.create_witin(|| "addr"); | ||
| let is_ram_register = cb.create_witin(|| "is_ram_register"); | ||
| let value = UInt::new(|| "value", cb)?; | ||
| let shard = cb.create_witin(|| "shard"); |
Collaborator
There was a problem hiding this comment.
follow comments here #1061 (comment)
we can remove shard and just keep global_clk
hero78119
reviewed
Oct 29, 2025
hero78119
reviewed
Oct 29, 2025
| .collect_vec() | ||
| }; | ||
| // build x[b,0], x[b,1], y[b,0], y[b,1] | ||
| let mut x0 = filter_bj(&xs, 0); |
Collaborator
There was a problem hiding this comment.
suggestion: will be nice to add TODO for define new api split_to_owned_even_odd in SmartSlice so we can use it like as_view_slice
hero78119
reviewed
Oct 29, 2025
| ]; | ||
|
|
||
| let prime = E::BaseField::order().to_u64_digits()[0]; | ||
| loop { |
Collaborator
There was a problem hiding this comment.
An improvement: we can do parallel search + find_any just like plonk3 grinding api
Co-authored-by: Ming <[email protected]>
Collaborator
Author
hero78119
reviewed
Oct 31, 2025
hero78119
reviewed
Oct 31, 2025
| } | ||
| } | ||
|
|
||
| #[derive(Clone)] |
Collaborator
There was a problem hiding this comment.
this seems to be left-over
- simplify index computation of affine_add(left, right) - allocate buffer once and parallel initialization
#1061 Cleanup - [x] clippy fixes - [x] remove old unused routines for generating witness for the ecc summation tower tree. Note this approach is abandoned. We prefer to pack $log_2(N)$ layers in one layer using the relation `p[1, b] = ec_add(p[0,b], p[1,b])` (first proposed in Quark paper.
fix few issue reported in global chip integration test
Collaborator
|
Blocking issues
Later TODOs
|
hero78119
reviewed
Nov 3, 2025
# Summary - [x] bump up `gkr-backend`. - [x] The global chip does not support goldilocks field for now. Therefore we need to disable `fibonacci + goldilocks` in the integration tests.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 6, 2025
To deal with > 1 shards and satisfy offline memory constrain related to #1061, #1063, #699, #698, #697, #696, #700 ### change scope - [x] separate mem bus to read/write 2 different map - [x] add new public input - [x] integrate local init chip - [x] integrate local final chip - [x] shift opcode & mem record to shard ts - [x] integrate mem bus chip - [x] refactor mock prover mock proving ### benchmarks bench on single chunk with fibonacci on CPU: 5900XT 32 cores, 64GB RAM | Benchmark | Median Time (s) | Median Change (%) | |------------------------------|----------------|----------------------------------------| | fibonacci_max_steps_1048576 | 2.8142 | +1.54% (Change within noise threshold) | | fibonacci_max_steps_2097152 | 4.9337 | +1.05% (Change within noise threshold) | | fibonacci_max_steps_4194304 | 9.1971 | -3.21% (Change within noise threshold) | which shows no performance impact --------- Co-authored-by: xkx <[email protected]>
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.
Summary
Sub tasks
cmd:
cargo test --release --lib test_ecc_quark_prover --features "sanity-check" -- --nocaptureGlobalchip @kunxian-xiasum != p[0] + p[1]