19:00:27 #startmeeting 19:00:27 Meeting started Fri May 8 19:00:27 2020 UTC. The chair is meshcollider. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:00:27 Useful Commands: #action #agreed #help #info #idea #link #topic. 19:00:30 hi 19:00:32 hi 19:00:38 #bitcoin-core-dev Wallet Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo marcofalke paveljanik NicolasDorier jl2012 achow101 meshcollider jnewbery maaku fanquake promag provoostenator aj Chris_Stewart_5 dongcarl gwillen jamesob ken281221 ryanofsky gleb moneyball ariard digi_james amiti fjahr 19:00:39 jeremyrubin emilengler jonatack hebasto jb55 19:00:40 hi 19:01:42 The main topic we have to discuss this week is the wallet DB 19:02:04 #topic wallet DB (achow101) 19:02:24 There was a bit of discussion earlier in the week already 19:02:33 hi 19:02:35 hi 19:04:19 yes 19:04:25 http://www.erisian.com.au/bitcoin-core-dev/log-2020-05-05.html 19:04:53 http://www.erisian.com.au/bitcoin-core-dev/log-2020-05-05.html#l-100 19:05:12 Also #18916 19:05:14 https://github.com/bitcoin/bitcoin/issues/18916 | Sqlite wallet storage · Issue #18916 · bitcoin/bitcoin · GitHub 19:06:05 (and probably 5 or so old issues and PRs) 19:06:52 Is there any reason why descriptor wallets are special when it comes to introducing new storage? 19:06:55 I'd like to move us off of BDB to something else. What that something else is, we can 19:06:59 *we can discuss later 19:07:26 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 19:07:32 since it's already backwards incompatible anyways 19:08:09 provoostenator: that's what the bulk of the discussion with sipa a few days ago was, he believes it's entirely orthogonal 19:08:11 True, assuming it's something we can get merge-ready before 0.21 19:08:27 then we can ditch bdb whenever we ditch legacy wallets 19:08:36 iirc the conclusion of that discussion was that descriptor wallets and the DB ought to be orthogonal concerns? 19:08:41 jonatack: yes 19:08:42 Exactly, it has to be 0.21 if it's going to be a descriptor wallet thing 19:09:03 I think the conclusion of that discussion was implement it agnostic of the wallet type since it is orthogonal 19:09:11 They are orthogonal, but it's nice to have if we can do these things at the same time. 19:09:13 then later we can restrict it to descriptor wallets if we get it in fast enough 19:09:25 a long time ago, we wanted to move to an append-only wallet file format 19:09:37 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 19:09:50 luke-jr: i'm aware :) 19:09:56 sipa: that makes sense 19:10:52 yep 19:10:53 luke-jr: I actually don't think append only makes a ton of sense for us anymore. we're constantly updating and rewriting records 19:11:10 e.g. CHDChain is updated for every single new key 19:11:18 (in legacy wallets) 19:11:39 'require sqlite wallets for descriptor wallets' seems like it could be a really nice thing. (I haven't read the previous discussion) 19:11:43 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 19:11:51 achow101: does it need to be? 19:12:11 achow101: that's not really relevant; an append-only format would still compact whenever too many records are rewritten 19:12:24 achow101: abstractly, we don't need to be rewriting often 19:12:59 do we know how good sqlite's compatibility guarantees are? or are we just going to end up int he same place soon? 19:13:13 can an old sqlite open a new sqlite db? 19:13:15 luke-jr: i think so? regardless, I'd also like to not touch application level stuff, just the blob storage to keep things simple 19:13:16 It can be run in javascript :-) 19:13:18 luke-jr: yes 19:13:35 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 19:13:39 luke-jr: sqlite promises that compatibility will be maintained to at least 2050 or something like that. and everything is public domain 19:13:40 I think sqlite is approximately the most-tested piece of software in existence 19:13:51 outside of extremely specialized things like the space shuttle 19:14:00 achow101: everything is open source in bdb4/5 too 19:14:22 luke-jr: public domain meaning no license attached 19:14:34 achow101: that's a bad thing, not a good one :P 19:14:53 how so? it means that we won't have them move to agpl like bdb did 19:15:12 achow101: they can move to AGPL just as easily as BDB did. But it's bad because PD isn't legal in some places 19:15:25 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 19:15:36 (which i think isn't worth it today) 19:15:41 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." 19:15:43 tbh maintaining bdb is probably easier than maintaining sqlite 19:15:45 I have never heard a credible lawyer claim that PD is "not legal" 19:15:47 luke-jr: is it illegal in places where Bitcoin is legal? 19:15:50 in any jurisdiction 19:15:57 I have heard some people who are not lawyers claim that 19:15:58 achow101: it's forward-compatible that is the concern 19:16:03 provoostenator: no idea 19:16:04 my impression is that they are mostly confused 19:16:43 https://www.sqlite.org/copyright.html 19:16:44 read that 19:17:30 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? 19:17:36 Not to downgrade all the way to 0.1 19:19:40 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, ...) 19:20:10 sipa: but how many that support downgrading? 19:20:10 provoostenator: at the very least, within the versions that are still maintained 19:20:39 luke-jr: bdb is terrible at forward compatibility, i don't understand 19:20:53 sipa: yes, I'm just not sure sqlite improves on this 19:20:57 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 19:20:57 Right, so that means we have to have a careful upgrade policy for the sqlite dependency, that's documented somewhere 19:21:00 https://www.usenix.org/system/files/conference/osdi14/osdi14-paper-pillai.pdf 19:21:01 and while bdb might be maintainable, sqlite is a lot more complex 19:21:02 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 19:21:08 sqlite in write-ahead-logging mode is literally the only one that passed 19:21:09 and AFAIK we don't gain anything from sqlite's complexity 19:21:15 luke-jr: sqlite is a single cpp file if you want 19:21:25 it couldn't be more simple 19:21:31 leveldb failed, they did not test bdb unfortunately 19:21:35 sipa: shoving everything into a single file doesn't impress me? 19:21:48 luke-jr: i'd like to make use of sqlite's complexity with actual tables and columns, just not initially 19:21:49 luke-jr: then use the library 19:21:56 sipa: not the point 19:22:03 then i don't understand your point 19:22:12 bdb is a maintenance nightmare in my impression 19:22:28 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. 19:22:33 my main point is we gain nothing by moving to sqlite afaik 19:22:40 luke-jr: did you read the paper I linked 19:22:51 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 19:22:59 sqlite explicitly has compatibility as a design goal 19:23:09 gwillen: no; in practice, we have bdb working today 19:23:18 sqlite is the only database engine that actually passes tests of correctness for safely storing data on disk 19:23:21 for bdb it seems like an afterthought that's effectively only achieved by it being abandoned 19:23:33 this is because sqlite is the most tested piece of software in existence 19:23:39 jnewbery: we do have tests for it, as of recently: https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_backwards_compatibility.py#L221 19:23:48 (admitting, again, that bdb was not included in the list tested, but I would be shocked if it managed to pass) 19:24:15 That test loads master branch wallets all the back to 0.17 19:24:33 (as in, it opens them using the 0.17 binary) 19:24:57 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 19:25:02 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 19:25:15 Doesn't BlueMatt_ have a Rust database? 19:25:21 achow101: sqlite has a write log 19:25:25 sipa: so does sqlite..>? 19:25:32 ok, but I still claim my second statement is true. Literally nobody wants to run a v0.20 wallet on v0.17 software. 19:25:39 sipa: it cleans up the journal when everything is flushed 19:25:40 luke-jr: no, sqlite is single-process 19:25:44 jnewbery: I might. 19:25:47 achow101: of course 19:25:53 jnewbery: I'm still using 0.13 for my real wallet 19:26:23 sipa: but you can use sqlite from multiple programs on the same db IIRC 19:26:24 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 19:26:32 luke-jr: not simultaneously 19:26:48 sipa: there's an option for that. but it's not relevant to us 19:26:51 jnewbery: it works today, so clearly not unrealistic.. 19:26:55 ah 19:26:58 maybe i'm wrong 19:27:01 If someone is still using 0.13, they probably *won't* ever edit it in master, that's probably the point 19:27:13 We also have wallet-tool now, so we could make it easier to dump wallets into a more universal text based format 19:27:16 luke-jr: I think you might be an edge case 19:27:24 A few versions of forward compatibility seems fine 19:27:32 And then you can use older versions to import from that text format 19:27:46 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 19:27:58 I am surprised to learn that forward-compatibility exists, isn't there a minimum version field in the wallet that gets rolled forward sometimes? 19:28:05 does that only happen if you actively use a new feature? 19:28:08 gwillen: only with explicity upgrades 19:28:09 gwillen: only when the user requests it 19:28:11 gwillen: only when you explicitly use -upgradewallet 19:28:16 or the RPC now 19:28:20 luke-jr: is your opposition to this because it makes it more difficult to maintain bitcoin knots if the dependencies in bitcoin core change? 19:28:28 jnewbery: no 19:28:30 It's just something we have to take into account early on, making sure dumpwallet can dump descriptor wallets. 19:29:14 jnewbery: I just don't see the benefit, and it seems quite possible we end up in a worse situation 19:29:39 you don't think there is a downside to being effectively locked with abandoned software? 19:29:57 no? that's just stability 19:30:05 I'm not familiar enough with the code differences between sqlite and bdb, but it seems like sqlite is alive. 19:30:15 And we can't jump to BDB 18 19:30:18 it sounds like there is rough consensus to look into eventually moving to sqlite 19:30:25 provoostenator: alive-ness seems like a negative here 19:30:30 luke-jr: ... 19:30:33 We would have to keep the bdb support for the long term anyway 19:30:39 for a wallet store, stability is needed, not constant change 19:30:48 at the very least, I think it would be good to move forward with refactoring and cleaning up the existing db storage code 19:30:53 format stability yes, which sqlite has 19:30:53 ^ 19:30:54 Non-deadness is good, too much change is bad. 19:31:04 in such a way that adding *some other* db in the end can be done more easily 19:31:13 whether that's sqlite, logdb, or something else 19:31:13 what is dead may never die 19:31:15 sipa: backwards-compatible is not forwards-compatible 19:31:32 achow101: certainly 19:31:35 I don't think anyone can complain about just a code cleanup in advance of a db introduction 19:31:45 ^ 19:31:51 Forwards compatilibyt isn't a big deal if we have a canonical dump format. 19:32:05 provoostenator: we don't have a canonical dump format, but we don't need one 19:32:08 csv works great 19:32:15 it's a key-value store ffs 19:32:20 luke-jr: sqlite has a table of format changes: https://sqlite.org/formatchng.html and it looks like the latest format hasn't changed 19:32:35 sipa: csv is fine by me 19:32:59 sqlite3 is btw what everything uses 19:33:05 achow101: ok, so that table seems to prove my point 19:33:16 what if we switch to sqlite3, and sqlite4 completely breaks compatibility? 19:33:27 luke-jr: your point is that sqlite has not changed its format in 16 years, since before bitcoin was invented? 19:33:28 "The new file format is very different and is completely incompatible with the version 2 file format." 19:33:35 and has a direct statement that it will never do so again? 19:33:36 that point? 19:33:41 sqlite3 was released in 2004 19:33:51 "In other words, since 2004 all SQLite releases have been backwards compatible, though not necessarily forwards compatible." 19:33:56 luke-jr: they had a sqlite4. it was killed 19:34:12 Which means we need to be able to dump (into a CSV )to compensate for lack of forwards compatibility. 19:34:23 If we just use basic features, like we do in BDB, there's even less of a chance of breaking forwards compatibility 19:34:23 But upgrading should be fine. 19:34:28 " Since 2004, there have been enhancements to SQLite such that newer database files are unreadable by older versions of the SQLite library." ☹ 19:34:28 provoostenator: that's more relevant, but i believe it's only when you use database features that were introduced later 19:34:46 I believe when you create an sqlite database file you can specify the file version 19:34:48 luke-jr: and the same is true for bdb 4.8, bdb 6, ... 19:34:52 luke-jr: what is your point? 19:34:58 provoostenator: I believe they have a minversion type thing like we do, but it's for new features that you have to explicitly use 19:35:00 so if you don't desire to use features that were added after 2004, you just set your file version to 2004 19:35:15 sipa: my point is we don't want to get stuck with sqlite3.31 19:35:16 if you think sticking to bdb 4.8 is fine, then sticking to sqlite 3.whatever it is now is also fine 19:35:21 then you can read or write it with any versiono of sqlite that exists 19:35:29 due to sqlite's activity, there is a lot more pressure to drop old verisons 19:35:31 luke-jr: but being stuck with bdb 4.8 is fine? 19:35:37 yes, bdb is dead 19:35:46 Anyway, achow101 did mention at the start that we can decide on the choice of db later 19:35:53 if it being dead is fine, then you can treat sqlite as dead too 19:36:00 i really don't comprehend your argument 19:36:00 you can't, because it isn't 19:36:08 distros will continue updating it 19:36:08 Is there anything else you want to discuss achow101, about the code abstraction or anything? 19:36:18 The idea was to include the source, right? 19:36:26 definitely NACK 19:36:40 provoostenator: i'm not sure that's a necessary 19:36:42 if anyone wants to read about the mess that is our loading code and what I would like to change: https://gist.github.com/achow101/cda3ea1bb07f585e56caaf969e842188 19:37:07 If we rely on distros then you can't pretend it's dead. 19:37:14 so the next point of possible contention is where to put salvagewallet because that needs to be moved out of loading code 19:37:39 I think i'll just dump it into wallettool for now 19:38:45 achow101: +1 19:38:55 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 19:38:58 that makes sense 19:39:13 I think we have to remove that support otherwise it will get in the way of the abstraction 19:39:49 achow101: is there a migration path? A way to move separate wallets into their own directories on load? 19:40:24 no need to rename, just -wallet=foo.dat 19:41:10 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 19:41:12 jnewbery: use your preferred file explorer and move some wallet file? 19:41:42 or maybe not, automatic migration might cause problems if people are doing automated backups of existing files 19:41:56 I would not go with automatic migration 19:42:19 what i've done currently is to give a very verbose warning about what you have to do manually 19:42:26 s/warning/error 19:43:01 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 19:43:02 giving manual instructions for something the user will only ever need to do once seems reasonable 19:43:16 +1 19:43:30 As long as the GUI offers a "wizard" for that? 19:43:36 presumably users that have configured multi-wallet are fairly advanced and can handle moving files into a directory 19:44:08 I would say RPC users should be able to use "mv" 19:44:11 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 19:44:26 jnewbery, it means they started bitcoin at some point with a -wallet=something command line argument, but maybe that is advanced enough 19:44:27 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. 19:44:28 At least those who made them before the multiwallet GUI create dialog 19:44:42 Which is well after separate directories was introduced 19:44:58 i think it predates the createwallet rpc? 19:45:09 yes, predates the rpcs and gui stuff iirc 19:45:14 Oh ok, if it doesn't affect single wallet users, and only effects multi-wallet users before the GUI supported that, it's fine. 19:46:32 Nice write-up btw achow101 19:46:50 it affects single wallet users who used -wallet option prior to multiwallet support too, but maybe they are even less of a concerns 19:48:08 does moving just wallet.dat work, or is some magic needed with the database/ dir? 19:48:18 I guess backupwallet is always an option 19:48:28 luke-jr: as long as everything was shut down cleanly, just moving the wallet.dat will work 19:48:43 you just need to create a directory with a "wallet.dat" file, assuming there are no old logs 19:49:21 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 19:50:26 [19:32:05] provoostenator: we don't have a canonical dump format, but we don't need one <-- btw, there is the dumpwallet RPC 19:50:40 which is a mess IMO 19:50:48 dumpwallet does not achieve that at all 19:50:52 just looked up, #1889 is the pr that introduce the -wallet option for wallets with custom names 19:50:54 https://github.com/bitcoin/bitcoin/issues/1889 | let user select wallet file with -wallet=foo.dat by tcatm · Pull Request #1889 · bitcoin/bitcoin · GitHub 19:50:59 luke-jr: yes, but dumpwallet is dsiabled for desciptor wallets, and needs to be redone properly anyway 19:51:09 it dumps private keys, and a few other things - it does not dump all the wallet data that lets you recreate the same wallet again 19:51:10 But that should be doable 19:51:30 My opinion on forward/backward wallet compatibility is that we should support 2 versions in either direction, and make sure that we have 100% test coverage of all features. The fact that we have code like https://github.com/bitcoin/bitcoin/blob/5b24f6084ede92d0f493ff416b4726245140b2c1/src/wallet/walletdb.cpp#L284 which is supposed to support some wallet quirk in 2012 that isn't tested and 19:51:33 sipa: there's a paired loadwallet too; it just isn't lossless 19:51:36 probably wasn't even in an actual release is crazy. 19:51:37 The reverse of that new dumpwallet just needs to be smart enough to ignore stuff it doesn't know. 19:52:31 provoostenator: we already have importwallet to do the inverse of dumpwallet 19:52:32 luke-jr: i'm aware, i think i wrote it 19:52:36 I think both need changes 19:53:16 agree 19:53:19 jnewbery: 3.16.0 it says? 19:53:48 if we were sticking to bdb, we could just tell people to use db4.8dump or whatever XD 19:55:02 Ok last 5 mins, anything else anyone wanted to discuss? 19:55:07 achow101: that is a really helpful writeup 19:56:32 meshcollider: blockers/high-priority? 19:56:56 Any changes for high priority? Probably covered yesterday 19:57:19 i'd just like to review beg for both #16946 and #17681 19:57:21 https://github.com/bitcoin/bitcoin/issues/16946 | wallet: include a checksum of encrypted private keys by achow101 · Pull Request #16946 · bitcoin/bitcoin · GitHub 19:57:23 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 19:57:42 People are always welcome to play with #16549 19:57:44 https://github.com/bitcoin/bitcoin/issues/16549 | UI external signer support (e.g. hardware wallet) by Sjors · Pull Request #16549 · bitcoin/bitcoin · GitHub 19:57:59 (draft, not high priority) 19:58:42 Alright, thanks everyone for the discussion :) 19:58:43 achow101: we're going to cover 17681 in review club next wednesday (ryanofsky hosting). It'd be great if you could join us 19:58:47 #endmeeting