Return consistent info for explicit and implicit cases

actors/verifreg/src/lib.rs

@@ -348,7 +348,8 @@ impl Actor {
         let client = params.client;
         let curr_epoch = rt.curr_epoch();
         let mut ret_gen = BatchReturnGen::new(params.allocation_ids.len());
-        // let mut recovered_datacap = DataCap::zero();
+        let mut considered_ids = params.allocation_ids.clone();
+        let mut recovered_datacap = DataCap::zero();
         let recovered_datacap = rt
             .transaction(|st: &mut State, rt| {
                 let mut allocs = st.load_allocs(rt.store())?;
@@ -362,14 +363,19 @@ impl Actor {
                                     ExitCode::USR_ILLEGAL_STATE,
                                     "failed to parse uint key",
                                 )?;
-                                to_remove.push(id);
+                                considered_ids.push(id);
                             }
                             Ok(())
                         })
                         .context_code(
                             ExitCode::USR_ILLEGAL_STATE,
                             "failed to iterate over allocations",
                         )?;
+                    to_remove = considered_ids.clone();
+                    ret_gen = BatchReturnGen::new(considered_ids.len());
+                    for _ in 0..considered_ids.len() {
+                        ret_gen.add_success();
+                    }
                 }
                 for alloc_id in params.allocation_ids {
                     // Check each specified allocation is expired.
@@ -394,7 +400,6 @@ impl Actor {
                     }
                 }
 
-                let mut recovered_datacap = DataCap::zero();
                 for id in to_remove {
                     let existing = allocs.remove(client, id).context_code(
                         ExitCode::USR_ILLEGAL_STATE,
@@ -417,7 +422,11 @@ impl Actor {
             )
         })?;
 
-        Ok(ret_gen.gen())
+        Ok(RemoveExpiredAllocationsReturn {
+            considered: considered_ids,
+            results: ret_gen.gen(),
+            datacap_recovered: recovered_datacap,
+        })
     }
 
     // Called by storage provider actor to claim allocations for data provably committed to storage.

actors/verifreg/src/types.rs

@@ -97,12 +97,25 @@ impl AddrPairKey {
 
 #[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)]
 pub struct RemoveExpiredAllocationsParams {
+    // Client for which to remove expired allocations.
     pub client: ActorID,
+    // Optional list of allocation IDs to attempt to remove.
+    // Empty means remove all eligible expired allocations.
     pub allocation_ids: Vec<AllocationID>,
 }
 impl Cbor for RemoveExpiredAllocationsParams {}
 
-pub type RemoveExpiredAllocationsReturn = BatchReturn;
+#[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)]
+pub struct RemoveExpiredAllocationsReturn {
+    // Ids of the allocations that were either specified by the caller or discovered to be expired.
+    pub considered: Vec<AllocationID>,
+    // Results for each processed allocation.
+    pub results: BatchReturn,
+    // The amount of datacap reclaimed for the client.
+    #[serde(with = "bigint_ser")]
+    pub datacap_recovered: DataCap,
+}
+impl Cbor for RemoveExpiredAllocationsReturn {}
 
 #[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)]
 pub struct SectorAllocationClaim {

actors/verifreg/tests/verifreg_actor_test.rs

@@ -422,8 +422,9 @@ mod clients {
 
 mod claims {
     use fvm_shared::error::ExitCode;
+    use num_traits::Zero;
 
-    use fil_actor_verifreg::State;
+    use fil_actor_verifreg::{AllocationID, DataCap, State};
     use fil_actors_runtime::runtime::Runtime;
     use harness::*;
 
@@ -445,49 +446,72 @@ mod claims {
 
         // Can't remove allocations that aren't expired
         let ret = h.remove_expired_allocations(&mut rt, &CLIENT, vec![1, 2], 0).unwrap();
-        assert_eq!(vec![ExitCode::USR_FORBIDDEN, ExitCode::USR_FORBIDDEN], ret.codes());
+        assert_eq!(vec![1, 2], ret.considered);
+        assert_eq!(vec![ExitCode::USR_FORBIDDEN, ExitCode::USR_FORBIDDEN], ret.results.codes());
+        assert_eq!(DataCap::zero(), ret.datacap_recovered);
 
         // Remove the first alloc, which expired.
         rt.set_epoch(100);
         let ret =
             h.remove_expired_allocations(&mut rt, &CLIENT, vec![1, 2], alloc1.size.0).unwrap();
-        assert_eq!(vec![ExitCode::OK, ExitCode::USR_FORBIDDEN], ret.codes());
+        assert_eq!(vec![1, 2], ret.considered);
+        assert_eq!(vec![ExitCode::OK, ExitCode::USR_FORBIDDEN], ret.results.codes());
+        assert_eq!(DataCap::from(alloc1.size.0), ret.datacap_recovered);
 
         // Remove the second alloc (the first is no longer found).
         rt.set_epoch(200);
         let ret =
             h.remove_expired_allocations(&mut rt, &CLIENT, vec![1, 2], alloc2.size.0).unwrap();
-        assert_eq!(vec![ExitCode::USR_NOT_FOUND, ExitCode::OK], ret.codes());
+        assert_eq!(vec![1, 2], ret.considered);
+        assert_eq!(vec![ExitCode::USR_NOT_FOUND, ExitCode::OK], ret.results.codes());
+        assert_eq!(DataCap::from(alloc2.size.0), ret.datacap_recovered);
 
         // Reset state and show we can remove two at once.
         rt.replace_state(&state_with_allocs);
         let ret = h.remove_expired_allocations(&mut rt, &CLIENT, vec![1, 2], total_size).unwrap();
-        assert_eq!(vec![ExitCode::OK, ExitCode::OK], ret.codes());
+        assert_eq!(vec![1, 2], ret.considered);
+        assert_eq!(vec![ExitCode::OK, ExitCode::OK], ret.results.codes());
+        assert_eq!(DataCap::from(total_size), ret.datacap_recovered);
+
+        // Reset state and show that only what was asked for is removed.
+        rt.replace_state(&state_with_allocs);
+        let ret = h.remove_expired_allocations(&mut rt, &CLIENT, vec![1], alloc1.size.0).unwrap();
+        assert_eq!(vec![1], ret.considered);
+        assert_eq!(vec![ExitCode::OK], ret.results.codes());
+        assert_eq!(DataCap::from(alloc1.size.0), ret.datacap_recovered);
 
         // Reset state and show that specifying none removes only expired allocations
         rt.set_epoch(0);
         rt.replace_state(&state_with_allocs);
         let ret = h.remove_expired_allocations(&mut rt, &CLIENT, vec![], 0).unwrap();
-        assert_eq!(Vec::<ExitCode>::new(), ret.codes()); // Empty response when no allocs specified
+        assert_eq!(Vec::<AllocationID>::new(), ret.considered);
+        assert_eq!(Vec::<ExitCode>::new(), ret.results.codes());
+        assert_eq!(DataCap::zero(), ret.datacap_recovered);
         assert!(h.load_alloc(&mut rt, &CLIENT, id1).is_some());
         assert!(h.load_alloc(&mut rt, &CLIENT, id2).is_some());
 
         rt.set_epoch(100);
         let ret = h.remove_expired_allocations(&mut rt, &CLIENT, vec![], alloc1.size.0).unwrap();
-        assert_eq!(Vec::<ExitCode>::new(), ret.codes());
+        assert_eq!(vec![1], ret.considered);
+        assert_eq!(vec![ExitCode::OK], ret.results.codes());
+        assert_eq!(DataCap::from(alloc1.size.0), ret.datacap_recovered);
         assert!(h.load_alloc(&mut rt, &CLIENT, id1).is_none()); // removed
         assert!(h.load_alloc(&mut rt, &CLIENT, id2).is_some());
 
         rt.set_epoch(200);
         let ret = h.remove_expired_allocations(&mut rt, &CLIENT, vec![], alloc2.size.0).unwrap();
-        assert_eq!(Vec::<ExitCode>::new(), ret.codes());
+        assert_eq!(vec![2], ret.considered);
+        assert_eq!(vec![ExitCode::OK], ret.results.codes());
+        assert_eq!(DataCap::from(alloc2.size.0), ret.datacap_recovered);
         assert!(h.load_alloc(&mut rt, &CLIENT, id1).is_none()); // removed
         assert!(h.load_alloc(&mut rt, &CLIENT, id2).is_none()); // removed
 
         // Reset state and show that specifying none removes *all* expired allocations
         rt.replace_state(&state_with_allocs);
         let ret = h.remove_expired_allocations(&mut rt, &CLIENT, vec![], total_size).unwrap();
-        assert_eq!(Vec::<ExitCode>::new(), ret.codes());
+        assert_eq!(vec![1, 2], ret.considered);
+        assert_eq!(vec![ExitCode::OK, ExitCode::OK], ret.results.codes());
+        assert_eq!(DataCap::from(total_size), ret.datacap_recovered);
         assert!(h.load_alloc(&mut rt, &CLIENT, id1).is_none()); // removed
         assert!(h.load_alloc(&mut rt, &CLIENT, id2).is_none()); // removed
     }

runtime/src/util/batch_return.rs

@@ -60,7 +60,7 @@ impl BatchReturnGen {
     }
 
     pub fn gen(&self) -> BatchReturn {
-        assert_eq!(self.expect_count, self.success_count + self.fail_codes.len(), "programmer error, mismatched batch size {} and processed coutn {} batch return must include success/fail for all inputs", self.expect_count, self.success_count + self.fail_codes.len());
+        assert_eq!(self.expect_count, self.success_count + self.fail_codes.len(), "programmer error, mismatched batch size {} and processed count {} batch return must include success/fail for all inputs", self.expect_count, self.success_count + self.fail_codes.len());
         BatchReturn { success_count: self.success_count, fail_codes: self.fail_codes.clone() }
     }
 }