Skip to content

Add Wallet::apply_changeset #231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+17 −0
Draft

Add Wallet::apply_changeset #231

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions wallet/src/wallet/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2249,6 +2249,23 @@ impl Wallet {
Ok(())
}

/// Applies a changeset.
///
/// # Warning
///
/// Applying changesets out of order may leave your wallet in an inconsistent state.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the OoO caveat apply to apply_update above too? If it does not I wonder if the increased serialized size (I'm getting a few kb) is a valid tradeoff

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point. I'm still a bit concerned about serializing Updates because of how CheckPoints can be used (as it is part of an Update).

CheckPoints are used to communicate between the wallet and the chain source.

  • The wallet uses CheckPoints to tell the chain source it's initial blockchain state (before any updates).
  • Then the chain source sends CheckPoints back to the wallet which is extended from what was provided from the wallet (as updates).

These CheckPoints can be sparse, or may include the whole chain. Additionally, we've been looking at making CheckPoints generic so that alternative data types such as BlockHeader (instead of just BlockHash) can be stored (which would allow us to calculate MTP properly).

Chain sources will be improved to incorporate more data into CheckPoints. For example, we've come to realize that the only correct way to use Electrum is basically download every block (except for some of those below Electrum checkpoints - different to BDK CheckPoints). Eventually, we'll have a streaming version of bdk_electrum that will include block headers in CheckPoints and have a lot more CheckPoints than what we have now.

To summarize, what I am saying is that I expect a lot more data being shoved in CheckPoints in the future (hope you are okay with this?). If you are okay with this point, I will be happy to support this feature.

However, because CheckPoint is a linked list (so it's a deeply nested data structure), we may risk blowing the thread-stack if we purely use derive-generated, purely recursive impls of Serialize and Deserialize. To move forward, I propose that we do some custom Serialize/Deserialize impls into a list. ChatGPT gave some suggestions (not sure if it compiles):

use serde::ser::{Serialize, Serializer, SerializeSeq};

impl Serialize for CheckPoint {
    fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
        // First, count how many nodes (or stash them somehow).
        let mut len = 0;
        let mut node = Some(self);
        while let Some(n) = node {
            len += 1;
            node = n.next.as_deref();
        }

        let mut seq = serializer.serialize_seq(Some(len))?;
        let mut node = Some(self);
        while let Some(n) = node {
            seq.serialize_element(&n.data)?;       // whatever you need to emit
            node = n.next.as_deref();
        }
        seq.end()
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way to do this, assuming that the wallet on the device is a "dumb wallet", is to just send over the canonical txs/UTXOs where the device is basically a slave of the main wallet. Not sure if this is appropriate with what you guys are going for.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, the caveat with ChangeSets will disappear once we modify/replace LocalChain with a GraphChain (which @ValuedMammal is working on).

pub fn apply_changeset(&mut self, changeset: ChangeSet) {
self.chain
.apply_changeset(&changeset.local_chain)
.expect("must not remove genesis block");
self.indexed_graph
.apply_changeset(indexed_tx_graph::ChangeSet {
tx_graph: changeset.tx_graph.clone(),
indexer: changeset.indexer.clone(),
});
self.stage.merge(changeset);
}

/// Get a reference of the staged [`ChangeSet`] that is yet to be committed (if any).
pub fn staged(&self) -> Option<&ChangeSet> {
if self.stage.is_empty() {
Expand Down
Loading