Skip to content
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

Wallet's persistence should have a fixed "header" section #1193

Open
Tracked by #87
evanlinjin opened this issue Nov 2, 2023 · 4 comments
Open
Tracked by #87

Wallet's persistence should have a fixed "header" section #1193

evanlinjin opened this issue Nov 2, 2023 · 4 comments
Labels
discussion There's still a discussion ongoing module-database new feature New feature or request

Comments

@evanlinjin
Copy link
Member

Describe the enhancement

Add a fixed header section to the wallet's persistence.

This is initially proposed by @LLFourn: #1178 (comment)

Use case

This can be used to store data that are mostly unchanging such as network type, version, etc.

Implementation proposal

pub struct Database<H, C>: PersistBackend<C> {
    fn write_header(&mut self, header: H) -> Result<(), Self::WriteError>;
    fn load_header(&self) -> Result<H, Self::LoadError>;
}
@evanlinjin evanlinjin added the new feature New feature or request label Nov 2, 2023
@evanlinjin evanlinjin added this to BDK Nov 2, 2023
@nondiremanuel nondiremanuel moved this to Todo in BDK Nov 5, 2023
@nondiremanuel
Copy link

During today's discussion in the Lib Team Call, we were assuming that this issue requires breaking changes and so that it could be required for alpha.3. What do you think @evanlinjin?

@ValuedMammal
Copy link
Contributor

ValuedMammal commented Dec 12, 2023

I can put together a draft for this. The plan will be:

  • Add a trait Database to bdk_chain::persist, and impl Database for bdk_file_store::Store
  • Add a struct in bdk::wallet for holding wallet metadata such as the bitcoin network (descriptor hashes should eventually go here as well).
  • Drop network member from wallet::ChangeSet and replace it with something like meta: Option<Meta>

I'm happy to collaborate if there are other ideas out there, and let me know if this isn't the solution you had in mind.

After staring at the code in store.rs, I tend to view persistence in terms of a flat file, so I'm wondering if the header concept will generalize just as well to an sqlite context - or if that would entail a totally new interface.

edit: I will shelf this idea for now

@LLFourn
Copy link
Contributor

LLFourn commented Dec 12, 2023

I think it will map fine to sqlite. My main concern is that this isn't a "pants on fire" problem for us yet. I'm not sure it's worth the API complexity to remove this error case where you try and change the network or genesis block at a later point. The actual APIs will never produce this changeset so we're talking about making some (but not all) programmer errors impossible. If you are applying the wrong changeset to the wrong thing is it really a big benefit to remove the invalid action of changing the network even when this still can add wrong transactions or blocks to the thing?

Definitely bring this up on dev call if you haven't already to see what others think.

@nondiremanuel nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0 Jan 6, 2024
@nondiremanuel nondiremanuel added the discussion There's still a discussion ongoing label Jan 25, 2024
@nondiremanuel nondiremanuel moved this from Todo to Discussion in BDK Jan 25, 2024
@notmandatory
Copy link
Member

I agree this isn't a critical issue and should be pushed to the post 1.0.0 milestone.

@nondiremanuel nondiremanuel removed this from the 1.0.0-alpha milestone Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion There's still a discussion ongoing module-database new feature New feature or request
Projects
Status: Discussion
Development

No branches or pull requests

5 participants