19:03:52 #startmeeting 19:03:52 Meeting started Thu Mar 29 19:03:52 2018 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:03:52 Useful Commands: #action #agreed #help #info #idea #link #topic. 19:03:56 my high-priority: #11775 (yay, I have one again) 19:03:58 https://github.com/bitcoin/bitcoin/issues/11775 | Move fee estimator into validationinterface/cscheduler thread by TheBlueMatt · Pull Request #11775 · bitcoin/bitcoin · GitHub 19:04:04 (DST sucks) 19:04:24 #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:04:37 hi. 19:04:39 hi 19:05:21 #topic high priority for review 19:05:42 BlueMatt: needs rebase again. Sorry! 19:05:42 BlueMatt: added 19:05:59 hi 19:06:15 jnewbery: well its a trivial rebase that shouldnt materially effect review 19:06:21 I'd like to nominate ryanofsky's #10244. The burden of rebasing/conflict resolution is high and I think it's in pretty good shape (though needs rebase atm). 19:06:25 https://github.com/bitcoin/bitcoin/issues/10244 | Refactor: separate gui from wallet and node by ryanofsky · Pull Request #10244 · bitcoin/bitcoin · GitHub 19:06:37 agreed 19:06:53 can we make that a topic? I'd like to discuss it in more depth 19:07:12 (10244, that is) 19:07:24 +1. Seems to be getting some review traction. It'd be a shame for that to go to waste 19:07:44 #topic separate gui from wallet and node (#10244) 19:07:48 https://github.com/bitcoin/bitcoin/issues/10244 | Refactor: separate gui from wallet and node by ryanofsky · Pull Request #10244 · bitcoin/bitcoin · GitHub 19:08:34 did you have a question BlueMatt? 19:08:38 yea, sec 19:09:02 I've... already said everything I wanted to said about that, won't repeat myself 19:09:18 so I guess I'm more of a fan of this than the wallet/main split, but I feel like we need to think a bit harder about the api between the gui/wallet+main before we go split it 19:09:30 I mean some of these things maybe shouldnt be blocking calls 19:09:40 TBH we discussed this at the new york meeting 19:09:52 and the agreement was that this could be improved after it goes in 19:09:57 ok, well I will shut up, then, if its been beaten to death 19:09:59 ok, nvm 19:10:12 I'm ok with that. I'd have preferred to make the GUI asynchronous first 19:10:20 but Iom' not going to beat that topic to death 19:10:21 right 19:10:25 api is definitely meant to be improved, especially the init stuff which is pretty ugly 19:10:32 are there any big blockers to asynchronous gui things? 19:10:38 yea, I mean that was what I was gonna say, but if there was agreement its not worth re-opening the book on that to discuss 19:10:42 no, it's just a different set of work 19:11:21 it's somewhat orthogonal to this - my gut just hates blocking RPC calls in GUI threads, it's more of an instinctive revulsion than anything I can explain, so I'll just go along 19:11:39 this PR introduces no RPC calls 19:11:47 I think part of the understanding was that this interface should be considered very much not final. 19:11:53 jamesob_: it introduces a whole new rpc interface... 19:11:57 it does, it introduces an RPC layer between the wallet and the core 19:12:03 Just having _an_ interface was step one. 19:12:06 please don't deny that 19:12:12 anyway, next topic? 19:12:22 yes, any other topic suggestions? 19:12:33 wumpus: i think jamesob_ means RPC as in the existing JSON RPC system 19:12:40 not RPC as a generic term 19:12:50 correct 19:12:56 ok, yes, RPC is a general term for cross-process calls 19:13:14 jamesob_, an earlier version of this pr did mention ipc, but i took that stuff out 19:13:15 This first step isn't cross-process 19:13:40 lol, ok, so any topics *aside* from debating rpc/ipc/whatever terminology? 19:13:44 yes... 19:14:12 topic suggestion (quick one): release notes conflicts 19:14:36 #topic release notes conflicts 19:14:41 I don't think it's a major issue, but it is irritating to have reviews invalidated due to release notes conflicts 19:14:57 options: 1) do nothing because it's not a huge issue 19:14:58 could do them in a separate commit, at the end 19:15:10 do we know if githubdeals correctly with the gitattributes merge=union stuff? 19:15:14 oh wait that doesn't help with rebases... 19:15:18 Maybe we should have the release notes dev wiki thing continuously up and people just add stuff to it as needed 19:15:33 2) don't use release_notes.md and just use a wiki for the whole release cycle 19:15:46 3) have separate release_notes files for each PR and merge them at the end 19:15:48 I mean as long as its a separate commit no reason to invalidate reviews 19:15:49 4) ? 19:16:16 4) is the merge=union thing? 19:16:29 merge=union doesn't help with github I think 19:16:33 I prefer 2 19:16:41 i don't like 2 19:16:47 too much process overhead 19:16:55 achow101: I think the only argument against 2 is that it decouples the merge from the release mode update 19:17:01 notes* 19:17:06 an option 4) would be to insert 50-100 blank lines in the file, and add release new notes in the blank space. this would avoid most conflicts 19:17:10 sipa: https://github.com/isaacs/github/issues/487 19:17:14 outside the box: notes can be added as individual files and aggregated at the end 19:17:23 so the author of the PR has to update the wiki after their thing was merged 19:17:25 jnewbery: right, but we also.don't really use github for merges 19:17:30 cfields: unless they somehow interact :) 19:17:38 i mean more... how does it affect our github merge scriot etc 19:17:41 cfields: I think that's 3 19:17:45 which compares with the github merge 19:17:55 sipa, would be annoying to see conflict on GUI and just hope it's a merge we can avoid directly handling 19:18:06 instagibbs: fair 19:18:07 jnewbery: ah yes, missed 3. 19:18:11 i think my preference is 3 19:18:15 cfields: I mean, sometimes an update to the release notes updates/extends earlier text - though 19:18:15 i like 3 too 19:18:17 maybe i need to learn that tool better, might give a better view of it 19:18:17 link describing option 4: https://about.gitlab.com/2015/02/10/gitlab-reduced-merge-conflicts-by-90-percent-with-changelog-placeholders/ 19:18:17 I like 3 19:18:30 option n) leave release notes as a comment on pr and tag the release-notes-needed issue 19:18:31 cfields: storing it *per section* would still help! 19:18:32 easy to merge at the end 19:18:37 and they exist in the pr itself 19:18:44 i also like 3 best 19:19:04 'leave it to the maintainer at the end' is not an option :p 19:19:23 it may be a release notes file per "feature" too, i think, if multiple PRs sequentially update the se thing 19:19:41 sipa: sounds reasonable, if they're serial 19:19:46 right 19:19:55 Yeah, I like the idea of basically having a file for each section in the current release notes 19:19:59 I mean what you want to avoid is that *unrelated* PRs collide in the release notes 19:20:17 wumpus: yyp 19:20:23 if PRs that already affect the same thing collide, that's not too bad, because the code likely does too 19:22:37 so yes, 3 sounds like a good idea to me, though it might be overdesign for something that doesn't cause too much trouble in practice, I wonder if anyone will actually do it 19:23:06 we can see how it plays out 19:23:11 if it's in the developer notes, then I think people will do it 19:23:22 I'll do it for my PRs to avoid conflicts 19:24:04 could add a lint step to the build that fails if the PR touches the main release notes files as well as src/ files 19:24:04 definitely needs to be in the developer notes, like "what directory to use for partial release notes' 19:24:11 oh no no more lints 19:24:31 I think that's probably enough discussion. As long as the maintainers don't object to partial release notes then individual contributors can start using them 19:24:38 I get quite angry if yet another redundant python import breaks travis 19:24:53 suggestion retracted :) 19:24:59 I don't even think there's contribution notes yet 19:25:03 for release notes 19:25:07 jamesob_: sorry :) 19:25:09 i had to ask promag 19:25:14 wumpus: is that not caught in the PR's travis run? 19:25:27 jnewbery: I think it is 19:26:56 topic suggestion: avoid undefined behaviour when it shouldn't matter? (#12789) 19:26:58 https://github.com/bitcoin/bitcoin/issues/12789 | Dont return a CExtPubKey filled with random data when DecodeExt{Pub,}Key is given input not passing DecodeBase58Check(...) by practicalswift · Pull Request #12789 · bitcoin/bitcoin · GitHub 19:27:11 #topic avoid undefined behaviour when it shouldn't matter? 19:27:18 ryanofsky: why not just create a separated pr editing the release notes after the actual pr doing things has been merged? 19:27:31 "shouldnt" 19:27:36 i bring it up here because it may be something we should or shouldn't have as a guideline 19:28:10 for example, should you initialize a variable that isn't read anywhere, because soke compiler warning fails to understand it isn't being read? 19:28:22 argument in favor: more deterministic failures 19:28:24 oh, well that isnt "shouldnt" 19:28:35 that is "doesnt, but compiler warns" 19:28:38 argument against: reduces the ability for tools to detect things stativally 19:29:03 Other argument in favor: means a linter can catch all uninitialized variables. 19:29:04 well i say shouldn't, because reviewers may be wrong and the compiler may be right 19:29:08 jtimon: that's a possibility too, though like the wiki option it decouples the code change from the release notes change itselff 19:29:25 jtimon: also: EVEN MORE PRs :( 19:29:36 wumpus: yep, although I guess the bigger drawback is more prs 19:29:38 right 19:29:43 I mean if its at all tricky to show that it *wont* be read, then should def follow the compiler, but the nonstop stream of "this compiler is shit and warned on something that it shouldnt be" prs is....not ideal 19:30:02 yeah... 19:30:24 honestly of all those pros/cons, the pr volume is probably the most important imnsho 19:30:26 so many *fix some and some false positive for my crappy static analysis tool/compiler with warnings jacked up* 19:30:37 i generally dislike the "compiler/analyzer/linter/tool doesn't understand X, let's initialize everything to shut it up" 19:30:43 me too 19:30:48 just fix your tools FFS 19:30:59 [13bitcoin] 15MarcoFalke closed pull request #12823: doc: Switch release-notes.md to union merge (06master...06Mf1803-docGitattributes) 02https://github.com/bitcoin/bitcoin/pull/12823 19:31:18 if it's correct, human-understandable C++ code and we know there's no problems with it, it should not be changes because compiler blabla 19:31:31 too risky, too 19:31:31 or improve the code so it is easier for tools (and humans) to see it is correct 19:31:42 if it's not broken don't change it 19:31:47 true 19:31:56 ok, just wanted to hear opinions about this 19:32:11 unless it's a refactor to prepare for osmething else, of course, but that wasn't the premise :) 19:32:51 so I think we agree 19:32:56 yes 19:32:59 any other topics? 19:33:49 BlueMatt: I don't know, will more volume of prs specific to release notes be that much more cumbersome? 19:34:07 jtimon: yes. In that case I prefer the wiki 19:34:10 less so than code-change pr volume 19:34:15 but whatever 19:34:25 wumpus: I agree 19:34:32 that's why we have the wiki-phase at all before releases, to prevent a jungle of update-release-notes PRs 19:34:33 yeah, I mean, I don't have a strong opinion either way 19:34:51 (which will also conflict with each other! though easier to rebase..) 19:35:05 jtimon, imo including release notes along with changes makes changes easier to understand, and also probably more well thought out 19:35:07 yes, it's better than code-change PR volume that's for sure 19:35:17 ryanofsky: hey that's a good point 19:36:19 sipa: sometimes warning are useful, sometimes they are not and it's alright to leave them there. but not sure what the discussion is. nobody is proposing we use -Werror, right? 19:36:33 I remember seeing the 'release notes per item' before in some project, not sure which 19:36:59 ryanofsky: I agree, but then you have to deal with rebases, I don't see a way around it 19:37:24 jtimon: warning being good or evil wasn't what the topic was about 19:37:45 jtimon: my view is (for example) that if you systemativally initialize every variable (even those for which you know won't be used), you will lose the ability for the compiler to give you warnings about accidentially uninitialized things 19:38:05 wumpus: that's what I'm saying, that I'm not sure what the topic is 19:38:15 this is more general than just compiler warnings, and variable initialization though 19:38:29 at ASML we had that as part of the C coding standard - every, single, variable had to be initialized 19:38:41 no I don't think we need that here :) 19:39:44 sipa: yes, i really like newer gcc/clang's ability to warn about being unitialized for one or more paths 19:40:27 I do think all class variables should be initialized in the constructor, in general 19:41:50 wumpus: agreed, but I'd like to start using more c++11 member-initialization for trivial types as it's so much less verbose 19:41:51 cfields: they had that in the static analyzer for quite a while, now it moved to a compiler warning, a good thing 19:42:16 right 19:42:54 cfields: yes, that's a nicer syntax 19:43:43 ok, any other topics? 19:44:23 seems not 19:44:25 #endmeeting