noir-lang / acvm

Apache License 2.0
46 stars 16 forks source link

Update `Backend` trait to use `&mut self` rather than `&self` #211

Closed TomAFrench closed 1 year ago

TomAFrench commented 1 year ago

Problem

Currently in https://github.com/noir-lang/aztec_backend/pull/117 we're planning on merging the Plonk and Barretenberg structs so that only the Backend trait methods remains public while any other methods remain private to the crate. The benefit of this is that we'll no longer need to spin up a new wasm instance for call to the AVCM interop methods.

When interacting with WASM we need to mutate the Barretenberg struct when allocating/freeing memory so the current usage of immutable references prevents us from doing this.

Proposed solution

We should then update the following methods to use mutable references:

solve
solve_black_box_function_call
get_exact_circuit_size
preprocess
prove_with_pk
verify_with_vk
eth_contract_from_vk

Alternatives considered

No response

Additional context

No response

Submission Checklist

phated commented 1 year ago

@TomAFrench Is this actually required? I thought you could add mut even if a trait didn't have it, see https://stackoverflow.com/questions/66700509/why-is-mut-self-in-trait-implementation-allowed-when-trait-definition-only-uses

TomAFrench commented 1 year ago

Changing from &self to &mut self results in the following error:

error[E0053]: method `get_exact_circuit_size` has an incompatible type for trait
  --> acvm_backend_barretenberg/src/acvm_interop/proof_system.rs:13:31
   |
13 |     fn get_exact_circuit_size(&mut self, circuit: &Circuit) -> u32 {
   |                               ^^^^^^^^^
   |                               |
   |                               types differ in mutability
   |                               help: change the self-receiver type to match the trait: `self: &Barretenberg`
   |
   = note: expected signature `fn(&Barretenberg, &common::acvm::acir::circuit::Circuit) -> _`
              found signature `fn(&mut Barretenberg, &common::acvm::acir::circuit::Circuit) -> _`
kevaundray commented 1 year ago

I think the way that Blaine is saying is to have the trait use self and you define the implementation of the trait on &mut T -- this is opposed to having the trait use &mut self and then defining the implementation on T

phated commented 1 year ago

@kevaundray I was unaware of the difference between self and &self in respect to mut but I'm intrigued by your comment. What would that look like in practice?

kevaundray commented 1 year ago

@kevaundray I was unaware of the difference between self and &self in respect to mut but I'm intrigued by your comment. What would that look like in practice?

I was thinking about something like this:

pub trait NumberPrinter {
    fn print_number(self) -> String;
}

pub struct Foo{
    num : u32
}

impl NumberPrinter for &mut Foo {
    fn print_number(self) -> String {
        format!("my number is : {}", self.num)
    }
}

fn main() {
    let mut foo = Foo{num : 10};
    println!("{}",foo.print_number());
    println!("{}",(&mut foo).print_number());
}

The trait definition takes a self but since I implemented the trait on &mut Foo , self is really &mut self

phated commented 1 year ago

Nice, some light hacking seems to seem that pub trait NumberPrinter would be need to be where Self: Sized. Is that right?

TomAFrench commented 1 year ago

Closing this as some core methods didn't actually need &mut self at all so it's moot.