fix(checkpoint): Relax Copy constraint to Clone

Previously, methods of `CheckPoint` were constrained by `D: Copy`,
and this was to prevent a possible memory leak caused by the
use of `mem::forget` in the Drop impl for `CheckPoint`. Since the
memory leak concern has been addressed (#2056), we relax the
constraint to `Clone` instead which permits instances of
`CheckPoint<D>` where `D` is not necessarily a Copy type.

Author: ValuedMammal

crates/chain/src/local_chain.rs

@@ -28,7 +28,7 @@ fn apply_changeset_to_checkpoint<D>(
     changeset: &ChangeSet<D>,
 ) -> Result<CheckPoint<D>, ApplyChangeSetError<D>>
 where
-    D: ToBlockHash + fmt::Debug + Copy,
+    D: ToBlockHash + fmt::Debug + Clone,
 {
     if let Some(start_height) = changeset.blocks.keys().next().cloned() {
         // changes after point of agreement
@@ -45,13 +45,13 @@ where
             }
         }
 
-        for (&height, &data) in &changeset.blocks {
+        for (height, data) in &changeset.blocks {
             match data {
                 Some(data) => {
-                    extension.insert(height, data);
+                    extension.insert(*height, data.clone());
                 }
                 None => {
-                    extension.remove(&height);
+                    extension.remove(height);
                 }
             };
         }
@@ -248,7 +248,7 @@ impl<D> LocalChain<D> {
 // Methods where `D: ToBlockHash`
 impl<D> LocalChain<D>
 where
-    D: ToBlockHash + fmt::Debug + Copy,
+    D: ToBlockHash + fmt::Debug + Clone,
 {
     /// Constructs a [`LocalChain`] from genesis data.
     pub fn from_genesis(data: D) -> (Self, ChangeSet<D>) {
@@ -280,7 +280,7 @@ where
 
     /// Construct a [`LocalChain`] from an initial `changeset`.
     pub fn from_changeset(changeset: ChangeSet<D>) -> Result<Self, MissingGenesisError> {
-        let genesis_entry = changeset.blocks.get(&0).copied().flatten();
+        let genesis_entry = changeset.blocks.get(&0).cloned().flatten();
         let genesis_data = match genesis_entry {
             Some(data) => data,
             None => return Err(MissingGenesisError),
@@ -430,7 +430,7 @@ where
             match cur.get(exp_height) {
                 Some(cp) => {
                     if cp.height() != exp_height
-                        || Some(cp.hash()) != exp_data.map(|d| d.to_blockhash())
+                        || Some(cp.hash()) != exp_data.as_ref().map(ToBlockHash::to_blockhash)
                     {
                         return false;
                     }
@@ -596,7 +596,7 @@ impl std::error::Error for ApplyHeaderError {}
 
 impl<D> LocalChain<D>
 where
-    D: ToBlockHash + fmt::Debug + Copy,
+    D: ToBlockHash + fmt::Debug + Clone,
 {
     /// Applies `update_tip` onto `original_tip`.
     ///

crates/core/src/checkpoint.rs

@@ -201,7 +201,7 @@ impl<D> CheckPoint<D> {
 // Methods where `D: ToBlockHash`
 impl<D> CheckPoint<D>
 where
-    D: ToBlockHash + fmt::Debug + Copy,
+    D: ToBlockHash + fmt::Debug + Clone,
 {
     /// Construct a new base [`CheckPoint`] from given `height` and `data` at the front of a linked
     /// list.
@@ -311,7 +311,7 @@ where
         // If we detect a conflict we need to revert to the previous checkpoint,
         // effectively displacing the old base. This will leave a gap in the chain
         // until the correct data is inserted.
-        if base.is_conflict_of(height, data) {
+        if base.is_conflict_of(height, &data) {
             base = base.prev().expect("cannot displace genesis block");
         }
 
@@ -334,7 +334,7 @@ where
             return Err(self);
         }
         // The pushed data must not conflict with self.
-        if self.is_conflict_of(height, data) {
+        if self.is_conflict_of(height, &data) {
             return Err(self);
         }
         Ok(Self(Arc::new(CPInner {
@@ -351,7 +351,7 @@ where
     ///
     /// This can happen whenever the incoming data tries to build upon the current
     /// height but `prev_blockhash` of `data` doesn't match the hash of this [`CheckPoint`].
-    fn is_conflict_of(&self, height: u32, data: D) -> bool {
+    fn is_conflict_of(&self, height: u32, data: &D) -> bool {
         self.height().saturating_add(1) == height
             && data
                 .prev_blockhash()
@@ -361,7 +361,7 @@ where
     /// Whether this [`CheckPoint`] may be considered a valid dependency (parent) of the given
     /// `height` and `data`.
     #[allow(unused)]
-    fn is_dependency_of(&self, height: u32, data: D) -> bool {
+    fn is_dependency_of(&self, height: u32, data: &D) -> bool {
         let height_diff = height.saturating_sub(self.height());
         // Either a gap exists, in which case there will not be a conflict
         height_diff > 1
@@ -413,7 +413,7 @@ pub struct CheckPointEntryIter<D> {
 
 impl<D> Iterator for CheckPointEntryIter<D>
 where
-    D: ToBlockHash + fmt::Debug + Copy,
+    D: ToBlockHash + fmt::Debug + Clone,
 {
     type Item = CheckPointEntry<D>;