From 9ede48182f541b565a63a294f3fc67a423128479 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 4 Jul 2022 15:55:07 +0100 Subject: [PATCH] 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 --- Cargo.lock | 4 +- actors/hierarchical_sca/src/checkpoint.rs | 36 ++++++++++------ actors/hierarchical_sca/src/lib.rs | 8 ++-- actors/hierarchical_sca/src/state.rs | 42 +++++++++---------- actors/hierarchical_sca/src/subnet.rs | 2 +- actors/hierarchical_sca/src/tcid/link.rs | 27 ++++++------ actors/hierarchical_sca/src/tcid/mod.rs | 21 ++++++++-- actors/hierarchical_sca/tests/harness/mod.rs | 14 ++++--- .../hierarchical_sca/tests/sca_actor_test.rs | 5 ++- 9 files changed, 95 insertions(+), 64 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8770cf8ff..9f41701ec 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1089,9 +1089,9 @@ dependencies = [ [[package]] name = "fvm_sdk" -version = "1.0.0-rc.3" +version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3bd162cff8cab78c3798762bc6879429ecbd0f5515856bcb3bc31d197208db70" +checksum = "d013aaca686496e85886d96e5c4c228555141849d59347d1f22d793d5be2572b" dependencies = [ "cid", "fvm_ipld_encoding", diff --git a/actors/hierarchical_sca/src/checkpoint.rs b/actors/hierarchical_sca/src/checkpoint.rs index 78c681a2c..181ba9659 100644 --- a/actors/hierarchical_sca/src/checkpoint.rs +++ b/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> { + &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, pub epoch: ChainEpoch, - pub prev_check: Cid, + pub prev_check: TCid>, pub children: Vec, pub cross_msgs: Vec, } +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>, 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, + pub checks: Vec>>, } impl Cbor for ChildCheck {} diff --git a/actors/hierarchical_sca/src/lib.rs b/actors/hierarchical_sca/src/lib.rs index 2e5842263..9b0b16bc8 100644 --- a/actors/hierarchical_sca/src/lib.rs +++ b/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") diff --git a/actors/hierarchical_sca/src/state.rs b/actors/hierarchical_sca/src/state.rs index e2b30f72c..7e6473fb9 100644 --- a/actors/hierarchical_sca/src/state.rs +++ b/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>, pub check_period: ChainEpoch, pub checkpoints: TCid>, - pub check_msg_registry: TCid>, + pub check_msg_registry: TCid>, CrossMsgs>>, pub nonce: u64, pub bottomup_nonce: u64, pub bottomup_msg_meta: TCid>, @@ -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( &mut self, store: &BS, - meta_cid: &Cid, + meta_cid: &TCid>, metas: Vec, - ) -> anyhow::Result { + ) -> anyhow::Result>> { 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,12 +314,12 @@ impl State { pub(crate) fn append_msg_to_meta( &mut self, store: &BS, - meta_cid: &Cid, + meta_cid: &TCid>, msg: &StorableMsg, - ) -> anyhow::Result { + ) -> anyhow::Result>> { 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")), }; @@ -327,7 +327,7 @@ impl State { 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,10 +523,10 @@ fn get_checkpoint<'m, BS: Blockstore>( fn put_msgmeta( registry: &mut Map, metas: CrossMsgs, -) -> anyhow::Result { - let m_cid = metas.cid()?; +) -> anyhow::Result>> { + 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) } @@ -534,13 +534,13 @@ fn put_msgmeta( /// insert a message meta and remove the old one. fn replace_msgmeta( registry: &mut Map, - prev_cid: &Cid, + prev_cid: &TCid>, meta: CrossMsgs, -) -> anyhow::Result { +) -> anyhow::Result>> { // 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) } diff --git a/actors/hierarchical_sca/src/subnet.rs b/actors/hierarchical_sca/src/subnet.rs index 3eb60e725..525bcc83d 100644 --- a/actors/hierarchical_sca/src/subnet.rs +++ b/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, } impl Cbor for Subnet {} diff --git a/actors/hierarchical_sca/src/tcid/link.rs b/actors/hierarchical_sca/src/tcid/link.rs index 3df5c782e..c1dce0558 100644 --- a/actors/hierarchical_sca/src/tcid/link.rs +++ b/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, 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>>` 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 Default for TCid, 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 Default for TCid, C> { fn default() -> Self { - Self::new_link(&MemoryBlockstore::new(), &T::default()).unwrap() + Self::from(cid::Cid::default()) } } diff --git a/actors/hierarchical_sca/src/tcid/mod.rs b/actors/hierarchical_sca/src/tcid/mod.rs index 71d5fe9c7..738000d61 100644 --- a/actors/hierarchical_sca/src/tcid/mod.rs +++ b/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 { } } +impl Display for TCid { + 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> = TCid::default(); let cid_untyped: TCid> = 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] diff --git a/actors/hierarchical_sca/tests/harness/mod.rs b/actors/hierarchical_sca/tests/harness/mod.rs index 049485d54..e032b02ea 100644 --- a/actors/hierarchical_sca/tests/harness/mod.rs +++ b/actors/hierarchical_sca/tests/harness/mod.rs @@ -2,6 +2,8 @@ use anyhow::anyhow; use cid::multihash::Code; use cid::multihash::MultihashDigest; use cid::Cid; +use fil_actor_hierarchical_sca::tcid::TCid; +use fil_actor_hierarchical_sca::tcid::TCidContent; use fil_actors_runtime::test_utils::expect_abort; use fil_actors_runtime::Array; use fvm_ipld_blockstore::Blockstore; @@ -382,7 +384,7 @@ impl Harness { let chmeta = &ch.data.cross_msgs[chmeta_ind]; let cross_reg = st.check_msg_registry.load(rt.store()).unwrap(); - let meta = get_cross_msgs(&cross_reg, &chmeta.msgs_cid).unwrap().unwrap(); + let meta = get_cross_msgs(&cross_reg, &chmeta.msgs_cid.cid()).unwrap().unwrap(); let msg = meta.msgs[expected_nonce as usize].clone(); assert_eq!(meta.msgs.len(), (expected_nonce + 1) as usize); @@ -398,7 +400,7 @@ impl Harness { } } - Ok(chmeta.msgs_cid) + Ok(chmeta.msgs_cid.cid()) } pub fn send_cross( @@ -473,7 +475,7 @@ impl Harness { let chmeta = &ch.data.cross_msgs[chmeta_ind]; let cross_reg = st.check_msg_registry.load(rt.store()).unwrap(); - let meta = get_cross_msgs(&cross_reg, &chmeta.msgs_cid).unwrap().unwrap(); + let meta = get_cross_msgs(&cross_reg, &chmeta.msgs_cid.cid()).unwrap().unwrap(); let msg = meta.msgs[nonce as usize].clone(); assert_eq!(meta.msgs.len(), (nonce + 1) as usize); @@ -629,8 +631,8 @@ pub fn has_childcheck_source<'a>( children.iter().find(|m| source == &m.source) } -pub fn has_cid<'a>(children: &'a Vec, cid: &Cid) -> bool { - children.iter().any(|c| c == cid) +pub fn has_cid<'a, T: TCidContent>(children: &'a Vec>, cid: &Cid) -> bool { + children.iter().any(|c| c.cid() == *cid) } pub fn add_msg_meta( @@ -641,7 +643,7 @@ pub fn add_msg_meta( value: TokenAmount, ) { let mh_code = Code::Blake2b256; - let c = Cid::new_v1(fvm_ipld_encoding::DAG_CBOR, mh_code.digest(&rand)); + let c = TCid::from(Cid::new_v1(fvm_ipld_encoding::DAG_CBOR, mh_code.digest(&rand))); let meta = CrossMsgMeta { from: from.clone(), to: to.clone(), msgs_cid: c, nonce: 0, value: value }; ch.append_msgmeta(meta).unwrap(); diff --git a/actors/hierarchical_sca/tests/sca_actor_test.rs b/actors/hierarchical_sca/tests/sca_actor_test.rs index 522fd16e5..17da58d6b 100644 --- a/actors/hierarchical_sca/tests/sca_actor_test.rs +++ b/actors/hierarchical_sca/tests/sca_actor_test.rs @@ -1,4 +1,5 @@ use cid::Cid; +use fil_actor_hierarchical_sca::tcid::TCid; use fil_actor_hierarchical_sca::{ get_bottomup_msg, subnet, Actor as SCAActor, Checkpoint, State, DEFAULT_CHECKPOINT_PERIOD, }; @@ -232,7 +233,7 @@ fn checkpoint_commit() { // Append a new checkpoint for the same subnet let mut ch = Checkpoint::new(shid.clone(), epoch + 11); - ch.data.prev_check = prev_cid; + ch.data.prev_check = TCid::from(prev_cid); h.commit_child_check(&mut rt, &shid, &ch, ExitCode::OK, TokenAmount::zero()).unwrap(); let st: State = rt.get_state(); let commit = st.get_window_checkpoint(rt.store(), epoch).unwrap(); @@ -353,7 +354,7 @@ fn checkpoint_crossmsgs() { h.fund(&mut rt, &funder, &shid, ExitCode::OK, amount.clone(), 1, &amount).unwrap(); let mut ch = Checkpoint::new(shid.clone(), epoch + 9); - ch.data.prev_check = prev_cid; + ch.data.prev_check = TCid::from(prev_cid); add_msg_meta( &mut ch, &shid,