19:00:32 <wumpus> #startmeeting
19:00:32 <lightningbot> 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 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:00:44 <moneyball> hi
19:00:45 <sdaftuar> hi
19:00:48 <sipa> hi
19:00:56 <achow101> hi
19:00:56 <moneyball> fyi there weren't any proposed topics in IRC the past week
19:01:00 <jonasschnelli> hi
19:01:02 <wumpus> #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 <promag> hi
19:01:19 <jamesob> hi
19:01:31 <wumpus> any proposed topics now?
19:01:42 <wumpus> thanks for keeping track moneyball
19:01:59 <achow101> topic: moving hwi under bitcoin-core org
19:02:13 <wumpus> hwi?
19:02:24 <achow101> hardware wallet interface thingy
19:02:25 <instagibbs> https://github.com/achow101/HWI
19:02:26 <jnewbery> hi
19:02:34 <wumpus> ohh ofc
19:02:47 <wumpus> #topic high priority for review
19:02:54 <wumpus> https://github.com/bitcoin/bitcoin/projects/8
19:03:02 <sipa> can i has #14955 ?
19:03:06 <gribble> 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 <wumpus> current ones: #11082 #14491 #14711 #14941 #14938
19:03:36 <gribble> 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 <gribble> https://github.com/bitcoin/bitcoin/issues/14491 | Allow descriptor imports with importmulti by MeshCollider · Pull Request #14491 · bitcoin/bitcoin · GitHub
19:03:41 <wumpus> sipa: sure
19:03:41 <gribble> 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 <gribble> 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 <gribble> https://github.com/bitcoin/bitcoin/issues/14938 | Support creating an empty wallet by Sjors · Pull Request #14938 · bitcoin/bitcoin · GitHub
19:04:06 <sdaftuar> i'd like to request #15141
19:04:08 <gribble> 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 <jnewbery> I think everyone's scared of reviewing that!
19:04:35 <sdaftuar> it's already been reviewed a bunch, just never was merged :(
19:05:00 <wumpus> added
19:05:08 <sdaftuar> thanks
19:06:10 <wumpus> #topic moving hwi under bitcoin-core org (achow101)
19:06:33 <phantomcircuit> hi
19:06:46 <wumpus> #link https://github.com/achow101/HWI
19:06:47 <jonasschnelli> Maybe we can discuss #11082 since there was the discussion of JSON vs INI?
19:06:50 <gribble> 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 <meshcollider> Hi
19:07:43 <achow101> 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 <wumpus> yes, that makes sense
19:08:17 <achow101> instagibbs: might have some thoughts on this?
19:08:25 <instagibbs> nothing against achow101 but also I'm actively using it and would be nice to be put under an established org
19:08:28 <instagibbs> ;)
19:08:38 <instagibbs> ACK in other words
19:09:02 <wumpus> no one seems to be opposed :)
19:09:31 <instagibbs> https://github.com/achow101/HWI/issues/91 for further background
19:09:43 <meshcollider> We should give achow merge permission on it imo
19:09:58 <wumpus> of course
19:10:05 <sipa> 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 <sipa> as opposed to copying it over
19:10:23 <instagibbs> yep, just an organizational thing, with established review/merge norms/contributor docs
19:10:25 <wumpus> he might still want the local clone for his own PRs
19:10:29 <achow101> sipa: yes, there's a transfer ownership button
19:10:53 <jnewbery> #link https://help.github.com/articles/transferring-a-repository/
19:10:56 <wumpus> but could create a new one as well
19:11:09 <wumpus> (clone it again after transfering)
19:11:35 <wumpus> any other topics?
19:11:51 <jimpo> BIP 157 index leveldb vs flatfiles?
19:12:08 <wumpus> ah yes jonasschnelli had one
19:12:27 <jonasschnelli> not an important one
19:12:40 <jonasschnelli> (and my internet connection goes up and down)
19:12:45 <wumpus> ok jimpo first then
19:12:46 <jonasschnelli> https://github.com/bitcoin/bitcoin/pull/11082#issuecomment-451406061
19:12:50 <jonasschnelli> ack
19:12:58 <wumpus> #topic BIP 157 index leveldb vs flatfiles (jimpo)
19:13:00 <jimpo> So there's some discussion on #14121
19:13:03 <gribble> https://github.com/bitcoin/bitcoin/issues/14121 | Index for BIP 157 block filters by jimpo · Pull Request #14121 · bitcoin/bitcoin · GitHub
19:13:27 <jimpo> basically there's three options 1) store entire filters along with metadata in separate LevelDB
19:13:43 <jimpo> 2) store filter metadata (hash, disk pos) in levelDB and filters in flat files
19:14:03 <jimpo> 3) add filter metadata to CBlockIndex and store filters in flat files, just like undo data
19:14:27 <jimpo> In terms of perf, #3 is probably the fastest and certainly the fastest with enough IO optimizations
19:14:30 <gribble> https://github.com/bitcoin/bitcoin/issues/3 | Encrypt wallet · Issue #3 · bitcoin/bitcoin · GitHub
19:14:35 <sipa> 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 <jonasschnelli> What speaks against being part of the main data structure?
19:15:14 <jimpo> I prefer 1/2 since I can imagine alternate filter types
19:15:21 <jonasschnelli> (like option 3)
19:15:22 <sipa> 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 <jamesob> jonasschnelli: https://github.com/bitcoin/bitcoin/pull/14121#issuecomment-451838178
19:15:39 <sipa> but if we envision it may become a consensus rule, having it separate is quite annoying
19:15:55 <jimpo> metadata is like 76 bytes (2 hashes + disk loc)
19:16:19 <jonasschnelli> 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 <jimpo> 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 <sipa> i guess it isn't really
19:17:29 <jimpo> would have to do some synchronization or build the filters twice (but that's really fast)
19:17:47 <jimpo> like you can validate the consensus rule w/o storing the filter
19:17:50 <sipa> 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 <jimpo> if you don't want to
19:17:55 <sipa> right
19:18:45 <jimpo> 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 <jimpo> and no need to bloat CBlockIndex if people might disable it
19:19:15 <sipa> 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 <sipa> 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 <sipa> and i saw you also have a PR to abstract out the flat files stuff, which seems useful in any case
19:20:54 <jimpo> I do agree that 2 is somewhat conceptually cleaner
19:21:13 <jimpo> 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 <jamesob> we don't know how much slower though, right? might be negligible
19:22:01 <gmaxwell> 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 <gmaxwell> I'm confused that its slower or at least why it would be under actual use though.
19:22:30 <jamesob> gmaxwell: we might use snapshotting to alleviate lock contention at some point...
19:22:46 <gmaxwell> jamesob: you wouldn't say that if you'd actually tested snapshotting.
19:22:49 <jimpo> just two separate reads instead of 1 for each filter I think
19:23:07 <sipa> gmaxwell: but do you think that choosing to use leveldb right now will complicate a hypothetical move to using something else?
19:23:12 <jimpo> but there are certainly ways to reduce that overhead which I haven't experimented with
19:23:20 <gmaxwell> jimpo: why two seperate reads?
19:23:28 <jamesob> 1 for leveldb, then 1 for flatfile
19:23:36 <jimpo> first read from the leveldb index to get the disk pos, then read the filter from flatfile
19:23:43 <sipa> well the leveldb part can be in memory, no?
19:23:51 <sipa> like CBlockIndex is now
19:23:54 <gmaxwell> Ah so it's directly reading the leveldb instead of having the index in memory like the block index is.
19:24:19 <jimpo> true, we could keep the whole index in memory
19:24:35 <jimpo> but shouldn't that be pretty much equivalent to setting the cache on the leveldb high enough?
19:24:36 <gmaxwell> 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 <gmaxwell> jimpo: not really, there are a bunch of overheads in the leveldb caching and it's not particularly intelligent.
19:25:23 <sipa> also a leveldb _read_ is not a single disk access
19:25:37 <sipa> (as opposed to something that hits a cache)
19:26:03 <jimpo> ok
19:26:06 <gmaxwell> 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 <sipa> so with index in memory + flatfile you're guaranteed one disk seek always
19:26:21 <gmaxwell> but if your flatfile is a leveldb access plus the file now I see why it couldn't be faster.
19:26:32 <gmaxwell> since you take the ldb overheads in both cases.
19:26:35 <jimpo> gmaxwell yes it was leveldb + flatfile
19:27:43 <jimpo> OK, so what if we have a leveldb index + flatfiles
19:28:04 <jimpo> then if that's slow we can add logic to pull the whole leveldb index into mem like the CChain structure
19:28:10 <jimpo> but as a separate structure
19:28:54 <sipa> 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 <jimpo> yes, it definitely breaks up the size of the code change
19:29:22 <gmaxwell> 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 <gmaxwell> 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 <jamesob> sipa: if we have leveldb read failures we've got bigger problems than serving block filters, no?
19:30:38 <jimpo> is a leveldb read failure significantly more likely than a flatfile read failure?
19:30:39 <sipa> gmaxwell: ok i see
19:30:58 <sipa> jimpo: it needs exception handling etc, which i remember was a pain to deal with in the UTXO code
19:31:43 <wumpus> doesn't all i/o need error handling?
19:32:11 <jimpo> I guess flatfile accesses just return error codes or null vs throwing exceptions? is that what you're saying sipa?
19:32:22 <sipa> 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 <wumpus> oh sure the way in which errors are reported will differ
19:32:58 <sipa> anyway, i'm fine with leveldb index + flatfiles, and moving the load into RAM to a later chance
19:33:26 <wumpus> one thing at least, for the first iteration it's useful to keep the amount of new code minimal
19:33:28 <sipa> 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 <gmaxwell> 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 <jimpo> potentially if core supports multiple filter types (dunno how likely that is?)
19:34:33 <sipa> jimpo: also to simplify building it in the background, i guess?
19:34:41 <wumpus> 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 <gmaxwell> it won't be.
19:36:10 <jimpo> it's a few hundred more lines
19:36:18 <jimpo> *after* the flatfile refactor
19:36:20 <wumpus> as long as you don't end up rolling your own k/v store
19:36:44 <jimpo> but I think the refactor is good to do anyway
19:36:55 <jamesob> agree with that
19:37:18 <sipa> agree, yes
19:37:28 <sipa> gmaxwell: what makes you say so?
19:37:50 <gmaxwell> 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 <jimpo> eh, depends how you think about complexity
19:38:36 <jimpo> I consider adding more stuff to CBlockIndex to be more complex than a separate, asynchronous system
19:38:54 <sipa> 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 <jimpo> otherwise it involves messing around with validation code before there's even a consensus change
19:39:05 <gmaxwell> 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 <wumpus> right, with decoupling at least changes can be localized and contained, instead of tying everything together
19:39:25 <gmaxwell> But building a pile of logical abstractions that add layers and layers is not actually a reduction in complexity.
19:39:32 <wumpus> exactly jimpo
19:39:34 <gmaxwell> It's decomplexity theater.
19:39:40 <sipa> please
19:39:55 <wumpus> is that what he's doing?
19:40:13 <jimpo> rich hickey would disagree, I think
19:40:57 <gmaxwell> 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 <wumpus> I don't think this is construcive anymore
19:41:14 <wumpus> any other topics?
19:41:29 <jonasschnelli> rw_config INI vs UniValue
19:41:36 <jamesob> 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 <wumpus> #topic rw_config INI vs UniValue (jonasschnelli)
19:41:53 <jonasschnelli> https://github.com/bitcoin/bitcoin/pull/11082#issuecomment-451406061
19:42:13 <jonasschnelli> The current rw_config PR from luke-jr uses .INI file structure (own parser)...
19:42:29 <jonasschnelli> The question is, if a simple JSON file wouldn't be simpler and more stable
19:42:39 <wumpus> 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 <wumpus> but apparently all of that was pointless —
19:43:14 <jimpo> JSON is somewhat less human-readable IMO
19:43:38 <jonasschnelli> Yes. But should its main focus be human editable / readable?
19:43:46 <jamesob> also more annoying to write (e.g. trailing commas on last key causes errors)
19:45:44 <jonasschnelli> 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 <jonasschnelli> Also, the rw_configs focus AFAIK is "edit through the application layer" rather then manually with a text editor
19:46:20 <ryanofsky> 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 <jamesob> I'm in favor of whatever is less code
19:46:54 <jonasschnelli> UniValue is very likely less code
19:47:17 <jimpo> Yeah, I'm on board with JSON
19:47:33 <jonasschnelli> But maybe we pick up the discussion when luke-jr is back or – if you care – comment on the PR
19:49:12 <jonasschnelli> other topics?
19:49:33 <jamesob> so can we proceed on jimpo's option (2), i.e. indexing in ldb and storing filters in flatfiles?
19:50:33 <wumpus> jamesob | I'm in favor of whatever is less code   <- same
19:51:16 <wumpus> no other topics?
19:51:44 <jnewbery> reminder: wallet IRC meeting tomorrow
19:52:52 <wumpus> #endmeeting