fix store cross-msg in checkpoint (#89)

* fix store cross-msg in checkpoint

* fix: take_cross_msgs was changing the committed checkpoint

gateway/src/checkpoint.rs

@@ -84,8 +84,8 @@ impl BottomUpCheckpoint {
 
     /// Take the cross messages out of the checkpoint. This will empty the `self.data.cross_msgs`
     /// and replace with None.
-    pub fn take_cross_msgs(&mut self) -> Option<Vec<CrossMsg>> {
-        self.data.cross_msgs.cross_msgs.take()
+    pub fn cross_msgs(&mut self) -> Option<Vec<CrossMsg>> {
+        self.data.cross_msgs.cross_msgs.clone()
     }
 
     pub fn ensure_cross_msgs_sorted(&self) -> anyhow::Result<()> {
@@ -201,14 +201,18 @@ pub struct ChildCheck {
 /// frozen and that is ready for signing and commitment in the
 /// current window.
 pub fn checkpoint_epoch(epoch: ChainEpoch, period: ChainEpoch) -> ChainEpoch {
+    // TODO: Once we consider different genesis_epoch different to zero
+    // we should account for this here.
     (epoch / period) * period
 }
 
 /// WindowEpoch returns the epoch of the active checkpoint window
 ///
-/// Determines the epoch to which new checkpoints and xshard transactions need
-/// to be assigned.
+/// Determines the epoch to which new checkpoints and cross-net transactions need
+/// to be assigned (i.e. the next checkpoint to be committed)
 pub fn window_epoch(epoch: ChainEpoch, period: ChainEpoch) -> ChainEpoch {
+    // TODO: Once we consider different genesis_epoch different to zero
+    // we should account for this here.
     let ind = epoch / period;
     period * (ind + 1)
 }

gateway/src/lib.rs

@@ -360,7 +360,7 @@ impl Actor {
                         e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "error flushing checkpoint")
                     })?;
 
-                    let cross_msgs = commit.take_cross_msgs();
+                    let cross_msgs = commit.cross_msgs();
 
                     // update prev_check for child
                     sub.prev_checkpoint = Some(commit);

gateway/src/state.rs

@@ -183,7 +183,7 @@ impl State {
         if epoch < 0 {
             return Err(anyhow!("epoch can't be negative"));
         }
-        let ch_epoch = checkpoint_epoch(epoch, self.bottomup_check_period);
+        let ch_epoch = window_epoch(epoch, self.bottomup_check_period);
         let checkpoints = self.bottomup_checkpoints.load(store)?;
 
         Ok(match get_checkpoint(&checkpoints, ch_epoch)? {

gateway/tests/gateway_test.rs

@@ -10,11 +10,11 @@ use fvm_shared::econ::TokenAmount;
 use fvm_shared::error::ExitCode;
 use fvm_shared::METHOD_SEND;
 use ipc_actor_common::vote::{EpochVoteSubmissions, UniqueVote};
-use ipc_gateway::checkpoint::BatchCrossMsgs;
+use ipc_gateway::checkpoint::{window_epoch, BatchCrossMsgs};
 use ipc_gateway::Status::{Active, Inactive};
 use ipc_gateway::{
     get_topdown_msg, BottomUpCheckpoint, CrossMsg, IPCAddress, PostBoxItem, State, StorableMsg,
-    TopDownCheckpoint, CROSS_MSG_FEE, DEFAULT_CHECKPOINT_PERIOD, SUBNET_ACTOR_REWARD_METHOD,
+    TopDownCheckpoint, CROSS_MSG_FEE, SUBNET_ACTOR_REWARD_METHOD,
 };
 use ipc_sdk::subnet_id::SubnetID;
 use ipc_sdk::{epoch_key, Validator, ValidatorSet};
@@ -263,8 +263,13 @@ fn checkpoint_commit() {
     h.commit_child_check(&mut rt, &shid, &ch, ExitCode::OK)
         .unwrap();
     let st: State = rt.get_state();
+    // the child checkpoint should be committed as a child_check
+    // in the next checkpoint being populated.
     let commit = st.get_window_checkpoint(rt.store(), epoch).unwrap();
-    assert_eq!(commit.epoch(), DEFAULT_CHECKPOINT_PERIOD);
+    assert_eq!(
+        commit.epoch(),
+        window_epoch(epoch, st.bottomup_check_period)
+    );
     let child_check = has_childcheck_source(&commit.data.children, &shid).unwrap();
     assert_eq!(&child_check.checks.len(), &1);
     assert_eq!(has_cid(&child_check.checks, &ch.cid()), true);
@@ -281,7 +286,10 @@ fn checkpoint_commit() {
         .unwrap();
     let st: State = rt.get_state();
     let commit = st.get_window_checkpoint(rt.store(), epoch).unwrap();
-    assert_eq!(commit.epoch(), DEFAULT_CHECKPOINT_PERIOD);
+    assert_eq!(
+        commit.epoch(),
+        window_epoch(epoch, st.bottomup_check_period)
+    );
     let child_check = has_childcheck_source(&commit.data.children, &shid).unwrap();
     assert_eq!(&child_check.checks.len(), &2);
     assert_eq!(has_cid(&child_check.checks, &ch.cid()), true);
@@ -311,7 +319,10 @@ fn checkpoint_commit() {
         .unwrap();
     let st: State = rt.get_state();
     let commit = st.get_window_checkpoint(rt.store(), epoch).unwrap();
-    assert_eq!(commit.epoch(), DEFAULT_CHECKPOINT_PERIOD);
+    assert_eq!(
+        commit.epoch(),
+        window_epoch(epoch, st.bottomup_check_period)
+    );
     let child_check = has_childcheck_source(&commit.data.children, &shid_two).unwrap();
     assert_eq!(&child_check.checks.len(), &1);
     assert_eq!(has_cid(&child_check.checks, &ch.cid()), true);
@@ -373,7 +384,10 @@ fn checkpoint_crossmsgs() {
         .unwrap();
     let st: State = rt.get_state();
     let commit = st.get_window_checkpoint(rt.store(), epoch).unwrap();
-    assert_eq!(commit.epoch(), DEFAULT_CHECKPOINT_PERIOD);
+    assert_eq!(
+        commit.epoch(),
+        window_epoch(epoch, st.bottomup_check_period)
+    );
     let child_check = has_childcheck_source(&commit.data.children, &shid).unwrap();
     assert_eq!(&child_check.checks.len(), &1);
     let prev_cid = ch.cid();
@@ -469,10 +483,14 @@ fn test_release() {
     let releaser = Address::new_id(1001);
     // Release funds
     let r_amount = TokenAmount::from_atto(5_u64.pow(18));
-    rt.set_balance(2 * r_amount.clone());
-    h.release(&mut rt, &releaser, ExitCode::OK, r_amount.clone(), 0)
+    rt.set_balance(4 * r_amount.clone());
+    h.release(&mut rt, &releaser, ExitCode::OK, r_amount.clone(), 2, 0, 0)
+        .unwrap();
+    h.release(&mut rt, &releaser, ExitCode::OK, r_amount.clone(), 3, 1, 1)
+        .unwrap();
+    h.release(&mut rt, &releaser, ExitCode::OK, r_amount.clone(), 10, 2, 0)
         .unwrap();
-    h.release(&mut rt, &releaser, ExitCode::OK, r_amount, 1)
+    h.release(&mut rt, &releaser, ExitCode::OK, r_amount, 11, 3, 1)
         .unwrap();
 }
 

gateway/tests/harness.rs

@@ -346,13 +346,17 @@ impl Harness {
         releaser: &Address,
         code: ExitCode,
         value: TokenAmount,
+        epoch: ChainEpoch,
         expected_nonce: u64,
+        expected_msg_index: usize,
     ) -> Result<Cid, ActorError> {
         rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, *releaser);
         rt.expect_validate_caller_type(SIG_TYPES.clone());
         // set value and include the cross_msg_fee
         set_rt_value_with_cross_fee(rt, &value);
 
+        rt.set_epoch(epoch);
+
         if code != ExitCode::OK {
             expect_abort(code, rt.call::<Actor>(Method::Release as MethodNum, None));
             rt.verify();
@@ -384,10 +388,12 @@ impl Harness {
         let parent = &self.net_name.parent().unwrap();
         let from = IPCAddress::new(&self.net_name, &BURNT_FUNDS_ACTOR_ADDR).unwrap();
         let to = IPCAddress::new(&parent, &TEST_BLS).unwrap();
-        rt.set_epoch(0);
-        let ch = st.get_window_checkpoint(rt.store(), 0).unwrap();
+        let ch = st.get_window_checkpoint(rt.store(), epoch).unwrap();
+        // check that is included in the next checkpoint to be committed and not
+        // in a checkpoint template of the past
+        assert_eq!(ch.data.epoch > epoch, true);
 
-        let msg = ch.data.cross_msgs.cross_msgs.unwrap()[expected_nonce as usize].clone();
+        let msg = ch.data.cross_msgs.cross_msgs.unwrap()[expected_msg_index].clone();
         assert_eq!(msg.msg.from, from);
         assert_eq!(msg.msg.to, to);
         assert_eq!(msg.msg.nonce, expected_nonce);