bitcoindevkit / bdk

A modern, lightweight, descriptor-based wallet library written in Rust!
Other
860 stars 307 forks source link

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

Open evanlinjin opened 12 months ago

evanlinjin commented 12 months ago

Describe the enhancement

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

This is initially proposed by @LLFourn: https://github.com/bitcoindevkit/bdk/pull/1178#discussion_r1379454497

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>;
}
nondiremanuel commented 11 months ago

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 commented 10 months ago

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

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 commented 10 months ago

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.

notmandatory commented 7 months ago

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