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

feat(avm2): avm redesign init #10906

Merged
merged 6 commits into from
Jan 11, 2025
Merged

feat(avm2): avm redesign init #10906

merged 6 commits into from
Jan 11, 2025

Conversation

fcarreiro
Copy link
Contributor

@fcarreiro fcarreiro commented Dec 20, 2024

This is a redesign of the witgen/proving part of the AVM. There's still a lot of work to be done, but I have to merge at some point to let others contribute :). Most of the content is PoC, not supposed to be real.

We'll eventually have a doc explaining everything, but for now, some highlights:

Architecture

The proving process is now divided in 3 parts:

  • Simulation (aka event generation): Intrinsically sequential. Executes bytecode and generates packed information (events) that summarize what happened. Examples would be a bytecode decomposition event, memory access event, etc. This part has no dependencies on BB or PIL beyond FF. It also has, in principle, no knowledge of the circuit or columns.
  • Trace generation: This part is parallelizable. The meat of it is translating events into columns in a (sparse!) trace. It is the glue between events and the circuit. It has knowledge of the columns, but not really about any relation or constrain (**) or PIL.
  • Constraining: This is parallelizable. It's the actual constraining/proving/check circuit. It's dependent on BB and the (currently) autogenerated relations from PIL. We convert the sparse trace to polynomials.

Possible future standalone simulation

Hints and DB accesses: The simulation/witgen process has no knowledge of hints (so far). We define a DB interface which the simulation process uses. This DB is then "seeded" with hints. This means that in the future it should be possible to switch the DB to a real DB and things should "just work™️".

I think we should try to follow this philosophy as much as possible and not rely on TS hints that we can compute ourselves.

Configurability: Other aspects of simulation are configurable. E.g., we can configure a fast simulation only variant that does no event generation and no bytecode hashing whereas for full proving you would do that (incurring in at least 25ms for a single bytecode hashing).

Philosophy

Dependency injection is used everywhere (without framework). You'll see references stored in classes and may not like it, but it's actually working well. See https://www.youtube.com/watch?v=kCYo2gJ3Y38 as well.

There are lots of interfaces for mocking. Blame C++ 🤷 .

I'm making it a priority to have the right separation of concerns and engineering practices. There's zero tolerance on hacks. If we need a hack, we trigger a refactor.

Testing

Whereas before our tests required setting up everything and basically do full proving or check circuit, now everything can be tested separately. We use a mockist approach (common in C++). Our old tests would take ~0.5s each, now they take microseconds. Simulation, tracegen, and constraining can be tested separate from each other. In particular, you can create tests for constraints at the relation or subrelation level.

Lookups/permutations

Not really supported yet. But you don't need to keep counts for lookups.

TS/C++ communication

AVM inputs are now (de)serialized with messagepack.

(**) It does require lookup/permutation settings.

Copy link
Contributor Author

fcarreiro commented Dec 20, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

const auto [contract_address, _] = memory.get(addr);
std::vector<FF> calldata = {};

auto nested_context = context_provider.make(std::move(contract_address),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can still choose to make a recursive call to execute here. Depends on how we set up things but I suspect that will be the case. We will likely still need a context stack though, we need to manage it explicitly so that we have the events we need.

@fcarreiro fcarreiro changed the base branch from fc/pilcom-extra-lookup-info to graphite-base/10906 December 20, 2024 12:40
@fcarreiro fcarreiro changed the base branch from graphite-base/10906 to master December 20, 2024 12:42
U128,
};

using MemoryAddress = std::size_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is the right choice to take size_t as it is a platform-specific type. It is the primitive type to address offsets in vector or array, but in our case if we use a map, we might well use uint32_t or explicitly uint64_t (with explicit memory bound checks)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, solved!

@fcarreiro fcarreiro added the C-avm Component: AVM related tickets (aka public VM) label Jan 6, 2025 — with Graphite App
@fcarreiro fcarreiro force-pushed the fc/vm2-prototype branch 3 times, most recently from ddca9ac to 1a2daf1 Compare January 6, 2025 17:38
namespace execution(256);

pol commit stack_pointer_val;
pol commit stack_pointer_tag;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if the naming "stack pointer" is fully appropriate compared to sthg not referring stack such as "base address". I understand that it comes from the way the brillig compiler works.

@@ -0,0 +1,16 @@
namespace alu(256);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is everywhere and a leftover but the "256" is ignored by our toolchain. IIRC this is the trace length in powdr.
Maybe we should decide how to change as this is misleading.

std::pair<AvmAPI::AvmProof, AvmAPI::AvmVerificationKey> AvmAPI::prove(const AvmAPI::ProvingInputs& inputs)
{
// Simulate.
info("Simulating...");
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that these tracing occurences at info level will be migrated to verbose level?

@@ -0,0 +1,19 @@
#pragma once

#include "barretenberg/vm2/common/ankerl_map.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent to have found this lib!

namespace bb::avm2::constraining {

// This is a version of check circuit that runs on the prover polynomials.
// Better versions could be done, but since this is for debugging only, it is enough for now.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you briefly mention the potential improvements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In principle you may be able to do check circuit over FFs instead of polynomials, and you could maybe do it over the Trace without creating the polynomials. However, tbh, using the polynomials might be the best since it's the closest to "real proving".

// TODO: My dispatch system makes me ahve a uint8_t tag. Rethink.
void Execution::set(ContextInterface& context, MemoryAddress dst_addr, uint8_t tag, MemoryValue value)
{
context.get_memory().set(dst_addr, std::move(value), static_cast<MemoryTag>(tag));
Copy link
Contributor

Choose a reason for hiding this comment

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

Following clangd warning:

Std::move of the variable 'value' of the trivially-copyable type 'MemoryValue' (aka 'bb::field<bb::Bn254FrParams>') has no effect; remove std::move()

This warning appears to occurences of std::move over a variable of FF type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah I have a lot of those, I need to think about it a bit more, but they don't hurt. See https://stackoverflow.com/questions/49241201/is-it-undesirable-to-defensively-apply-stdmove-to-trivially-copyable-types

public:
AvmTraceGenHelper() = default;

tracegen::TraceContainer generate_trace(simulation::EventsContainer&& events);
Copy link
Contributor

Choose a reason for hiding this comment

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

Both generate_trace and generate_precomputed_columns can be made static.
Do not change this if you are sure that we will need an object state in AvmTraceGenHelper though.

**Current state**

- Both lookups and permutations are supported.
- When you lookup into a precomputed table, you need to define a fast way to find the row for a tuple. See bitwise example.
Copy link
Contributor

Choose a reason for hiding this comment

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

"you need to define a fast way to find the row for a tuple" Is it meant here an implementation of find_in_dst()
if so, it would be nice mention it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in follow up PR.

// We use an RLC for the key instead of the tuple, to save memory.
// FIXME: reconsider, what if beta is 0.
unordered_flat_map<FF, uint32_t> row_idx;
FF beta = FF::random_element();
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a note that challenges derived like this is very likely insecure. We should probably use the get_challenge() which AFAIK is using Fait-Shamir under the hood to derive challenges.
In any case, this should be reviewed/checks by our crypto guys.

Copy link
Collaborator

@dbanks12 dbanks12 left a comment

Choose a reason for hiding this comment

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

Beautiful!

Comment on lines +9 to +21
PublicInputs PublicInputs::from(const std::vector<uint8_t>& data)
{
PublicInputs inputs;
msgpack::unpack(reinterpret_cast<const char*>(data.data()), data.size()).get().convert(inputs);
return inputs;
}

AvmProvingInputs AvmProvingInputs::from(const std::vector<uint8_t>& data)
{
AvmProvingInputs inputs;
msgpack::unpack(reinterpret_cast<const char*>(data.data()), data.size()).get().convert(inputs);
return inputs;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

beautiful!

Comment on lines +64 to +71
for (size_t r = 0; r < num_rows; ++r) {
Relation::accumulate(lookup_result, polys.get_row(r), params, 1);
}
for (auto r : lookup_result) {
if (!r.is_zero()) {
throw std::runtime_error(format("Lookup ", Relation::NAME, " failed."));
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these two separate loops? For main relations we do this r.is_zero() check inside the loop.

Comment on lines +35 to +40
using Relation = std::tuple_element_t<i, typename AvmFlavor::MainRelations>;
checks.push_back([&]() {
typename Relation::SumcheckArrayOfValuesOverSubrelations result{};

for (size_t r = 0; r < num_rows; ++r) {
Relation::accumulate(result, polys.get_row(r), {}, 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the bb dummy in the room, what does Relation::accumulate do? Checks the relation at row r and accumulates the result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's take it in a huddle.

Comment on lines +22 to +33
{
using Accumulator = typename std::tuple_element_t<0, ContainerOverSubrelations>;
auto tmp = (new_term.alu_sel_op_add * (FF(1) - new_term.alu_sel_op_add));
tmp *= scaling_factor;
std::get<0>(evals) += typename Accumulator::View(tmp);
}
{
using Accumulator = typename std::tuple_element_t<1, ContainerOverSubrelations>;
auto tmp = ((new_term.alu_ia + new_term.alu_ib) - new_term.alu_ic);
tmp *= scaling_factor;
std::get<1>(evals) += typename Accumulator::View(tmp);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can someone finally explain to me what this is is doing, in particular the Accumulator::View()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nobody knows! (the view thing). I can explain over a huddle a bit of the other things.

Comment on lines +35 to +38
template <typename AllEntities> static inline auto inverse_polynomial_is_computed_at_row(const AllEntities& in)
{
return (in.execution_sel == 1 || in.execution_sel == 1);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's going on here with return (s == 1 || s == 1)? Just something silly because it's a dummy class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Basically it says that the inverse polynomial will be computed when either the source or target selectors of the lookup are active. It's the same for real lookups.

Comment on lines +35 to +44
// Write the shifted values.
for (const auto& col : TO_BE_SHIFTED_COLUMNS_ARRAY) {
visit_column(col, [&](size_t row, const FF& value) {
if (row == 0) {
return;
}
auto shifted = shift_column(col);
full_row_trace[row - 1].get_column(shifted.value()) = value;
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not super clear to me why we have so much special logic sprinkled around for "shifted" columns. I guess it's because we don't want to shift things too early, so we hold off until later?

Comment on lines +29 to +46
auto build_precomputed_columns_jobs(TraceContainer& trace)
{
return std::array<std::function<void()>, 2>{
[&]() {
PrecomputedTraceBuilder precomputed_builder;
AVM_TRACK_TIME("tracegen/precomputed/misc", precomputed_builder.process_misc(trace));
},
[&]() {
PrecomputedTraceBuilder precomputed_builder;
AVM_TRACK_TIME("tracegen/precomputed/bitwise", precomputed_builder.process_bitwise(trace));
},
};
}

void execute_jobs(std::span<std::function<void()>> jobs)
{
parallel_for(jobs.size(), [&](size_t i) { jobs[i](); });
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool!

Comment on lines +67 to +78
[&]() {
ExecutionTraceBuilder exec_builder;
AVM_TRACK_TIME("tracegen/execution",
exec_builder.process(events.execution, events.addressing, trace));
clear_events(events.execution);
clear_events(events.addressing);
},
[&]() {
AluTraceBuilder alu_builder;
AVM_TRACK_TIME("tracegen/alu", alu_builder.process(events.alu, trace));
clear_events(events.alu);
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is saying "process the execution trace and alu trace in parallel. After execution trace is done, free up execution & addressing events. When alu is done, free up alu events?

Comment on lines +83 to +100
// Now we can compute lookups and permutations.
{
auto jobs_interactions = std::array<std::function<void()>, 3>{
[&]() {
LookupIntoBitwise<lookup_dummy_precomputed_lookup_settings> lookup_execution_bitwise;
lookup_execution_bitwise.process(trace);
},
[&]() {
LookupIntoDynamicTable<lookup_dummy_dynamic_lookup_settings> lookup_execution_execution;
lookup_execution_execution.process(trace);
},
[&]() {
PermutationBuilder<perm_dummy_dynamic_permutation_settings> perm_execution_execution;
perm_execution_execution.process(trace);
},
};
AVM_TRACK_TIME("tracegen/interactions", execute_jobs(jobs_interactions));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very nice job getting this all nicely executed in parallel!

Comment on lines +979 to +987
// TODO: for now I only convert app logic requests?
for (const enqueuedCall of this.avmHints.enqueuedCalls.items) {
inputs.enqueuedCalls.push({
contractAddress: enqueuedCall.contractAddress,
sender: new Fr(0), // FIXME
args: enqueuedCall.calldata.items,
isStatic: false, // FIXME
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO: for now I only convert app logic requests?

You're looping over all of the enqueued call hints though

@fcarreiro fcarreiro marked this pull request as ready for review January 10, 2025 13:34
@fcarreiro fcarreiro force-pushed the fc/vm2-prototype branch 2 times, most recently from 95c1b42 to 6632e7d Compare January 10, 2025 16:43
@fcarreiro fcarreiro merged commit 231f017 into master Jan 11, 2025
49 checks passed
Copy link
Contributor Author

Merge activity

  • Jan 11, 3:55 PM EST: A user merged this pull request with Graphite.

@fcarreiro fcarreiro deleted the fc/vm2-prototype branch January 11, 2025 20:55
Copy link
Contributor Author

@fcarreiro fcarreiro left a comment

Choose a reason for hiding this comment

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

Followup cleanup: #11186

Comment on lines +35 to +40
using Relation = std::tuple_element_t<i, typename AvmFlavor::MainRelations>;
checks.push_back([&]() {
typename Relation::SumcheckArrayOfValuesOverSubrelations result{};

for (size_t r = 0; r < num_rows; ++r) {
Relation::accumulate(result, polys.get_row(r), {}, 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's take it in a huddle.

namespace bb::avm2::constraining {

// This is a version of check circuit that runs on the prover polynomials.
// Better versions could be done, but since this is for debugging only, it is enough for now.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In principle you may be able to do check circuit over FFs instead of polynomials, and you could maybe do it over the Trace without creating the polynomials. However, tbh, using the polynomials might be the best since it's the closest to "real proving".


// The entities that will be used in the flavor.
// clang-format off
#define AVM2_PRECOMPUTED_ENTITIES precomputed_bitwise_input_a, precomputed_bitwise_input_b, precomputed_bitwise_op_id, precomputed_bitwise_output, precomputed_clk, precomputed_first_row, precomputed_sel_bitwise
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 is a column in pil that is called first_row it has a 1 just on the first row. It's needed to anchor other things.

Comment on lines +22 to +33
{
using Accumulator = typename std::tuple_element_t<0, ContainerOverSubrelations>;
auto tmp = (new_term.alu_sel_op_add * (FF(1) - new_term.alu_sel_op_add));
tmp *= scaling_factor;
std::get<0>(evals) += typename Accumulator::View(tmp);
}
{
using Accumulator = typename std::tuple_element_t<1, ContainerOverSubrelations>;
auto tmp = ((new_term.alu_ia + new_term.alu_ib) - new_term.alu_ic);
tmp *= scaling_factor;
std::get<1>(evals) += typename Accumulator::View(tmp);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nobody knows! (the view thing). I can explain over a huddle a bit of the other things.

Comment on lines +35 to +38
template <typename AllEntities> static inline auto inverse_polynomial_is_computed_at_row(const AllEntities& in)
{
return (in.execution_sel == 1 || in.execution_sel == 1);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Basically it says that the inverse polynomial will be computed when either the source or target selectors of the lookup are active. It's the same for real lookups.

Comment on lines 334 to 354
assert(pos < bytecode_length);
// if (pos >= length) {
// info("Position is out of range. Position: " + std::to_string(pos) +
// " Bytecode length: " + std::to_string(length));
// return InstructionWithError{
// .instruction = Instruction(WireOpCode::LAST_WireOpCode_SENTINEL, {}),
// .error = AvmError::INVALID_PROGRAM_COUNTER,
// };
// }

const uint8_t opcode_byte = bytecode[pos];

// if (!Bytecode::is_valid(WireOpCode_byte)) {
// info("Invalid WireOpCode byte: " + to_hex(WireOpCode_byte) + " at position: " + std::to_string(pos));
// return InstructionWithError{
// .instruction = Instruction(WireOpCode::LAST_WireOpCode_SENTINEL, {}),
// .error = AvmError::INVALID_WireOpCode,
// };
// }
pos++;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep!

Comment on lines +38 to +40
auto operands = ex_event.wire_instruction.operands;
assert(operands.size() <= operand_columns);
operands.resize(operand_columns, simulation::Operand::ff(0));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We probably don't care but I expect it to be a copy. auto operands hopefully does not magically insert a auto& operands there.

**Current state**

- Both lookups and permutations are supported.
- When you lookup into a precomputed table, you need to define a fast way to find the row for a tuple. See bitwise example.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in follow up PR.

Comment on lines +88 to +95
void TraceContainer::visit_column(Column col, const std::function<void(uint32_t, const FF&)>& visitor) const
{
auto& column_data = (*trace)[static_cast<size_t>(col)];
std::shared_lock lock(column_data.mutex);
for (const auto& [row, value] : column_data.rows) {
visitor(row, value);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep!

Comment on lines +97 to +104
void TraceContainer::clear_column(Column col)
{
auto& column_data = (*trace)[static_cast<size_t>(col)];
std::unique_lock lock(column_data.mutex);
column_data.rows.clear();
column_data.max_row_number = 0;
column_data.row_number_dirty = false;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed.

TomAFrench added a commit that referenced this pull request Jan 13, 2025
* master: (329 commits)
  fix(avm): mac build (#11195)
  fix: docs rebuild patterns (#11191)
  chore: refactor Solidity Transcript and improve error handling in  sol_honk flow (#11158)
  chore: move witness computation into class plus some other cleanup (#11140)
  fix: get_next_power_exponent off by 1 (#11169)
  chore(avm): vm2 followup cleanup (#11186)
  fix: underconstrained bug (#11174)
  refactor: VariableMerkleTree readability improvements (#11165)
  chore: removing noir bug workaround (#10535)
  chore(docs): Remove node pages  (#11161)
  git subrepo push --branch=master noir-projects/aztec-nr
  git_subrepo.sh: Fix parent in .gitrepo file. [skip ci]
  chore: replace relative paths to noir-protocol-circuits
  git subrepo push --branch=master barretenberg
  feat(avm2): avm redesign init (#10906)
  feat: Sync from noir (#11138)
  feat: simulator split (#11144)
  chore: rpc server cleanup & misc fixes (#11145)
  git subrepo push --branch=master noir-projects/aztec-nr
  git_subrepo.sh: Fix parent in .gitrepo file. [skip ci]
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-avm Component: AVM related tickets (aka public VM)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants