barretenberg icon indicating copy to clipboard operation
barretenberg copied to clipboard

Clarify Protogalaxy state

Open codygunton opened this issue 11 months ago • 0 comments

We should clarify and simplify the handling of state in Protogalaxy. Examples:

  • auto is overused and makes it hard to navigate.
  • the name "accumulator" is used for objects that are not being treated as an accumulator (the idea is that these objects have the same type as an accumulator, but IMO the naming here is confusing and should be improved)
  • the is_accumulator flag in the ProverInstance is a bit of a misnomer--I believe this sometimes is more properly an "is_first_round".
  • There is a bad pattern in that part of the code where objects are default constructed when probably they should be copy constructed.

Example of the final point: I tried to rewrite

auto next_accumulator = std::make_shared<Instance>();
next_accumulator->is_accumulator = true;
next_accumulator->instance_size = instances[0]->instance_size;
next_accumulator->log_instance_size = instances[0]->log_instance_size;

to

auto next_accumulator = instances[0];
next_accumulator->is_accumulator = true;
next_accumulator->instance_size = instances[0]->instance_size;
next_accumulator->log_instance_size = instances[0]->log_instance_size;

in order to give next_accumulator a copy of the commitment key. This ran into trouble (Decider tests failed) but

auto next_accumulator = std::make_shared<Instance>();
next_accumulator->is_accumulator = true;
next_accumulator->instance_size = instances[0]->instance_size;
next_accumulator->log_instance_size = instances[0]->log_instance_size;
next_accumulator->commitment_key = instances[0]->commitment_key;

works. IMO this is clear example of brittleness in our handling of state.

codygunton avatar Feb 29 '24 15:02 codygunton