Fix MinerActor.prove_replica_updates not to require Blockstore to be Cloned

[anorth]

Introduced in https://github.com/filecoin-project/builtin-actors/pull/50/files

After resolving, remove Clone from the blockstore trait bounds defined in ActorCode.invoke_method.

Also consider improving prove_replica_updates to not be a 400-line behemoth single method.

[anorth]

See #137

[Stebalien]

Unfortunately, the EVM also requires Blockstore: Clone and that's much trickier to fix.

[anorth]

A thought I had recently: I'm increasingly of the opinion that having components be generic in the blockstore is probably a design error. Instead, they should work with a dynamic dispatch reference. On the scale of the work done for a blockstore read/write, a vtable lookup will be unmeasurable. Even a pure in-memory store is probably dominated by hashing (the internal hashtable) even if one ignores all the data de/encoding and CID generation that almost always accompanies it.

The blockstore generic infects a huge amount of code and causes real development friction that I don't think is justified.

Unfortunately, resolving it probably requires working bottom-up from things like the H/AMT implementations, which are shared between repos, and so can't be trialled in a limited scope like the built-in actors.

[Stebalien]

This is a bit tricky in rust, unfortunately.

• We can't just use &dyn Blockstore because we have cases where we want to take ownership.
• We can't just use Box<dyn Blockstore> because we might need to share it.
• We may be able to use, Rc<dyn Blockstore> but I expect we'll have cases where we need thread safety.

So all of our APIs would likely need to take an Arc<dyn Blockstore>.

Alternatively:

• We could have our APIs take impl Blockstore.
• Internally, we could "box" that Box::new(bs) as Box<dyn Blockstore>.

But if we're not careful, we'll have blockstores wrapped in boxes wrapped in rcs wrapped in boxes etc.

[anorth]

we have cases where we want to take ownership

Could you expand on those? One candidate would be for mutations, but the current trait has no mut methods and so implementations must use interior mutability, which seems fine. Where would we want to take ownership?

cases where we need thread safety

Could you expand on those too?

My framing is explicitly for actor development. In this frame, I'm not aware of examples of either of these. If there are different contexts that have different needs, perhaps they should use a different trait and an adaptor (or actors uses the adaptor). We should be able to find a pattern than works best for this context of on-chain smart contracts without suffering too much from unrelated uses of similar functionality.

[Stebalien]

Could you expand on those? One candidate would be for mutations, but the current trait has no mut methods and so implementations must use interior mutability, which seems fine. Where would we want to take ownership?

I was assuming that the runtime would own a Box<dyn Blockstore> and "lend out" a reference. E.g.:

let mut rt = Runtime::new(Box::new(some_bs) as Box<dyn Blockstore>);
let hamt = Hamt::new(rt.blockstore());
rt.delete_actor(...); // oops, needs to borrow `rt` as mutable but it's already borrowed by the HAMT.

However, we could trivially solve this my removing all &mut receivers from the runtime, relying on interior mutability instead. Which is what we should likely do anyways.

cases where we need thread safety

Could you expand on those too?

I can't actually remember why I said that. Maybe for testing? But yeah, that's probably not an issue.

---

The remaining issue with &dyn Blockstore is that put and put_many are generic (generic methods are not object safe). They're generic to abstract over passing the data by-value and/or by-reference.

But that should be doable: https://github.com/filecoin-project/ref-fvm/pull/1694.

[anorth]

Here's another case where a B: Blockstore trait needed to be introduced in order to abstract over the type of blockstore, but holding a reference would probably have been preferable. https://github.com/filecoin-project/builtin-actors/pull/1259

[anorth]

However, we could trivially solve this my removing all &mut receivers from the runtime, relying on interior mutability instead. Which is what we should likely do anyways.

Now done! #1248

[anorth]

Note the nested H/AMT structures like MapMap, MultiMap require the blockstore to be Clone too, unless we change their APIs. This may be reasonable if we better establish a blockstore reference as being a handle to a store.