refactor!: Implement generics for CheckPoint, LocalChain, and spk_client types

[LagginTimes]

Implements #1937.

Description

This PR is a step towards header checkpointing for bdk_electrum. The goal is to be able to store whole headers in CheckPoint so they do not have to be re-downloaded. Storing headers this way would be a prerequisite for caching of merkle proofs and for median time passed.

Notes to the reviewers

Changelog notice

• CheckPoint takes in a generic.
• LocalChain and ChangeSet take in generics.
• spk_client types can take in generics.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

[ValuedMammal]

Tests are passing for me.

But I think we should still have the function _check_changeset_is_applied. Edit: unless I misread an earlier comment saying it is no longer needed. I tested with and without it, plus the unit tests should be sufficient, so feel free to ignore.

diff

--- a/crates/chain/src/local_chain.rs
+++ b/crates/chain/src/local_chain.rs
@@ -270,8 +270,9 @@ where
         };
 
         let (mut chain, _) = Self::from_genesis(genesis_data);
         chain.apply_changeset(&changeset)?;
+        debug_assert!(chain._check_changeset_is_applied(&changeset));
         Ok(chain)
     }
 
     /// Construct a [`LocalChain`] from a given `checkpoint` tip.
@@ -303,16 +304,18 @@ where
         update: CheckPoint<D>,
     ) -> Result<ChangeSet<D>, CannotConnectError> {
         let (new_tip, changeset) = merge_chains(self.tip.clone(), update)?;
         self.tip = new_tip;
+        debug_assert!(self._check_changeset_is_applied(&changeset));
         Ok(changeset)
     }
 
     /// Apply the given `changeset`.
     pub fn apply_changeset(&mut self, changeset: &ChangeSet<D>) -> Result<(), MissingGenesisError> {
         let old_tip = self.tip.clone();
         let new_tip = apply_changeset_to_checkpoint(old_tip, changeset)?;
         self.tip = new_tip;
+        debug_assert!(self._check_changeset_is_applied(changeset));
         Ok(())
     }
 
     /// Derives an initial [`ChangeSet`], meaning that it can be applied to an empty chain to
@@ -401,8 +404,30 @@ where
             None => return Ok(ChangeSet::default()),
         };
         Ok(changeset)
     }
+
+    fn _check_changeset_is_applied(&self, changeset: &ChangeSet<D>) -> bool {
+        let mut cur = self.tip.clone();
+        for (&exp_height, exp_data) in changeset.blocks.iter().rev() {
+            match cur.get(exp_height) {
+                Some(cp) => {
+                    if cp.height() != exp_height
+                        || Some(cp.hash()) != exp_data.map(|d| d.to_blockhash())
+                    {
+                        return false;
+                    }
+                    cur = cp;
+                }
+                None => {
+                    if exp_data.is_some() {
+                        return false;
+                    }
+                }
+            }
+        }
+        true
+    }
 }

[ValuedMammal]

ACK e1e1959

[ValuedMammal]

ACK dabb869

[ValuedMammal]

ACK 0a39fa7 pending a CI fix.

I left comments mostly for discussion. Also there was a comment #1582 (review) suggesting to use B instead of D. I don't have much preference though.

[notmandatory]

I haven't done a deep review but I support merging this now that we're done with the bdk_wallet 2.1 release.

[oleonardolima]

utACK d4bd7e5

The code looks good to me, I only left a minor question and nit. I didn't test it on a complicated example yet.

[ValuedMammal]

ACK 4dce5ad

[notmandatory]

utACK 4dce5ad

I also re-read through the whole history and this one has certainly been a journey so thanks to @LagginTimes and all the reviewers for sticking with it.

I look forward to the followup PRs to integrate chain client specific data in the checkpoints. I expect custom data will break existing persistence so being able to persist the data in a generic way and having a good persistence test suite such as #2012 will be handy.

[oleonardolima]

ACK 4dce5ad

[evanlinjin]

Looking back through the codebase, I noticed that we've introduced a condition that wasn't there before, and I’m wondering about the best way to handle it.

Since a Header contains the previous block hash, two CheckPoint<Header> chains can be merged even if their heights differ. For example, if chain A ends at height 10 and chain B only has a single block at height 11, we can still connect them if B’s previous block points to A’s height 10.

---

One idea is to modify ToBlockHash to have knowledge of whether the type has the previous blockhash.

pub trait ToBlockHash {
    /* OTHER METHODS */

    /// Returns `None` if the type has no knowledge of the previous blockhash.
    fn prev_blockhash(&self) -> Option<BlockHash>;
}

Then we need to modify merge_chains to take it into account.

Should we modify CheckPoint to allow empty values (knowing the block hash & height, but not the data yet)?

• Edit: On second thought, no. Allowing CheckPoint to hold empty data would increase the risk of inconsistent representations—for example, having a Header at height 10 while forgetting to include the “ghost checkpoint” at height 9.
• A better idea?: I think we should modify the CheckPointIter instead to also return "ghost checkpoint"s. The Iterator::Item type needs to be changed to MaybeCheckPoint<D> or something.
• Caveat: Methods that construct/extend/insert-into CheckPoint-chains should really error/fail if an inserted block's prevhash does not match the previous value (if it exists).

[ValuedMammal]

Since a Header contains the previous block hash, two CheckPoint<Header> chains can be merged even if their heights differ. For example, if chain A ends at height 10 and chain B only has a single block at height 11, we can still connect them if B’s previous block points to A’s height 10.

One idea is to modify ToBlockHash to have knowledge of whether the type has the previous blockhash.

pub trait ToBlockHash {
    /* OTHER METHODS */

    /// Returns `None` if the type has no knowledge of the previous blockhash.
    fn prev_blockhash(&self) -> Option<BlockHash>;
}

Then we need to modify merge_chains to take it into account.

Yes in principle I agree with this if the update is strictly extending the parent chain by the next consecutive block. If the type doesn't know the previous hash, like in the case of CheckPoint<BlockHash>, then I think we'd still require the existence of a point of agreement.

[evanlinjin]

I've started experimenting with my suggestion in #1582 (comment).

This is the WIP commit: evanlinjin@8f374cc. I'm quite happy with the implementation of it (unless proven otherwise).

What is missing:

• Figure out best practice for handling CheckPointEntry::Backrefs (ghost-checkpoints) that connect.
• Methods that build on CheckPoint (i.e. push, from_blocks, extend, insert) need to check Backrefs match.
• Tests.
• More docs.

Feel free to continue this work as you see fit.

---

I'm wondering if we should merge this PR as is, and introduce "ghost-checkpoint"s in a separate PR?

[LagginTimes]

I've started experimenting with my suggestion in #1582 (comment).

This is the WIP commit: evanlinjin@8f374cc. I'm quite happy with the implementation of it (unless proven otherwise).

What is missing:

* Figure out best practice for handling `CheckPointEntry::Backref`s (ghost-checkpoints) that connect.

* Methods that build on `CheckPoint` (i.e. `push`, `from_blocks`, `extend`, `insert`) need to check `Backref`s match.

* Tests.

* More docs.

Feel free to continue this work as you see fit.

I'm wondering if we should merge this PR as is, and introduce "ghost-checkpoint"s in a separate PR?

I think it makes sense to merge #1582 as it is now. The additional work around ghost-checkpoints, backref checks, and more docs/tests can be done in a follow-up PR to keep the scope focused and avoid resetting review. I would be happy to continue the work in a follow-up PR.

[evanlinjin]

@LagginTimes I'll be happy with that. Do we have a plan for back-porting the non-merged chain-source fixes (#2005, #2011, #2000)? Note that the CheckPointEntry PR may break the API again.

[ValuedMammal]

if chain A ends at height 10 and chain B only has a single block at height 11, we can still connect them if B’s previous block points to A’s height 10.

For this I would look at things we already have such as insert_block.

[evanlinjin]

ACK 8bc2391