< sipa>
maybe in a few years, but we need to remain compatible with old wallet files
< yanmaani>
ah, and I guess there's no such thing as a import-only db4 hollowed out shell
< sipa>
even if there was, we couldn't use it
< sipa>
people downgrade after upgrading sometimes
< sipa>
it's of course hard to guarantee that being possible arbitrarily long, but 1 or 2 major versions at least makes sense (which matches our support window)
< yanmaani>
But will new wallets from here on out be made using sqlite?
< bitcoin-git>
[bitcoin] grubles opened pull request #19903: Update build-openbsd.md with GUI support (master...update-openbsd-build-instructions) https://github.com/bitcoin/bitcoin/pull/19903
< luke-jr>
I think I prefer bdb over sqlite
< yanmaani>
how come?
< yanmaani>
also, why isn't bdb4.8 in the tree, but leveldb is?
< sipa>
leveldb is consensus critical
< sipa>
though for release builds it doesn't really matter, gitian uses pinned versions of all dependencies
< luke-jr>
yanmaani: bdb is time-proven to work reliably and simpler I think
< luke-jr>
yanmaani: sqlite brings nothing much to the table IMO, and it's closed development
< * sipa>
could not disagree more
< sipa>
bdb reliable? simple?
< luke-jr>
sipa: when was the last time someone had a wallet lost due to bdb?
< luke-jr>
and simplicity is compared to sqlite: there isn't a whole SQL layer involved
< sipa>
luke-jr: me, very recently, after an unclean shutdown... thankfully one without any coins in it
< luke-jr>
sipa: !
< luke-jr>
sipa: 4.8?
< sipa>
5.3
< luke-jr>
hmm
< sipa>
yes, i agree something like sqlite without the sql layer would be even better
< sipa>
but sqlite is incredibly well tested
< sipa>
so that's a feature i'm happy to take along, we can just ignore it
< luke-jr>
can we?
< luke-jr>
at the very least we need to figure out safe escaping of stuff (though I suspect sqlite includes that)
< sipa>
no, prepared statements
< luke-jr>
anyway, time will show if sqlite works well
< sipa>
you pass in the data as arguments, rather than serialized as strings
< luke-jr>
sipa: I thought that just let sqlite turn them into strings?
< luke-jr>
but I haven't read sqlite code lately so maybe I'm outdated
< sipa>
luke-jr: i suspect no conversion to string and back is done, but i don't think it matters... regardless of what it does, it means we don't need to deal with the complexity of it
< yanmaani>
sqlite is extremely well tested
< yanmaani>
IIRC, it uses int representations internally for ints
< sipa>
well we'd pass everything as byte arrays
< yanmaani>
It's not so big, and I think you can use compile flags to gut the SQL engine by some fraction
< yanmaani>
If you're looking to switch database though, why specifically sqlite, and not say lmdb? Or leveldb?
< luke-jr>
or we can go the KDE route and use MySQL
< sipa>
luke-jr: aaaargh
< sipa>
:)
< yanmaani>
[screams internally]
< luke-jr>
wait, let's stick to sqlite and just use MySQL for the cache
< yanmaani>
[screams externally]
< luke-jr>
(KDE literally uses MySQL for caches)
< sipa>
yanmaani: 100s of KB to megabytes, typically
< yanmaani>
can't you just use both?
< yanmaani>
That would have pretty much all the desiderata
< sipa>
both what?
< luke-jr>
keys in bdb and metadata in sqlite?
< yanmaani>
databases
< yanmaani>
at once
< sipa>
both what?
< yanmaani>
* Reliable - SQLite is reliable
< sipa>
what are the two options
< yanmaani>
Both databases
< sipa>
sqlite and ...?
< yanmaani>
Both SQLite and BDB4.8 for wallet storage
< sipa>
w t f would you want that
< luke-jr>
lol
< yanmaani>
Lots of reasons
< sipa>
worst of all worlds
< yanmaani>
upgrades are seamless - old versions just disregard the sqlite files
< yanmaani>
downgrades are seamless too
< yanmaani>
the reliability is sqlite's, of course, since those would be used as master by newer versions
< yanmaani>
so you'd have full ACID
< sipa>
i'm not sure what you're suggesting
< yanmaani>
you get a shorter time to tear out db4
< sipa>
if what you mean is use bdb for old wallets and sqlite for new ones... that's what the PR does
< yanmaani>
You use both databases (SQLite3 and BDB4.8) at once. You do not have any internal distinction between "new" and "old" wallets.
< sipa>
...
< sipa>
wtf does that mean
< yanmaani>
Whenever you open a BDB-only wallet file, it automatically creates a corresponding sqlite wallet
< yanmaani>
When you create a new wallet, it creates a sqlite and a bdb wallet
< sipa>
good lord
< yanmaani>
No, this has lots of good properties!
< * luke-jr>
backs away slowly
< sipa>
go away, please
< yanmaani>
So, first off, you get the nice properties of SQLite.
< sipa>
...
< yanmaani>
You wouldn't have the data problems that bdb has. If BDB crashes, too bad, who cares.
< yanmaani>
But the old versions can still read the BDB version of the database.
< sipa>
whyyyyyyy
< luke-jr>
so you basically mean switch to sqlite, but keep a BDB copy updated in case of sudden urge to downgrade?
< yanmaani>
The code complexity is about the same.
< yanmaani>
luke-jr: Yes. Because then you can tear out db4 faster.
< sipa>
i wish you good luck
< luke-jr>
I don't think that follows
< yanmaani>
If you do this, then everyone switches to the new version, there will be a sqlite file.
< yanmaani>
Unless they migrate from v_nosqlite to v_onlysqlite directly
< luke-jr>
hmm
< sipa>
all the management issues get multiplied
< sipa>
what if files are out of sync?
< yanmaani>
as long as they migrate through one of the intermediate sqlite versions, there will be a sqlite wallet created
< sipa>
what if people migrated one, but not the other, from one system to another?
< yanmaani>
How would you migrate only one of them?
< yanmaani>
If there's only one file, use that
< sipa>
users are stupid
< sipa>
if every user was a system administrator, BDB would be an awesome choice
< yanmaani>
If there's two files, load both and take the one last modified
< sipa>
that's what it's designed for
< sipa>
but users aren't
< yanmaani>
If the users only copy one database file, there's no problem though
< sipa>
seriously, i can't comprehend how you think that makes anything better
< yanmaani>
It makes cross-version migration more seamless
< sipa>
all the issues with the mental overhead of understanding what a wallet is get worse
< yanmaani>
a wallet is the wallet_dat/ folder
< sipa>
that's specifially what we should get rid of
< sipa>
among other things
< yanmaani>
Yes, when db4 is retired you can get rid of it
< yanmaani>
and return to wallet.dat(sqlite)
< sipa>
yay, great
< yanmaani>
But with this approach, you'll never have the problem of "user installed sqlite version but didn't upgrade his wallet"
< sipa>
i don't see how making our database a frankenstein-monster-of-bdb-and-sqlite helps with that
< yanmaani>
because wallet upgrades will be done transparently in the background
< yanmaani>
because it's only temporary - once everyone has upgraded, you can rip out db4 entirely
< sipa>
no you can't
< sipa>
backups need to keep working
< sipa>
even years old ones
< yanmaani>
With "old wallet" and "new wallet", if you're to rip out db4, users will at some point in time have to convert them
< sipa>
yes
< yanmaani>
can't they just be asked to download an old version? But yes, that's a good point otherwise
< sipa>
but i don't see how doing what you suggests allows doing this any earlier
< yanmaani>
In my model, users would never have to upgrade formats manually
< yanmaani>
first they have db4-only wallets
< yanmaani>
then they have db4+sqlite wallets
< yanmaani>
then db4 is dropped, and they have <garbage>+sqlite wallets
< yanmaani>
the conversion would be done silently and transparently to the user
< sipa>
i really really don't see the advantage
< sipa>
even ignoring all the disadvantages
< yanmaani>
easier migration process, no need for users to make the effort of upgrading wallet from sqlite to db4
< sipa>
unless they missed the window during which versions with mixed support existed
< yanmaani>
yeah, but that's still much less user effort isn't it?
< sipa>
eh
< sipa>
it's something that i expect will happen in 5 years or so
< sipa>
maybe more
< sipa>
i'm more concerned about things working in the next release than what happens then
< yanmaani>
You might also just say "we're done with db4, if you load a db4 wallet you will be given the option to convert or exit, you can't just load it"
< yanmaani>
whenever you get fed up with it
< sipa>
and i really don't want to think about all the things that can go wrong with duplicated bdb+sqlite wallets
< yanmaani>
Will you be using sqlite's encryption layer too?
< meshcollider>
yanmaani: no, not using their encryption stuff
< achow101>
sipa, yanmaani: armory does something like that. they use some custom binary format that isn't super upgradeable. So to migrate to the new storage method, they make a copy of the wallet in the new storage
< achow101>
yanmaani: migrating a bdb wallet to sqlite could be done without bdb. The database structure is known, so it wouldn't be too hard to write something that just deserializes the database to fetch the records, then writes them to a new sqlite wallet
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #19905: Remove dead CheckForkWarningConditionsOnNewFork (master...2009-noFork) https://github.com/bitcoin/bitcoin/pull/19905
< bitcoin-git>
bitcoin/master 46d955d Jeremy Rubin: Remove mapLinks in favor of entry inlined structs with iterator type erasu...
< bitcoin-git>
bitcoin/master 296be8f Jeremy Rubin: Get rid of unused functions CTxMemPool::GetMemPoolChildren, CTxMemPool::Ge...
< bitcoin-git>
bitcoin/master 2583966 MarcoFalke: Merge #19478: Remove CTxMempool::mapLinks data structure member
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #19478: Remove CTxMempool::mapLinks data structure member (master...epoch-mempool-clean-split-3) https://github.com/bitcoin/bitcoin/pull/19478
< bitcoin-git>
[bitcoin] naumenkogs opened pull request #19906: Bugfix: don't make collision from "tried" a feeler (master...2020-09-feeler-no-collisions) https://github.com/bitcoin/bitcoin/pull/19906
< luke-jr>
achow101: btw, have you looked into if sqlite ever leaves stale/deleted data in db files?
< achow101>
luke-jr: it does, but there's an option that can be enabled to overwrite the data
< luke-jr>
cool
< achow101>
that operation can also be done as a one time thing. IIRC I have set it to do that after encrypting
< phantomcircuit>
achow101, you going to use wal or no?
< achow101>
phantomcircuit: no
< achow101>
wal is the cause of a bunch of bdb issues
< phantomcircuit>
yeah
< phantomcircuit>
it's why there's flush's everywhere and those do i think 3 fsync each call
< achow101>
yeah, the PR explicitly disables wal too
< phantomcircuit>
achow101, should set application_id as well
< achow101>
hmm
< achow101>
I suppose so
< phantomcircuit>
achow101, is there manual vacuum being done? i dont see it
< achow101>
It's in the Rewrite() function
< phantomcircuit>
also need to set checkpoint_fullfsync if you want real fsync on osx
< phantomcircuit>
wait no you want fullfsync
< achow101>
You should leave comments on the PR
< phantomcircuit>
achow101, btw i think sqlite in it's normal, non wal operation does create a separate journal file, but only for the time between the transaction commit and the fsync completing
< achow101>
phantomcircuit: yes, but it deletes it later
< achow101>
everything still ends up in the database file after the write
< phantomcircuit>
yeah immediately after the fsync completes
< phantomcircuit>
achow101, did you consider opening the file with an exclusive lock?
< achow101>
yes, but we already lock the .walletlock file in the directory so I don't think it's needed
< phantomcircuit>
im writting a comment on the pr with some of this stuff btw
< phantomcircuit>
achow101, also i notice that you implemented EraseKey which is called by nothing
< achow101>
eh? Isn't it called by EraseIC?
< phantomcircuit>
achow101, once again github search lies to me
< achow101>
lol
< phantomcircuit>
it feels like an elastic search cluster that's missing a server constantly
< phantomcircuit>
(i think it's literally that, but azure)
< bitcoin-git>
[bitcoin] jnewbery opened pull request #19910: net processing: Move peer_map to PeerManager (master...2020-08-peer-plv2) https://github.com/bitcoin/bitcoin/pull/19910
< bitcoin-git>
[bitcoin] narula opened pull request #19911: net: guard vRecvGetData with cs_vRecv and orphan_work_set with g_cs_orphans (master...cs_vRecv) https://github.com/bitcoin/bitcoin/pull/19911