Skip to content

Commit

Permalink
fix(avm): AVM circuit fixes related calldata, returndata and call_ptr (
Browse files Browse the repository at this point in the history
…#11207)

The AVM circuit code did not correctly compute col_offset (defined in
mem_slice.pil) in the context of multiple enqueued calls. In this case,
the calldata of these top-level calls are concatenated and therefore
col_offset needs to take into account the previous concatenated
calldata. We needed also to relax the constraint #[COL_OFFSET_INCREMENT]
which needs to be "reset" at call boundaries.

Similar fix applies for returndata.

In addition, we identified some missing call_ptr member in trace row of
several opcodes.
  • Loading branch information
jeanmon authored Jan 15, 2025
1 parent d1b57db commit 2f05dc0
Show file tree
Hide file tree
Showing 27 changed files with 1,547 additions and 1,504 deletions.
6 changes: 4 additions & 2 deletions barretenberg/cpp/pil/avm/gadgets/mem_slice.pil
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,10 @@ namespace slice(256);
sel_mem_active * (cnt - 1 - cnt') = 0;
#[ADDR_INCREMENT]
sel_mem_active * (addr + 1 - addr') = 0;
#[COL_OFFSET_INCREMENT]
sel_mem_active * (col_offset + 1 - col_offset') = 0;

// #[COL_OFFSET_INCREMENT]
// sel_mem_active * (col_offset + 1 - col_offset') = 0;

#[SAME_CLK]
sel_mem_active * (clk - clk') = 0;
#[SAME_SPACE_ID]
Expand Down

Large diffs are not rendered by default.

12 changes: 5 additions & 7 deletions barretenberg/cpp/src/barretenberg/vm/avm/generated/flavor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -813,12 +813,11 @@ AvmFlavor::AllConstRefValues::AllConstRefValues(
, slice_addr_shift(il[805])
, slice_clk_shift(il[806])
, slice_cnt_shift(il[807])
, slice_col_offset_shift(il[808])
, slice_sel_cd_cpy_shift(il[809])
, slice_sel_mem_active_shift(il[810])
, slice_sel_return_shift(il[811])
, slice_sel_start_shift(il[812])
, slice_space_id_shift(il[813])
, slice_sel_cd_cpy_shift(il[808])
, slice_sel_mem_active_shift(il[809])
, slice_sel_return_shift(il[810])
, slice_sel_start_shift(il[811])
, slice_space_id_shift(il[812])
{}

AvmFlavor::ProverPolynomials::ProverPolynomials(ProvingKey& proving_key)
Expand Down Expand Up @@ -1643,7 +1642,6 @@ AvmFlavor::AllConstRefValues AvmFlavor::ProverPolynomials::get_row(size_t row_id
slice_addr_shift[row_idx],
slice_clk_shift[row_idx],
slice_cnt_shift[row_idx],
slice_col_offset_shift[row_idx],
slice_sel_cd_cpy_shift[row_idx],
slice_sel_mem_active_shift[row_idx],
slice_sel_return_shift[row_idx],
Expand Down
7 changes: 4 additions & 3 deletions barretenberg/cpp/src/barretenberg/vm/avm/generated/flavor.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,11 @@ class AvmFlavor {

static constexpr size_t NUM_PRECOMPUTED_ENTITIES = 21;
static constexpr size_t NUM_WITNESS_ENTITIES = 743;
static constexpr size_t NUM_SHIFTED_ENTITIES = 50;
static constexpr size_t NUM_SHIFTED_ENTITIES = 49;
static constexpr size_t NUM_WIRES = NUM_WITNESS_ENTITIES + NUM_PRECOMPUTED_ENTITIES;
// We have two copies of the witness entities, so we subtract the number of fixed ones (they have no shift), one for
// the unshifted and one for the shifted
static constexpr size_t NUM_ALL_ENTITIES = 814;
static constexpr size_t NUM_ALL_ENTITIES = 813;
// The total number of witnesses including shifts and derived entities.
static constexpr size_t NUM_ALL_WITNESS_ENTITIES = NUM_WITNESS_ENTITIES + NUM_SHIFTED_ENTITIES;

Expand Down Expand Up @@ -351,6 +351,7 @@ class AvmFlavor {
class VerificationKey : public VerificationKey_<PrecomputedEntities<Commitment>, VerifierCommitmentKey> {
public:
using FF = VerificationKey_::FF;
static constexpr size_t NUM_PRECOMPUTED_COMMITMENTS = NUM_PRECOMPUTED_ENTITIES;

VerificationKey() = default;

Expand All @@ -366,7 +367,7 @@ class AvmFlavor {

VerificationKey(const size_t circuit_size,
const size_t num_public_inputs,
std::array<Commitment, NUM_PRECOMPUTED_ENTITIES> const& precomputed_cmts)
std::array<Commitment, NUM_PRECOMPUTED_COMMITMENTS> const& precomputed_cmts)
: VerificationKey_(circuit_size, num_public_inputs)
{
for (auto [vk_cmt, cmt] : zip_view(this->get_all(), precomputed_cmts)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
namespace bb::avm {

class AvmProver {
public:
using Flavor = AvmFlavor;
using FF = Flavor::FF;
using PCS = Flavor::PCS;
Expand All @@ -20,8 +21,8 @@ class AvmProver {
using ProverPolynomials = Flavor::ProverPolynomials;
using CommitmentLabels = Flavor::CommitmentLabels;
using Transcript = Flavor::Transcript;
using Proof = HonkProof;

public:
explicit AvmProver(std::shared_ptr<ProvingKey> input_key, std::shared_ptr<PCSCommitmentKey> commitment_key);
AvmProver(AvmProver&& prover) = default;
virtual ~AvmProver() = default;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ template <typename FF_> class mem_sliceImpl {
public:
using FF = FF_;

static constexpr std::array<size_t, 11> SUBRELATION_PARTIAL_LENGTHS = { 2, 3, 3, 3, 3, 3, 3, 3, 4, 4, 4 };
static constexpr std::array<size_t, 10> SUBRELATION_PARTIAL_LENGTHS = { 2, 3, 3, 3, 3, 3, 3, 4, 4, 4 };

template <typename ContainerOverSubrelations, typename AllEntities>
void static accumulate(ContainerOverSubrelations& evals,
Expand Down Expand Up @@ -51,43 +51,36 @@ template <typename FF_> class mem_sliceImpl {
}
{
using Accumulator = typename std::tuple_element_t<5, ContainerOverSubrelations>;
auto tmp = (new_term.slice_sel_mem_active *
((new_term.slice_col_offset + FF(1)) - new_term.slice_col_offset_shift));
auto tmp = (new_term.slice_sel_mem_active * (new_term.slice_clk - new_term.slice_clk_shift));
tmp *= scaling_factor;
std::get<5>(evals) += typename Accumulator::View(tmp);
}
{
using Accumulator = typename std::tuple_element_t<6, ContainerOverSubrelations>;
auto tmp = (new_term.slice_sel_mem_active * (new_term.slice_clk - new_term.slice_clk_shift));
auto tmp = (new_term.slice_sel_mem_active * (new_term.slice_space_id - new_term.slice_space_id_shift));
tmp *= scaling_factor;
std::get<6>(evals) += typename Accumulator::View(tmp);
}
{
using Accumulator = typename std::tuple_element_t<7, ContainerOverSubrelations>;
auto tmp = (new_term.slice_sel_mem_active * (new_term.slice_space_id - new_term.slice_space_id_shift));
auto tmp = ((new_term.slice_sel_mem_active * new_term.slice_sel_mem_active_shift) *
(new_term.slice_sel_return - new_term.slice_sel_return_shift));
tmp *= scaling_factor;
std::get<7>(evals) += typename Accumulator::View(tmp);
}
{
using Accumulator = typename std::tuple_element_t<8, ContainerOverSubrelations>;
auto tmp = ((new_term.slice_sel_mem_active * new_term.slice_sel_mem_active_shift) *
(new_term.slice_sel_return - new_term.slice_sel_return_shift));
(new_term.slice_sel_cd_cpy - new_term.slice_sel_cd_cpy_shift));
tmp *= scaling_factor;
std::get<8>(evals) += typename Accumulator::View(tmp);
}
{
using Accumulator = typename std::tuple_element_t<9, ContainerOverSubrelations>;
auto tmp = ((new_term.slice_sel_mem_active * new_term.slice_sel_mem_active_shift) *
(new_term.slice_sel_cd_cpy - new_term.slice_sel_cd_cpy_shift));
tmp *= scaling_factor;
std::get<9>(evals) += typename Accumulator::View(tmp);
}
{
using Accumulator = typename std::tuple_element_t<10, ContainerOverSubrelations>;
auto tmp = (((FF(1) - new_term.slice_sel_mem_active) * new_term.slice_sel_mem_active_shift) *
(FF(1) - new_term.slice_sel_start_shift));
tmp *= scaling_factor;
std::get<10>(evals) += typename Accumulator::View(tmp);
std::get<9>(evals) += typename Accumulator::View(tmp);
}
}
};
Expand All @@ -108,16 +101,14 @@ template <typename FF> class mem_slice : public Relation<mem_sliceImpl<FF>> {
case 4:
return "ADDR_INCREMENT";
case 5:
return "COL_OFFSET_INCREMENT";
case 6:
return "SAME_CLK";
case 7:
case 6:
return "SAME_SPACE_ID";
case 8:
case 7:
return "SAME_SEL_RETURN";
case 9:
case 8:
return "SAME_SEL_CD_CPY";
case 10:
case 9:
return "SEL_MEM_INACTIVE";
}
return std::to_string(index);
Expand All @@ -128,12 +119,11 @@ template <typename FF> class mem_slice : public Relation<mem_sliceImpl<FF>> {
static constexpr size_t SR_SLICE_CNT_ZERO_TEST2 = 2;
static constexpr size_t SR_SLICE_CNT_DECREMENT = 3;
static constexpr size_t SR_ADDR_INCREMENT = 4;
static constexpr size_t SR_COL_OFFSET_INCREMENT = 5;
static constexpr size_t SR_SAME_CLK = 6;
static constexpr size_t SR_SAME_SPACE_ID = 7;
static constexpr size_t SR_SAME_SEL_RETURN = 8;
static constexpr size_t SR_SAME_SEL_CD_CPY = 9;
static constexpr size_t SR_SEL_MEM_INACTIVE = 10;
static constexpr size_t SR_SAME_CLK = 5;
static constexpr size_t SR_SAME_SPACE_ID = 6;
static constexpr size_t SR_SAME_SEL_RETURN = 7;
static constexpr size_t SR_SAME_SEL_CD_CPY = 8;
static constexpr size_t SR_SEL_MEM_INACTIVE = 9;
};

} // namespace bb::avm
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@
namespace bb::avm {

class AvmVerifier {
public:
using Flavor = AvmFlavor;
using FF = Flavor::FF;
using Commitment = Flavor::Commitment;
using VerificationKey = Flavor::VerificationKey;
using VerifierCommitmentKey = Flavor::VerifierCommitmentKey;
using Transcript = Flavor::Transcript;

public:
explicit AvmVerifier(std::shared_ptr<VerificationKey> verifier_key);
AvmVerifier(AvmVerifier&& other) noexcept;
AvmVerifier(const AvmVerifier& other) = delete;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,7 @@ AvmError Execution::execute_enqueued_call(AvmTraceBuilder& trace_builder,
.internal_return_ptr_stack = {},
};
trace_builder.next_context_id++;
trace_builder.update_calldata_size_values(static_cast<uint32_t>(enqueued_call_hint.calldata.size()));
// Find the bytecode based on contract address of the public call request
std::vector<uint8_t> bytecode;
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,20 @@ void AvmSliceTraceBuilder::create_calldata_copy_slice(std::vector<FF> const& cal
uint8_t space_id,
uint32_t col_offset,
uint32_t copy_size,
uint32_t direct_dst_offset)
uint32_t direct_dst_offset,
uint32_t top_calldata_offset)
{
create_slice(calldata, clk, space_id, col_offset, copy_size, direct_dst_offset, true);
create_slice(calldata, clk, space_id, col_offset, copy_size, direct_dst_offset, top_calldata_offset, true);
}

void AvmSliceTraceBuilder::create_return_slice(
std::vector<FF> const& returndata, uint32_t clk, uint8_t space_id, uint32_t direct_ret_offset, uint32_t ret_size)
void AvmSliceTraceBuilder::create_return_slice(std::vector<FF> const& returndata,
uint32_t clk,
uint8_t space_id,
uint32_t direct_ret_offset,
uint32_t ret_size,
uint32_t top_returndata_offset)
{
create_slice(returndata, clk, space_id, 0, ret_size, direct_ret_offset, false);
create_slice(returndata, clk, space_id, 0, ret_size, direct_ret_offset, top_returndata_offset, false);
}

void AvmSliceTraceBuilder::create_slice(std::vector<FF> const& col_data,
Expand All @@ -40,6 +45,7 @@ void AvmSliceTraceBuilder::create_slice(std::vector<FF> const& col_data,
uint32_t col_offset,
uint32_t copy_size,
uint32_t addr,
uint32_t top_column_offset,
bool rw)
{
for (uint32_t i = 0; i < copy_size; i++) {
Expand All @@ -48,15 +54,16 @@ void AvmSliceTraceBuilder::create_slice(std::vector<FF> const& col_data,
.space_id = space_id,
.addr_ff = FF(addr + i),
.val = col_data.at(col_offset + i),
.col_offset = col_offset + i,
.col_offset = top_column_offset + col_offset + i,
.cnt = copy_size - i,
.one_min_inv = FF(1) - FF(copy_size - i).invert(),
.sel_start = i == 0,
.sel_cd_cpy = rw,
.sel_return = !rw,
});

rw ? cd_lookup_counts[col_offset + i]++ : ret_lookup_counts[col_offset + i]++;
rw ? cd_lookup_counts[top_column_offset + col_offset + i]++
: ret_lookup_counts[top_column_offset + col_offset + i]++;
}

// Last extra row for a slice operation. cnt is zero and we have to add extra dummy
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,14 @@ class AvmSliceTraceBuilder {
uint8_t space_id,
uint32_t col_offset,
uint32_t copy_size,
uint32_t direct_dst_offset);
uint32_t direct_dst_offset,
uint32_t top_calldata_offset);
void create_return_slice(std::vector<FF> const& returndata,
uint32_t clk,
uint8_t space_id,
uint32_t direct_ret_offset,
uint32_t ret_size);
uint32_t ret_size,
uint32_t top_returndata_offset);

private:
std::vector<SliceTraceEntry> slice_trace;
Expand All @@ -54,6 +56,7 @@ class AvmSliceTraceBuilder {
uint32_t col_offset,
uint32_t copy_size,
uint32_t addr,
uint32_t top_column_offset,
bool rw);
};

Expand Down
Loading

1 comment on commit 2f05dc0

@AztecBot
Copy link
Collaborator

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'C++ Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.

Benchmark suite Current: 2f05dc0 Previous: 0a4b763 Ratio
wasmClientIVCBench/Full/6 80074.753179 ms/iter 71694.107147 ms/iter 1.12
wasmconstruct_proof_ultrahonk_power_of_2/20 14569.986035 ms/iter 13539.790007 ms/iter 1.08

This comment was automatically generated by workflow using github-action-benchmark.

CC: @ludamad @codygunton

Please sign in to comment.