< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #18875: fuzz: Stop nodes in process_message* fuzzers (master...2005-fuzzStopNodes) https://github.com/bitcoin/bitcoin/pull/18875
< meshcollider>
maybe #9381 should go on high priority just to get it in finally, it has been sitting around for years with some decent review between rebases, if a few people reviewed it this week before it requires rebase again, it could definitely go in
< bitcoin-git>
bitcoin/master ff046ae practicalswift: wallet: Make sure no DescriptorScriptPubKeyMan members are uninitialized a...
< bitcoin-git>
bitcoin/master 2a78098 practicalswift: wallet: Make sure no WalletDescriptor members are uninitialized after cons...
< bitcoin-git>
bitcoin/master ec79b5f Samuel Dobson: Merge #18782: wallet: Make sure no DescriptorScriptPubKeyMan or WalletDesc...
< bitcoin-git>
[bitcoin] meshcollider merged pull request #18782: wallet: Make sure no DescriptorScriptPubKeyMan or WalletDescriptor members are left uninitialized after construction (master...uninitialized-members) https://github.com/bitcoin/bitcoin/pull/18782
< bitcoin-git>
[bitcoin] luke-jr opened pull request #18880: [0.20] wallet: Add BlockUntilSyncedToCurrentChain to dumpwallet (0.20...pr18671-0.20) https://github.com/bitcoin/bitcoin/pull/18880
< achow101>
#proposedwalletmeetingtopic Moving to a new storage database for descriptor wallets
< sipa>
reviving #5686?
< gribble>
https://github.com/bitcoin/bitcoin/issues/5686 | [Wallet] replace BDB with internal append only (logdb) backend by jonasschnelli · Pull Request #5686 · bitcoin/bitcoin · GitHub
< achow101>
possibly
< achow101>
or something else entirely. that's for us to discuss/bikeshed I guess
< sipa>
sqlite has been suggested as well before
< achow101>
that was one option I was looking at
< achow101>
another was lmdb
< sipa>
lmdb is very performant, but has an architecture-dependent format
< sipa>
also, all of this is overkill - we don't need a database
< sipa>
just a list of key value pairs that are loaded at startup and occasionally overwritten
< achow101>
yeah but I don't want to write/review something from scratch
< sipa>
so it's not worth making many convenience compromises for
< sipa>
that's very reasonable
< achow101>
one thing that I would like to do is to have each ScriptPubKeyMan manage its own storage. and that would be achievable through giving each a table or subdatabase (that's a think in bdb and lmdb)
< sipa>
why?
< sipa>
as in separate files?
< achow101>
no in the same file
< sipa>
what does that mean/accomplish?
< achow101>
but the specifics of how stuff is stored and what is stored is self contained within a ScriptPubKeyMan's module
< achow101>
i guess it's bringing the encapsulation of the code to the storage layer as well
< sipa>
that could be done by just giving each a unique prefix for its keys
< achow101>
i guess...
< sipa>
i meam, if the db system used exposes a feature to do this more natively, sure use it
< sipa>
but i don't think it's worth picking one db system over another for, or at least not if it comes with other compromises
< sipa>
compatibility of wallet files i think would be a dealbreaker for lmdb
< achow101>
right
< achow101>
i've been primarily looking for things that are self contained single files, platform independent, and not likely to have changing formtas
< sipa>
sqlite seems to match that
< achow101>
lmdb has some build options that make it independent of bitness, but not endianness
< sipa>
or text files ;)
< achow101>
apparently monero uses lmdb for something and the guy that wrote/writes/maintains lmdb contributes there
< sipa>
yes
< sipa>
but lmdb is very focused on performance
< sipa>
for the wallet we couldn't care less about that
< achow101>
well I'd like to care about performace :)
< sipa>
why?
< achow101>
there was that one time importmulti would take so long writing everthing to disk that the RPC would timeout before it finished
< sipa>
that was just due to silliness of flushing in between any two records writte
< sipa>
no.
< sipa>
?
< sipa>
if writing a few kb takes more than milliseconds it's not a fault of the db system
< achow101>
it was. but a performant db system means I can do dumb things like that and not have a noticeable impact on the user experince :p
< sipa>
no
< sipa>
this is literally not something any db system could make reasnable
< achow101>
I was really only looking at lmdb since it seemed to be almost a drop in replacement for bdb
< achow101>
yeah, i know. i'm just trolling you
< sipa>
okay :D
< achow101>
i'm not sure that a key-value store is really what we want though. what we have now just feels like jamming separate values to fit into a key-value store. we have a bunch of records that are multiple values that could fit better into table
< achow101>
although I would agree with a relation db system is way overkill
< sipa>
well, you're right that it's not quite exactly a key-value store
< sipa>
but it is how it works now
< sipa>
changing the organization of the data as well as the backend storage is a way bigger change
< achow101>
that was part of the plan
< sipa>
even things like google's bigtable is effectively leveldb but with keys that are sort of a hierarchy of fields
< sipa>
ugh :)
< sipa>
seems unnecesary
< achow101>
I would like to do some things like consolidated address book data and changing unenecrypted privkey storage format that would make it diverge from what we currently have
< sipa>
you can do that without changing the organization or the storage format
< achow101>
address book stuff would be different records entirely so that's changing the organization
< sipa>
i mean: it doesn't require moving away from a key-value store
< achow101>
nothing strictly requires one or the other so it's just a function of how much work you want to do
< jonasschnelli>
achow101: what would speak against a simple append only file-format? Keep all the wallets in memory for fast performance. And if memory gets a concern, load the transactions when required.
< jonasschnelli>
I'd say even for large wallets, that should be reasonable fast.
< achow101>
jonasschnelli: i don't think we should do append only. we currently do things that would not work well with append only and it seems reasonable that we would do things similarly in the future
< achow101>
for example UpgradeKeyMetadata modifies CKeyMetadata and doing this in an append only format would not be ideal
< jonasschnelli>
If one wants to increase the performance of tx loading / decomposing, one could implement kinda subtotals... which would reduce the balance algorithms to not always scan the whole tx database
< jonasschnelli>
achow101: just write the new records as append only,... which would internally overwrite the old recods...
< jonasschnelli>
just disk size and loading time would increase
< jonasschnelli>
until one performs a compact-save (write 100% again)
< achow101>
well yeah, that's the problem
< jonasschnelli>
what? the compact write?
< achow101>
the duplicate records which impacts disk size and loading times
< jonasschnelli>
or the disk size?
< jonasschnelli>
who cares about disk size if you have 300GB+ blockchain data? :-)
< jonasschnelli>
and if you care, do a compact-save
< jonasschnelli>
(optimizewallet call or so)
< jonasschnelli>
and you could write special "overwrite" records for things that happen often and use little space.
< jonasschnelli>
Not often. but take abandontransaction as example.
< jonasschnelli>
just write a special record type that abandons a tx id rather than stroing the whole tx stucture again.
< jonasschnelli>
(would only be required if you care about disk size though)
< achow101>
a compact save seems like a possibility to lose data
< achow101>
and I don't really want the user to have to manually initiate one, doesn't feel like good ux
< jonasschnelli>
achow101: not if done right... compact write to a second file, validate integrity, atomic swap
< jonasschnelli>
achow101: you could calculate the % overhead on each load,.. if to large, compact save.
< jonasschnelli>
is matters of milliseconds.
< jonasschnelli>
integrity can be guaranteed if done right
< jonasschnelli>
no external dependencies....
< jonasschnelli>
sha256 hash per record to have a database "made" for secrets
< sipa>
yeah, that's what logdb did
< jonasschnelli>
I would avoid getting caught in sqlite3 or lmdb pitfalls
< jonasschnelli>
sipa: exactly
< jonasschnelli>
the only question for me is, if it is acceptable to have everything in memory and if not, how hard it would be to have a disk based tx decomposer
< sipa>
i'd be ok with sqlite
< jonasschnelli>
yeah,... for the tx history,... but just for the keys, scripts, it seems to be an overkill
< sipa>
it's absolutely overkill
< achow101>
jonasschnelli: it's slightly difficult for right now to not have all the txs in memory
< achow101>
bunch of gui stuff relies on that fact
< achow101>
i have a branch that attempted to load txs on demand and it was kinda awful
< sipa>
but it's also a well-tested existing standalone library that does what we need - and a lot we don't need
< jonasschnelli>
achow101: indeed. Optimizing performance would require some sub-total (balance-up-to-this-tx) cache
< sipa>
i would avoid lmdb, it seems designed for very different goals
< jonasschnelli>
Yes. Makes little sense for a wallet IMO
< achow101>
IIRC Armory is using/was going to use lmdb for the wallet. dunno how far that got
< achow101>
but yeah, after looking at lmdb, it doesn't seem like a good fit, particularly the arch dependent format
< achow101>
bikeshedding over what format to use can come later though. I first want to know if trying to do this now will even get anywhere
< jonasschnelli>
first it would be good to layer out bdb...
< achow101>
one of my concerns is whether we should even try to do this for descriptor wallets now
< jonasschnelli>
the code needs to be ready to support both formats
< achow101>
or if it's too late
< jonasschnelli>
since you dont want to abandon old wallets
< sipa>
achow101: you mean *only* for descriptor wallets?
< achow101>
jonasschnelli: my idea was to make it a descriptor wallets only thing
< jonasschnelli>
yeah. that would be an ideal time
< sipa>
i think that's crazy
< achow101>
the reasoning being that since descriptor wallets is a wholly new thing, might be a good time to also bring in a new disk format
< sipa>
if will complicate maintaining support for legacy wallets, by duplicating all storage level stuff work now for bdb and whatever is chosen
< sipa>
imo it's something to do completely orthogonally
< jonasschnelli>
sipa: but you have to support both storage backends anyways. right?
< achow101>
sipa: how so?
< achow101>
the thought was that legacy wallets would only use bdb, and descriptor wallets only the new thing
< achow101>
then whenver we get around to killing legacy wallets, bdb goes with it
< sipa>
achow101: yeah, i think that's crazy
< sipa>
you'll need two walletdb
< jonasschnelli>
If an storage upgrade is required for legacy wallets (which would be a no-go IMO), it could go over bitcoin-wallet (where we still need both storage backend in the code)
< sipa>
you'll need two walletdb.cpp's
< jonasschnelli>
yes.
< sipa>
and you'll duplicate work for all improvements that aren't specific to descriptor wallets
< jonasschnelli>
I once tried to make the storage backend flexible, changable,... not very easy
< achow101>
but you need that already
< sipa>
and it seems completely unnecessary
< sipa>
as long as what is exposed is a key-value store, the choice of storage format can be independent of what the keys in it meam
< sipa>
and sure, descriptor walleta would by default by the new thing
< jonasschnelli>
yes. it needs a new layers.
< sipa>
and.old wallets would generally be the old thing
< achow101>
sipa: I think you could still get it to be shared even with different db backends
< sipa>
achow101: yes, if the ly're orthogonal
< achow101>
i get that the separation doesn't make sense technically. but i'm also trying to make it a logical separation to
< sipa>
i think you shouldn't try to make this a descriptor-wallet-only thing
< sipa>
that risks huge compatibility problems if it doesn't get in by 0.21
< achow101>
and that's the other problem
< sipa>
and there's no need, i think - you'll want to abstract out the storage layer, amlnd if that's done cleanly, there should be no reason why you couldn't supoort a legacy new-format wallet or a descriptor bdb wallet
< sipa>
(perhaps you wouldn't want to support creating such things through RPC or whatever - but i think internally you want things separated that way)
< achow101>
well yeah, that was kind of the idea. that in theory you could write a legacy wallet to the new storage, but we just wouldn't allow it
< sipa>
right, but the decision on what to allow can come late
< sipa>
if things are in by 0.21 you could indeed say descriptor wallets only in the new format
< achow101>
i suppose so
< sipa>
but i think you'll want support for legacy new-format wallets in any caze too
< sipa>
eventually someone may want to run a convert tool to convert a bdb wallet to a new format wallet
< sipa>
if bdb gets dropped
< sipa>
but that process can't convert a legacy wallet into a descriptor wallet
< achow101>
right
< jonasschnelli>
I think the bdb support can't be drop in near and longterm future.
< jonasschnelli>
If an "tool" (like bitcoin-wallet) would allow the migration from bdb-storage to new-storage, we still would require the bdb support
< achow101>
so bdb's file format isn't that hard to parse so it wouldn't be all that hard to move to new storage
< jonasschnelli>
an it would be foolish to tell people to use a third party tool for that
< jonasschnelli>
achow101: you mean a simple record by record migration... yeah. maybe. Still would required the bdb dependency... and some parts of our bdb db.cpp code
< jonasschnelli>
which makes me think supporting both storage backends in the long term future is easier and unavoidable
< achow101>
jonasschnelli: #18836 has a pure python deserializer for bdb files. That with python3's sqlite lib should make a python script that can do a record for record conversion in <200 LOC
< achow101>
(the python deserializer is to inspect wallet files to make sure that records are being updated by upgradedwallet)
< sipa>
achow101: wow.
< jonasschnelli>
achow101: I think only python scripts is not an adequate option to a must-migrate approach
< sipa>
i'm surprised and impressed that it's so simple
< jonasschnelli>
Think of the novice gui only windows users
< achow101>
jonasschnelli: right. my point is that it wouldn't be that hard to do add a function within bitcoind itself and it doesn't require bdb
< jonasschnelli>
also,.. I think it would be non-ideal to tell people to use third party libraries (python stack) to migrate their most crucial database
< jonasschnelli>
achow101: ah. I see. Yes. Thats a good point.
< jonasschnelli>
Just; we where really careful to not force walletupgrade for things like HD, etc. Then we suddenly force people to upgrade due to a storage engine change?
< jonasschnelli>
I'd rather support both storage backends long term
< achow101>
same. I think supporting both would be required for quite a while. then the super long term solution is an upgrade with a minimal tool/function
< jonasschnelli>
achow101: yes.
< sipa>
that's starting to look much more reasonable now
< sipa>
given how simple bdb is to read
< sipa>
not short or medium term
< sipa>
but perhaps long term
< achow101>
sipa: I was surprised too. Unfortunately none of this was documented anywhere and it required rooting around in the source code for a while. at least all the things were in structs in one file in their source tarball
< jonasschnelli>
At one point, bdb depdency could be optional / disabled by default, leaving only the internal conversion tool.
< jonasschnelli>
later, the bdb dependency will be removed and users are forced to upgrade (long long term)
< achow101>
sipa: part of the simplicity is also that we don't use advanced feature of bdb so the page layout is also very predictable
< sipa>
right
< sipa>
sqlite3 is very well defined apparently; all releases back 3.0 can read/write those filss
< sipa>
ah, not perfectly forward compatible :(
< sipa>
i also like that it's just a single file
< jonasschnelli>
sqlite3 would have some nice benefits for large (many tx) wallets allowing some nice queries
< jonasschnelli>
Though I wonder how much of a concern it will be in 10 years and if we would not end up at the same point as we are now with bdb
< * midnight>
would cheer if bdb went away
< achow101>
i think that bdb isn't that bad of a choice for a wallet file format. my main gripe is that we're using a version that's now more than 10 years old. and also oracle messing with the licensing
< sipa>
achow101: my... experience is different
< achow101>
i'm sure it is :) I know there were a lot of consistency and corruption issues before, but I haven't seen a lot of those recently
< jonasschnelli>
achow101: Yes. And how big would be the risk of repeating that when using another library like sqlite?
< sipa>
my impression is that bdb was origonally designed for large multi-process databases, where there was a dedicated and competent database mantainer who would manually perform tweaking, and upgrades
< sipa>
rather than as an application storage format
< jonasschnelli>
*just send me your wallets, i'm a competent database maintainer.*
< bitcoin-git>
[bitcoin] fanquake opened pull request #18882: build: fix -Wformat-security check when compiling with GCC (master...fix_gcc_wformat_security) https://github.com/bitcoin/bitcoin/pull/18882
< achow101>
jonasschnelli: with sqlite itself, I would expect there to be few to no compatibility or licensing issues. For consistency and corruption, i'd trust it more than a homegrown format tbh
< jonasschnelli>
Maybe. Yes.
< sipa>
i believe sqlite is very well tested
< jonasschnelli>
Also,.. it would allow better handling of larger wallets
< sipa>
how so?
< jonasschnelli>
(without more homegrown code)
< jonasschnelli>
A decent option to query stuff if we use not just k/v storage
< jonasschnelli>
right now,.. all txns are in memory... assume you just want to show the first page of txns (the most recent 10)
< sipa>
i think that's just asking for problems again
< sipa>
if we want a chance of.doing this, it can't be tied to any other changes
< sipa>
and should be just a straight change from one storage format to another (i.e. remain a k/v store, with the same keys and the same meaning)
< sipa>
maybe later (much later...) you could start using other features, but i'm unconvinced that's even worth it
< bitcoin-git>
[bitcoin] robot-visions opened pull request #18890: test: disconnect_nodes should warn if nodes were already disconnected (master...disconnect-nodes) https://github.com/bitcoin/bitcoin/pull/18890
< luke-jr>
anyone ever seen this before? /usr/include/db4.8/db_cxx.h:59:10: fatal error: iostream.h: No such file or directory
< jonatack>
luke-jr: i may have seen that once or twice...
< luke-jr>
hmm, looks like bdb got installed without C++ support, yet still installs the header :x
< jonatack>
testing #18870 rn with bdb 5.3
< gribble>
https://github.com/bitcoin/bitcoin/issues/18870 | build: Allow BDB between 4.7 and 5.3 without --with-incompatible-bdb by achow101 · Pull Request #18870 · bitcoin/bitcoin · GitHub
< bitcoin-git>
[bitcoin] promag opened pull request #18894: gui: Fix manual coin control with multiple wallets loaded (master...2019-11-fix-15725.2) https://github.com/bitcoin/bitcoin/pull/18894
< bitcoin-git>
[bitcoin] promag closed pull request #17457: gui: Fix manual coin control with multiple wallets loaded (master...2019-11-fix-15725) https://github.com/bitcoin/bitcoin/pull/17457