Skip to content

Separate Bit Transpose from Bittable - #137

Open
xrvdg wants to merge 23 commits into
mainfrom
xr/bittable
Open

xrvdg wants to merge 23 commits into
mainfrom
xr/bittable

Conversation

@xrvdg

@xrvdg xrvdg commented Sep 28, 2026 •

Copy link
Copy Markdown
Collaborator

Preparation for the lookup table optimisation for GKR, which builds lookups from short bit strings during tree construction and the sumcheck rounds. That needs bit layouts that no protocol Shape describes.

The witness is now read as u128 words, with StepIter yielding them S bits at a time. BitTable stays the shaped view of the witness, and its transpose returns a geometry-only BitMatrix that GrandProductCircuit::new builds the leaves from.

Tables with fewer than 128 columns are now rejected by BitZParams instead of taking the prover's bit-at-a-time fallback; production shapes never have them. Commit 336e809 has a more general transpose that also takes rows shorter than a word, which might be useful later but isn't needed now.

BitTable has been split into BitTable (protocol specific) and BitMatrix (bit operations) to not have the extra restrictions when building the lookup table.

@xrvdg
xrvdg marked this pull request as draft September 28, 2026 10:02
@xrvdg xrvdg changed the title Bit transpose for all power of twos Bit transpose and bit indexing for all power of twos Sep 28, 2026
@xrvdg
xrvdg marked this pull request as ready for review October 5, 2026 12:06
Comment thread crates/common/src/params.rs Outdated
@xrvdg
xrvdg requested a review from zkfriendly October 5, 2026 12:14
xrvdg added 4 commits October 6, 2026 14:37
… WordColumns view and let transpose take fewer than 128 columns
…y BitMatrix, moving the t >= 7 gate back to Shape
… BitZParams

Production never transposes a table with fewer than 128 columns: the CLI
builds t = 7, s = m - 7 shapes and the reference split has s >= 8. Go
back to origin/main's 128x128 block kernel and drop the small-dim1
machinery (bit_transpose_blocks, rotate_bit_index, rotation_swaps and
StepIter::preloaded) together with its tests.

That version lives in 336e809. The
bit_transpose doc comment points there and notes that it extends to
dim2 < 128 if needed.

BitZParams::new now rejects s < 7 with ParamsError::ColumnCountTooNarrow
rather than letting the reduction panic in the transpose. It is not a
protocol bound but a limit of the kernel, and restoring that version
lifts it.
@xrvdg xrvdg changed the title Bit transpose and bit indexing for all power of twos Separate Bit Transpose from Bittable Oct 7, 2026
xrvdg added 2 commits October 8, 2026 11:52
…d let GrandProductCircuit::new take the table

BitTable::as_matrix wraps the table's words as an [columns][rows]
BitMatrix without moving a bit, and transposing goes only through
BitMatrix::transpose. To borrow the words, BitMatrix now holds a
Cow<'a, [Word]>; transpose always returns an owned BitMatrix<'static>.
The crate-private transposed helper is gone, and BitMatrix computes no
ilog2.

GrandProductCircuit::new takes the BitTable and does
as_matrix().transpose() itself, so the prover and the verifier test no
longer transpose before building the circuit.

BitMatrix::bit was only read by tests, so it moves into the matrix test
module as the oracle there; the as_matrix test reads columns through
bits() instead.

@zkfriendly zkfriendly left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, the PR looks clean and correct. Left some minor comments!

Comment thread crates/gkr/src/lib.rs
impl GrandProductCircuit {
// Fails if the leafs are 0.
pub fn new(mut leafs: Vec<Field>) -> Self {
pub fn new(row_images: &[F128], bittable: &BitTable) -> Self {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We are assuming row_images has the same length as bittable rows. Worth explicitly checking it?

assert_eq!(row_images.len(), bittable.shape().rows());

Comment thread crates/gkr/src/lib.rs
Comment on lines +350 to +351
let dim2 = matrix.dim2();
let dim = matrix.dim1() * dim2;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

After transposition, dim2 means columns and dim is num_leaves. It would be easier to follow if use the more descriptive names

let packed = packed.into();
let bits = packed.len() * BITS;
assert!(
bits.is_power_of_two() && (1 << log_dim2) <= bits,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

1 << log_dim2 can wrap in optimized builds, so the check can accept dimensions larger than the available bits. We could do log_dim2 <= bits.ilog2() instead.

Comment on lines +13 to +15
const DIM1: usize = 128;
/// `2^20` bits per row: a 16 MiB matrix, well past the last-level cache.
const DIM2: usize = 1 << 20;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The CLI uses 128 for rows, and 1 << 20 columns, and I understand DIM1 here is the columns, in that case, this benchmark should have DIM1 = 1 << 20 and DIM2 = 128 to represent realistic witnesses

/// anything but a checked parameter set.
pub fn table<'a>(&self, packed: &'a [F128]) -> Result<BitTable<'a>, TableError> {
BitTable::new(self.shape, packed)
BitTable::new(self.shape, bytemuck::cast_slice(packed))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Doesn't cast_slice depend on byte order? seems to me that this will not work properly on big endian machines.

out
}

/// Benchmark-only access to the private transposes; see `benches/table.rs`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

file was renamed to benches/matrix.rs

impl<'a> BitTable<'a> {
pub const BITS: usize = Word::BITS as usize;

/// Wraps `packed` in `shape`, least significant bit first inside `lo`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

outdated comment here, lo no longer exists

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.

2 participants