19:00:52 #startmeeting 19:00:52 Meeting started Thu Sep 27 19:00:52 2018 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:00:52 Useful Commands: #action #agreed #help #info #idea #link #topic. 19:01:02 hi 19:01:19 #bitcoin-core-dev Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr btcdrak sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircu 19:01:21 hi 19:01:22 it codeshark michagogo marcofalke paveljanik NicolasDorier jl2012 achow101 meshcollider jnewbery maaku fanquake promag provoostenator 19:01:26 hi 19:01:29 hi 19:02:01 Hi 19:02:24 Status of 0.15.2 and 0.14.3? 19:02:45 hi 19:03:27 jonasschnelli: good question; are there enough gitian sigs to upload binaries? 19:03:42 I think so... 5 or 6 19:03:47 yeah 19:04:15 hi 19:04:19 0.14.3 has 6, 0.15.2 has 5 19:04:19 6 for 0.14.3 and 5 for 0.15.2 AFAIK 19:04:34 ok, will do that then, I'm back from Riga so I'm able to sign/upload binaries again 19:04:36 due to my shitty setup, I can't compile trusty stuff on Gitian anymore 19:04:46 thanks wumpus 19:05:05 0.14/0.15 build still seems to work here 19:05:31 It took some pain for me as well, but I still keep an old Gitian VM for backports. 19:05:32 #topic 0.17.0 release 19:06:24 Not sure if #14339 is something we want to address for 0.17... probably not 19:06:24 https://github.com/bitcoin/bitcoin/issues/14339 | Qt 0.17.0rc4 (and master) not running on Ubuntu 14.04.5 LTS · Issue #14339 · bitcoin/bitcoin · GitHub 19:06:43 So macOS GUI compilation seems completely broken: #14327, but that wouldn't stop cross compling a release I suppose. 19:06:44 https://github.com/bitcoin/bitcoin/issues/14327 | macOS Mojave QT 5.11 compilation fails · Issue #14327 · bitcoin/bitcoin · GitHub 19:06:49 so the blocker for 0.17 is #14289 19:06:50 https://github.com/bitcoin/bitcoin/issues/14289 | Unbounded growth of scheduler queue · Issue #14289 · bitcoin/bitcoin · GitHub 19:07:25 the GUI problems are annoying and need to be fixed but are not blocking the release at this stage, IMO 19:07:25 hi 19:07:32 provoostenator: hmm.. compiled master yesterday on a fresh Mojave installation 19:07:36 (no problems) 19:07:50 wumpus: agree 19:08:18 jonasschnelli: ok, maybe it's machine specific? Can you and others comment on that issue? 19:08:26 (will do later) 19:09:23 (I'm trying now on my Macbook as well, maybe it's just my iMac being a pain) 19:09:42 What about #14289 and #14104? 19:09:43 https://github.com/bitcoin/bitcoin/issues/14289 | Unbounded growth of scheduler queue · Issue #14289 · bitcoin/bitcoin · GitHub 19:09:44 https://github.com/bitcoin/bitcoin/issues/14104 | 0.17.2rc issue (standardness change for bare multisig) · Issue #14104 · bitcoin/bitcoin · GitHub 19:09:50 although, it's likely that #14289 is not a regression for 0.17 it's still something that needs to be addressed in some way 19:09:51 https://github.com/bitcoin/bitcoin/issues/14289 | Unbounded growth of scheduler queue · Issue #14289 · bitcoin/bitcoin · GitHub 19:10:05 jonasschnelli: I don't think #14104 is a blocker, but the other one is nasty 19:10:06 https://github.com/bitcoin/bitcoin/issues/14104 | 0.17.2rc issue (standardness change for bare multisig) · Issue #14104 · bitcoin/bitcoin · GitHub 19:10:23 14289: one "solution" could be to recommend people to resync if they're still on v0.13, since it's impractically slow anyway even without the memory problem. 19:10:37 We just need to make sure to communicate the standardness change in 0.17.0 19:10:55 Or they can install 0.15 first, wait for that process to finish and then install 0.17 19:11:05 meh 19:11:10 hi, i'm sortof here - ping me if needed 19:11:17 provoostenator: agree; though putting a message in the code itself if people are upgrading from 0.13.0 would make sense, for those not carefully reading the release notes, but anyhow 19:11:57 I think even though 0.16 appears to have added the replay bloat, 0.17 makes bloat worse because it adds an additional place where they're queued. (this doesn't change that I think notices are probably the best move for now) 19:12:00 but yes for the most common case (0.13.0 upgrade rollback), adding a message to the release notes would be acceptable solution 19:12:02 couldn't we detect the reorg needed, and just prompt for user action instead of trying to upgrade it? 19:12:12 If it can be done safely, having the upgrade simply refuse and throw an error message seems safer than just a release note. 19:12:24 gmaxwell: i'm not sure anything was added in 0.17 19:12:40 i blamed the txindex change, but the asynchronous processing was added earlier 19:12:48 so I guess there isn't really a hurry to release 0.17.0 at this point 19:12:51 sipa: txindex also schedulers queues block connections and disconnection, no? 19:13:28 regardless... We don't yet have a proper fix for the issue, and I don't think we're still learning much about 0.17rc. 19:13:52 gmaxwell: i think (sorry no time to look now) is that 0.17 just added the txindex as a listener for those blockconnected events; the issue is not that, but the events in the first place 19:14:05 ah. 19:14:35 my suggestion is to just point out in release notes that upgrading from 0.13.0 or before is a known memory bloat issue, which can be worked around with -reindex 19:14:42 I didn't walk through the patches so it wasn't clear to me that the events existed before. Got it. 19:14:45 (ActivateBestChain actually checks for the queue length and blocks on it getting caught up) 19:14:59 luke-jr: it does; but RewindBlock and InvalidateBlock don't 19:15:13 sipa: would it be so bad if they did? 19:15:31 luke-jr: they need to release cs_main for that, which would be a major refactor for those functions 19:15:33 luke-jr: that can be tricky to do safely as car has to be taken to make sure they don't wait while holding any locks. 19:15:39 care* 19:15:42 hmm 19:15:57 but we can probably remove the upgrading logic from pre-segwit blocks anyway - as provoostenator says, it's already pretty unwieldy even without the memory bloat issue 19:16:12 yea, reindex might already actually be faster. 19:16:19 i'm pretty sure it is 19:16:25 but I assumed we'd want to use the rewind for future softforks. 19:16:30 Does reindex just toss out block files it doesn't need? 19:16:37 i don't care so much that invalidateblock takes a lot of memory - it would be a nice to have if we could actually make it revert to genesis, but it's not a priority 19:16:42 provoostenator: yes 19:16:58 sipa: uh it's a little worse than that. 19:17:25 I mean it hits 64+GB usage rewinding only a couple months of blocks. 19:17:45 yeah, ok 19:17:46 And it doesn't stop once it's going. 19:18:03 indeed, and it doesn't return the memory, ever. 19:18:11 we'll need to refactor InvalidateBlock to deal with that; doesn't sound impossible, but probably for 0.17.1? 19:18:30 Not a reason to hold 0.17, but it's not an irrelevant inefficiency. 19:18:38 fair 19:18:42 sipa: sounds fine to me. 19:18:56 <2% of the network (<2000 nodes) run non-segwit software at this point; throwing an error that we can't upgrade them anymore seems reasonable 19:19:27 yes 19:19:50 luke-jr: that's a useful statistics 19:19:52 I still think we shouldn't just can the rewinding code though. 19:19:55 Maybe also throw an error if the user tries to invalidate more than 10K blocks? They can always do it in increments. 19:20:06 ugh. 19:20:33 just release note it, then we'll fix it later, please don't add additional falure cases to the function. 19:20:40 agree with gmaxwell 19:20:45 please don't overdesign temporary code 19:20:58 the refactor will effectively just do that - split it up in batches of X blocks to disconnect at once 19:21:12 this needs to be fixed properly, in master, and in the future we should be careful to review anything that adds a queue or global data structure for this possiblity 19:21:20 agree 19:21:22 but don't spend too much time on the workaround 19:21:49 I guess anyone upgrading all the way from 0.13 will probably pay more than average attention to the Upgrade Notice section in bold at the top... 19:22:32 I think most people still running 0.13.x do so intentionally, and won't be upgrading to 0.17.x any time soon 19:22:43 (not those nodes, at least!) 19:22:56 most 0.13 nodes are 0.13.2 anyway 19:23:07 yes; 0.13 is not the issue; 0.13.0 is 19:23:12 sipa: https://luke.dashjr.org/programs/bitcoin/files/charts/services.html fwiw 19:23:23 e.g. some users want to run old node software to cross-validate 19:25:01 so: for 0.17.0, we should add a mention to the release notes for users upgrading from 0.13.0. This would require no new rc, so the way to final can continue as normal. 19:25:42 there's a pr for backports, will that be fore 0.17.1, or are those going in for .0? 19:26:04 I haven't seen it, but unless they solve critical problems, they are not going in .0 19:26:18 #14328 19:26:20 https://github.com/bitcoin/bitcoin/issues/14328 | [0.17] Backports by MarcoFalke · Pull Request #14328 · bitcoin/bitcoin · GitHub 19:27:35 I don't think any of those are very serious 19:27:59 potential unititialized value sounds dangerous, but looking at the PR, it's impossible to trigger in practice 19:28:08 I hate that kind of PR naming 19:28:39 PR bait? :-) 19:28:42 I've complained about that a number of times in the past. 19:28:57 me too, so many times, the guy doesn't listen to me 19:29:02 so does that mean 0.17.0 final after the meeting? 19:29:11 (or gal, I don't really know) 19:29:19 Works for me. 19:29:40 I guess so? would be nice to have the release notes change in 19:29:47 before tagging 19:31:05 just needs a one liner, no? "If upgrading from 0.13 or prior, you must start with -reindex" 19:31:33 yah 19:31:58 I've noted it here https://github.com/bitcoin/bitcoin/issues/12391#issuecomment-425211080 19:32:01 k 19:32:08 we also need to add a known issues section 19:33:32 yepp 19:33:35 any other topics? 19:33:56 can anybody reproduce the travis error on #14336 19:33:58 https://github.com/bitcoin/bitcoin/issues/14336 | net: implement poll by pstratem · Pull Request #14336 · bitcoin/bitcoin · GitHub 19:34:02 i cannot reproduce it locally 19:34:25 #topic Travis error on poll() PR 19:34:49 IMO after the meeting 19:34:58 I guess this is a action item, that people shoudl try the PR locally? 19:34:58 instagibbs had a related question, where are the bitcoind exit codes coming from. phantomcircuit's travis failure bitcoind has a return value of -6 19:35:12 after the meeting, yes, doesn't make sense to do a real-time debugging session I think :) 19:35:32 I shall wait 19:35:38 would be fun... but yes. Better later. 19:35:42 wumpus: topic suggestion: multiwallet 19:35:43 #action try to run tests on #14336 on different environments to see if it reproduces travis error 19:35:45 High-Prio list? 19:35:45 https://github.com/bitcoin/bitcoin/issues/14336 | net: implement poll by pstratem · Pull Request #14336 · bitcoin/bitcoin · GitHub 19:36:02 #topic multiwallet (promag) 19:36:12 cc jnewbery 19:36:25 just want some feedback regarding https://github.com/bitcoin/bitcoin/pull/13100#issuecomment-424342813 19:36:34 also, regarding listwalletdir 19:36:49 jonasschnelli: I haven't paid attention to the high-prio list at all this week, with the CVE issue and Riga travel so I'm not sure there's much to do there, but sure we can discuss it 19:37:12 I think Concept ack on both (promag)! Will test more soon. 19:37:20 and I'm going to submit a refactor PR introducing WalletsModel 19:37:37 that combines loaded wallets and existing wallets 19:37:53 wumpus: Yeah. I only wanted to ask for a review on #14046 from gmaxwell and sipa since they both commented already on it (fixed stuff) 19:37:55 https://github.com/bitcoin/bitcoin/issues/14046 | net: Refactor message parsing (CNetMessage), adds flexibility by jonasschnelli · Pull Request #14046 · bitcoin/bitcoin · GitHub 19:38:01 otherwise #13100 looks like junk code.. 19:38:03 https://github.com/bitcoin/bitcoin/issues/13100 | gui: Add dynamic wallets support by promag · Pull Request #13100 · bitcoin/bitcoin · GitHub 19:39:53 any objections to WalletsModel? IIRC it was previously suggested 19:40:56 I'll look at 14046 again, thanks for the ping. 19:42:54 topic proposal: factor out berkeley-db 19:43:06 #topic factor out berekey-db 19:43:11 (jonasschnelli) 19:43:15 I tried this serval times but seems more complex then initially though.. 19:43:32 But I think we should slowly consider alternative database systems (internal) for wallet files 19:43:46 And factroing out BDB should be done sooner then later 19:43:58 where did the complexity come from? 19:44:06 Could be combined with jnewbery's standalone wallet tool, which can hold on to the direct dependnecy a bit longer. 19:44:20 jamesob: I think mostly due to the complex code layering... 19:44:47 I think switching the database backend on runtime should be possible.... 19:45:08 switching the database backend on runtime should be possible <- why? 19:45:11 I hope someone working on the wallet can initiate that refactor: jamesob, jnewbery, ryanofsky 19:45:23 I assume it makes most sense to move it to leveldb since we're using that for most other things? 19:45:33 leveldb is very annoying 19:45:34 what gah no 19:45:42 I don't think so; leveldb can't use a single .dat-ish file 19:45:43 promag: we must assume there will be a pretty long "transition" phase from the old to the new database layer 19:45:45 Not leveldb... 19:45:48 it needs whole directories, and provides way too many features 19:45:50 and not a particularly good fit for what its used for here. 19:45:56 sipa one wrote a simple append only database... 19:46:00 sqlite IMO seems like a good bet 19:46:13 jonasschnelli: do we need to assume there is a long transistion instead of a standalone conversion util? 19:46:16 yeah, though FWIW for the berkekeydb we also suggest a whole directory per wallet at the moment 19:46:21 (logdb),... there is a 2-4 year old PR somewhere (search after logdb) 19:46:25 sqlite is fine, though we also don't need any of its features, apart from safe updating 19:46:29 wumpus: yes, but we know we don't like to do that. :) 19:46:35 or honestly just a raw format of our own creation 19:46:47 but the dangerous thing here is that anything you choose for the wallet will need to be supported more or less forever, so it's not an easy choice 19:46:49 gmaxwell: could also be a conversion utility,.. but at least the utility must be capable to run both database systems,... so won't change that much for the refactroing) 19:46:52 If we add another dependency, maybe pick one that's potentially useful for block and index storage? 19:46:54 esp since we just end up loading the whole thing into memory, a fancy database is kinda overkill. 19:47:13 gmaxwell: agree 19:47:24 provoostenator: those things actually need efficient querying, atomic batch updates, ... 19:47:27 we don't *need* to load the whole thing in memroy, that's a current design choice 19:47:36 provoostenator: for the wallet we just need a key-value store with some append only records :) 19:47:37 (and also makes the files much more fragile and harder to work with than they would be otherwise) 19:47:38 sqlite is also nice in that they provide a monolithic source file and encourage direct integration. 19:47:47 if there's proper indexing, loading every single transaction into memory could be avoided 19:47:50 yeah, i'm not opposed to sqlite 19:47:54 logdb (#5686) would be a simple append only hard to corrupt "database"... everything is hold in memory 19:47:57 https://github.com/bitcoin/bitcoin/issues/5686 | [Wallet] replace BDB with internal append only (logdb) backend by jonasschnelli · Pull Request #5686 · bitcoin/bitcoin · GitHub 19:47:57 it has very thorough tests 19:48:00 cfields: that's not a good thing. -.- 19:48:00 Someone once complained that the wallet didn't have atomicity guarantees. 19:48:02 wumpus: indeed. but that decision should be made jointly. 19:48:03 Or sqlite... yeah 19:48:11 I like sqlite, especially with deterministic wallets it wouldn't need to store all the keys 19:48:20 sqlite seems like a pretty safe bet 19:48:22 luke-jr: the alternative is the reason we're switching away... 19:48:24 so it's pretty much a metadata database, and sqlite is great for metadata and querying metadata 19:48:32 wumpus: with descriptor based wallets we don't need that anyway :) 19:48:35 cfields: what? 19:48:41 sipa: even beter 19:48:42 I don't think sqlite makes much sense unless the intent is also to move away from pulling everything into memory. 19:48:50 Is it guaranteed that sqlite databases are interoperatable between platforms and versions of sqlite? 19:48:59 gmaxwell: what would you propose in lieu? 19:49:01 please move away from loading everyting into memory in the long run 19:49:02 And if we're going to do that, the scheme in the database matters a lot, so that change should probably be made at the same time. 19:49:11 on the short term it's not a priority 19:49:14 but don't make it impossible 19:49:27 (in a new format) 19:49:35 jamesob: if we're loading the whole thing into memory, a simple serialized format like logdb is I think vastly superior. 19:49:37 I agree with gmaxwell: sqlite makes most sense if we want to one active handling of merchant size wallets 19:49:46 and not sure if we want that 19:49:56 does it have to be an embedded database? 19:49:58 some large users of the wallet run into memory issues, and have to remake a new wallet perioidically because of this limitation 19:50:06 promag: no 19:50:19 if we just use sqllite but then just treat it like a blob holder, then the whole schema will need to change to avoid memory loading it in any case. 19:50:20 jonasschnelli, think it makes most sense to have a tool which is a separate binary to convert from bdb to "new" wallet format 19:50:21 I don't think it has to be a "database" at all 19:50:21 (due to storing all the transactions in memory, and also the time overhead of loading the whopping thing at startup) 19:50:22 wumpus, or abusing rpc calls to whiddle it down 19:50:27 and for the new wallet format to simply be a flat file 19:50:30 instagibbs: oh :-) 19:50:38 phantomcircuit: I agree. But that tools would require the refactoring also 19:50:40 luke-jr: eh, not worth getting into it and muddying the conversation 19:50:43 instagibbs: I mean, :-( 19:50:47 wumpus: More than memory issues, they run into problems that many of our rpc operations iterate over all txn in the wallet and then become super slow. 19:50:55 gmaxwell, yeah that one 19:51:07 jonasschnelli, yes but has the advantage that you can write the conversion tool and then just rip out a ton of the walletdb logic entirely 19:51:08 gmaxwell: right - another lmitation of not having indexing, either in memory or on disk 19:51:08 i know people who delete completely spent tx(plus 100 confs or something) to speed it wallets 19:51:21 which makes refactoring much easier, cause you dont have to support both simultaneously 19:51:31 phantomcircuit: the logic must still be available somewhere,... could be in a tool source only. yeah 19:51:49 cfields, sqlite doesn't actually provide a monolithic file in the same way bdb doesn't 19:51:50 instagibbs: ah yes, the "wallet only needs a view of current utxos, not all of history" view 19:52:05 "pruned wallet" 19:52:06 wumpus, either that or listunspent takes forever :( 19:52:07 phantomcircuit: eh? They for sure used to. 19:52:08 to operate in the fast safe mode it needs a separate write ahead log file 19:52:09 gmaxwell: right 19:52:15 well, at least we don't need to keep the history in memory 19:52:23 luke-jr: indeed 19:52:24 phantomcircuit: oh, I was talking about source file, not the database format. 19:52:24 cfields, you cant have a single file but it's amazingly slow 19:52:28 oh 19:52:34 yes it does have that but like 19:52:35 meh 19:52:40 that's where something like sqlite would be, more or less, useful, I like how clightning uses it 19:52:50 going back to the prior point. ... if the history isn't in memory, then the database storing the wallet needs to be structured in a way that suports that 19:53:15 phantomcircuit: that makes integration into our build a no-brainer. That's a signifacant feature imo. 19:53:49 gmaxwell, and needs to be quite fast actually 19:54:05 integrating sqlite into a project is trivial, indeed can be done as a single .cpp file if that's desirable 19:54:16 if we're thinking longterm (esp. about not loading everything into memory simultaneously), I think it makes sense to come up with a normalized, relational schema for the wallet and use sqlite. shouldn't be hard to come up with something non-controversial (famous last words) 19:54:33 any reason to not consider postgres for instance? 19:54:38 AHHHH 19:54:38 gmaxwell: i don't think the choice of container format and the choice of database layout need to be made at the same time 19:54:41 promag: god why 19:54:42 wat 19:54:43 promag: uh, lots? 19:55:04 Oracle? 19:55:09 haha 19:55:11 jonasschnelli: :-) <3 19:55:19 Oracle BDB? 19:55:22 luke-jr: name one 19:55:28 I think however we proceed (sqlite, logdb, etc.), factoring out BDB in a nice layered way will be require (even helps if we keep BDB forever) 19:55:32 I mean, if we're using sqlite, the queries could be compatible with multiple backends, but expecting regular users to set up Postgres is crazy.. 19:55:35 I hope someone picks that up 19:55:35 promag: let's do that outside this meeting 19:55:39 this might work better in terms of concrete proposals rather than rounds of "how about xyz?" 19:55:45 Also BDB is a compile pitfall 19:55:48 sure 19:56:03 cfields: good point 19:56:12 'what about mongodb?' :') 19:56:28 any other topics? 4 minutes left 19:56:29 haha 19:57:04 We should just store it on a blockchain. 19:57:21 provoostenator: it would be nice if it was possible to commit to it in such a way 19:57:40 eg, if you could get a historical hash of the wallet state for commitments 19:57:45 luke-jr: right, optional support for a large-scale DBM like postgres would be useful for really big users, but that's really a long term goal I suppose, if at all 19:57:51 I'd prefer it if just ban anyone that ever directly uses the name of any database system from the channel. 19:58:09 but leveldb! 19:58:11 except oracle, of course *ducks* 19:58:17 sipa: "container" is basically my point, if we're just using it as a "container" a simple log would be a lot better. 19:58:19 heh 19:58:52 #endmeering 19:58:55 #endmeeting