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

Block Machine: Handle case where number of rows is just enough #2070

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions executor/src/witgen/machines/block_machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,13 @@ impl<'a, T: FieldElement> Machine<'a, T> for BlockMachine<'a, T> {
This might violate some internal constraints."
);
}
self.degree = compute_size_and_log(&self.name, self.data.len(), self.degree_range);
self.degree = compute_size_and_log(
&self.name,
// At this point, the still contains the dummy block, which will be removed below.
georgwiese marked this conversation as resolved.
Show resolved Hide resolved
// Therefore, we subtract the block size here.
Copy link
Member

Choose a reason for hiding this comment

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

This feels kinda hacky..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, so I think the hacky part is that the data has a block that will be removed later, right? Which was the case before this PR?

I tried refactoring it, but it's kinda complicated. LMK if you have a suggestion.

self.data.len() - self.block_size,
self.degree_range,
);

if matches!(self.connection_type, ConnectionKind::Permutation) {
// We have to make sure that *all* selectors are 0 in the dummy block,
Expand Down Expand Up @@ -465,7 +471,7 @@ impl<'a, T: FieldElement> BlockMachine<'a, T> {
));
}

if self.rows() + self.block_size as DegreeType >= self.degree {
if self.rows() + self.block_size as DegreeType > self.degree {
return Err(EvalError::RowsExhausted(self.name.clone()));
}

Expand Down Expand Up @@ -541,7 +547,7 @@ impl<'a, T: FieldElement> BlockMachine<'a, T> {
/// unused cells in the previous block.
fn append_block(&mut self, mut new_block: FinalizableData<T>) -> Result<(), EvalError<T>> {
assert!(
(self.rows() + self.block_size as DegreeType) < self.degree,
(self.rows() + self.block_size as DegreeType) <= self.degree,
"Block machine is full (this should have been checked before)"
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ impl<'a, T: FieldElement> DoubleSortedWitnesses16<'a, T> {
assignments = assignments.report_side_effect();
}

if self.trace.len() >= (self.degree as usize) {
if self.trace.len() > (self.degree as usize) {
return Err(EvalError::RowsExhausted(self.name.clone()));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ impl<'a, T: FieldElement> DoubleSortedWitnesses32<'a, T> {
assignments = assignments.report_side_effect();
}

if self.trace.len() >= (self.degree as usize) {
if self.trace.len() > (self.degree as usize) {
return Err(EvalError::RowsExhausted(self.name.clone()));
}

Expand Down
1 change: 1 addition & 0 deletions parser/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ mod test {
};
eprint!("\t{sign}{change}");
}
eprintln!("The following string was re-parsed:\n{orig_asm_to_string}");
georgwiese marked this conversation as resolved.
Show resolved Hide resolved
panic!("parsed and re-parsed ASTs differ for file: {file}");
}
}
Expand Down
8 changes: 8 additions & 0 deletions pipeline/tests/asm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,14 @@ fn sqrt_asm() {
regular_test_without_small_field(f, &i);
}

#[test]
fn block_machine_exact_number_of_rows_asm() {
let f = "asm/block_machine_exact_number_of_rows.asm";
// This test needs machines to be of unequal length. Also, this is mostly testing witgen, so
// we just run one backend that supports variable-length machines.
test_plonky3_with_backend_variant::<GoldilocksField>(f, Vec::new(), BackendVariant::Monolithic);
}

#[test]
fn challenges_asm() {
let f = "asm/challenges.asm";
Expand Down
33 changes: 33 additions & 0 deletions test_data/asm/block_machine_exact_number_of_rows.asm
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
use std::machines::binary::ByteBinary;
use std::machines::large_field::binary::Binary;

// This test is a simpler version of: test_data/std/binary_large_test.asm
// It tests that witness generation for block machines also works when the number of rows
// is exactly what's needed to fit the required number of blocks.
// The large-field Binary machine is a good test-case, because it has an irregular (i.e.,
// non-rectangular) block shape.

// Currently, we need min_degree != max_degree, otherwise the linker sets all degrees
// to the main degree.
machine Main with min_degree: 32, max_degree: 64 {
reg pc[@pc];
reg X0[<=];
reg X1[<=];
reg X2[<=];
reg A;

ByteBinary byte_binary;
// We'll call the binary machine twice and the block size
// is 4, so we need exactly 8 rows.
Binary binary(byte_binary, 8, 8);

instr and X0, X1 -> X2 link ~> X2 = binary.and(X0, X1);

function main {

A <== and(0xaaaaaaaa, 0xaaaaaaaa);
A <== and(0xffffffff, 0xffffffff);

return;
}
}
Loading