19:00:32 #startmeeting 19:00:32 Meeting started Thu Jan 10 19:00:32 2019 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:00:32 Useful Commands: #action #agreed #help #info #idea #link #topic. 19:00:44 hi 19:00:45 hi 19:00:48 hi 19:00:56 hi 19:00:56 fyi there weren't any proposed topics in IRC the past week 19:01:00 hi 19:01:02 #bitcoin-core-dev 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 19:01:12 hi 19:01:19 hi 19:01:31 any proposed topics now? 19:01:42 thanks for keeping track moneyball 19:01:59 topic: moving hwi under bitcoin-core org 19:02:13 hwi? 19:02:24 hardware wallet interface thingy 19:02:25 https://github.com/achow101/HWI 19:02:26 hi 19:02:34 ohh ofc 19:02:47 #topic high priority for review 19:02:54 https://github.com/bitcoin/bitcoin/projects/8 19:03:02 can i has #14955 ? 19:03:06 https://github.com/bitcoin/bitcoin/issues/14955 | Switch all RNG code to the built-in PRNG by sipa · Pull Request #14955 · bitcoin/bitcoin · GitHub 19:03:33 current ones: #11082 #14491 #14711 #14941 #14938 19:03:36 https://github.com/bitcoin/bitcoin/issues/11082 | Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr · Pull Request #11082 · bitcoin/bitcoin · GitHub 19:03:38 https://github.com/bitcoin/bitcoin/issues/14491 | Allow descriptor imports with importmulti by MeshCollider · Pull Request #14491 · bitcoin/bitcoin · GitHub 19:03:41 sipa: sure 19:03:41 https://github.com/bitcoin/bitcoin/issues/14711 | Remove uses of chainActive and mapBlockIndex in wallet code by ryanofsky · Pull Request #14711 · bitcoin/bitcoin · GitHub 19:03:43 https://github.com/bitcoin/bitcoin/issues/14941 | rpc: Make unloadwallet wait for complete wallet unload by promag · Pull Request #14941 · bitcoin/bitcoin · GitHub 19:03:45 https://github.com/bitcoin/bitcoin/issues/14938 | Support creating an empty wallet by Sjors · Pull Request #14938 · bitcoin/bitcoin · GitHub 19:04:06 i'd like to request #15141 19:04:08 https://github.com/bitcoin/bitcoin/issues/15141 | Rewrite DoS interface between validation and net_processing by sdaftuar · Pull Request #15141 · bitcoin/bitcoin · GitHub 19:04:19 I think everyone's scared of reviewing that! 19:04:35 it's already been reviewed a bunch, just never was merged :( 19:05:00 added 19:05:08 thanks 19:06:10 #topic moving hwi under bitcoin-core org (achow101) 19:06:33 hi 19:06:46 #link https://github.com/achow101/HWI 19:06:47 Maybe we can discuss #11082 since there was the discussion of JSON vs INI? 19:06:50 https://github.com/bitcoin/bitcoin/issues/11082 | Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr · Pull Request #11082 · bitcoin/bitcoin · GitHub 19:06:54 Hi 19:07:43 so hwi is currently under my account. but if we are going to use it for hardware wallet support in core, then perhaps it would be better to have it under a proper github org. the obvious one that comes to mind is bitcoin-core 19:08:14 yes, that makes sense 19:08:17 instagibbs: might have some thoughts on this? 19:08:25 nothing against achow101 but also I'm actively using it and would be nice to be put under an established org 19:08:28 ;) 19:08:38 ACK in other words 19:09:02 no one seems to be opposed :) 19:09:31 https://github.com/achow101/HWI/issues/91 for further background 19:09:43 We should give achow merge permission on it imo 19:09:58 of course 19:10:05 achow101: iirc there is a way to 'transfer' a repo to another org, that way github will automatically set up redirects etc 19:10:09 as opposed to copying it over 19:10:23 yep, just an organizational thing, with established review/merge norms/contributor docs 19:10:25 he might still want the local clone for his own PRs 19:10:29 sipa: yes, there's a transfer ownership button 19:10:53 #link https://help.github.com/articles/transferring-a-repository/ 19:10:56 but could create a new one as well 19:11:09 (clone it again after transfering) 19:11:35 any other topics? 19:11:51 BIP 157 index leveldb vs flatfiles? 19:12:08 ah yes jonasschnelli had one 19:12:27 not an important one 19:12:40 (and my internet connection goes up and down) 19:12:45 ok jimpo first then 19:12:46 https://github.com/bitcoin/bitcoin/pull/11082#issuecomment-451406061 19:12:50 ack 19:12:58 #topic BIP 157 index leveldb vs flatfiles (jimpo) 19:13:00 So there's some discussion on #14121 19:13:03 https://github.com/bitcoin/bitcoin/issues/14121 | Index for BIP 157 block filters by jimpo · Pull Request #14121 · bitcoin/bitcoin · GitHub 19:13:27 basically there's three options 1) store entire filters along with metadata in separate LevelDB 19:13:43 2) store filter metadata (hash, disk pos) in levelDB and filters in flat files 19:14:03 3) add filter metadata to CBlockIndex and store filters in flat files, just like undo data 19:14:27 In terms of perf, #3 is probably the fastest and certainly the fastest with enough IO optimizations 19:14:30 https://github.com/bitcoin/bitcoin/issues/3 | Encrypt wallet · Issue #3 · bitcoin/bitcoin · GitHub 19:14:35 so the first question is between 1/2 and 3, i think; whether the filters are their own index semantically, or part of the main data structures 19:15:10 What speaks against being part of the main data structure? 19:15:14 I prefer 1/2 since I can imagine alternate filter types 19:15:21 (like option 3) 19:15:22 and i guess that depends on how they'll be used; if they're mostly optional things that some people want to enable, a separate index is the obvious way to go 19:15:29 jonasschnelli: https://github.com/bitcoin/bitcoin/pull/14121#issuecomment-451838178 19:15:39 but if we envision it may become a consensus rule, having it separate is quite annoying 19:15:55 metadata is like 76 bytes (2 hashes + disk loc) 19:16:19 From what I see is that there are plans to commit the filters at one point so it'll be part of the consensus, right? 19:17:06 it's more convenient to have it in CBlockIndex if part of consensus rules, yes, but I'm not sure how much worse the separate index is there 19:17:18 i guess it isn't really 19:17:29 would have to do some synchronization or build the filters twice (but that's really fast) 19:17:47 like you can validate the consensus rule w/o storing the filter 19:17:50 it can go from an asynchronously maintained one to a synchronous one perhap at that time, but still remain a separate db/directory 19:17:50 if you don't want to 19:17:55 right 19:18:45 which is another strong (IMO) argument in favor of 1/2: not everyone needs to or should be forced to store filters 19:19:00 and no need to bloat CBlockIndex if people might disable it 19:19:15 agree; i forgot about the possibility that even in case of it being consensus, there is no requirement to actually store the filters 19:20:22 about the difference between 1 and 2... i prefer the flat files approach slightly (the filters aren't that big that using leveldb is insurmountable, but they're still append-only data like blocks/undo) 19:20:43 and i saw you also have a PR to abstract out the flat files stuff, which seems useful in any case 19:20:54 I do agree that 2 is somewhat conceptually cleaner 19:21:13 but it does make the code more complex and is slower right now unless we add a caching layer to the flatfile stuff 19:21:53 we don't know how much slower though, right? might be negligible 19:22:01 I feel sketchy about adding a requirement to store blobs in leveldb, I think in the long run we won't end up using leveldb -- we now no longer need basically any interesting properties of it, it's just a MAP on disk for us now. 19:22:24 I'm confused that its slower or at least why it would be under actual use though. 19:22:30 gmaxwell: we might use snapshotting to alleviate lock contention at some point... 19:22:46 jamesob: you wouldn't say that if you'd actually tested snapshotting. 19:22:49 just two separate reads instead of 1 for each filter I think 19:23:07 gmaxwell: but do you think that choosing to use leveldb right now will complicate a hypothetical move to using something else? 19:23:12 but there are certainly ways to reduce that overhead which I haven't experimented with 19:23:20 jimpo: why two seperate reads? 19:23:28 1 for leveldb, then 1 for flatfile 19:23:36 first read from the leveldb index to get the disk pos, then read the filter from flatfile 19:23:43 well the leveldb part can be in memory, no? 19:23:51 like CBlockIndex is now 19:23:54 Ah so it's directly reading the leveldb instead of having the index in memory like the block index is. 19:24:19 true, we could keep the whole index in memory 19:24:35 but shouldn't that be pretty much equivalent to setting the cache on the leveldb high enough? 19:24:36 I would be saying these pointers should just be in the blockindex. But there was some talk about switching this on and off above. 19:25:08 jimpo: not really, there are a bunch of overheads in the leveldb caching and it's not particularly intelligent. 19:25:23 also a leveldb _read_ is not a single disk access 19:25:37 (as opposed to something that hits a cache) 19:26:03 ok 19:26:06 it's quasi log(n) plus a bunch of rather expensive bloom filter checks, which is why I was surprised the flatfile was slower.. 19:26:06 so with index in memory + flatfile you're guaranteed one disk seek always 19:26:21 but if your flatfile is a leveldb access plus the file now I see why it couldn't be faster. 19:26:32 since you take the ldb overheads in both cases. 19:26:35 gmaxwell yes it was leveldb + flatfile 19:27:43 OK, so what if we have a leveldb index + flatfiles 19:28:04 then if that's slow we can add logic to pull the whole leveldb index into mem like the CChain structure 19:28:10 but as a separate structure 19:28:54 is that easier than doing it immediately? (i'm thinking about dealing with the possibility of leveldb read failure outside of startup etc) 19:29:17 yes, it definitely breaks up the size of the code change 19:29:22 sipa: I think right now our leveldb usage can be replaced with something that very simply serializes the things like blockindexes, and an open-hash table for the utxo set. e.g. like the one the ripple people did... and it would likely be a pretty massive speedup. I suppose you could just convert this to a flatfile /then/, so it's not the end of the world. We also take on some increased 19:29:22 Q/A,dev overhead if we start storing blobs in ldb because thats just a whole other set of access patterns that we need to worry about that might result in misbehaviors (like, what is ldb's peak memory usage look like when storing 40kb blobs in it?) 19:29:51 sipa: if we have leveldb read failures we've got bigger problems than serving block filters, no? 19:30:38 is a leveldb read failure significantly more likely than a flatfile read failure? 19:30:39 gmaxwell: ok i see 19:30:58 jimpo: it needs exception handling etc, which i remember was a pain to deal with in the UTXO code 19:31:43 doesn't all i/o need error handling? 19:32:11 I guess flatfile accesses just return error codes or null vs throwing exceptions? is that what you're saying sipa? 19:32:22 sure, but an "if (!read) return false" in the startup code is easier than a wrapper database object that catches exceptions etc 19:32:35 oh sure the way in which errors are reported will differ 19:32:58 anyway, i'm fine with leveldb index + flatfiles, and moving the load into RAM to a later chance 19:33:26 one thing at least, for the first iteration it's useful to keep the amount of new code minimal 19:33:28 performance at this point doesn't matter that much for this application, but it's nice that in the future it wouldn't be a compatibility break 19:33:45 is the only reason to not store it in the blockindex is to just avoid the size of the blockindex objects increasing when the filter isn't used? 19:34:17 potentially if core supports multiple filter types (dunno how likely that is?) 19:34:33 jimpo: also to simplify building it in the background, i guess? 19:34:41 so if using leveldb results in much less code than implementing some manual file handling, that might be handier for review, as said it can be optimized later 19:35:30 it won't be. 19:36:10 it's a few hundred more lines 19:36:18 *after* the flatfile refactor 19:36:20 as long as you don't end up rolling your own k/v store 19:36:44 but I think the refactor is good to do anyway 19:36:55 agree with that 19:37:18 agree, yes 19:37:28 gmaxwell: what makes you say so? 19:37:50 The whole process of making these things super optional seems to add a lot of complexity. If instead we didn't think of it that way the filters could just be handled right along side, say, undo data. 19:38:21 eh, depends how you think about complexity 19:38:36 I consider adding more stuff to CBlockIndex to be more complex than a separate, asynchronous system 19:38:54 gmaxwell: agree, it adds complexity to make it optional and separate, but that's a different question than whether or not everything should be in leveldb or partially in flat files 19:39:05 otherwise it involves messing around with validation code before there's even a consensus change 19:39:05 If I drop out support for upgrades and what not, I believe I could add support for including indexes with two dozen lines of code. Obviously not a fair comparison. 19:39:06 right, with decoupling at least changes can be localized and contained, instead of tying everything together 19:39:25 But building a pile of logical abstractions that add layers and layers is not actually a reduction in complexity. 19:39:32 exactly jimpo 19:39:34 It's decomplexity theater. 19:39:40 please 19:39:55 is that what he's doing? 19:40:13 rich hickey would disagree, I think 19:40:57 and then we just spend our entire lives on pointless refactors shifting around the deckchairs on this fractal of layers, while seldom actually advancing visible functionality, which is exactly what the project has been doing for the last year. 19:41:10 I don't think this is construcive anymore 19:41:14 any other topics? 19:41:29 rw_config INI vs UniValue 19:41:36 operation of cblockindex is consensus critical; atm serving filter blocks is not => decoupling failure of the optional feature from the critical one is safer 19:41:49 #topic rw_config INI vs UniValue (jonasschnelli) 19:41:53 https://github.com/bitcoin/bitcoin/pull/11082#issuecomment-451406061 19:42:13 The current rw_config PR from luke-jr uses .INI file structure (own parser)... 19:42:29 The question is, if a simple JSON file wouldn't be simpler and more stable 19:42:39 jamesob: FWIW I agree, as long as it is not consensus critical, it's better to have it separate from the consensus critical code, this was the same idea with separating out the other indexes 19:43:08 but apparently all of that was pointless — 19:43:14 JSON is somewhat less human-readable IMO 19:43:38 Yes. But should its main focus be human editable / readable? 19:43:46 also more annoying to write (e.g. trailing commas on last key causes errors) 19:45:44 I think using a JSON file is more straight forward since we have the parser/writer logic in place (rather then adding another file format) 19:46:14 Also, the rw_configs focus AFAIK is "edit through the application layer" rather then manually with a text editor 19:46:20 the question isn't really "do you like ini or json better?" the question is how do you want to replace QSettings? with a new ini parser, or existing ini parser, or with existing univalue parser? 19:46:34 I'm in favor of whatever is less code 19:46:54 UniValue is very likely less code 19:47:17 Yeah, I'm on board with JSON 19:47:33 But maybe we pick up the discussion when luke-jr is back or – if you care – comment on the PR 19:49:12 other topics? 19:49:33 so can we proceed on jimpo's option (2), i.e. indexing in ldb and storing filters in flatfiles? 19:50:33 jamesob | I'm in favor of whatever is less code <- same 19:51:16 no other topics? 19:51:44 reminder: wallet IRC meeting tomorrow 19:52:52 #endmeeting