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.

Sign up for GitHub

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

Jump to bottom

feat: read_calldata #5409

Merged
merged 3 commits into from
Mar 23, 2024
Merged

feat: read_calldata #5409

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
23bab36
add a read_caldata method
ledwards2225 Mar 22, 2024
c608847
change naming from lookup to read for consistency
ledwards2225 Mar 22, 2024
641ce83
Merge branch 'master' into lde/calldata_read
ledwards2225 Mar 22, 2024
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
33 changes: 28 additions & 5 deletions ...enberg/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,7 @@ template <typename FF> void GoblinUltraCircuitBuilder_<FF>::add_gates_to_ensure_
add_public_calldata(FF(25)); // ensure there is at least one entry in calldata
uint32_t raw_read_idx = 0; // read first entry in calldata
auto read_idx = this->add_variable(raw_read_idx);
FF calldata_value = this->get_variable(public_calldata[raw_read_idx]);
auto value_idx = this->add_variable(calldata_value);
create_calldata_lookup_gate({ read_idx, value_idx });
// TODO(https://github.com/AztecProtocol/barretenberg/issues/821): automate updating of read counts
calldata_read_counts[raw_read_idx]++;
read_calldata(read_idx);

// mock a poseidon external gate, with all zeros as input
this->blocks.poseidon_external.populate_wires(this->zero_idx, this->zero_idx, this->zero_idx, this->zero_idx);
Expand Down Expand Up @@ -220,6 +216,33 @@ template <typename FF> void GoblinUltraCircuitBuilder_<FF>::set_goblin_ecc_op_co
equality_op_idx = this->put_constant_variable(FF(EccOpCode::EQUALITY));
}

/**
* @brief Read from calldata
* @details Creates a calldata lookup gate based on the read data
*
* @tparam FF
* @param read_idx_witness_idx Variable index of the read index
* @return uint32_t Variable index of the result of the read
*/
template <typename FF> uint32_t GoblinUltraCircuitBuilder_<FF>::read_calldata(const uint32_t& read_idx_witness_idx)
{
// Get the raw index into the calldata
const uint32_t read_idx = static_cast<uint32_t>(uint256_t(this->get_variable(read_idx_witness_idx)));

// Ensure that the read index is valid
ASSERT(read_idx < public_calldata.size());

// Create a variable corresponding to the result of the read. Note that we do not in general connect reads from
// calldata via copy constraints (i.e. we create a unique variable for the result of each read)
FF calldata_value = this->get_variable(public_calldata[read_idx]);
uint32_t value_witness_idx = this->add_variable(calldata_value);

create_calldata_lookup_gate({ read_idx_witness_idx, value_witness_idx });
calldata_read_counts[read_idx]++;

return value_witness_idx;
}

/**
* @brief Create a calldata lookup/read gate
*
Expand Down
3 changes: 2 additions & 1 deletion ...enberg/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ template <typename FF> class GoblinUltraCircuitBuilder_ : public UltraCircuitBui
void populate_ecc_op_wires(const ecc_op_tuple& in);
ecc_op_tuple decompose_ecc_operands(uint32_t op, const g1::affine_element& point, const FF& scalar = FF::zero());
void set_goblin_ecc_op_code_constant_variables();
void create_calldata_lookup_gate(const databus_lookup_gate_<FF>& in);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving this to the private access specifier to enforce the use of read_calldata as the interface. This method is called from within read_calldata.


public:
GoblinUltraCircuitBuilder_(const size_t size_hint = 0,
Expand Down Expand Up @@ -136,7 +137,7 @@ template <typename FF> class GoblinUltraCircuitBuilder_ : public UltraCircuitBui
return index;
}

void create_calldata_lookup_gate(const databus_lookup_gate_<FF>& in);
uint32_t read_calldata(const uint32_t& read_idx_witness_idx);

void create_poseidon2_external_gate(const poseidon2_external_gate_<FF>& in);
void create_poseidon2_internal_gate(const poseidon2_internal_gate_<FF>& in);
Expand Down
1 change: 0 additions & 1 deletion barretenberg/cpp/src/barretenberg/sumcheck/instance/prover_instance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ void ProverInstance_<Flavor>::construct_databus_polynomials(Circuit& circuit)
// Note: We do not utilize a zero row for databus columns
for (size_t idx = 0; idx < circuit.public_calldata.size(); ++idx) {
public_calldata[idx] = circuit.get_variable(circuit.public_calldata[idx]);
// TODO(https://github.com/AztecProtocol/barretenberg/issues/821): automate updating of read counts
calldata_read_counts[idx] = circuit.calldata_read_counts[idx];
}

Expand Down
52 changes: 18 additions & 34 deletions barretenberg/cpp/src/barretenberg/ultra_honk/databus_composer.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,6 @@ class DataBusComposerTests : public ::testing::Test {

using Curve = curve::BN254;
using FF = Curve::ScalarField;
using Point = Curve::AffineElement;
using CommitmentKey = bb::CommitmentKey<Curve>;

/**
* @brief Generate a simple test circuit that includes arithmetic and goblin ecc op gates
*
* @param builder
*/
void generate_test_circuit(auto& builder)
{
// Add some ecc op gates and arithmetic gates
GoblinMockCircuits::construct_simple_circuit(builder);
}
};

/**
Expand All @@ -49,39 +36,36 @@ TEST_F(DataBusComposerTests, CallDataRead)
{
auto op_queue = std::make_shared<bb::ECCOpQueue>();

// Add mock data to op queue to simulate interaction with a previous circuit
GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit(op_queue);

auto builder = GoblinUltraCircuitBuilder{ op_queue };

// Create a general test circuit
generate_test_circuit(builder);
// Add some ecc op gates and arithmetic gates
GoblinMockCircuits::construct_simple_circuit(builder);

// Add some values to calldata and store the corresponding witness index
std::array<FF, 5> calldata_values = { 7, 10, 3, 12, 1 };
// Add some values to calldata
std::vector<FF> calldata_values = { 7, 10, 3, 12, 1 };
for (auto& val : calldata_values) {
builder.add_public_calldata(val);
}

// Define some indices at which to read calldata
std::array<uint32_t, 2> read_index_values = { 1, 4 };
// Define some raw indices at which to read calldata (these will be ASSERTed to be valid)
std::vector<uint32_t> read_indices = { 1, 4 };

// Create some calldata lookup gates
for (uint32_t& read_index : read_index_values) {
// Create some calldata read gates. (Normally we'd use the result of the read. Example of that is below)
for (uint32_t& read_idx : read_indices) {
// Create a variable corresponding to the index at which we want to read into calldata
uint32_t read_idx_witness_idx = builder.add_variable(read_index);
uint32_t read_idx_witness_idx = builder.add_variable(read_idx);

// Create a variable corresponding to the result of the read. Note that we do not in general connect reads from
// calldata via copy constraints (i.e. we create a unique variable for the result of each read)
ASSERT_LT(read_index, builder.public_calldata.size());
FF calldata_value = builder.get_variable(builder.public_calldata[read_index]);
uint32_t value_witness_idx = builder.add_variable(calldata_value);

builder.create_calldata_lookup_gate({ read_idx_witness_idx, value_witness_idx });
// TODO(https://github.com/AztecProtocol/barretenberg/issues/821): automate updating of read counts
builder.calldata_read_counts[read_index]++;
builder.read_calldata(read_idx_witness_idx);
}

// In general we'll want to use the result of a calldata read in another operation. Here's an example using
// an add gate to show that the result of the read is as expected:
uint32_t read_idx = 2;
FF expected_result = calldata_values[read_idx];
uint32_t read_idx_witness_idx = builder.add_variable(read_idx);
uint32_t result_witness_idx = builder.read_calldata(read_idx_witness_idx);
builder.create_add_gate({ result_witness_idx, builder.zero_idx, builder.zero_idx, 1, 0, 0, -expected_result });
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this is a stupid question cus i'm missing sth - but is it in the 'responsabilities' of the circuit writer to ensure the calldata_value corresponds to the calldata_read? just wondering if this gate shouldn't be implicitly added by the circuit writer somewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand your question correctly, this is exactly the purpose of the calldata read gate - it shows that the value read is indeed present in the calldata at the corresponding index

Copy link
Contributor

Choose a reason for hiding this comment

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

So this final add gate here is solely for testing purposes

Copy link
Contributor

Choose a reason for hiding this comment

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

I was just wondering whether read_calldata actually needs to return the index because I only see it being used in testing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that’s what I was trying to get at in the comments. In practice the whole point of doing the read is to then use the result somewhere else. I realize now my choice of gate was a little misleading, but it was meant to just be an arbitrary add gate. But there would be no reason to ever do a read unless you use the result. I’ll add a comment to that effect to the test. There will be many more incremental PRs in the near future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value proposition of the databus is it allows us to pass large amounts of data from circuit to circuit but only pay a gate for the values we actually want to read. This is valuable since the nature of the data is that we only want to read a small amount of what’s actually there, the rest is just being carried on for future use

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explaining!


// Construct and verify Honk proof
auto instance = std::make_shared<ProverInstance_<GoblinUltraFlavor>>(builder);
// For debugging, use "instance_inspector::print_databus_info(instance)"
Expand Down
Loading