Using Option for previous checkpoints (#10)

* Add TCid to two more Cid fields, although these are not peristed on their own.

* Use Option for previous checkpoints.

* Fix the type of msgs_cid and use TCid where possible.

* Don't use Option, and also don't use T::default()

* Add TCid::is_default

actors/hierarchical_sca/src/checkpoint.rs

@@ -9,7 +9,10 @@ use fvm_shared::bigint::bigint_ser;
 use fvm_shared::clock::ChainEpoch;
 use fvm_shared::econ::TokenAmount;
 
-#[derive(Default, PartialEq, Eq, Clone, Debug, Serialize_tuple, Deserialize_tuple)]
+use crate::tcid::{TCid, TLink};
+use crate::CrossMsgs;
+
+#[derive(PartialEq, Eq, Clone, Debug, Serialize_tuple, Deserialize_tuple)]
 pub struct Checkpoint {
     pub data: CheckData,
     #[serde(with = "serde_bytes")]
@@ -19,10 +22,7 @@ impl Cbor for Checkpoint {}
 
 impl Checkpoint {
     pub fn new(id: SubnetID, epoch: ChainEpoch) -> Self {
-        Self {
-            data: CheckData { source: id, epoch: epoch, ..Default::default() },
-            ..Default::default()
-        }
+        Self { data: CheckData::new(id, epoch), sig: Vec::new() }
     }
 
     /// return cid for the checkpoint
@@ -54,8 +54,8 @@ impl Checkpoint {
     }
 
     /// return the cid of the previous checkpoint this checkpoint points to.
-    pub fn prev_check(&self) -> Cid {
-        self.data.prev_check
+    pub fn prev_check(&self) -> &TCid<TLink<Checkpoint>> {
+        &self.data.prev_check
     }
 
     /// return cross_msg metas included in the checkpoint.
@@ -90,7 +90,7 @@ impl Checkpoint {
     /// Add the cid of a checkpoint from a child subnet for further propagation
     /// to the upper layerse of the hierarchy.
     pub fn add_child_check(&mut self, commit: &Checkpoint) -> anyhow::Result<()> {
-        let cid = commit.cid();
+        let cid = TCid::from(commit.cid());
         match self.data.children.iter_mut().find(|m| commit.source() == &m.source) {
             // if there is already a structure for that child
             Some(ck) => {
@@ -115,23 +115,35 @@ impl Checkpoint {
     }
 }
 
-#[derive(Default, PartialEq, Eq, Clone, Debug, Serialize_tuple, Deserialize_tuple)]
+#[derive(PartialEq, Eq, Clone, Debug, Serialize_tuple, Deserialize_tuple)]
 pub struct CheckData {
     pub source: SubnetID,
     #[serde(with = "serde_bytes")]
     pub tip_set: Vec<u8>,
     pub epoch: ChainEpoch,
-    pub prev_check: Cid,
+    pub prev_check: TCid<TLink<Checkpoint>>,
     pub children: Vec<ChildCheck>,
     pub cross_msgs: Vec<CrossMsgMeta>,
 }
+impl CheckData {
+    pub fn new(id: SubnetID, epoch: ChainEpoch) -> Self {
+        Self {
+            source: id,
+            tip_set: Vec::new(),
+            epoch,
+            prev_check: TCid::default(),
+            children: Vec::new(),
+            cross_msgs: Vec::new(),
+        }
+    }
+}
 impl Cbor for CheckData {}
 
 #[derive(PartialEq, Eq, Clone, Debug, Default, Serialize_tuple, Deserialize_tuple)]
 pub struct CrossMsgMeta {
     pub from: SubnetID,
     pub to: SubnetID,
-    pub msgs_cid: Cid,
+    pub msgs_cid: TCid<TLink<CrossMsgs>>,
     pub nonce: u64,
     #[serde(with = "bigint_ser")]
     pub value: TokenAmount,
@@ -151,7 +163,7 @@ impl CrossMsgMeta {
 #[derive(PartialEq, Eq, Clone, Debug, Serialize_tuple, Deserialize_tuple)]
 pub struct ChildCheck {
     pub source: SubnetID,
-    pub checks: Vec<Cid>,
+    pub checks: Vec<TCid<TLink<Checkpoint>>>,
 }
 impl Cbor for ChildCheck {}
 

actors/hierarchical_sca/src/lib.rs

@@ -310,15 +310,15 @@ impl Actor {
 
                     // if this is not the first checkpoint we need to perform some
                     // additional verifications.
-                    if sub.prev_checkpoint != Checkpoint::default() {
-                        if sub.prev_checkpoint.epoch() > commit.epoch() {
+                    if let Some(ref prev_checkpoint) = sub.prev_checkpoint {
+                        if prev_checkpoint.epoch() > commit.epoch() {
                             return Err(actor_error!(
                                 illegal_argument,
                                 "checkpoint being committed belongs to the past"
                             ));
                         }
                         // check that the previous cid is consistent with the previous one
-                        if sub.prev_checkpoint.cid() != commit.prev_check() {
+                        if commit.prev_check().cid() != prev_checkpoint.cid() {
                             return Err(actor_error!(
                                 illegal_argument,
                                 "previous checkpoint not consistente with previous one"
@@ -356,7 +356,7 @@ impl Actor {
                     })?;
 
                     // update prev_check for child
-                    sub.prev_checkpoint = commit;
+                    sub.prev_checkpoint = Some(commit);
                     // flush subnet
                     st.flush_subnet(rt.store(), &sub).map_err(|e| {
                         e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "error flushing subnet")

actors/hierarchical_sca/src/state.rs

@@ -19,7 +19,7 @@ use std::collections::HashMap;
 use std::str::FromStr;
 
 use crate::atomic::AtomicExec;
-use crate::tcid::{TAmt, TCid, THamt};
+use crate::tcid::{TAmt, TCid, THamt, TLink};
 
 use super::checkpoint::*;
 use super::cross::*;
@@ -36,7 +36,7 @@ pub struct State {
     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 check_msg_registry: TCid<THamt<TCid<TLink<CrossMsgs>>, CrossMsgs>>,
     pub nonce: u64,
     pub bottomup_nonce: u64,
     pub bottomup_msg_meta: TCid<TAmt<CrossMsgMeta, CROSSMSG_AMT_BITWIDTH>>,
@@ -106,7 +106,7 @@ impl State {
                     circ_supply: TokenAmount::zero(),
                     status: Status::Active,
                     nonce: 0,
-                    prev_checkpoint: Checkpoint::default(),
+                    prev_checkpoint: None,
                 };
                 set_subnet(subnets, &id, subnet)?;
                 Ok(true)
@@ -221,8 +221,8 @@ impl State {
             match ch.crossmsg_meta_index(&self.network_name, &to) {
                 Some(index) => {
                     let msgmeta = &mut ch.data.cross_msgs[index];
-                    let prev_cid = msgmeta.msgs_cid;
-                    let m_cid = self.append_metas_to_meta(store, &prev_cid, metas)?;
+                    let prev_cid = &msgmeta.msgs_cid;
+                    let m_cid = self.append_metas_to_meta(store, prev_cid, metas)?;
                     msgmeta.msgs_cid = m_cid;
                     msgmeta.value += value;
                 }
@@ -258,8 +258,8 @@ impl State {
         match ch.crossmsg_meta_index(&sfrom, &sto) {
             Some(index) => {
                 let msgmeta = &mut ch.data.cross_msgs[index];
-                let prev_cid = msgmeta.msgs_cid;
-                let m_cid = self.append_msg_to_meta(store, &prev_cid, msg)?;
+                let prev_cid = &msgmeta.msgs_cid;
+                let m_cid = self.append_msg_to_meta(store, prev_cid, msg)?;
                 msgmeta.msgs_cid = m_cid;
                 msgmeta.value += &msg.value;
             }
@@ -288,18 +288,18 @@ impl State {
     pub(crate) fn append_metas_to_meta<BS: Blockstore>(
         &mut self,
         store: &BS,
-        meta_cid: &Cid,
+        meta_cid: &TCid<TLink<CrossMsgs>>,
         metas: Vec<CrossMsgMeta>,
-    ) -> anyhow::Result<Cid> {
+    ) -> anyhow::Result<TCid<TLink<CrossMsgs>>> {
         self.check_msg_registry.modify(store, |cross_reg| {
             // get previous meta stored
-            let mut prev_meta = match cross_reg.get(&meta_cid.to_bytes())? {
+            let mut prev_meta = match cross_reg.get(&meta_cid.cid().to_bytes())? {
                 Some(m) => m.clone(),
                 None => return Err(anyhow!("no msgmeta found for cid")),
             };
             prev_meta.add_metas(metas)?;
             // if the cid hasn't changed
-            let cid = prev_meta.cid()?;
+            let cid = TCid::from(prev_meta.cid()?);
             if &cid == meta_cid {
                 Ok(cid)
             } else {
@@ -314,20 +314,20 @@ impl State {
     pub(crate) fn append_msg_to_meta<BS: Blockstore>(
         &mut self,
         store: &BS,
-        meta_cid: &Cid,
+        meta_cid: &TCid<TLink<CrossMsgs>>,
         msg: &StorableMsg,
-    ) -> anyhow::Result<Cid> {
+    ) -> anyhow::Result<TCid<TLink<CrossMsgs>>> {
         self.check_msg_registry.modify(store, |cross_reg| {
             // get previous meta stored
-            let mut prev_meta = match cross_reg.get(&meta_cid.to_bytes())? {
+            let mut prev_meta = match cross_reg.get(&meta_cid.cid().to_bytes())? {
                 Some(m) => m.clone(),
                 None => return Err(anyhow!("no msgmeta found for cid")),
             };
 
             prev_meta.add_msg(&msg)?;
 
             // if the cid hasn't changed
-            let cid = prev_meta.cid()?;
+            let cid = TCid::from(prev_meta.cid()?);
             if &cid == meta_cid {
                 Ok(cid)
             } else {
@@ -523,24 +523,24 @@ fn get_checkpoint<'m, BS: Blockstore>(
 fn put_msgmeta<BS: Blockstore>(
     registry: &mut Map<BS, CrossMsgs>,
     metas: CrossMsgs,
-) -> anyhow::Result<Cid> {
-    let m_cid = metas.cid()?;
+) -> anyhow::Result<TCid<TLink<CrossMsgs>>> {
+    let m_cid = TCid::from(metas.cid()?);
     registry
-        .set(m_cid.to_bytes().into(), metas)
+        .set(m_cid.cid().to_bytes().into(), metas)
         .map_err(|e| e.downcast_wrap(format!("failed to set crossmsg meta for cid {}", m_cid)))?;
     Ok(m_cid)
 }
 
 /// insert a message meta and remove the old one.
 fn replace_msgmeta<BS: Blockstore>(
     registry: &mut Map<BS, CrossMsgs>,
-    prev_cid: &Cid,
+    prev_cid: &TCid<TLink<CrossMsgs>>,
     meta: CrossMsgs,
-) -> anyhow::Result<Cid> {
+) -> anyhow::Result<TCid<TLink<CrossMsgs>>> {
     // add new meta
     let m_cid = put_msgmeta(registry, meta)?;
     // remove the previous one
-    registry.delete(&prev_cid.to_bytes())?;
+    registry.delete(&prev_cid.cid().to_bytes())?;
     Ok(m_cid)
 }
 

actors/hierarchical_sca/src/subnet.rs

@@ -33,7 +33,7 @@ pub struct Subnet {
     #[serde(with = "bigint_ser")]
     pub circ_supply: TokenAmount,
     pub status: Status,
-    pub prev_checkpoint: Checkpoint,
+    pub prev_checkpoint: Option<Checkpoint>,
 }
 
 impl Cbor for Subnet {}

actors/hierarchical_sca/src/tcid/link.rs

@@ -4,7 +4,7 @@ use std::marker::PhantomData;
 use super::{CodeType, TCid, TCidContent};
 use crate::tcid_ops;
 use anyhow::Result;
-use fvm_ipld_blockstore::{Blockstore, MemoryBlockstore};
+use fvm_ipld_blockstore::Blockstore;
 use fvm_ipld_encoding::CborStore;
 use serde::de::DeserializeOwned;
 use serde::ser::Serialize;
@@ -94,18 +94,21 @@ where
 
 tcid_ops!(TLink<T : Serialize + DeserializeOwned>, C: CodeType => StoreContent<'s, S, T>);
 
-/// This `Default` implementation is unsound in that while it
-/// creates `TCid` instances with a correct `Cid` value, this value
-/// is not stored anywhere, so there is no guarantee that any retrieval
-/// attempt from a random store won't fail.
+/// This `Default` implementation is there in case we need to derive `Default` for something
+/// that contains a `TCid`, but also to be used as a null pointer, in cases where using an
+/// `Option<TCid<TLink<T>>>` is not the right choice.
 ///
-/// The main purpose is to allow the `#[derive(Default)]` to be
-/// applied on types that use a `TCid` field, if that's unavoidable.
-impl<T, C: CodeType> Default for TCid<TLink<T>, C>
-where
-    T: Serialize + DeserializeOwned + Default,
-{
+/// For example if something has a previous link to a parent item in all cases bug Genesis,
+/// it could be more convenient to use non-optional values, than to match cases all the time.
+///
+/// The reason we are not using `T::default()` to generate the CID is because it is highly
+/// unlikely that the actual store we deal with won't have an entry for that, so we would
+/// not be able to retrieve it anyway, and also because in Go we would have just a raw
+/// `Cid` field, which, when empty, serializes as `"nil"`. We want to be compatible with that.
+///
+/// Also, using `T::default()` with non-optional fields would lead to infinite recursion.
+impl<T, C: CodeType> Default for TCid<TLink<T>, C> {
     fn default() -> Self {
-        Self::new_link(&MemoryBlockstore::new(), &T::default()).unwrap()
+        Self::from(cid::Cid::default())
     }
 }

actors/hierarchical_sca/src/tcid/mod.rs

@@ -1,4 +1,4 @@
-use std::marker::PhantomData;
+use std::{fmt::Display, marker::PhantomData};
 
 use cid::{multihash::Code, Cid};
 
@@ -61,6 +61,12 @@ impl<'d, T: TCidContent, C> serde::Deserialize<'d> for TCid<T, C> {
     }
 }
 
+impl<T: TCidContent, C> Display for TCid<T, C> {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        self.cid.fmt(f)
+    }
+}
+
 /// Assuming that the type implements `maybe_load` and `flush`,
 /// implement some convenience methods.
 ///
@@ -83,6 +89,13 @@ macro_rules! tcid_ops {
           $(, $code : $ct)?
         > TCid<$typ<$($gen),+> $(, $code)?>
         {
+            /// Check that the underlying `Cid` is for the empty use case.
+            ///
+            /// What that means depends on the content.
+            pub fn is_default(&self) -> bool {
+                self.cid == Self::default().cid()
+            }
+
             /// Read the underlying `Cid` from the store or return a `ActorError::illegal_state` error if not found.
             /// Use this method for content that should have already been correctly initialized and maintained.
             /// For content that may not be present, consider using `maybe_load` instead.
@@ -185,12 +198,12 @@ mod test {
     }
 
     #[test]
-    fn default_cid_and_default_hamt_differ() {
+    fn default_cid_and_default_hamt_equal() {
         let cid_typed: TCid<TLink<TestRecordTyped>> = TCid::default();
         let cid_untyped: TCid<TLink<TestRecordUntyped>> = TCid::default();
         // The stronger typing allows us to use proper default values,
-        // but this is a breaking change from the invalid values that came before.
-        assert_ne!(cid_typed.cid(), cid_untyped.cid());
+        // but this should not be a breaking change, they should be treated as null pointers.
+        assert_eq!(cid_typed.cid(), cid_untyped.cid());
     }
 
     #[test]