Convert ChainDB to a trait #42
Conversation
…mmersbald default optional dependency.
|
Do you think it might be better to separate the disk storage and in memory storage so that the api is just store header, fetch header, fetch tip hash, store tip hash? There are also a few hammersbald specific functions that maybe shouldn't be part of the api like shutdown and batch. What are your thoughts? Maybe a separate chain storage and chain db trait? |
|
@jrawsthorne good suggestions, working on an update now to create separate ChainDB and HeaderCache traits. I think the only hammersbald specific ChainDB methods are "batch" and "shutdown". I can remove "shutdown" since I added it to try and make murmel work on Android but it didn't fix anything. The "batch" method should be equivalent in SQLite to commit transaction, I may rename it for now and decide what to do with it after the SQLite feature is implemented. |
|
Concept ACK. Test passes. I am not sure I fully got why its beneficial to have separate traits for disk and memory storage. It seems to me that the DB should have its internal logic to manage it and the API should not be exposed over this logic. That's how hammersbald do it currently. Possibly I am missing something but separation here seems counterproductive? |
|
After taking a stab at extracting the header cache functions into a new trait, I think @rajarshimaitra is right that we shouldn't do it right now. I also would like focus on a new MR to add the SQLite DB. After that's done we can look at both DB implementations and see what API changes make the most sense. @jrawsthorne is that OK for you? |
|
@notmandatory Yeah if you want to work on that go ahead. At this point in the project it's fine to iterate quickly and perfect later on. I think the reasoning was that each impl of the trait has to implement all the functionality related to the header cache like init headers and init to genesis so it would be best to extract that out. E.g. if we added sqlite as well you'd have an sqlite struct with a header cache as well as the sqlite stuff. But as I said its fine for now |
|
Cool, down the line it might be good to hide the distinction between CachedHeader and StoredHeader (and other types) from the rest of the code, should be an implementation detail only the ChainDB provider needs to care about. |
|
Seems good, sorry for the delay! :) |
4 participants
The purpose of the PR is to de-couple murmel from Hammersbald so that alternative chain data stores can be used. In particular I'd like to use sqlite with murmel to store chain data for the bitcoindevkit/bdk project. A follow-on MR will add sqlite as an optional feature and dependency that also implements the ChainDB trait.