-
Notifications
You must be signed in to change notification settings - Fork 44
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.
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
base: master
Are you sure you want to change the base?
Add Wallet::apply_changeset
#231
Conversation
Pull Request Test Coverage Report for Build 15037397098Details
💛 - Coveralls |
| /// | ||
| /// # Warning | ||
| /// | ||
| /// Applying changesets out of order may leave your wallet in an inconsistent state. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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()
}
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
The device we want this feature for is connected via BLE (Bluetooth Low Energy). We are yet to optimize the throughput but even now we're talking seconds and I don't foresee any UX issues as long serialized data is not in tens of megabytes range.
However, because
CheckPointis 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 ofSerializeandDeserialize. To move forward, I propose that we do some customSerialize/Deserializeimpls 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() } }
Sounds good!
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.
Generally we like the idea of the device wallet being "full blown" with the exception of sync. In broad strokes what would a hypothetical wallet that synced just the txs and utxos be lacking compared to that? I assume this is something that also needs to be built out?
Description
My proposed alternative for #229.
To Do:
Expand on the "Warning" for the
apply_changesetdoc comments. We really don't want most users doing this untilLocalChainchangesets become monotone.Changelog notice
Wallet::apply_chanesetmethod.Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features: