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