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: Circuit simulator #580

Closed
wants to merge 42 commits into from
Closed

feat: Circuit simulator #580

wants to merge 42 commits into from

Conversation

codygunton
Copy link
Collaborator

Description

Please provide a paragraph or two giving a summary of the change, including relevant motivation and context.

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • The branch has been merged with/rebased against the head of its merge target.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • There are no circuit changes, OR a cryptographer has been assigned for review.
  • New functions, classes, etc. have been documented according to the doxygen comment format. Classes and structs must have @brief describing the intended functionality.
  • If existing code has been modified, such documentation has been added or updated.
  • No superfluous include directives have been added.
  • I have linked to any issue(s) it resolves.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

@codygunton codygunton self-assigned this Jul 5, 2023
multiplicative_constant = 1;
witness_index = IS_CONSTANT;
// context -> stored_value = value.witness;
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm why is this if constexpr needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Normally values of field elements are stored in the variables array of a given circuit builder. The index in that array is called a "real variable index". In order to avoid redundantly storing variable values, a witness object wraps an index idx and the corresponding variable value is accessed as in variables[real_variable_index[idx]]. I'm trying out a model where we do away with the variables array altogether, but just have a witness wrap a value. Therefore to initialize a field element from a witness I need to supply the witness info not through providing a witness index, but a witness value itself. The right way to do that is through setting the additive_constant, since 'normalizing' the field element would put the same value there. That said, I'm not sure this is the best approach.

Copy link
Collaborator Author

@codygunton codygunton Jul 6, 2023

Choose a reason for hiding this comment

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

But if you're just talking about the constexpr then I think we could avoid code generation by just defining the function for the particular template parameter CircuitSimulatorBN254. We will also have a CircuitSimulatorGrumpkin though... anyway, experimenting.

@@ -0,0 +1,113 @@
#pragma once
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TODO: share code with CircuitConstructorBase

@@ -299,6 +300,10 @@ concept IsPlonkFlavor = IsAnyOf<T, plonk::flavor::Standard, plonk::flavor::Turbo
template <typename T>
concept IsHonkFlavor = IsAnyOf<T, honk::flavor::Standard, honk::flavor::Ultra, honk::flavor::StandardGrumpkin, honk::flavor::UltraGrumpkin>;

// WORKTODO: move? smart way of not referring to instances?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We will probably want a CircuitSimulatorGrumpkin

@@ -1,13 +1,14 @@
#include "barretenberg/crypto/blake2s/blake2s.hpp"
#include "barretenberg/proof_system/circuit_builder/ultra_circuit_builder.hpp"
#include "barretenberg/proof_system/circuit_builder/circuit_simulator.hpp"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(blake2s tests pass)

@@ -7,6 +7,7 @@
#include "barretenberg/stdlib/primitives/point/point.hpp"
#include "barretenberg/stdlib/primitives/witness/witness.hpp"

// TODO: This does not belong in barretenberg.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The notion of an "address" I mean... just a side note.

@@ -397,7 +397,9 @@ void bool_t<ComposerContext>::assert_equal(const bool_t& rhs, std::string const&
const bool_t lhs = *this;
ComposerContext* ctx = lhs.get_context() ? lhs.get_context() : rhs.get_context();

if (lhs.is_constant() && rhs.is_constant()) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We want to put the composer in a failing state without changing existing code. We can't reuse the "both are witnesses" code because the assert_equal that takes in values (ant not witness indices) only exists on the simulator.

@@ -351,9 +351,9 @@ typename byte_array<Composer>::byte_slice byte_array<Composer>::split_byte(const

if (byte.is_constant()) {
field_t<Composer> low(context, low_value);
field_t<Composer> high(context, high_value);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CIRCUIT BUG: we were not correctly setting bits in the constant case.


const auto out = arr.get_value();
EXPECT_EQ(out[0], uint8_t(0));
EXPECT_EQ(out[1], uint8_t(7));
EXPECT_EQ(out[3], uint8_t(5));
EXPECT_EQ(out[1], uint8_t(7)); // start with 0000'0010, want 0000'0111, get 0000'0110
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TODO: clean up comment

uint32_t put_constant_variable([[maybe_unused]] const barretenberg::fr& variable) { return 1028; }
void set_public_input([[maybe_unused]] const uint32_t witness_index)
{
// WORKTODO Public input logic?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Haven't thought through potential subtleties with set_public_input or fix_witness.

// return 0; // WORKTODO: return part of `in` for debugging purposes?
// }

inline uint32_t add_variable([[maybe_unused]] const barretenberg::fr index) const { return 1028; }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

1028 = 0x404, recognizable when printing if something went wrong in the simulation.

size_t get_num_constant_gates() { return 1028; };
// maybe this shouldn't be implemented?

bool create_range_constraint(FF const& elt,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My goal is to preserve the composer interface when I can, but without tracking witness indices. In this case I decided to change the interface (input is a field element elt rather than a witness index). One way to unify interfaces would be to pass around witness_t's rather than indices, since this is a container for both an index and a vale. But this would undermine the optimization of tracking witness indices.

return { 1028 };
};

void assert_equal(FF left, FF right, std::string const& msg)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another departure of the form "witness index becomes a value".

@@ -10,7 +10,7 @@ namespace proof_system {
* A cache that wraps an underlying external store. It favours holding the largest polynomials in it's cache up
* to max_cache_size_ polynomials. This saves on many expensive copies of large amounts of memory to the external
* store. Smaller polynomials get swapped out, but they're also much cheaper to read/write.
* The default ctor sets the cache size to 70.
* The default ctor sets the cache size to 40.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unrelated: I should have changed this in a PR earlier this week.

@@ -2,5 +2,5 @@

namespace proof_system::merkle {
// TODO(Cody) Get rid of this?
Copy link
Collaborator Author

@codygunton codygunton Jul 7, 2023

Choose a reason for hiding this comment

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

IMO the various enum's that track types should go away.

@@ -50,8 +50,7 @@ TYPED_TEST(BoolTest, TestBasicOperations)
bool result = composer.check_circuit();
EXPECT_EQ(result, true);

auto gates_after = composer.get_num_gates();
EXPECT_EQ(gates_after - gates_before, 6UL);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These tests that function like benchmarks should break, since we don't track gates, but they aren't testing circuit logic, so I felt comfortable deleting them.

@codygunton codygunton linked an issue Jul 17, 2023 that may be closed by this pull request
@codygunton codygunton changed the title feat: Add circuit simulator feat: Circuit simulator Jul 17, 2023
@codygunton
Copy link
Collaborator Author

Subsumed by AztecProtocol/aztec-packages#1195, which @maramihali took over the line

@codygunton codygunton closed this May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spike: how hard is it to implement a DummyComposer?
2 participants