19:00:06 <wumpus> #startmeeting
19:00:06 <lightningbot> Meeting started Thu Jan 31 19:00:06 2019 UTC.  The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:06 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:00:18 <achow101> hi
19:00:21 <jnewbery> hi
19:00:22 <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:00:28 <dongcarl> hi
19:00:29 <meshcollider> hi
19:00:48 <instagibbs> hola
19:00:58 <wumpus> topics?
19:01:01 <jonasschnelli> hi
19:01:15 <promag> hi
19:01:18 <gwillen> buenos dias
19:01:30 <luke-jr> I want to suggest we change rebasing policy/expectations
19:01:33 <promag> boa noite
19:01:34 <sipa> hello, half here
19:01:56 <wumpus> #topic High priority for review
19:02:15 <wumpus> https://github.com/bitcoin/bitcoin/projects/8
19:02:16 <provoostenator> hi
19:02:45 <wumpus> still 7 PRs left, don't think we should add anything
19:03:30 <wumpus> but open to suggestions if there's replacements etc that need to be made
19:03:45 <promag> would appreciate some more feedback on #15153 (and it's dependency)
19:03:48 <gribble> https://github.com/bitcoin/bitcoin/issues/15153 | gui: Add Open Wallet menu by promag · Pull Request #15153 · bitcoin/bitcoin · GitHub
19:04:09 <wumpus> note that tomorrow is strings freeze for 0.18
19:04:14 <jnewbery> promag: I'm going to try to look at that today
19:04:24 <wumpus> and in two weeks the feature freeze
19:05:09 <promag> jnewbery: maybe we should postpone multiwallet gui for 0.19? and maybe backport to 0.18.1?
19:05:09 <sdaftuar> i would like to review beg for #14897 -- in addition to being a useful feature in its own right, it paves the way for several simple transaction download improvements (some of which i'm hoping could land in 0.18)
19:05:12 <gribble> https://github.com/bitcoin/bitcoin/issues/14897 | randomize GETDATA(tx) request order and introduce bias toward outbound by naumenkogs · Pull Request #14897 · bitcoin/bitcoin · GitHub
19:05:26 <jamesob> will take a look today (fwiw)
19:05:40 <wumpus> sdaftuar: that one is already in high prio I think?
19:05:44 <sdaftuar> wumpus: yep
19:06:07 <sdaftuar> i think it's nearly ready (the nits i left on the PR could be dealt with later)
19:06:10 <sdaftuar> but needs more review
19:06:32 <wumpus> okay, great!
19:06:41 <jnewbery> #11082 has required rebase for 10 days and has outstanding review comments from December. Should it be removed from high priority?
19:06:45 <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:07:13 <provoostenator> jnewbery: I don't think it's a good idea to merge that so close to release, as much as I'd like to have it
19:07:34 <provoostenator> Also it doesn't really do anything on its own afaik, and the stuff on top of it isn't ready.
19:07:36 <wumpus> I tend to agree
19:07:43 <wumpus> okay, going to remove it
19:08:02 <provoostenator> As for multiwallet: it would be nice to get opening a wallet in
19:08:33 <provoostenator> But not end of the world if not.
19:08:37 <jnewbery> I agree with trying to get multiwallet open into v0.18
19:08:41 <jonasschnelli> Agree. Open wallet would be nice.
19:08:43 <promag> I know I have one HP PR, but it depends on #15280 so if you don't mind that could be in the list too
19:08:44 <wumpus> #15153 is on the high prio list
19:08:45 <gribble> https://github.com/bitcoin/bitcoin/issues/15280 | gui: Fix shutdown order by promag · Pull Request #15280 · bitcoin/bitcoin · GitHub
19:08:47 <sdaftuar> if we're thinking about pruning from the high priority list to focus on 0.18, then i think #15141 could be removed as well.  it's ready for review but not essential for 0.18 IMO (and maybe we'd want it to simmer in master for longer before a release anyway)
19:08:47 <gribble> https://github.com/bitcoin/bitcoin/issues/15153 | gui: Add Open Wallet menu by promag · Pull Request #15153 · bitcoin/bitcoin · GitHub
19:08:51 <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:09:40 <jamesob> likewise for #15118 if 0.18 is the focus
19:09:43 <provoostenator> wumpus: regarding strings, if some GUI changes misses the string deadline, then that part is just English-only, right?
19:09:43 <gribble> https://github.com/bitcoin/bitcoin/issues/15118 | Refactor block file logic by jimpo · Pull Request #15118 · bitcoin/bitcoin · GitHub
19:09:46 <wumpus> sdaftuar: nah not necessarily! it's just that if PRs have outstanding comments for a long time, and are not being updated, there's not that much urgency apparently
19:09:47 <provoostenator> Or does it explode?
19:10:16 <wumpus> provoostenator: idieally it would be avoided completely, but yes that's the effect
19:10:34 <wumpus> provoostenator: I'm okay with *adding* strings after that if we can't avoid it, but not changing them
19:11:11 <sdaftuar> wumpus: ok happy to keep it on there too, it's holding up other work i have going on!  we can revisit as we get closer to feature freeze i guess
19:11:15 <wumpus> (e.g. no "improve wording" PRs)
19:11:56 <wumpus> but yeah it's good to give the translators some time
19:13:49 <wumpus> ok, that concludes the topic I think
19:14:09 <wumpus> #topic rebasing policy/expectations (luke-jr)
19:14:47 <promag> we don't require rebase do we?
19:14:57 <luke-jr> a lot of time seems wasted on rebasing needlessly; I'd like to suggest we only expect rebasing when there's a major conflict, or the PR is literally about to be merged
19:15:30 <luke-jr> promag: apparently some people consider it a show-stopper on progress for PRs if it "needs" rebase
19:16:14 <wumpus> people usually want to test PRs on top of master, which is not straightforward if they need rebase, but yea for review it shouldn't strictly be necessary
19:16:32 <wumpus> it's up to you really
19:16:40 <promag> I think that's "requested" after trivial review
19:17:07 <meshcollider> Rebase can often partially invalidate reviews anyway unless its trivial in which case theres no point not doing it
19:17:17 <provoostenator> Lack of rebase normally won't stop me from reviewing, unless I expect a problem.
19:17:17 <wumpus> though everything that makes people more willing to review your PR might be welcome given how many there are ...
19:17:32 <provoostenator> Maybe though we need additional tag "Really Needs Rebase"
19:17:46 <wumpus> I had it under 242 some weeks ago but ugh
19:17:58 <luke-jr> well, we have a bot more recently that closes stuff and nags over even trivial rebases
19:18:23 <provoostenator> Ah that's a good point, maybe Drahtbot should be less aggressive in that regard.
19:18:32 <provoostenator> Only close if Really Needs Rebase is set? :-)
19:18:46 <wumpus> drahtbot doesn't close PRs
19:18:54 <jnewbery> I think Drahtbot only closes if a PR has needed rebase for a _really_ long time
19:19:08 <achow101> drahtbot will close and reopen PRs to retrigger travis
19:19:13 <wumpus> it only adds a label "needs rebase" and posts a message in that regard
19:19:14 <jamesob> doesn't drahtbot only ask for a rebase if there are conflicts?
19:19:23 <meshcollider> It does after like a very long time and tags it with up for grabs
19:19:32 <meshcollider> Close PRs ^
19:19:35 <wumpus> meshcollider: I don't think it does so automatically
19:19:52 <wumpus> or at least I've never noticed
19:20:04 <jamesob> drahtbot does close and mark up-for-grabs, e.g. #13200
19:20:04 <jnewbery> example: https://github.com/bitcoin/bitcoin/pull/12965#issuecomment-423611058
19:20:08 <luke-jr> part of my motivation for bringing this up, is that (without naming names) we've apparently lost devs in part over the constant rebasing
19:20:08 <gribble> https://github.com/bitcoin/bitcoin/issues/13200 | Process logs in a separate thread by jamesob · Pull Request #13200 · bitcoin/bitcoin · GitHub
19:20:09 <promag> FWIW I don't mind rebasing
19:20:10 <meshcollider> Marco runs an extra script occasionally I think
19:20:17 <wumpus> though if it's after a very long time I don't mind ...
19:20:19 <provoostenator> I've seen it happen a few times as well
19:20:31 <wumpus> there's probaly quite a few PRs that could be closed
19:20:41 <provoostenator> But often the problem isn't a lack of rebase, it's either a lack of feedback or a lack of addressing feedback.
19:20:47 <wumpus> sometimes I just want to close them all and start anew xD
19:21:03 <sdaftuar> luke-jr: i agree with the sentiment you bring up, but its unclear to me how much of the irritation is from being nagged about rebasing, versus the repo activity that is requiring so many rebases in the first place
19:21:19 <jnewbery> wumpus: I agree. If something's needed rebase for > 6 months then it's clearly not a priority for the contributor. It can always be re-opened if it becames a priority
19:21:29 <jonasschnelli> also,... there are some PR not meant to be merged (WIP / Experimental)
19:21:46 <wumpus> jonasschnelli: I think that's great
19:21:59 <provoostenator> jnewbery: not necessarily, there's no point in rebasing if you're not getting enough concept ACK and agreement on technical direction
19:21:59 <wumpus> (if they're clearly marked as that)
19:22:05 <jonasschnelli> Yes. Some PR are to attract developers and spun up new ideas
19:22:09 <provoostenator> So it could be a priority for the developer, just not for the reviewers.
19:23:05 <jnewbery> provoostenator: I think Draht only closes if there's been no activity at all. If you're not getting _any_ interaction from other contributors then again, it's probably not a priority for anyone
19:23:07 <wumpus> so it's a pretty busy repository and a lot of changes are happening, there's nothing to be done about that, it's the same for other popular open source projects
19:23:53 <provoostenator> Ok, if _any_ activity is fine, then it's not unreasonable to expect e.g. the contributor themselves to give a quick "anybody out there?" update.
19:24:05 <luke-jr> wumpus: I think it would help, if maintainers indicated they're prepared to merge a PR, and it got rebased specifically for the merge. (assuming reviewers continue to review despite trivial rebases)
19:24:06 <wumpus> provoostenator: yes, that's fine!
19:24:10 <achow101> the only issue i have with rebases is that someone would comment "needs rebase", the author rebases, and then receives no reviews until the next "needs rebase" comment from someone
19:24:25 <wumpus> it's *really* common for a PR to be waiting for *any* response from the author to comments
19:24:37 <wumpus> even if that's "I prefer not to address this as it's out of scope"
19:24:49 <wumpus> but you need to reply to comments
19:24:56 <provoostenator> Yes, one thing that might help is if people are less shy to just take over PRs.
19:25:17 <provoostenator> achow101 took one over from me pretty quickly, changing it somewhat, which is great, saves me work.
19:25:33 <wumpus> no one is going to merge a PR with un-addressed comments
19:26:28 <wumpus> I mean we can prod people to review all we want, if reviews just go ignored there's no point
19:26:37 <provoostenator> A slightly less drastic alternative to opening an alternative PR is to link to a branch in the comments, but branches are not easy to review.
19:27:02 <provoostenator> Github, if you're listening, you should add a PR fork feature :-)
19:27:09 <wumpus> heh
19:27:27 <luke-jr> it'd be nice if the PR # could stay the same with multiple contributors :P
19:27:40 <jnewbery> Speaking from personal experience, I've never had much of an issue with rebases. I only ever have a handful of PRs open at maximum, so rebasing isn't too onerous. I think it only becomes a problem if you have a lot of open PRs
19:28:11 <jamesob> rebasing should be MORE onerous ;)
19:28:13 <wumpus> it also depends on the kind of PR, if you only have localized changes it's not too bad
19:28:45 <sdaftuar> some PRs are definitely a pain to rebase
19:28:46 <provoostenator> It gets exponentially bad if you build multiple PRs on top of each other.
19:28:54 <wumpus> also means you need to rebase less often because there's less chance of it colliding with other changes
19:29:02 <wumpus> avoid change-all-over-the-place PRs
19:29:52 <jnewbery> provoostenator - shouldn't be exponential. In my experience it's sub-linear if the PRs are a series because rebasing the first is often the only actual work.
19:30:23 <jnewbery> (again, just personal experience. Yours may vary!)
19:30:25 <luke-jr> I think I have a tendency to rebase once per release, and on the rare occasion someone pings me for a merge
19:31:44 <jonasschnelli> A rebase quick before a merge is dangerous IMO...
19:31:48 <MarcoFalke> I agree with what meshcollider said earlier: [14:17] <meshcollider> Rebase can often partially invalidate reviews anyway unless its trivial in which case theres no point not doing it
19:31:48 <jonasschnelli> we had this in the past
19:31:48 <provoostenator> It might just be my lacking git skills, still haven't found an optimal way to say "start with master, add branch X, then branch Y, then rebase the new stuff on the current branch"
19:32:17 <jnewbery> I don't think this is actually a project-wide policy. I personally tend not to review PRs that have needed rebase for a long time because: - it signals that the contributor may not actively be working on the PR; - my review will be invalidated by the rebase anyway.
19:32:19 <jonasschnelli> Constant rebasing is part of QA (rebased versions gets reviews)... and a sadly necessary IMO
19:32:25 <MarcoFalke> Indeed, rebases often go wrong (as in the conflict is solved in the wrong way)
19:33:04 <provoostenator> I often check rebases with:  PREV=... N=... && git range-diff `git merge-base --all HEAD $PREV`...$PREV HEAD~$N...HEAD
19:33:18 <provoostenator> Where PREV is the last thing I acked, and N is the number of commits in the branch
19:33:23 <jonasschnelli> Also, constant rebasing makes you also up do date with changes around your code (which you otherwise would miss) *duck*
19:33:52 <wumpus> jnewbery: I agree
19:34:50 <wumpus> and yes, this isn't a project-wide policy, but one that every reviewer determines for themselves, how do you consider what to review?
19:34:52 <provoostenator> Drahtbot might be able to help verify that a straight rebase is just that, maybe saying something like "existing ACK ... is the same as [new hash] rebased"
19:35:18 <wumpus> I don' think we can make much progress here, besides knowing each other's perspective
19:35:25 <MarcoFalke> Also, DrahtBot will list all conflicts with other pull requests, so it should give some idea what best to review first or what to prioritize (or for the author) if it might help to split up the pull request into smaller changes
19:35:34 <luke-jr> provoostenator: that's trusting the bot a bit too much
19:36:08 <MarcoFalke> provoostenator: We don't need a rebase if it is a effective "fast forward"
19:36:15 <provoostenator> I didn't say "trust the bot"
19:36:43 <MarcoFalke> Either the bot tracks it as conflict or it doesn't need rebase
19:36:47 * sipa has little opinion
19:37:04 <wumpus> we don't have any other topics do we :<
19:37:24 <gwillen> provoostenator: I can probably answer git questions on "how to convince it to do X", given some details
19:37:36 <jnewbery> I'd like to quickly mention the residency again, but only at the end if we don't have other topics
19:37:49 <wumpus> PSA: release schedule for 0.18.0: https://github.com/bitcoin/bitcoin/issues/14438
19:38:05 <promag> even a straight rebase can result in broken travis
19:38:14 <wumpus> feature freeze is in roughly two weeks! hurry up :<
19:38:20 * sdaftuar types faster
19:38:23 <wumpus> hehe
19:38:56 <wumpus> March 1 is planned branch split-off
19:40:21 <wumpus> #topic Chaincode residency (jnewbery)
19:40:50 <jnewbery> Thanks wumpus. A couple of things: 1. I emailed a bunch of you to ask about mentorship. Thank you to everyone who replied!
19:41:37 <jnewbery> 2. we officially announced the residency today. We're still looking for great engineers who want to spend summer working on Bitcoin or Lightning with us. If you know anyone who might be appropriate, please send them our way at https://residency.chaincode.com/#apply
19:41:44 <jnewbery> (endtopic)
19:42:34 <wumpus> thanks!
19:43:36 <wumpus> anyone with other topics ?
19:45:29 <wumpus> #endmeeting