< bitcoin-git>
[bitcoin] practicalswift opened pull request #18912: ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors (master...fuzzers-under-valgrind) https://github.com/bitcoin/bitcoin/pull/18912
< jonasschnelli>
jonatack really strange the fuzz crash you get with the v2 deserializer
< jonasschnelli>
GetMessage() sets m_raw_message_size to header_size + msg.m_message_size,... which a few lines later gets tested through assert(msg.m_raw_message_size == header_size + msg.m_message_size); (which fails)
< jb55>
smells like memory corruption
< jonasschnelli>
jonatack: found the issue revealed by the fuzzer
< dfmb_>
Hi im a noob, just wondering who decides which updates to deploy on bitcoin code? Letś say I make a pull request on the bitcoin github who decides that udpate is valid or it isn't ?
< belcher>
focusing on the github is a bit of a misdirection IMO, there have been situations in the past where the github code said one thing but users in practice ran some other code
< bitcoin-git>
[bitcoin] brakmic opened pull request #18917: fuzz: fix vector size problem in system fuzzer (master...fix-system-fuzzer) https://github.com/bitcoin/bitcoin/pull/18917
< meshcollider>
#startmeeting
< lightningbot>
Meeting started Fri May 8 19:00:27 2020 UTC. The chair is meshcollider. Information about MeetBot at http://wiki.debian.org/MeetBot.
< achow101>
(and probably 5 or so old issues and PRs)
< provoostenator>
Is there any reason why descriptor wallets are special when it comes to introducing new storage?
< achow101>
I'd like to move us off of BDB to something else. What that something else is, we can
< achow101>
*we can discuss later
< achow101>
I wanted to do this specifically for descriptor wallets because descriptor wallets are a wholly new thing so it made logical sense to me to introduce another wholly new thing with the descriptor wallets
< achow101>
since it's already backwards incompatible anyways
< meshcollider>
provoostenator: that's what the bulk of the discussion with sipa a few days ago was, he believes it's entirely orthogonal
< provoostenator>
True, assuming it's something we can get merge-ready before 0.21
< achow101>
then we can ditch bdb whenever we ditch legacy wallets
< jonatack>
iirc the conclusion of that discussion was that descriptor wallets and the DB ought to be orthogonal concerns?
< achow101>
jonatack: yes
< meshcollider>
Exactly, it has to be 0.21 if it's going to be a descriptor wallet thing
< achow101>
I think the conclusion of that discussion was implement it agnostic of the wallet type since it is orthogonal
< provoostenator>
They are orthogonal, but it's nice to have if we can do these things at the same time.
< achow101>
then later we can restrict it to descriptor wallets if we get it in fast enough
< luke-jr>
a long time ago, we wanted to move to an append-only wallet file format
< sipa>
i think it should be developed as orthogonal - the way it's exposed to users can be to require sqlite wallets for descriptor wallets if it's ready for 0.21, and otherwise it should remain orthogonal
< sipa>
luke-jr: i'm aware :)
< provoostenator>
sipa: that makes sense
< meshcollider>
yep
< achow101>
luke-jr: I actually don't think append only makes a ton of sense for us anymore. we're constantly updating and rewriting records
< achow101>
e.g. CHDChain is updated for every single new key
< achow101>
(in legacy wallets)
< jnewbery>
'require sqlite wallets for descriptor wallets' seems like it could be a really nice thing. (I haven't read the previous discussion)
< sipa>
but it's kind of reinventing the wheel - sqlite is overkill, but it's (in my understanding) very well tested and does what we'd need; probably with more assurances than what a self-developed solution can provide at the levels of effort we'd be willing to put into it
< luke-jr>
achow101: does it need to be?
< sipa>
achow101: that's not really relevant; an append-only format would still compact whenever too many records are rewritten
< luke-jr>
achow101: abstractly, we don't need to be rewriting often
< luke-jr>
do we know how good sqlite's compatibility guarantees are? or are we just going to end up int he same place soon?
< luke-jr>
can an old sqlite open a new sqlite db?
< achow101>
luke-jr: i think so? regardless, I'd also like to not touch application level stuff, just the blob storage to keep things simple
< provoostenator>
It can be run in javascript :-)
< sipa>
luke-jr: yes
< sipa>
achow101: we're still rewriting very infrequently compared to everything stored in the file, so i think the approach of an append-only format that gets occasionally compacted makes perfect sense
< achow101>
luke-jr: sqlite promises that compatibility will be maintained to at least 2050 or something like that. and everything is public domain
< gwillen>
I think sqlite is approximately the most-tested piece of software in existence
< gwillen>
outside of extremely specialized things like the space shuttle
< luke-jr>
achow101: everything is open source in bdb4/5 too
< achow101>
luke-jr: public domain meaning no license attached
< luke-jr>
achow101: that's a bad thing, not a good one :P
< achow101>
how so? it means that we won't have them move to agpl like bdb did
< luke-jr>
achow101: they can move to AGPL just as easily as BDB did. But it's bad because PD isn't legal in some places
< sipa>
achow101: it's just whether we're willing to put effort into building and maintaining a new format just to avoid some of the complexity of sqlite
< sipa>
(which i think isn't worth it today)
< achow101>
From their website: "The SQLite file format is stable, cross-platform, and backwards compatible and the developers pledge to keep it that way through at least the year 2050." and "SQLite source code is in the public-domain and is free to everyone to use for any purpose."
< luke-jr>
tbh maintaining bdb is probably easier than maintaining sqlite
< gwillen>
I have never heard a credible lawyer claim that PD is "not legal"
< provoostenator>
luke-jr: is it illegal in places where Bitcoin is legal?
< gwillen>
in any jurisdiction
< gwillen>
I have heard some people who are not lawyers claim that
< luke-jr>
achow101: it's forward-compatible that is the concern
< luke-jr>
provoostenator: no idea
< gwillen>
my impression is that they are mostly confused
< provoostenator>
How far do we need to be forward-compatible? The purpose of that is to make it easier to downgrade from a bad soft fork or bug, right?
< provoostenator>
Not to downgrade all the way to 0.1
< sipa>
if existing use is any evidence, there are tons of applications using sqlite for application-level storage like we do - but those who are using bdb seems to be mostly old software that's moving away from it (slapd to lmdb, subversion to fsfs, ...)
< luke-jr>
sipa: but how many that support downgrading?
< achow101>
provoostenator: at the very least, within the versions that are still maintained
< sipa>
luke-jr: bdb is terrible at forward compatibility, i don't understand
< luke-jr>
sipa: yes, I'm just not sure sqlite improves on this
< gwillen>
btw here is the paper that tested a bunch of database engines for whether they misuse filesystem APIs in ways that make them vulnerable to data loss
< provoostenator>
Right, so that means we have to have a careful upgrade policy for the sqlite dependency, that's documented somewhere
< luke-jr>
and while bdb might be maintainable, sqlite is a lot more complex
< ryanofsky>
should be aware most of the work involved in this is just abstracting the existing code. if it's just a key value store and you want to swap out sqlite for lukedb that should be pretty easy
< gwillen>
sqlite in write-ahead-logging mode is literally the only one that passed
< luke-jr>
and AFAIK we don't gain anything from sqlite's complexity
< sipa>
luke-jr: sqlite is a single cpp file if you want
< sipa>
it couldn't be more simple
< gwillen>
leveldb failed, they did not test bdb unfortunately
< luke-jr>
sipa: shoving everything into a single file doesn't impress me?
< achow101>
luke-jr: i'd like to make use of sqlite's complexity with actual tables and columns, just not initially
< sipa>
luke-jr: then use the library
< luke-jr>
sipa: not the point
< sipa>
then i don't understand your point
< sipa>
bdb is a maintenance nightmare in my impression
< jnewbery>
I think it's kinda weird that we claim forward-compatibility. We don't have any tests for it, and I can't imagine anyone cares, except for (maybe) downgrading one version.
< luke-jr>
my main point is we gain nothing by moving to sqlite afaik
< gwillen>
luke-jr: did you read the paper I linked
< meshcollider>
Yes the existing code needs a significant tidy-up to abstract before a new db can be introduced, that's something achow101 messaged me about yesterday, it's going to be quite a difficult job
< sipa>
sqlite explicitly has compatibility as a design goal
< luke-jr>
gwillen: no; in practice, we have bdb working today
< gwillen>
sqlite is the only database engine that actually passes tests of correctness for safely storing data on disk
< sipa>
for bdb it seems like an afterthought that's effectively only achieved by it being abandoned
< gwillen>
this is because sqlite is the most tested piece of software in existence
< gwillen>
(admitting, again, that bdb was not included in the list tested, but I would be shocked if it managed to pass)
< provoostenator>
That test loads master branch wallets all the back to 0.17
< provoostenator>
(as in, it opens them using the 0.17 binary)
< achow101>
luke-jr: we currently have a ton of hacks to make bdb consistent and not corrupt itself. I think sqlite gives us better guarantees of that. additionally it doesn't have persistent log files like bdb, everything is mostly self contained within a single file with the exception being active writes
< sipa>
bdb also has a ton of complexity we don't use, though of a different nature than sqlite; it is designed for multi-process applications accessing the same database simultaneously, with locking, synchronization, ... overhead
< provoostenator>
Doesn't BlueMatt_ have a Rust database?
< sipa>
achow101: sqlite has a write log
< luke-jr>
sipa: so does sqlite..>?
< jnewbery>
ok, but I still claim my second statement is true. Literally nobody wants to run a v0.20 wallet on v0.17 software.
< achow101>
sipa: it cleans up the journal when everything is flushed
< sipa>
luke-jr: no, sqlite is single-process
< luke-jr>
jnewbery: I might.
< sipa>
achow101: of course
< luke-jr>
jnewbery: I'm still using 0.13 for my real wallet
< luke-jr>
sipa: but you can use sqlite from multiple programs on the same db IIRC
< jnewbery>
then still use it. But the expectation that you can take that wallet, load it in master, do something with it and then reload it in 0.13 is unrealistic
< sipa>
luke-jr: not simultaneously
< achow101>
sipa: there's an option for that. but it's not relevant to us
< luke-jr>
jnewbery: it works today, so clearly not unrealistic..
< sipa>
ah
< sipa>
maybe i'm wrong
< meshcollider>
If someone is still using 0.13, they probably *won't* ever edit it in master, that's probably the point
< provoostenator>
We also have wallet-tool now, so we could make it easier to dump wallets into a more universal text based format
< achow101>
luke-jr: I think you might be an edge case
< meshcollider>
A few versions of forward compatibility seems fine
< provoostenator>
And then you can use older versions to import from that text format
< sipa>
as long as achow101 doesn't go through with his start-using-table-features, a trivial dump+load with command-line tools would always be possible to convert between bdb and sqlite even
< gwillen>
I am surprised to learn that forward-compatibility exists, isn't there a minimum version field in the wallet that gets rolled forward sometimes?
< gwillen>
does that only happen if you actively use a new feature?
< achow101>
gwillen: only with explicity upgrades
< luke-jr>
gwillen: only when the user requests it
< sipa>
gwillen: only when you explicitly use -upgradewallet
< sipa>
or the RPC now
< jnewbery>
luke-jr: is your opposition to this because it makes it more difficult to maintain bitcoin knots if the dependencies in bitcoin core change?
< luke-jr>
jnewbery: no
< provoostenator>
It's just something we have to take into account early on, making sure dumpwallet can dump descriptor wallets.
< luke-jr>
jnewbery: I just don't see the benefit, and it seems quite possible we end up in a worse situation
< sipa>
you don't think there is a downside to being effectively locked with abandoned software?
< luke-jr>
no? that's just stability
< provoostenator>
I'm not familiar enough with the code differences between sqlite and bdb, but it seems like sqlite is alive.
< provoostenator>
And we can't jump to BDB 18
< jonatack>
it sounds like there is rough consensus to look into eventually moving to sqlite
< luke-jr>
provoostenator: alive-ness seems like a negative here
< sipa>
luke-jr: ...
< meshcollider>
We would have to keep the bdb support for the long term anyway
< luke-jr>
for a wallet store, stability is needed, not constant change
< achow101>
at the very least, I think it would be good to move forward with refactoring and cleaning up the existing db storage code
< sipa>
format stability yes, which sqlite has
< meshcollider>
^
< provoostenator>
Non-deadness is good, too much change is bad.
< achow101>
in such a way that adding *some other* db in the end can be done more easily
< achow101>
whether that's sqlite, logdb, or something else
< ryanofsky>
what is dead may never die
< luke-jr>
sipa: backwards-compatible is not forwards-compatible
< luke-jr>
achow101: certainly
< meshcollider>
I don't think anyone can complain about just a code cleanup in advance of a db introduction
< jonatack>
^
< provoostenator>
Forwards compatilibyt isn't a big deal if we have a canonical dump format.
< sipa>
provoostenator: we don't have a canonical dump format, but we don't need one
< sipa>
csv works great
< sipa>
it's a key-value store ffs
< achow101>
luke-jr: sqlite has a table of format changes: https://sqlite.org/formatchng.html and it looks like the latest format hasn't changed
< provoostenator>
sipa: csv is fine by me
< sipa>
sqlite3 is btw what everything uses
< luke-jr>
achow101: ok, so that table seems to prove my point
< luke-jr>
what if we switch to sqlite3, and sqlite4 completely breaks compatibility?
< gwillen>
luke-jr: your point is that sqlite has not changed its format in 16 years, since before bitcoin was invented?
< luke-jr>
"The new file format is very different and is completely incompatible with the version 2 file format."
< gwillen>
and has a direct statement that it will never do so again?
< gwillen>
that point?
< sipa>
sqlite3 was released in 2004
< provoostenator>
"In other words, since 2004 all SQLite releases have been backwards compatible, though not necessarily forwards compatible."
< achow101>
luke-jr: they had a sqlite4. it was killed
< provoostenator>
Which means we need to be able to dump (into a CSV )to compensate for lack of forwards compatibility.
< meshcollider>
If we just use basic features, like we do in BDB, there's even less of a chance of breaking forwards compatibility
< provoostenator>
But upgrading should be fine.
< luke-jr>
" Since 2004, there have been enhancements to SQLite such that newer database files are unreadable by older versions of the SQLite library." ☹
< sipa>
provoostenator: that's more relevant, but i believe it's only when you use database features that were introduced later
< gwillen>
I believe when you create an sqlite database file you can specify the file version
< sipa>
luke-jr: and the same is true for bdb 4.8, bdb 6, ...
< sipa>
luke-jr: what is your point?
< achow101>
provoostenator: I believe they have a minversion type thing like we do, but it's for new features that you have to explicitly use
< gwillen>
so if you don't desire to use features that were added after 2004, you just set your file version to 2004
< luke-jr>
sipa: my point is we don't want to get stuck with sqlite3.31
< sipa>
if you think sticking to bdb 4.8 is fine, then sticking to sqlite 3.whatever it is now is also fine
< gwillen>
then you can read or write it with any versiono of sqlite that exists
< luke-jr>
due to sqlite's activity, there is a lot more pressure to drop old verisons
< sipa>
luke-jr: but being stuck with bdb 4.8 is fine?
< luke-jr>
yes, bdb is dead
< meshcollider>
Anyway, achow101 did mention at the start that we can decide on the choice of db later
< sipa>
if it being dead is fine, then you can treat sqlite as dead too
< sipa>
i really don't comprehend your argument
< luke-jr>
you can't, because it isn't
< luke-jr>
distros will continue updating it
< meshcollider>
Is there anything else you want to discuss achow101, about the code abstraction or anything?
< provoostenator>
The idea was to include the source, right?
< luke-jr>
definitely NACK
< sipa>
provoostenator: i'm not sure that's a necessary
< provoostenator>
If we rely on distros then you can't pretend it's dead.
< achow101>
so the next point of possible contention is where to put salvagewallet because that needs to be moved out of loading code
< achow101>
I think i'll just dump it into wallettool for now
< jnewbery>
achow101: +1
< achow101>
and another thing to discuss: whether we still want to support the old multiwallet method where you renamed the wallet.dat file. so multiple wallets were in the same dbenv
< sipa>
that makes sense
< achow101>
I think we have to remove that support otherwise it will get in the way of the abstraction
< jnewbery>
achow101: is there a migration path? A way to move separate wallets into their own directories on load?
< luke-jr>
no need to rename, just -wallet=foo.dat
< ryanofsky>
jnewbery: it is possible to move them manually, wallet-tool could also be extended to help with this, but I think automatic migration would be the best
< achow101>
jnewbery: use your preferred file explorer and move some wallet file?
< ryanofsky>
or maybe not, automatic migration might cause problems if people are doing automated backups of existing files
< achow101>
I would not go with automatic migration
< achow101>
what i've done currently is to give a very verbose warning about what you have to do manually
< achow101>
s/warning/error
< ryanofsky>
i don't think it should actually be that big of a deal to keep supporting them either, but I can see why you don't want to do that
< jnewbery>
giving manual instructions for something the user will only ever need to do once seems reasonable
< ryanofsky>
+1
< provoostenator>
As long as the GUI offers a "wizard" for that?
< jnewbery>
presumably users that have configured multi-wallet are fairly advanced and can handle moving files into a directory
< provoostenator>
I would say RPC users should be able to use "mv"
< achow101>
I think any user still using that behavior had to got into multiwallet very early on where you had to set startup options. so presumably they know what they're doing
< ryanofsky>
jnewbery, it means they started bitcoin at some point with a -wallet=something command line argument, but maybe that is advanced enough
< jnewbery>
provoostenator: I don't think it's necessary, and the development/testing overhead to get right that maybe a few hundred(?) people will only use once seems disproportionate.
< meshcollider>
At least those who made them before the multiwallet GUI create dialog
< meshcollider>
Which is well after separate directories was introduced
< achow101>
i think it predates the createwallet rpc?
< ryanofsky>
yes, predates the rpcs and gui stuff iirc
< provoostenator>
Oh ok, if it doesn't affect single wallet users, and only effects multi-wallet users before the GUI supported that, it's fine.
< meshcollider>
Nice write-up btw achow101
< ryanofsky>
it affects single wallet users who used -wallet option prior to multiwallet support too, but maybe they are even less of a concerns
< luke-jr>
does moving just wallet.dat work, or is some magic needed with the database/ dir?
< luke-jr>
I guess backupwallet is always an option
< achow101>
luke-jr: as long as everything was shut down cleanly, just moving the wallet.dat will work
< ryanofsky>
you just need to create a directory with a "wallet.dat" file, assuming there are no old logs
< ryanofsky>
if there are logs in the directory, they are shared between all the wallet files in the directory, so there is some risk of data loss if you don't copy them too
< luke-jr>
[19:32:05] <sipa> provoostenator: we don't have a canonical dump format, but we don't need one <-- btw, there is the dumpwallet RPC
< luke-jr>
which is a mess IMO
< sipa>
dumpwallet does not achieve that at all
< ryanofsky>
just looked up, #1889 is the pr that introduce the -wallet option for wallets with custom names
< gribble>
https://github.com/bitcoin/bitcoin/issues/17681 | wallet: Keep inactive seeds after sethdseed and derive keys from them as needed by achow101 · Pull Request #17681 · bitcoin/bitcoin · GitHub
< provoostenator>
People are always welcome to play with #16549
< jnewbery>
I have to say that fighting against our CIs is really painful. It seems like that's where I've spent most of my time for this PR.
< jnewbery>
ok, it was a silent merge conflict with 16224. Thanks to jonatack for pointing me in the right direction
< sipa>
jnewbery: re compatibility...i think for backward compatibility we essentially want to be able to load any old wallet file, back to 0.1... but for very far back formats i think it's fine if that needs an explicit conversion step first (possibly using wallet-tool or something similar)
< sipa>
2 major releases is not much though...
< jnewbery>
sipa: what if that explicit convertion step is "load it on v0.xx first"?
< jnewbery>
I'd argue that if we don't test something we can't really claim that we support it.
< sipa>
then we should test it :)
< jnewbery>
Maybe, but it's probably not worth the developer time. I'm personally not interested in writing tests for v0.1 wallets.
< sipa>
fair, but there is a difference between supporting and not gratuitously breaking
< sipa>
i think it'd be unfortunate if someone needs to jump through hoops like running already-unsupported versions just to get access to a years-old wallet file
< jnewbery>
If it's to improve code quality and remove debt, it's not gratuitous
< jnewbery>
unfortunate but not the end of the world
< sipa>
ok, let me then be more clear: i think it's ok if someone with a wallet file from 2012 has to go through some hoops to load a wallet
< sipa>
i don't think that's ok for a wallet from 2017
< sipa>
these things can be very long lived
< sipa>
and sometimes forgotten about
< jnewbery>
that also seems reasonable to me. 2 versions is maybe too aggresive.
< sipa>
for forward compatibility i think shorter timelines are acceptable; nobody expects being able to downgrade to years old software
< sipa>
but being able to roll back after an incompatibility introduced in one version vs the next is very desirable
< jnewbery>
yes, I think 1 or 2 is acceptable for forward compatibility
< sipa>
when using the snap package, is it expected that the binaries are called bitcoin-core.daemon bitcoin-core.cli ... instead of the usual ones?
< sipa>
fwiw, sqlite 3.6.10 (compiled with everything --disable-...) from 2009 can open a database i created with 3.31.1 (compiled with every --enable-...) fine
< sipa>
3.6.10 because it's the earliest available in the git clone i'm using
< sipa>
also, there is a db creation option that guarantees compatibility with 3.0.0
< sipa>
otherwise it seems forward compatibility exists down to 3.3.0, except when newer extensions are explicitly used
< luke-jr>
[21:56:55] <sipa> also, there is a db creation option that guarantees compatibility with 3.0.0 <-- nice
< luke-jr>
is there one that guarantees compatibility with the current stable version in major distros?
< luke-jr>
(eg, if Core were compiled against some future version we can't anticipate)
< sipa>
no, it seems there have not been any actual format changes since 3.3.0 in 2006
< sipa>
only extension
< luke-jr>
ok, so what we'd want is an option to force compat with 3.3
< luke-jr>
if someone builds against some future sqlite 3.90, we'd want to be sure it remains compatible with what we're using now