Strongly typed Cid fields

[aakoshh]

To improve type safety of using Cid fields in State like here, we just merged a PR in the fork Alfonso's fork for Hierarchical Subnet Coordinator Actors: https://github.com/adlrocha/builtin-actors/pull/8

The PR added a couple of new types: TCid is a typed CID that can take TLink, THamt or TAmt as a generic type descriptor. With that the State definition looks like this:

pub struct State {
    pub network_name: SubnetID,
    pub total_subnets: u64,
    #[serde(with = "bigint_ser")]
    pub min_stake: TokenAmount,
    pub subnets: TCid<THamt<Cid, Subnet>>,
    pub check_period: ChainEpoch,
    pub checkpoints: TCid<THamt<ChainEpoch, Checkpoint>>,
    pub check_msg_registry: TCid<THamt<Cid, CrossMsgs>>,
    pub nonce: u64,
    pub bottomup_nonce: u64,
    pub bottomup_msg_meta: TCid<TAmt<CrossMsgMeta, CROSSMSG_AMT_BITWIDTH>>,
    pub applied_bottomup_nonce: u64,
    pub applied_topdown_nonce: u64,
    pub atomic_exec_registry: TCid<THamt<Cid, AtomicExec>>,
}

Creating an instance of it no longer requires initializing fields separately, it simplifies to assignments such as:

subnets: TCid::new_hamt(store)?,

These fields can be manipulated with a few convenience methods that make sure the content is loaded, flushed, and the underlying CID updated:

        let deleted = self.subnets.modify(store, |subnets| {
            subnets
                .delete(&id.to_bytes())
                .map_err(|e| e.downcast_wrap(format!("failed to delete subnet for id {}", id)))
                .map(|x| x.is_some())
        })?;

We reckon that all actor writers would benefit from using this module, if we could find a good place for it. Perhaps in fil_actors_runtime or fvm_shared?

[Stebalien]

YES!

In case you haven't seen it, I did a slightly more magical experiment with Deref and "global" stores: https://github.com/Stebalien/ipld-exp/blob/master/src/link.rs

But this is probably a better place to start.

However, I would consider aborting immediately (at least on state access) if something can't be loaded as there's generally nothing we can do about it. A significant portion of our current code is useless error handling that just bubbles the errors.

[aakoshh]

@Stebalien thanks for pointing out the Link type! I didn't fully grasp how exactly OneCell semantics are applied, but it looks cool for fields to be implicitly read/written, it seems closer to how you really want to use an object with links in it. Although I see that the generic Store seeps into the type, and in the actor code it's generally not available outside method calls.

Regarding error handling, I agree that seeing the same pattern every couple of lines that just attaches some context of what failed is distracting. To make things a bit less verbose, but still stick to the spirit of the code that was already, instead of including which field failed, I pushed error handling into the methods and put the full type description into the error message, which is hopefully descriptive enough.

But otherwise I thought the ? operator behaves exactly as you described, aborting early. Not sure what the alternative there could be.

[Stebalien]

Although I see that the generic Store seeps into the type, and in the actor code it's generally not available outside method calls.

Yeah, I'd get rid of that. I was trying to mimic the allocator API, but it didn't work.

But otherwise I thought the ? operator behaves exactly as you described, aborting early. Not sure what the alternative there could be.

? should be good enough in most cases, but it would be kind of nice to be able to just "dereference" CIDs, having the CID abstraction call abort on the runtime if something fails.

[aakoshh]

Actually, now that I took a second look at the actor implementations themselves, rather than just the state (sorry, I'm new to this codebase) I can see that the general structure of message handling allows State to be generic in the Blockstore type.

It's a relatively simple change:

fn my_actor_method<BS, RT>(rt: &mut RT, my_params: MyParams) -> Result<MyResult, ActorError>
    where
        BS: Blockstore,
        RT: Runtime<BS>,
    {
        rt.transaction(|st: &mut State<BS>, rt| {
...

With such changes, your Link type should be usable as well, and I would not have needed the indirections with THamt and TAmt 🤦‍♂️ (Although I use them to store the bit width as well, so maybe it doesn't make that much difference for me).

[aakoshh]

@Stebalien another thing you could try is using a thread local static variable holding an optional dyn Store to get rid of the generic type. Install the store instance at the beginning of the transaction, and panic if it's not there during dereferencing.

This is what I ended up with for transactions during atomic operations in an STM library I worked on, to keep the rest of the API a bit more streamlined.

[aakoshh]

@Stebalien I see you closed this issue but it's not clear to me what the conclusion was.

[Stebalien]

Sorry, I'm not sure why I didn't see your comment. I closed this because this was the day where github was closing issues when ctrl+enter was pressed on firefox...