Multi descriptor: Introduce DatabaseFactory trait
#654
Conversation
evanlinjin
force-pushed
the
database-factory
branch
from
7ed0e1f to
f484619
Compare
evanlinjin
changed the title
DatabaseFactory traitDatabaseFactory trait
evanlinjin
changed the title
DatabaseFactory traitDatabaseFactory trait
evanlinjin
changed the title
DatabaseFactory traitDatabaseFactory trait
evanlinjin
force-pushed
the
database-factory
branch
4 times, most recently
from
cdc0f56 to
68da4b3
Compare
| /// Each built database is stored in path of format: `<path_root>_<hash>.<path_ext>` | ||
| /// Where `hash` contains identifying data derived with inputs provided by | ||
| /// [`DatabaseFactory::build`] or [`DatabaseFactory::build_with_change`] calls. | ||
| pub struct SqliteDatabaseFactory<P> { |
There was a problem hiding this comment.
I had an idea for this, let me know what you think: in theory it should be possible to just have a single database by adding an extra column to all the relevant tables that identifies to which descriptor the rows belong.
So my idea for the factory would be something that wraps the db (probably in an Arc) and all the actual SqliteDatabase struct would have a member inside to keep track of what id it belongs to. If you create a SqliteDatabase directly this could either be None or just an empty string.
Then we just have to add an additional WHERE clause to the queries and everything should work magically.
Why we may want to do this? Mainly because I guess having a single file makes it easier to work with, backup, etc. And also this opens the possibility to add more methods in the future that iterate over the whole db, which would probably bring a performance benefit to MultiTracker.
Disadvantages? I guess the extra WHERE clause might cause a small performance hit? But we can also add an index on that column if we feel like it.
There was a problem hiding this comment.
I think this is a great idea! I would assume that performance wouldn't be too much of an issue since we are already using indexes for almost all tables/queries.
However, I feel like this should be in it's separate issue/PR since we need to change the "nature" of SqliteDatabase? What do you think?
There was a problem hiding this comment.
I think @afilini's suggestion can be done in this PR if you want to give it a shot. Once that's in this should be ready for general review right?
There was a problem hiding this comment.
@notmandatory Thank you for including this PR in the last call (which I failed to attend 😢 ). I have not touched this PR since the last review because I am skeptical on whether this is actually needed.
My current thinking is that Database itself is NOT an ideal model for implementing multi-descriptor, and the correct approach seems to be that of which bdk_core is doing. I think more discussion is needed before I would be willing to continue/close this PR.
There was a problem hiding this comment.
No problem, I think it's fine to leave this one on hold and see how a better approach shapes up with bdk_core.
This is part of bitcoindevkit#486 to add multi-descriptor wallet support to BDK.
evanlinjin
force-pushed
the
database-factory
branch
from
0f6292b to
98b74c4
Compare
6db5b4a Introduce `get_checksum_bytes` method and improvements (志宇) Pull request description: ### Description `get_checksum_bytes()` returns a descriptor checksum as `[u8; 8]` instead of `String`, potentially improving performance and memory usage. In addition to this, since descriptors only use characters that fit within a UTF-8 8-bit code unit ([US-ASCII](https://www.charset.org/charsets/us-ascii)), there is no need to use the `char` type (which is 4 bytes). This can also potentially bring in some performance and memory-usage benefits. ### Notes to the reviewers This is useful because we will be using descriptor checksums for indexing operations in the near future (multi-descriptor wallets #486 ). Refer to comments by @afilini : * #647 (comment) * #647 (comment) * #654 (comment) ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### New Features: * [x] I've added tests for the new feature * [x] I've added docs for the new feature * [x] I've updated `CHANGELOG.md` ACKs for top commit: afilini: ACK 6db5b4a Tree-SHA512: 1cecc3a1514a3ec3ac0a50775f6b3c4dd9785e3606390ceba57cc6248b8ff19c4023add0643c48dd9d84984341c506c036c4880fca4a4358ce1b54ccb4c56687
Description
This is part of #486 to add multi-descriptor wallet support to BDK.
As each
DescriptorTrackershould have it's own database, and aMultiTrackerstructure will handle an aggregate ofDescriptorTrackers, we need a factory structure that theMultiTrackercan use to createDatabases for eachDescriptorTracker.Notes to the reviewers
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
CHANGELOG.md