19:00:27 #startmeeting 19:00:27 Meeting started Thu May 10 19:00:27 2018 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:00:27 Useful Commands: #action #agreed #help #info #idea #link #topic. 19:00:35 hi 19:00:39 hi 19:00:42 #bitcoin-core-dev Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr btcdrak sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo marcofalke paveljanik NicolasDorier jl2012 achow101 meshcollider jnewbery maaku fanquake promag provoostenator 19:00:50 Hi 19:00:51 hi 19:00:59 proposed topics? 19:01:05 ohai 19:01:11 hi 19:01:15 hey folks 19:01:36 hello 19:01:38 topic proposal: review coordination 19:02:19 ok thanks,let's start with the usual 19:02:20 #topic High priority for review 19:02:28 https://github.com/bitcoin/bitcoin/projects/8 19:03:00 hi 19:03:03 #10740 #13097 #12979 #12560 #10757 19:03:09 https://github.com/bitcoin/bitcoin/issues/10740 | [wallet] `loadwallet` RPC - load wallet at runtime by jnewbery · Pull Request #10740 · bitcoin/bitcoin · GitHub 19:03:11 https://github.com/bitcoin/bitcoin/issues/13097 | ui: Support wallets loaded dynamically by promag · Pull Request #13097 · bitcoin/bitcoin · GitHub 19:03:14 https://github.com/bitcoin/bitcoin/issues/12979 | Split validationinterface into parallel validation/mempool interfaces by TheBlueMatt · Pull Request #12979 · bitcoin/bitcoin · GitHub 19:03:19 https://github.com/bitcoin/bitcoin/issues/10757 | RPC: Introduce getblockstats to plot things by jtimon · Pull Request #10757 · bitcoin/bitcoin · GitHub 19:03:24 https://github.com/bitcoin/bitcoin/issues/12560 | [wallet] Upgrade path for non-HD wallets to HD by achow101 · Pull Request #12560 · bitcoin/bitcoin · GitHub 19:03:45 hi 19:03:51 i'm back 19:03:56 welcome back! 19:04:00 hi back, i'm pieter 19:04:09 oh man... 19:04:12 XD 19:04:19 mr back? 19:04:56 10740 should be pretty close to mergeable (but getting unicorns right now) 19:05:18 Can I request #12254 for hi pri? 19:05:22 https://github.com/bitcoin/bitcoin/issues/12254 | BIP 158: Compact Block Filters for Light Clients by jimpo · Pull Request #12254 · bitcoin/bitcoin · GitHub 19:05:54 god if this unicorns thing persists we're gonna have to move off github 19:06:17 (actually) 19:06:33 jimpo: ye 19:06:52 The GitHub load timeouts seem to be location specific maybe -- have you tried loading over VPN from different places? 19:06:58 Like VPN near GitHub servers? 19:07:04 yeah imo 10740 is good to go 19:07:19 mostly it works if you refresh enough or are logged out, but neither of those is a solution when the refresh count is about 10 19:07:24 no, I haven't tried using a vpn, when I tried tor it didn't help though 19:07:34 but we cant use a platform where half the contributors cant load prs 19:07:43 I left some late comments on 10740 yesterday. Biggest one is around the memory leak fix. 19:08:02 usually refreshing helps but sometimes it doesn't for very large PRs with lots of comments 19:08:13 agree github is pretty much useless this way 19:08:25 jimpo: +1, beside existing comments 19:08:31 everyone knows the emulate-mobile/incognito workaround, right? 19:09:34 (if you don't: use chrome developer tools to emulate a mobile device and problematic PRs should load) 19:10:00 jamesob: thanks 19:10:51 #topic Cache witness hash in CTransaction #13011 (MarcoFalke) 19:10:53 https://github.com/bitcoin/bitcoin/issues/13011 | Cache witness hash in CTransaction by MarcoFalke · Pull Request #13011 · bitcoin/bitcoin · GitHub 19:11:01 Background: The witness hash is used for all loose transactions, so caching it would speed up validation (e.g. ATMP and compact block relay). Also, the witness hash is equal to the "normal hash" for transactions without a witness, so there is overhead for rescan/reindex is currently minimal (since there are not many transactions with witness). The gains of caching the witness hash dwarf any overhead during rescan/reindex, imo. And of course, we 19:11:02 can just rework rescan in a future pr. 19:11:39 The code changes are trivial, so at least that shouldn't be an issue 19:12:10 would be nice if that paragraph was in the PR description 19:12:30 Downside is that it makes the transactions larger, and hardcodes some validation sprcific logic into the transaction data structure (which for example also affects serving blocks from disks etc) 19:12:53 upside is we rectify a significant performance regression 19:12:55 So my view is that we should separate the transactions-for-validation tyoe and simple mutable transactions 19:13:08 and obviously all pre-segwit-activation blocks will not be served any slower 19:13:10 sipa: I think those can easily be fixed in future prs (I have one open for the blocks serving from disks, and wumpus as well) 19:13:26 MarcoFalke: i'm aware, not objecting to amything 19:13:28 personally, I find it really unacceptable that we have this huge performance regression and arent taking it as something to be fixed asap 19:13:40 sipa: I know, just posting for reference 19:13:41 just pointing out that we don't want to do this work everywhere 19:13:55 so concept ack, if we commit to separating those types 19:14:08 agree with sipa 19:14:13 yes, imo we should merge the per as-is now, and then when we look towards MarcoFalke's next pr splitting out the types (which is gonna be a lot more work) we will probably pull out both hashes then anyway 19:14:23 there are other reasons for separating those types as well, btw 19:14:30 * BlueMatt would kinda recommending backporting the pr as-is, though 19:14:35 For reference, the "separating those types" is hidden in #13098 19:14:36 let's not make the code a mess to rush ahead 19:14:38 https://github.com/bitcoin/bitcoin/issues/13098 | Skip tx-rehashing on historic blocks by MarcoFalke · Pull Request #13098 · bitcoin/bitcoin · GitHub 19:14:40 yes, I mean just fixing the txid hashing would be hugely nice 19:14:41 such as using more efficient memory representation of scripts without individial mallocs etc 19:14:52 hi. 19:15:27 yes 19:15:37 topic suggestion: ^^ one big malloc 19:15:43 (when this one's done) 19:15:54 wumpus: if anything it makes the code simpler cause the witness and non-witness code is mirrored 19:16:10 Should I put #13011 in hi priority and for backport? 19:16:12 https://github.com/bitcoin/bitcoin/issues/13011 | Cache witness hash in CTransaction by MarcoFalke · Pull Request #13011 · bitcoin/bitcoin · GitHub 19:16:20 BlueMatt: I just don't like confounding all kinds of aspects on the CTransaction object through, it's used in too many places 19:16:36 good thing miners dont pay much attention, or they wouldnt be mining segwit txn 19:17:22 wumpus: agreed, but point being the txid and wtxid code being symmetric isnt that much of a "mess" 19:17:32 BlueMatt: right... 19:17:37 and obviously agree we should be looking into rescan with a non-txid/non-wtxid type 19:17:44 fait 19:17:47 fair 19:17:51 but I have a feeling that is gonna be a lot of work 19:18:00 or at least take a while review-wise 19:19:09 MarcoFalke: ok added to high priority 19:19:15 wumpus: thx 19:19:27 I will update the OP, as requested by jamesob 19:19:34 not sure about the backport, I think it makes sense to focus on master for performance optimizations 19:19:59 I don't care about the backport, but BlueMatt was asking for it 19:20:12 it slows down compact block relay, too, which is mostly why it sucks 19:20:13 unless it's really easy and low risk to backport it 19:20:18 otherwise I might care less 19:20:24 agree with wumpus. I'm sure the changes are straightforward, but that's still risky for backport 19:20:25 the code diff is ~trivial 19:20:37 i'm against the backport, but i'm often against backports. i think they don't get enough review, so they need to be really justified to do them 19:20:48 but normally I'd prefer not to backport things that are not bugfixes, or required to support bugfixes 19:21:11 So let's leave it for now only in master, seems to be the conclusion 19:21:12 I do like having more reason for miners to upgrade to 0.16.1 19:21:33 MarcoFalke: yes 19:22:12 Alright, other topics? 19:22:33 cfields: had one 19:23:03 #topic one big malloc (cfields) 19:23:24 i like working on that, after #13062 19:23:24 sipa and I have discussed this briefly, I thought it might be helpful to have a quick discussion here 19:23:27 https://github.com/bitcoin/bitcoin/issues/13062 | Make script interpreter independent from storage type CScript by sipa · Pull Request #13062 · bitcoin/bitcoin · GitHub 19:23:31 because there are a few different approaches 19:23:39 yea, I think thats the One Big motivation for 13062 19:24:01 one big alloc for the entire process? 19:24:02 sipa: is your plan to move to Spans inside of CTransaction? 19:24:18 with a pool for the script data tacked onto the block somewhere? 19:24:38 maybe, there are a lot of possibilities 19:24:46 without it 13062 makes less sense than just haveing a ScriptSig and ScriptPubKey types 19:24:47 wumpus: one malloc for script data per-block 19:24:48 or so 19:24:49 but 13062 is a prerequisite 19:24:57 2cf 19:25:03 cfields: ok! 19:25:06 (and more; there are a lot.of things that at CScript specific that shouldn't) 19:25:22 well another option is std::allocator magic, without having to switch to Span 19:25:30 yes happy to see "span" hapening 19:25:34 noooo 19:25:43 heh, that would be a lot less code for similar results........ 19:25:47 no magic 19:26:12 I mean you're gonna have similar levels of magic (with more layer violations) to deserialize a whole block into one pool 19:26:14 yes, but much less easy to understand code 19:26:46 wumpus: i can't argue with that, it looks like voodoo 19:27:02 damn cool voodoo. 19:27:05 but voodoo. 19:27:05 c++ is already too much voodoo 19:27:12 true, fuck voodoo 19:27:20 let's switch to BASIC 19:27:33 * jonasschnelli ... 19:27:40 though absolutely NACK 13062 unless there is demonstrated branch that is realisticly-mergeable that uses it 19:27:41 anyway, it got me wondering if it'd be worthwhile to change the p2p format to be more agreeable with allocations 19:27:50 BlueMatt: :( 19:27:52 (next time we're changing something that is, not for this alone) 19:27:59 BlueMatt: i completely disagree :) 19:28:10 * BlueMatt isnt generally a huge fan of making everything a span, its just a ton more complexity 19:28:25 cfields: can you make an example for the p2p allocation issue? 19:28:25 and with C++ very easy to fuck up references and have them break 19:28:27 not everything will become a span? 19:28:37 spans are only used temporarily 19:28:46 BlueMatt: how so? Seems absolutely necessary to me if we're ever going to untangle our subsystems 19:28:54 cfields: you mean things like inv size comes before the acctual inv data? 19:28:56 I am a fan of making everything a span, at least for the inner processing, obviously not to keep hold of the data 19:29:01 sure, of course, but I see no reason to be moving the script interpreter to a generic Span thing vs using a CScript type unless there is demonstrated use 19:29:03 exactly 19:29:05 jonasschnelli: right 19:29:12 it's abstracting representation from processing 19:29:20 why? if its just operating on a CScript, it should just operate on a CScript 19:29:26 I've always found that wedding CScript to a specific backing storage type was a bad idea 19:29:29 Stumbled over this today. 19:29:46 I mean you can template it if you want :p 19:29:54 * sipa jumps off bridge 19:30:00 it implies lots of extra allocations and copying 19:30:17 BlueMatt: if something is just dumb bytes, why not represent it that way? 19:30:17 sipa: that was obviously a joke 19:30:24 BlueMatt: so was my jumping :) 19:30:25 its not just dumb bytes 19:30:26 wumpus: enough to actually be an issue? 19:30:36 a script is just dumb bytes 19:30:50 any sequence of bytes is a valid scriptPubKey at least 19:30:52 right - it's simply a span of bytes 19:31:09 yea, I mean ok, sure, dont disagree in principal, but I /really/ hate taking references to things in C++ 19:31:20 ok 19:31:24 also, I love the idea of a MultiSpan for batched operations 19:31:25 oops, something realloc'd and now we're executing uninitialized memory 19:31:45 BlueMatt: c++ already uses references for ~everything 19:32:08 kinda, at least you're taking an iterator and its clear whats going on, introduce a new type and we start passing them around in the script executor and..... 19:32:10 cfields: scatter gather lists? 19:32:24 when its Execute(script, script) you know the thing is fine cause you're giving it a reference to the byte storage 19:32:42 wumpus: right. 19:32:43 not an object that holds a refernece to it that was created sometime in the past 19:32:49 and now it will be Execute(Span(script), Span(script)) 19:32:51 cfields: can we not? 19:32:54 same thing. 19:32:59 BlueMatt: hmm? 19:33:20 no, it'll be push_to_background_thread(Span(script), Span(script)) 19:33:30 maybe 19:33:40 and more layers makes it less easy to see exactly whats going on 19:33:49 you can still copy of spans are the API 19:33:52 it doesn't restrict you in that 19:34:03 it just gives the *option* to use a direct memory buffer 19:34:03 hmm? 19:34:06 you'd pass script down everything 19:34:14 just copy a vector and make a span to it 19:34:14 fwiw, it'd be possible to add an aliasing shared_ptr into Span (or a different SafeSpan maybe) which would keep its owner from deallocating 19:34:16 and only when invoking the execution logic you create a span 19:34:18 yes, I get it, and I like the option...when we have a user 19:34:22 that seems wrong though. 19:34:56 anyway, i'd like to start by making scriotSig a different type than scriptPubKey 19:34:59 I mean I'm fine with a refactor that doesnt change the exposed interface as step 1 19:35:00 I really don't see why not to do it, but anyhow 19:35:09 there are a few more changes to do before that is possible 19:35:16 and have a wrapper in executor.cpp that just calls EvalScript(Span(arg1), Span(arg2)) 19:35:24 as the scriot executiin is not the only thing we do with scripts 19:35:30 anybody has a cfw for antminer s5 or s7? 19:35:39 also, it'd be nice to have a type check on ScriptSig/ScriptPUbKey types in the public interface :p 19:35:43 Odin: not here, go away 19:35:50 Odin: I've replied to this before, go away 19:36:08 BlueMatt: type check? 19:36:19 where I can find that, what room you suggest? 19:36:26 Odin: #bitcoin 19:36:26 cfields: well if we make scriptpubkey + scriptsig different types, you cant pass them in the wrong order to EvalScript :p 19:36:58 ...I'm pretty sure you'd know quickly enough 19:37:28 the libbitcoinconsensus wrapper in rust was the wrong way around for like 3 months, and had users :p 19:37:36 (turns out that mostly just results in it always returning true) 19:37:40 "users" 19:38:25 anyway, no reason to discuss this further i think 19:38:27 completely thread/memory safe though :p 19:38:44 there will be reasons that type/execution separation will be useful for 19:38:53 agreed, thanks. Just wanted a bit of general discussion around it. 19:39:02 (prevector for scriotSig is reaaally bad) 19:40:00 #topic review coordination (jimpo) 19:40:43 so this might just be clarifying what hi pri is 19:40:51 my understanding is it's blockers on longer term things 19:40:58 and mostly more established contributors 19:40:58 that's the intent, yes 19:41:16 I think there's space for another list of things that have been concept ack'ed for people to review 19:41:35 bitcoinacks.com ? :) 19:41:35 so that no everyone is reviewing different stuff and there's some actual coordination 19:41:47 Yes, bitcoinacks.com is great 19:42:03 I'm curious if people think there should be more of a process around this 19:42:05 the current list is already not working well, I don't really want to add another project 19:42:11 yes bitcoinacks is great 19:42:13 but yes, i agree - having a better overview on what is concept acks (anf similarly, encouraging people to concept ack/nack quickly) would be good 19:42:17 apparently it doesnt distinguish between nacks and acks 19:42:21 ha. 19:42:26 pf. 19:42:39 lol that's an interesting bug 19:42:53 "nack" "nack" "nack" "merged!" 19:42:56 who maintains bitcoinacks.com? 19:43:03 pierre, apparently 19:43:09 pierre r.? 19:43:12 yeah 19:43:12 yes 19:43:36 wumpus: I have thoughts, but why do you say the current list isn't working well? 19:43:42 Lets try to get some feature in. I think an additional layer over github would really help 19:43:59 jimpo: it generates very little actual review 19:44:09 jimpo: people do not review it "with high priority" 19:44:25 I think this is also because of the nature of OSS 19:44:25 jimpo: this comes up every meeting, so I don't particularly like to discuss it, but it doesn't attract very much review. Also because the things that end up there tend to be large, complex, hard to review changes. 19:44:42 so does that mean 1) people don't want to review 2) people don't want to review large/hard changes 3) the list isn't actually hi pri? 19:44:51 yes, it's the nature of OSS, people can review or not review whatever they want 19:45:12 also reviewing small things is easier, and we've had an influx of small prs over the past 6+ months 19:45:16 People want to review it,.. but maybe have other priorities? 19:45:39 I realize this comes up a lot, but that's because it's a big problem IMO 19:46:02 jonasschnelli: they might just not understand the change well enough 19:46:11 the solution might be just reducing the number of large changes in the hi pri list and prioritizing smaller ones that are more likely to get merged 19:46:14 jimpo: 4) some require deep understanding 5) usually UI stuff has fewer reviews 19:46:16 its not clear there's a solution, the problem is motivation and time to review big changes, and more lists doesnt help with that 19:46:16 wumpus: and that... 19:46:32 jimpo: even weeks with fewer entries dont get more review 19:47:12 What if it was a list of 1? 19:47:19 maybe more entries is better -> not everyone is able to review the same things! 19:47:43 wumpus: yeah, true... 19:48:19 My main point is that if everyone is reviewing different stuff because there's no coordination, then reviews go to waste 19:48:20 jimpo: then what? and who decides anyway? the point is now that everyone can have a PR on there that's blocking them, that should foster cooporation 19:48:28 Just as a side note: I try to respect the high priority pull requests when I merge things and avoid merging if they create a merge conflict with one of them 19:50:10 How do other people decide what to review if it's not on the hi pri list? 19:50:14 jimpo: you could ask someone to review your PR and you'd review theirs 19:50:32 depending on what they find interesting or they see as their part of the code 19:50:51 jimpo: i generally sort by recently modified 19:51:45 Agree with wumpus, currently the best way to get review is to find contributors in your "area" and trade review with them 19:52:01 And coordinate with them what should go in in what order 19:52:06 jimpo: also, every contributor has its own "roadmap". The high-prio list was usually "one PR per contributor" to "share the roadmap" 19:52:06 what MarcoFalke said, git blame and ping the guys here 19:52:20 jimpo: personally, I just browse the PR list 19:52:51 I don't do review :/ 19:53:06 I tend to pay most attention to bugfixes, RPC changes, GUI and networking things 19:53:11 achow101: its' great 19:53:17 OK, I don't have much more to say on the topic other than that I think it would be more helpful if there were more clarity around who needs to review what and what should get review. 19:53:43 everything on the PR list needs review 19:53:47 otherwise it should be closed 19:53:50 or merged 19:53:56 jimpo: what do you mean with *needs to review" ? 19:54:01 Personally, I'd just be interested in reviewing whatever needs review, so I try to go through the hi pri list. 19:54:30 Like, if there's a PR and it won't be merged until at least n of this set of loose owners reviews it. 19:54:40 then, eventually they need to weigh in 19:54:43 the point of the PR list is to keep track of what needs review, it's crazy that there's so much on there now, but we can't really help it 19:55:15 jimpo: IMO you should try to see all PR's 19:55:17 right, all open PRs need review 19:55:22 Yes. Even a full time reviewer like promag could not bring down the open PR # 19:55:24 otherwise they wouldn't be open 19:55:32 and as said, it helps to ping people that edited the code before you, they probaly know the code 19:56:14 jimpo: I agree, the million-papercut-pr review approach doesnt get us anywhere as a project 19:56:22 reviewing things on the highprio list does, at least to me 19:56:27 if the entire PR list is valid and there's limited time for reviews and we are not willing to close stuff quickly, then there's just a fundamental problem 19:56:37 I generally try to get through the high prio list once a week, though often fail 19:56:46 a lot of open source projects don't have PR lists at all, for example the freedesktop projects have a mailing list where patches are posted, and the poster of the patch has the responsibility to cc: people that are relevant for review 19:57:05 the 'dump everyone on a list' approach of github doesn't scale that well 19:57:14 the PR's just sit there waiting for feedback.. sometimes a concept ACK can help push it forward 19:57:16 at least the have no unicorns. :/ 19:57:22 well, I'm basically proposing a ranking 19:57:32 otherwise people keep submitting PR's 19:57:46 Not sure if a "global" ranking is possible. 19:57:46 who ranks? 19:57:48 jimpo: if you have specific proposals on what to close, feel free to let me know 19:57:51 Because that would assume we have central planing 19:57:58 ^ 19:58:02 # of concept acks is a ranking 19:58:09 which bitcoinacks.com is helpful for 19:58:31 but if you only see hp then thats biased.. 19:59:14 Could one say a concept ACK of sipa has the same "value" as one from John Doe? 19:59:39 That is up to you to decide. 19:59:40 jonasschnelli: people longer in the projects are seen to hae more expertise 19:59:58 Assign a ranking to each Concept ACK :p 20:00:01 if 1000 sybil accounts show up and ack something, we'd obviously not fall for that 20:00:05 whatever, I dont think we're making any more progress this week than we have the last 10 times we've discussed this 20:00:12 But I kinda understand jimpo. We should extend bitcoinacks and make it flexible in terms of "getting the ranked list". 20:00:20 I agree, this is too meta of a topic, it's not constructive 20:00:24 #endmeeting