Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 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
Original file line number Diff line number Diff line change
Expand Up @@ -118,16 +118,12 @@ void process_ROM_operations(Builder& builder,
// For a ROM table, constant read should be optimized out:
// The rom_table won't work with a constant read because the table may not be initialized
ASSERT(op.index.q_l != 0);
// We create a new witness w to avoid issues with non-valid witness assignements:
// if witness are not assigned, then w will be zero and table[w] will work
fr w_value = 0;
if (has_valid_witness_assignments) {
// If witness are assigned, we use the correct value for w
w_value = index.get_value();

Copy link
Contributor

Choose a reason for hiding this comment

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

much better! can you update the comment to say why this is done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's a comment inside the if. but will add some extra explanation

if (!has_valid_witness_assignments) {
// If witness are not assigned, we set the index value to 0
builder.set_variable(index.witness_index, 0);
}
field_ct w = field_ct::from_witness(&builder, w_value);
value.assert_equal(table[w]);
w.assert_equal(index);
value.assert_equal(table[index]);
}
}

Expand All @@ -145,11 +141,10 @@ void process_RAM_operations(Builder& builder,
field_ct value = poly_to_field_ct(op.value, builder);
field_ct index = poly_to_field_ct(op.index, builder);

// We create a new witness w to avoid issues with non-valid witness assignements.
// If witness are not assigned, then index will be zero and table[index] won't hit bounds check.
fr index_value = has_valid_witness_assignments ? index.get_value() : 0;
// Create new witness and ensure equal to index.
field_ct::from_witness(&builder, index_value).assert_equal(index);
if (!has_valid_witness_assignments) {
// If witness are not assigned, we set the index value to 0
builder.set_variable(index.witness_index, 0);
}

if (op.access_type == 0) {
value.assert_equal(table.read(index));
Expand Down Expand Up @@ -179,14 +174,11 @@ void process_call_data_operations(Builder& builder,
BB_ASSERT_EQ(op.access_type, 0);
field_ct value = poly_to_field_ct(op.value, builder);
field_ct index = poly_to_field_ct(op.index, builder);
fr w_value = 0;
if (has_valid_witness_assignments) {
// If witness are assigned, we use the correct value for w
w_value = index.get_value();
if (!has_valid_witness_assignments) {
// If witness are not assigned, we set the index value to 0
builder.set_variable(index.witness_index, 0);
}
field_ct w = field_ct::from_witness(&builder, w_value);
value.assert_equal(calldata_array[w]);
w.assert_equal(index);
value.assert_equal(calldata_array[index]);
}
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ field_t<Builder> databus<Builder>::bus_vector::operator[](const field_pt& index)
if (index.is_constant()) {
index_witness_idx = context->put_constant_variable(index.get_value());
} else {
index_witness_idx = index.normalize().get_witness_index();
index_witness_idx = index.get_normalized_witness_index();
}

// Read from the bus vector at the specified index. Creates a single read gate
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,102 @@ TEST(Databus, CallDataAndReturnData)
EXPECT_TRUE(CircuitChecker::check(builder));
}

/**
* @brief An expository test demonstrating the functionality of the databus in a small use case when the entries are
* constant witnesses
*/
TEST(Databus, ConstantEntryAccess)
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if the read indices are constant witnesses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

handled in another test. I chucked both cases for the indices in the same test.

{

Builder builder;
databus_ct databus;
fr value_0 = 13;
fr value_1 = 12;
auto constant_0 = witness_ct::create_constant_witness(&builder, value_0);
auto constant_1 = witness_ct::create_constant_witness(&builder, value_1);
databus.return_data.set_values({ constant_0, constant_1 });
field_ct idx_0(witness_ct(&builder, 0));
field_ct idx_1(witness_ct(&builder, 1));

field_ct read_result_0 = databus.return_data[idx_0];
field_ct read_result_1 = databus.return_data[idx_1];

EXPECT_EQ(value_0, read_result_0.get_value());
EXPECT_EQ(value_1, read_result_1.get_value());
EXPECT_TRUE(CircuitChecker::check(builder));
}

/**
* @brief An expository test demonstrating the functionality of the databus in a small use case when the entries of the
* bus_vector are not normalized
*/
TEST(Databus, UnnormalizedEntryAccess)
{

Builder builder;
databus_ct databus;
std::array<fr, 3> raw_calldata_entries = { 3, 2, 1 };
std::array<fr, 3> raw_returndata_entries = { 3, 2, 1 };
std::vector<field_ct> calldata_entries;
for (fr entry : raw_calldata_entries) {
calldata_entries.emplace_back(witness_ct(&builder, entry));
field_ct entry_witness = witness_ct(&builder, entry);
}
std::vector<field_ct> returndata_entries;
for (fr entry : raw_returndata_entries) {
field_ct entry_witness = witness_ct(&builder, entry);
// add the value to itself to make it unnormalized (the multiplicative constant will be 2)
returndata_entries.emplace_back(entry_witness + entry_witness);
}
databus.calldata.set_values(calldata_entries);
databus.return_data.set_values(returndata_entries);
field_ct idx_0 = witness_ct(&builder, 0);
field_ct idx_1 = witness_ct(&builder, 1);
field_ct idx_2 = witness_ct(&builder, 2);
databus.return_data[idx_0].assert_equal(databus.calldata[idx_0] + databus.calldata[idx_0]);
databus.return_data[idx_1].assert_equal(databus.calldata[idx_1] + databus.calldata[idx_1]);
databus.return_data[idx_2].assert_equal(databus.calldata[idx_2] + databus.calldata[idx_2]);
EXPECT_TRUE(CircuitChecker::check(builder));
}

/**
* @brief An expository test demonstrating the functionality of the databus in a small use case where the indices are
* constant and/or unnormalized
*/
TEST(Databus, ConstantAndUnnormalizedIndices)
Copy link
Contributor

Choose a reason for hiding this comment

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

ah!

{
Builder builder;
databus_ct databus;
std::array<fr, 3> raw_calldata_values = { 54, 32, 30 };
std::array<fr, 3> raw_returndata_values = { 54, 32, 116 };
// Populate the calldata in the databus
std::vector<field_ct> calldata_values;
for (auto& value : raw_calldata_values) {
calldata_values.emplace_back(witness_ct(&builder, value));
}
databus.calldata.set_values(calldata_values);

// Populate the return data in the databus
std::vector<field_ct> returndata_values;
for (auto& value : raw_returndata_values) {
returndata_values.emplace_back(witness_ct(&builder, value));
}
databus.return_data.set_values(returndata_values);

// constant first index
field_ct idx_0(witness_ct::create_constant_witness(&builder, 0));
field_ct idx_1(witness_ct(&builder, 1));
// un-normalized index (with multiplicative constant 2)
field_ct idx_2 = idx_1 + idx_1;
field_ct sum = databus.calldata[idx_0] + databus.calldata[idx_1] + databus.calldata[idx_2];

databus.return_data[idx_0].assert_equal(databus.calldata[idx_0]);
databus.return_data[idx_1].assert_equal(databus.calldata[idx_1]);
databus.return_data[idx_2].assert_equal(sum);

EXPECT_TRUE(CircuitChecker::check(builder));
}

/**
* @brief A failure test demonstrating that trying to prove (via a databus read) that an erroneous value is present in
* the databus will result in an invalid witness.
Expand Down Expand Up @@ -179,4 +275,4 @@ TEST(Databus, DuplicateRead)
databus.return_data[idx_1];

EXPECT_TRUE(CircuitChecker::check(builder));
}
}