Option for partitioned trace committment #336

Al-Kindi-0 · 2024-10-18T12:38:11Z

Addresses #335.
This still in draft as two points need to be clarified:

Once we have the resulting num_partitions parts we need to hash all of them in order to get a final digest per row of the trace we are committing to. The issue is that directly hashing digests is not possible, or at least I couldn't see how to do it. One pass through the bytes representation but I am not sure this was the desired implementation. For now, the code just uses a naive merge in order to fold all the digests into one digest.
The separation between num_partitions equal to 1 vs greater than 1 is not very elegant but I couldn't see a way to merge handling of the two cases.

irakliyk · 2024-10-18T15:59:58Z

Once we have the resulting num_partitions parts we need to hash all of them in order to get a final digest per row of the trace we are committing to. The issue is that directly hashing digests is not possible, or at least I couldn't see how to do it. One pass through the bytes representation but I am not sure this was the desired implementation. For now, the code just uses a naive merge in order to fold all the digests into one digest.

I wonder if we should define a new method on the Hasher trait - something like:

pub trait Hasher {
    fn merge_many(values: &[Self::Digest]) -> Self::Digest;
}

Al-Kindi-0 · 2024-10-18T16:16:48Z

I wonder if we should define a new method on the Hasher trait - something like:
pub trait Hasher {
    fn merge_many(values: &[Self::Digest]) -> Self::Digest;
}

I don't see a better solution. I will go with this unless there are any other remarks.

crypto/src/hash/rescue/rp64_256/mod.rs

irakliyk

Thank you! Looks good! I left some comments inline.

Another thing I'm wondering is whether it is better to use partition_size rather than num_partitions after all. Or maybe we should use a combination of both - e.g., something like num_partitions and min_partition_size.

The motivation is that we actually compute commitments 3 times: main trace, auxiliary trace, and constraint evaluations. All of these may have different widths. Let's for example assume that we have

main race: 72 columns
aux trace: 7 columns (degree 2 extension)
constraint evaluations: 8 columns (degree 2 extension)

With num_partitions = 4, we'd split things into:

main trace: 4 partitions with 18 columns each.
aux trace: 3 partitions with 4 columns each + 1 partition with 2 columns
constraint evaluations: 4 partitions with 4 columns each.

But if we have num_partitions = 4 and min_partition_size = 8, we'd get the following:

main trace: 4 partitions with 18 columns each.
aux trace: 1 partition with 8 columns and 1 partition with 6 columns.
constraint evaluations: 2 partitions with 8 columns each.

This seems like a slight better arrangement - but also not sure if the complexity is worth it.

We can also specify num_partitions/partition_size separately for each of the 3 components - but not sure if that's worth it either.

cc @TheMenko

air/src/options.rs

prover/src/matrix/row_matrix.rs

prover/src/trace/trace_lde/default/mod.rs

prover/src/lib.rs

prover/src/trace/trace_lde/mod.rs

verifier/src/channel.rs

TheMenko · 2024-10-21T19:51:18Z

With num_partitions = 4, we'd split things into:

* main trace: 4 partitions with 18 columns each.
* aux trace: 3 partitions with 4 columns each + 1 partition with 2 columns
* constraint evaluations: 4 partitions with 4 columns each.

This is how GPU code currently operates.

But if we have num_partitions = 4 and min_partition_size = 8, we'd get the following:
* main trace: 4 partitions with 18 columns each.
* aux trace: 1 partition with 8 columns and 1 partition with 6 columns.
* constraint evaluations: 2 partitions with 8 columns each.

This wouldn't be a lot of work on the GPU side. The only thing is that we should measure the performance for these two cases.
My current assumption is that for relatively small number of columns, doing min_partition_size = 8 would have a positive impact.

Al-Kindi-0 · 2024-10-22T13:09:11Z

@irakliyk , I added a proposal to implement your idea of minimal partition size and it is included in the penultimate commit

irakliyk

Thank you! Looks good! I've added a few more comments. The main one is about potentially introducing PartitionOptions struct.

air/src/options.rs

verifier/src/channel.rs

prover/src/trace/trace_lde/default/tests.rs

prover/src/lib.rs

prover/src/trace/trace_lde/default/mod.rs

Al-Kindi-0 · 2024-10-23T07:35:05Z

I have also benchmarked the code to get an idea on the delta after this PR. The results are logical:

Next

Generated 128 private-public key pairs in 4294 ms
Signed 128 messages in 1 ms
Verified 128 signature in 2866 ms
Generating proof for verifying 128 Lamport+ signatures
INFO     generate_proof [ 4.73s | 6.95% / 100.00% ]
INFO     ┝━ generate_execution_trace [ 956ms | 20.19% ] num_cols: 22
INFO     ┝━ build_domain [ 3.48ms | 0.07% ] trace_length: 131072 | lde_domain_size: 1048576
INFO     ┝━ commit_to_main_trace_segment [ 1.02s | 0.00% / 21.60% ]
INFO     │  ┝━ extend_execution_trace [ 924ms | 19.51% ] num_cols: 22 | blowup: 8
INFO     │  ┕━ compute_execution_trace_commitment [ 98.6ms | 2.08% ] commitment_domain_size: 1048576
INFO     ┝━ evaluate_constraints [ 1.39s | 29.46% ] ce_domain_size: 1048576
INFO     ┝━ commit_to_constraint_evaluations [ 784ms | 0.00% / 16.55% ]
INFO     │  ┝━ build_composition_poly_columns [ 112ms | 2.37% ] num_columns: 6
INFO     │  ┝━ evaluate_composition_poly_columns [ 570ms | 12.04% ]
INFO     │  ┕━ compute_constraint_evaluation_commitment [ 101ms | 2.14% ] log_domain_size: 20
INFO     ┝━ build_deep_composition_poly [ 103ms | 2.19% ]
INFO     ┝━ evaluate_deep_composition_poly [ 71.7ms | 1.52% ]
INFO     ┝━ compute_fri_layers [ 53.2ms | 1.12% ] num_layers: 4
INFO     ┝━ determine_query_positions [ 343µs | 0.01% ] grinding_factor: 16 | num_positions: 28
INFO     ┕━ build_proof_object [ 16.0ms | 0.34% ]
---------------------
Proof generated in 4733 ms
Proof size: 94.4 KB
Proof security: 99 bits (55 proven)
Proof hash: e74c7a32f4c8f137a161d531915117df744146d401614778caf8b6ad00cadfb1
---------------------
Proof verified in 2.3 ms

num_partitions=1 & min_partition_size=1

Generated 128 private-public key pairs in 4341 ms
Signed 128 messages in 1 ms
Verified 128 signature in 2906 ms
Generating proof for verifying 128 Lamport+ signatures
INFO     generate_proof [ 4.78s | 6.99% / 100.00% ]
INFO     ┝━ generate_execution_trace [ 951ms | 19.87% ] num_cols: 22
INFO     ┝━ build_domain [ 3.70ms | 0.08% ] trace_length: 131072 | lde_domain_size: 1048576
INFO     ┝━ commit_to_main_trace_segment [ 1.04s | 0.00% / 21.78% ]
INFO     │  ┝━ extend_execution_trace [ 938ms | 19.62% ] num_cols: 22 | blowup: 8
INFO     │  ┕━ compute_execution_trace_commitment [ 104ms | 2.17% ] commitment_domain_size: 1048576
INFO     ┝━ evaluate_constraints [ 1.40s | 29.36% ] ce_domain_size: 1048576
INFO     ┝━ commit_to_constraint_evaluations [ 799ms | 0.00% / 16.70% ]
INFO     │  ┝━ build_composition_poly_columns [ 109ms | 2.28% ] num_columns: 6
INFO     │  ┝━ evaluate_composition_poly_columns [ 584ms | 12.20% ]
INFO     │  ┕━ compute_constraint_evaluation_commitment [ 106ms | 2.22% ] log_domain_size: 20
INFO     ┝━ build_deep_composition_poly [ 105ms | 2.19% ]
INFO     ┝━ evaluate_deep_composition_poly [ 73.2ms | 1.53% ]
INFO     ┝━ compute_fri_layers [ 54.9ms | 1.15% ] num_layers: 4
INFO     ┝━ determine_query_positions [ 230µs | 0.00% ] grinding_factor: 16 | num_positions: 28
INFO     ┕━ build_proof_object [ 16.0ms | 0.33% ]
---------------------
Proof generated in 4783 ms
Proof size: 95.0 KB
Proof security: 99 bits (55 proven)
Proof hash: c57076047ef71a4eab840140bca38f7b4ab61470dfb15905974b1ec333fcdb8b
---------------------
Proof verified in 2.4 ms

num_partitions=2 & min_partition_size=4

Generated 128 private-public key pairs in 4512 ms
Signed 128 messages in 1 ms
Verified 128 signature in 3047 ms
Generating proof for verifying 128 Lamport+ signatures
INFO     generate_proof [ 4.91s | 6.51% / 100.00% ]
INFO     ┝━ generate_execution_trace [ 941ms | 19.17% ] num_cols: 22
INFO     ┝━ build_domain [ 3.54ms | 0.07% ] trace_length: 131072 | lde_domain_size: 1048576
INFO     ┝━ commit_to_main_trace_segment [ 1.13s | 0.00% / 23.02% ]
INFO     │  ┝━ extend_execution_trace [ 946ms | 19.29% ] num_cols: 22 | blowup: 8
INFO     │  ┕━ compute_execution_trace_commitment [ 183ms | 3.74% ] commitment_domain_size: 1048576
INFO     ┝━ evaluate_constraints [ 1.45s | 29.45% ] ce_domain_size: 1048576
INFO     ┝━ commit_to_constraint_evaluations [ 818ms | 0.00% / 16.67% ]
INFO     │  ┝━ build_composition_poly_columns [ 129ms | 2.64% ] num_columns: 6
INFO     │  ┝━ evaluate_composition_poly_columns [ 581ms | 11.84% ]
INFO     │  ┕━ compute_constraint_evaluation_commitment [ 108ms | 2.20% ] log_domain_size: 20
INFO     ┝━ build_deep_composition_poly [ 106ms | 2.16% ]
INFO     ┝━ evaluate_deep_composition_poly [ 72.1ms | 1.47% ]
INFO     ┝━ compute_fri_layers [ 54.7ms | 1.11% ] num_layers: 4
INFO     ┝━ determine_query_positions [ 1.19ms | 0.02% ] grinding_factor: 16 | num_positions: 28
INFO     ┕━ build_proof_object [ 16.5ms | 0.34% ]
---------------------
Proof generated in 4907 ms
Proof size: 96.0 KB
Proof security: 99 bits (55 proven)
Proof hash: f8fc4c8d1fab445366f6556ada92c04770981cf188ffd56dd0e7cabc274c920e
---------------------
Proof verified in 2.2 ms

irakliyk

Looks good! Thank you! I left a few more small comments inline.

air/src/options.rs

prover/src/lib.rs

verifier/src/channel.rs

irakliyk

All looks good! Thank you!

feat: partition trace committment

5e27ec4

facebook-github-bot added the cla signed label Oct 18, 2024

Al-Kindi-0 added 2 commits October 21, 2024 09:28

feat: add merge_many method on Hasher

3314255

feat: use merge_many

3b6ccd9

Al-Kindi-0 marked this pull request as ready for review October 21, 2024 08:37

fix: clippy

0fd8c31

irakliyk reviewed Oct 21, 2024

View reviewed changes

crypto/src/hash/rescue/rp64_256/mod.rs Outdated Show resolved Hide resolved

chore: simplify implementations of merge_many

c585026

irakliyk reviewed Oct 21, 2024

View reviewed changes

Al-Kindi-0 added 4 commits October 22, 2024 10:10

chore: address feedback

cce53e2

chore: address feedback

3d023bf

feat: add a minimal partition size

1c7a35c

fix: clippy

2aa90a0

irakliyk reviewed Oct 22, 2024

View reviewed changes

Al-Kindi-0 added 4 commits October 22, 2024 21:19

chore: address feedback

90b36fc

docs: update constructor docs

c206dbb

fix: change to partition_size

930268e

feat: introduce PartitionOption

84b9373

irakliyk reviewed Oct 23, 2024

View reviewed changes

chore: address comments

0ac341e

irakliyk approved these changes Oct 24, 2024

View reviewed changes

irakliyk merged commit 6204d61 into facebook:next Oct 24, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option for partitioned trace committment #336

Option for partitioned trace committment #336

Al-Kindi-0 commented Oct 18, 2024

irakliyk commented Oct 18, 2024

Al-Kindi-0 commented Oct 18, 2024

irakliyk left a comment

TheMenko commented Oct 21, 2024

Al-Kindi-0 commented Oct 22, 2024

irakliyk left a comment

Al-Kindi-0 commented Oct 23, 2024

irakliyk left a comment

irakliyk left a comment

Option for partitioned trace committment #336

Option for partitioned trace committment #336

Conversation

Al-Kindi-0 commented Oct 18, 2024

irakliyk commented Oct 18, 2024

Al-Kindi-0 commented Oct 18, 2024

irakliyk left a comment

Choose a reason for hiding this comment

TheMenko commented Oct 21, 2024

Al-Kindi-0 commented Oct 22, 2024

irakliyk left a comment

Choose a reason for hiding this comment

Al-Kindi-0 commented Oct 23, 2024

irakliyk left a comment

Choose a reason for hiding this comment

irakliyk left a comment

Choose a reason for hiding this comment