Injectable primitives #1470
Injectable primitives #1470
Conversation
alexytsu
force-pushed
the
alexytsu/1348-injectable-primitives
branch
from
445ca30 to
bb9ab41
Compare
alexytsu
changed the base branch from
master
to
alexytsu/1348-testable-values
|
@anorth Can you please let me know if this would have solved your testing needs in #1370 (comment)? If so, I can apply this more widely to other behaviours of the TestVM (the rest of the methods in Verifier and Primitives to start with) |
alexytsu
force-pushed
the
alexytsu/1348-testable-values
branch
from
5aee49e to
649515a
Compare
test_vm/src/lib.rs
Outdated
| @@ -51,7 +51,7 @@ pub use messaging::*; | |||
|
|
|||
| /// An in-memory rust-execution VM for testing builtin-actors that yields sensible stack traces and debug info | |||
| pub struct TestVM { | |||
| pub primitives: FakePrimitives, | |||
| pub primitives: RefCell<FakePrimitives>, | |||
There was a problem hiding this comment.
If this is going to be boxed, can we use RefCell<dyn Primitives> instead? It would be much nicer to be able to set_primitives(...) rather than expose a method for each one individually.
There was a problem hiding this comment.
Currently relying on FakePrimitives being Cloneable to pass out a Primitives instance.
dyn Primitives wouldn't be cloneable.
A workaround might be to add an explicit "clone" method on the primitives trait that returns a Box
There was a problem hiding this comment.
Another option is for Primitives to break the interior-mutability pattern and require a mutable instance to set_primitives. This avoids having the RefCell allowing us to revert to returning a &Primitives (which means we don't have to clone)
actors/miner/src/lib.rs
Outdated
| @@ -1054,7 +1054,7 @@ impl Actor { | |||
| let quant = state.quant_spec_for_deadline(rt.policy(), dl_idx); | |||
|
|
|||
| for update in &decls_by_deadline[&dl_idx] { | |||
| rt.verify_replica_update(&update.proof_inputs).with_context_code( | |||
| Verifier::verify_replica_update(rt, &update.proof_inputs).with_context_code( | |||
There was a problem hiding this comment.
I don't think requiring this calling convention will be reasonable given the method is available directly on rt. I also can't tell why the change is needed though. If there is a real thing happening here, I think it's too subtle for future developers to follow.
There was a problem hiding this comment.
Since runtime implements both Primitives and Verifier which both now have verify_replica_update with the same signature, we need to specify which method to dispatch to.
The Primitives could have differently named functions - perhaps prefixed?
There was a problem hiding this comment.
Since runtime implements both Primitives and Verifier which both now have verify_replica_update with the same signature
Ok I think the design problem originates here. Primitives and Verifier are IMO just a boring and unnecessary separation of the methods required by a complete Runtime impl. It shouldn't drive any other code structure. We could in principle just inline them all into Runtime, and perhaps should in practise if the separation cause problems. We'd still then need a trait for external behaviour to inject into a VM, and forwarding methods. But the Primitives trait is doing two jobs here, and perhaps should do just one.
- One solution might be to keep the separation and have Primitives and Verifier separately injectable into the VM. I.e. not "Define all injectable behaviour as a method of the Primitives trait".
- Another is to delete the Verifier trait and merge its methods into Primitives. The distinction is pretty arbitrary anyway. There might be some friction around the
fake-proofsbuild config though.
alexytsu
force-pushed
the
alexytsu/1348-injectable-primitives
branch
from
9dfaecc to
52ff5c1
Compare
* export FakePrimitives from fakes * export FakePrimitives
#1371
The design here is to:
- Define all injectable behaviour as a method of thePrimitivestrait- FakePrimitives stores optional override behaviour for each method- Expose the ability to inject this behaviour on thevm_api::VMtrait- Where the behaviour overlaps with "runtime" behaviour such as with theVerifiertrait, the implementer ofVerifiershould delegate to the VM's primitives.For example, hereverify_replica_updateis added toPrimitivesand implemented inFakePrimitives. TheVMapi allows test authors to override this behaviour. InvocationCtx, delegates to the VM instance so as to select the (potentially) overriden behavior.All Verifier functionality has been moved to Primitives. Primitives is Faked by FakedPrimitives in the TestVM which provides default behaviour or can be overridden via the trait methods of VM