Move byte packing / unpacking to a distinct table #1212

Nashtare · 2023-09-06T14:23:12Z

Following suggestion for points 1 and 2 in #1120, this PR introduces a new BytePacking table to reduce the CPU/Memory overhead induced by the %mload_packing and %mstore_unpacking operations in packing.asm.

More specifically (thanks @nbgl for the explanations!), there are now two new instructions, MLOAD_32BYTES and MSTORE_32BYTES which can handle sequence of bytes of length up to 32. This allows to move the heavy packing / unpacking logic from software to constraints.

In terms of trace lengths reduction, see the tables in this comment for a comparison between the current main branch and this one, against a few EVM test cases. It does incur a light overhead for native token transfer in terms of proving time but significantly reduces the number of CPU ops and memory accesses for precompiles.

Note that because both packing/unpacking are done within the same table, the range check necessary for writing is also (needlessly) applied on the read bytes.

~~In terms of # of columns, I'd ideally remove an extra one, so that we would remove one Poseidon permutation per leaf hashing, but I assume this can be left as a follow-up PR.~~
EDIT: I actually removed the FILTER column, effectively getting 1 less Poseidon permutation per leaf hashing. We still may remove other columns but this becomes less important now.

Nashtare · 2023-09-06T23:24:15Z

@unzvfu Would you mind taking a look at this?

unzvfu · 2023-09-07T09:02:21Z

@unzvfu Would you mind taking a look at this?

Sure, maybe tomorrow with any luck.

Noticed that you're force-pushing to the branch. This is almost never necessary and should absolutely be avoided on public repos since it breaks the repo on others' machines. Much better to just have the extra commits rather than rewrite the history. Perhaps check your config in case it's being enabled automatically?

Nashtare · 2023-09-07T10:39:33Z

Sure, maybe tomorrow with any luck.

Take your time, and thanks again!

Noticed that you're force-pushing to the branch. This is almost never necessary and should absolutely be avoided on public repos since it breaks the repo on others' machines.

I agree, though some were necessary because of breaking changes in latest main. I've dealt with them before requesting for a review though to not have you or anyone looking get history changes in the process.

Nashtare · 2023-09-07T15:52:11Z

Note that we may also be able to combine the two new flags on the CPU side, though I haven't put any thoughts on this yet. This can probably be left as a follow-up PR.

…acking

unzvfu

Thanks Robin, this is excellent! Could just do with a bit more documentation in places.

evm/src/byte_packing/byte_packing_stark.rs

evm/src/byte_packing/columns.rs

evm/src/cpu/kernel/interpreter.rs

evm/src/memory/memory_stark.rs

evm/src/witness/operation.rs

evm/src/byte_packing/byte_packing_stark.rs

Nashtare · 2023-09-12T14:39:58Z

Thanks again for the review @unzvfu! 🙏🏼
I still have to deal with the extensive documentation, but I've addressed most of the technical comments.

I'll ping you again once I've added the documentation.

Nashtare · 2023-09-12T15:31:56Z

@unzvfu I've pushed a first attempt at a more descriptive module explanation. I'm not sure how extensive you'd like it to be (and as I wrote the code I'm not sure where the dark spots are) though I tried addressing all the points you mentioned.

unzvfu · 2023-09-12T18:45:30Z

@unzvfu I've pushed a first attempt at a more descriptive module explanation. I'm not sure how extensive you'd like it to be (and as I wrote the code I'm not sure where the dark spots are) though I tried addressing all the points you mentioned.

That's great, thanks @Nashtare.

* Make GateRef value public * Better document constraints on addcy carries (0xPolygonZero#1139) * Add missing constraints on addcy carries. * Remove bit-checks; make documentation clearer. * Constrain keccak general * Reuse set_public_value_targets * Fix endianness in benefiary limbs * Convert to u32 instead of u64 * Clippy * Silence Poseidon warnings for ARM targets * Remove unused attributes * Fix trait import. (0xPolygonZero#1163) * Error instead of panicking for missing preprocessed circuits (0xPolygonZero#1159) * Set exception flag to 1. * Connect public values in aggregation circuit (0xPolygonZero#1169) * Connect public values in aggregation circuit * Minor * Write trie roots to memory before kernel bootstrapping (0xPolygonZero#1172) * Write trie roots * Remove CPU trace length * Update hash_initial/final_tries * Fix tests * Minor * PR feedback * Connect SHL/SHR operations to the Arithmetic table (0xPolygonZero#1166) * Add corresponding arithmetic operations to shift ones * Include SHL/SHR in the arithmetic CTL * Prevent overflow * Expand documentation for ctl_data_ternops() * Combine AND and OR flags in CpuStark * Reduce reallocations * Address review * Apply Nicholas comment * Revert changes in cyclic_subgroup_unknown_order * Fix logic CTL * Combine all logic flags together * Comment * Combine EQ and ISZERO flags * Combine jump flags * Add guidance for external contributors to README.md * Use Keccak config in simple tests * Remove is_cpu_cycle * Remove is_bootstrap_kernel column * Implement inverse from Fermat little theorem (0xPolygonZero#1176) * Add inverse from Fermat little theorem * Remove inlining for goldilocks try_inverse method * Remove copy on write for mpt_insert and mpt_delete * Combine a few constraints * Reduce overconstraining in decode module * Remove filtering in membus * Observe public values * Update tests to have a blockgaslimit fitting u32s * Update BlockBaseFee to fit in 2 limbs * Refactor * Apply comment * Combine get_context and set_context into one flag * Remove unnecessary changes in the Operation enum * Apply comment * Patched plonky2 to use a patch for eth_trie_utils * Implement receipts and logs Co-authored-by: Hamy Ratoanina <[email protected]> Co-authored-by: Linda Guiga <[email protected]> * Cleanup * Fix tests and address comments * Change receipts_trie in basic_smart_contract and self_balance_gas_cost * Cleanup * Clippy * Made `PublicValues` serializable - Needed by the prover scheduler. * make generate partial_witness pub * Update range from ReceiptTrie PR * fix: constrain higher bits of reg_preimage * Keccak STARK: constraint preimage to equal A on first round * Constrain next row's stack length * Add missing links between public values * Implement receipts of types 1 and 2 * Apply comments * Fix comment in `proof.rs` * Apply comments * Replace genesis state trie check with TODO * feat: serde for targets * Display actual trace lengths instead of number of ops * Remove duplicate code * Remove filter column for KeccakStark * Added mock feature flag and test * remove spurious * Add blockhash sys opcode * Apply comments * Change h256_ulimbs * Fix overflow check and test. Remove [..8] when using h256_limbs. * Clippy * Fix memop reads, from_prover_inputs and cleanup. * Fix Clippy * Removed mock feature flag and added mock_build * clippy * Now builds on the latest nightly * Also included clippy fixes introduced by new nightly * Apply Nick's comment * Apply Nick's comment * Now refers to sub-crates using paths (and removed `patch` section) - Previously refered to specific `crates.io` versions. - Motivation for this is to allow external projects to be able use specific revisions of this repo. Without this, a `[patch]` section is almost always required in the external project in order to force the internal plonky2 sub-crates to the same version, an approach which comes with its own issues. * Fixes * clippy * Made visibilities outside of crate to allow for forking partial witness gen outside of crate * latest nightly in CI and rust-toolchain * suppress incorrect Clippy error * fmt * clippy suggestions * clippy suggestions * clippy suggestions * clippy suggestions * clippy suggestions * more clippy suggestions * Move byte packing / unpacking to a distinct table (0xPolygonZero#1212) * Duplicate Memory trace into BytePacking one * Add mload_32bytes instruction * Use dedicated ops for byte packing trace * Change witness generation to reduce memory reads for MLOAD_32BYTES * Remove segments * Fix stack * Fix extra product when fixing CTL for byte_packing * Write output value in trace * Add constraints for BYTE_PACKING table * Add recursive constraints for BYTE_PACKING table * Fix constraints * Add address in trace and constraints * Add timestamp and batch inputs into BytePackingOp struct * Add extra column * Fix BytePackingStark CTL * Tiny fix in witness generation * Fix the Memory CTL * Add constraints for the new columns * Remove 1 column * Remove limb columns * Fix * Fix recursive circuit of BytePackingTable * Fix constraints * Fix endianness * Add MSTORE_32BYTES instruction and move decomposition to packing table * Add missing constraint * Add range-check for all bytes * Add extra constraint * Cleanup * Remove REMAINING_LEN column * Add corresponding implementations in interpreter * Fix recursive version * Remove debug assertion because of CI * Remove FILTER column * Update new test from rebasing * Reorder STARK modules to match TraceCheckPoint ordering * Address comments * Pacify clippy * Add documentation to the packing module * Fix doctest * Swap ordering in stack macro (0xPolygonZero#1230) * Swap ordering in stack macro * Update comment * Fix self_balance_gas_cost and basic_smart_contract. (0xPolygonZero#1227) * Fix self_balance_gas_cost and basic_smart_contract. * Fix Clippy * Combine arithmetic flags on the CPU side (0xPolygonZero#1187) * Combine FP254 flags * Combine basic binary ops together and do CTL with opcode value * Combine ternary ops together * Combine MUL DIV and MOD * Combine shift operations * Combine byte with other binary ops * Fix tests * Clean leftover comment * Update from latest main * Put the 'is_simulated' flag inside the Operation enum * Cleaner way to handle "simulated" operations SHL and SHR. * Fix comments. * Minor: suggestion for re-expressing `combined_ops`. * Update comment --------- Co-authored-by: Hamish Ivey-Law <[email protected]> * Remove redundant Keccak sponge cols (0xPolygonZero#1233) * Rename columns in KeccakSponge for clarity * Remove redundant columns * Apply comments * Combine mstore_general and mload_general into one flag (0xPolygonZero#1188) * Combine mstore_general and mload_general into one flag * Add comments and make stack constraints cleaner. * Fix number of native instructions * Ordering * Cleanup * Update calls to stack eval from latest main --------- Co-authored-by: Robin Salen <[email protected]> --------- Co-authored-by: Dimo99 <[email protected]> Co-authored-by: Hamish Ivey-Law <[email protected]> Co-authored-by: Robin Salen <[email protected]> Co-authored-by: Robin Salen <[email protected]> Co-authored-by: Jacqueline Nabaglo <[email protected]> Co-authored-by: Nicholas Ward <[email protected]> Co-authored-by: Linda Guiga <[email protected]> Co-authored-by: wborgeaud <[email protected]> Co-authored-by: Hamy Ratoanina <[email protected]> Co-authored-by: BGluth <[email protected]> Co-authored-by: Linda Guiga <[email protected]> Co-authored-by: John Guibas <[email protected]> Co-authored-by: Daniel Lubarov <[email protected]> Co-authored-by: Ayush Shukla <[email protected]> Co-authored-by: Uma Roy <[email protected]> Co-authored-by: Hamish Ivey-Law <[email protected]>

Nashtare added this to the Optimization - Phase 1 milestone Sep 6, 2023

Nashtare self-assigned this Sep 6, 2023

Nashtare force-pushed the packing branch 2 times, most recently from 5c53f01 to 7eef294 Compare September 6, 2023 19:23

Nashtare added 26 commits September 6, 2023 15:41

Duplicate Memory trace into BytePacking one

65e4220

Add mload_32bytes instruction

78c1333

Use dedicated ops for byte packing trace

bba1998

Change witness generation to reduce memory reads for MLOAD_32BYTES

eb15361

Remove segments

6844573

Fix stack

d82066e

Fix extra product when fixing CTL for byte_packing

0bb094f

Write output value in trace

bb2b04b

Add constraints for BYTE_PACKING table

c1ce80d

Add recursive constraints for BYTE_PACKING table

862c1df

Fix constraints

99ed5f0

Add address in trace and constraints

f72f901

Add timestamp and batch inputs into BytePackingOp struct

06eaf71

Add extra column

5c410ef

Fix BytePackingStark CTL

f7fe9ef

Tiny fix in witness generation

bbabe55

Fix the Memory CTL

fcb6920

Add constraints for the new columns

e88744a

Remove 1 column

3d9be52

Remove limb columns

017c29f

Fix

0193626

Fix recursive circuit of BytePackingTable

3e0a58e

Fix constraints

5827e84

Fix endianness

f3e84c1

Add MSTORE_32BYTES instruction and move decomposition to packing table

03d5614

Add missing constraint

2aa699a

Nashtare added 4 commits September 6, 2023 15:41

Add corresponding implementations in interpreter

ef420a3

Fix recursive version

41ddd55

Remove debug assertion because of CI

f350c22

Remove FILTER column

e2ca182

Nashtare force-pushed the packing branch from 7eef294 to 7ff1985 Compare September 6, 2023 19:41

Update new test from rebasing

ce47858

Nashtare force-pushed the packing branch from 7ff1985 to ce47858 Compare September 6, 2023 20:00

Nashtare changed the title ~~Move byte sequence packing / unpacking in a distinct table~~ Move byte packing / unpacking to a distinct table Sep 6, 2023

Reorder STARK modules to match TraceCheckPoint ordering

e526987

unzvfu self-requested a review September 7, 2023 09:12

Merge branch 'main' of https://github.com/mir-protocol/plonky2 into p…

83dc90d

…acking

unzvfu approved these changes Sep 12, 2023

View reviewed changes

Nashtare added 2 commits September 12, 2023 10:36

Address comments

1479ee3

Pacify clippy

a13f509

Add documentation to the packing module

ce09a63

Fix doctest

e9a837b

unzvfu merged commit 9508b49 into 0xPolygonZero:main Sep 12, 2023
2 checks passed

Nashtare deleted the packing branch September 12, 2023 21:49

Nashtare mentioned this pull request Sep 12, 2023

Swap ordering in stack macro #1230

Merged

Nashtare mentioned this pull request Sep 18, 2023

Reuse new packing instructions for MLOAD and MSTORE #1239

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move byte packing / unpacking to a distinct table #1212

Move byte packing / unpacking to a distinct table #1212

Nashtare commented Sep 6, 2023 •

edited

Loading

Nashtare commented Sep 6, 2023

unzvfu commented Sep 7, 2023

Nashtare commented Sep 7, 2023

Nashtare commented Sep 7, 2023

unzvfu left a comment

Nashtare commented Sep 12, 2023

Nashtare commented Sep 12, 2023

unzvfu commented Sep 12, 2023

Move byte packing / unpacking to a distinct table #1212

Move byte packing / unpacking to a distinct table #1212

Conversation

Nashtare commented Sep 6, 2023 • edited Loading

Nashtare commented Sep 6, 2023

unzvfu commented Sep 7, 2023

Nashtare commented Sep 7, 2023

Nashtare commented Sep 7, 2023

unzvfu left a comment

Choose a reason for hiding this comment

Nashtare commented Sep 12, 2023

Nashtare commented Sep 12, 2023

unzvfu commented Sep 12, 2023

Nashtare commented Sep 6, 2023 •

edited

Loading