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

Add Tests for Segment Proving Without Keccak Tables #648

Merged
merged 23 commits into from
Oct 2, 2024

Conversation

sai-deng
Copy link
Contributor

@sai-deng sai-deng commented Sep 23, 2024

This PR prepares tests for the Issue: #620
Extracted from #624

[2024-09-23T17:07:54Z INFO evm_arithmetization::generation] CPU trace padded to 512 cycles
[2024-09-23T17:07:54Z INFO evm_arithmetization::witness::traces] Trace lengths (before padding): TraceCheckpoint { arithmetic_len: 121, byte_packing_len: 2, cpu_len: 512, keccak_len: 0, keccak_sponge_len: 0, logic_len: 4, memory_len: 67684 }, mem_before_len: 66136, mem_after_len: 66173
[2024-09-23T17:08:10Z INFO plonky2::util::timing] 29.7726s to Segment Proof Generation
[2024-09-23T17:08:10Z INFO plonky2::util::timing] | 14.2296s to Create all recursive circuits
[2024-09-23T17:08:10Z INFO plonky2::util::timing] | 0.0000s to Generate dummy payload
[2024-09-23T17:08:10Z INFO plonky2::util::timing] | 15.5369s to Prove segment
[2024-09-23T17:08:10Z INFO plonky2::util::timing] | 0.0034s to Verify segment proof
test fixed_recursive_verifier::tests::test_segment_proof_generation_without_keccak ... ok

@github-actions github-actions bot added the crate: evm_arithmetization Anything related to the evm_arithmetization crate. label Sep 23, 2024
@sai-deng sai-deng force-pushed the sai/skip_table_in_root_circuit branch from 84860a9 to f63363b Compare September 23, 2024 16:32
@sai-deng sai-deng changed the title add tests for segment proving without keccak tables Add Tests for Segment Proving Without Keccak Tables Sep 23, 2024
@sai-deng sai-deng marked this pull request as ready for review September 23, 2024 17:29
@sai-deng sai-deng requested a review from hratoanina September 23, 2024 17:36
Copy link
Collaborator

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

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

As a general rule, we try to not add any #[ignore] tests as these end up being forgotten / outdated (this happened more than once in the past).

I'm also curious about the main goal behind this. Is it purely for testing the functionality, or benchmarking it (cf the timing.print())? Either way, it may be best to craft / fetch an actual payload that would achieve what we want. We could replace some of the currently used block artifacts if you wanted to test this on a regular basis (i.e. through a CI job).

#[test]
#[ignore]
fn test_segment_proof_generation_without_keccak() -> anyhow::Result<()> {
let timing = &mut TimingTree::new("Segment Proof Generation", log::Level::Info);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will capture everything, I'd assume you want to initialize it right before proving?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is what I intend to do. I also want to know recursive circuits generation time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, the timing tree name is slightly misleading then but not really blocking,

Comment on lines 2999 to 3008
let max_cpu_len_log = 9;
let segment_iterator = SegmentDataIterator::<F>::new(&dummy_payload, Some(max_cpu_len_log));

let mut proofs_without_keccak = vec![];

let skip_proofs_before_index = 3;
for (i, segment_run) in segment_iterator.enumerate() {
if i < skip_proofs_before_index {
continue;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems highly specific, as any modification in the KERNEL ASM may break your assumption that the 3rd segment has no keccak operation.
Also this is breaking compilation with cdk_erigon feature flag, as in this case we're missing a range for the Poseidon table in the initialization of AllRecursiveCircuits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. Do you have any suggestions on how to craft a payload that generates segments without Keccak? Regarding cdk_erigon, I can add a feature flag to disable it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I think we'd need some around segment generation. Basically we'd want to do the same as in init_exc_stop.rs but I don't think this will play well with how we actually generate segment data.

@sai-deng
Copy link
Contributor Author

As a general rule, we try to not add any #[ignore] tests as these end up being forgotten / outdated (this happened more than once in the past).

I'm also curious about the main goal behind this. Is it purely for testing the functionality, or benchmarking it (cf the timing.print())? Either way, it may be best to craft / fetch an actual payload that would achieve what we want. We could replace some of the currently used block artifacts if you wanted to test this on a regular basis (i.e. through a CI job).

The main purpose of this PR is to add a test for root proving without Keccak tables. I will remove the #[ignore] attribute when I upload the main PR.

I’m not entirely sure how to craft a payload that achieves the desired outcome. This is the fastest test I could come up with for now.

@hratoanina
Copy link
Contributor

I think the cleanest way would be to add a dummy segment to a complete transaction. It was done before, but I believe the logic got scrapped at some point since we don't need them anymore.

@sai-deng sai-deng requested a review from wborgeaud as a code owner September 25, 2024 18:15
@sai-deng
Copy link
Contributor Author

I will try to improve this test, but this does not block merging other PRs related to #620

@sai-deng
Copy link
Contributor Author

Thanks for the reviews! I have made the following changes to the unit tests:

  1. Added an empty payload, however it still includes the Keccak operations.

  2. I am using a smaller segmentation threshold; the first segment now has only 47 cycles before reaching the halting state. (Modifying the ASM code to remove Keccak operations will not be faster than the current method. )

  3. I added a check to ensure there are no Keccak operations in the segmentation.

@Nashtare
Copy link
Collaborator

Added an empty payload, however it still includes the Keccak operations.

Yes empty payloads would still need some light keccak ops. I'll try crafting / refactoring the segment generation handling tomorrow so that you can use it as is in your test without needing a convoluted (and not super future-proof) approach

Copy link
Collaborator

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

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

I don't want this to be blocking you, so I'll leave an approval as the test works for now, and would panic if we were to hit a KECCAK instruction in this given segment.

As a general indication for a clean way to generate fully dummy segments (just the init portion in the KERNEL), the issue lies in the initialization of the GenerationState which, as it is only used for actual proof runs (unlike the interpreter state), is using the hardcoded halt_final offset as only halting point, and doesn't take customizable ones unlike segment data generation (cc @hratoanina).

Copy link
Contributor

@LindaGuiga LindaGuiga left a comment

Choose a reason for hiding this comment

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

LGTM (especially given the discussion)

@sai-deng sai-deng merged commit edb62a1 into develop Oct 2, 2024
20 checks passed
@sai-deng sai-deng deleted the sai/skip_table_in_root_circuit branch October 2, 2024 16:03
@sai-deng
Copy link
Contributor Author

sai-deng commented Oct 9, 2024

I don't want this to be blocking you, so I'll leave an approval as the test works for now, and would panic if we were to hit a KECCAK instruction in this given segment.

As a general indication for a clean way to generate fully dummy segments (just the init portion in the KERNEL), the issue lies in the initialization of the GenerationState which, as it is only used for actual proof runs (unlike the interpreter state), is using the hardcoded halt_final offset as only halting point, and doesn't take customizable ones unlike segment data generation (cc @hratoanina).

This has already been addressed per our offline discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate: evm_arithmetization Anything related to the evm_arithmetization crate.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants