Refactor the blockstore #1694
Refactor the blockstore #1694
Conversation
First, remove the multihash "code" from the arguments and use a plain u64. This will let us compile the actors without importing hashing libraries. fixes #1238 fixes #504 Second, replace the concrete `Block` type with a trait. We can then implement this trait for, e.g., `fvm_ipld_encoding::IpldBlock`. Finally, make the blockstore object safe. This will let us work with the blockstore as `&dyn Blockstore` in most cases.
Stebalien
force-pushed
the
feat/blockstore-refactor
branch
from
5bed4e0 to
c74826b
Compare
| self.put_keyed(&k, block.as_ref())?; | ||
| Ok(k) | ||
| } | ||
| fn put(&self, mh_code: u64, block: &dyn Block) -> Result<Cid>; |
There was a problem hiding this comment.
- I'm using a trait for
Blockto allow abstracting over blocks that own their data, and blocks that borrow their data. - After implementing this, I think we need to replace
mh_codewith some form ofMultihashType { size: u32, code: u64 }.
There was a problem hiding this comment.
Alternatively, we could simply remove the multihash-code parameter and say that the blockstore gets to decide.
There was a problem hiding this comment.
I don't think removing it is right at this base level. Currying it in to a wrapper store could make sense though.
|
@anorth can I get feedback on the blockstore trait (and the block trait)? You can ignore everything else. |
There was a problem hiding this comment.
It feels like there's two different interfaces for storing blocks to express here:
- The core one of
(CID, [u8]) - A convenience method in terms of
(MHCode, Codec, [u8])that computes the CID
In both cases the codec is assumed to match the data format. In the first one also the hash is assumed to have been computed with the MH code in the CID. The point being that the blockstore isn't checking anything.
Given that, it doesn't make much sense to me why in put() the MH code is separate from the Block, but the codec isn't. A more convenient method might compute both the serialisation and the CID, from appropriate parameters. But this probably would be less good than the pattern of wrapping, like CborStore.
So forgive me if I've overstepped the scope here, but I don't really get why the core Blockstore trait and implementations should do hashing and CID construction. Instead they'd be simpler as a pure (CID, data) store abstraction, and then wrappers can curry in either or both the hashing function and serialisation method, no neither need to be passed around. How bad would it be to remove (deprecate) put? I think there are few call sites, since most code uses a wrapper like CborStore.
[Unfortunately I can see one piece of friction in that most actor code declares to use a Blockstore but in fact uses CborStore via impl<T: Blockstore> CborStore for T {}, so we'd probably need to (a) restrict all those declarations and (b) make CborStore wrap rather than extend BlockStore.]
An alternative intent might be that the blockstore should be responsible for the correctness of hashes corresponding to the data (even if it can't verify encoding), but in that case put_keyed() should not exist, and the API look more like the FVM storage syscalls.
In that latter case, or if my earlier thoughts are off the rails, then yes I think these traits look about right.
| self.put_keyed(&k, block.as_ref())?; | ||
| Ok(k) | ||
| } | ||
| fn put(&self, mh_code: u64, block: &dyn Block) -> Result<Cid>; |
There was a problem hiding this comment.
I don't think removing it is right at this base level. Currying it in to a wrapper store could make sense though.
The issue is that I'm to support two distinct use-cases:
I can probably drop the |
|
The built-in-actors test VM is another example with the two use cases: untrusted use from within the actors, but trusted use from the surrounding test infrastructure code. However, we can probably support these with two different traits. |
First, remove the multihash "code" from the arguments and use a plain u64. This will let us compile the actors without importing hashing libraries.
fixes #1238
fixes #504
Second, replace the concrete
Blocktype with a trait. We can then implement this trait for, e.g.,fvm_ipld_encoding::IpldBlock.Finally, make the blockstore object safe. This will let us work with the blockstore as
&dyn Blockstorein most cases.