15:01:02 <jnewbery> #startmeeting 
15:01:02 <core-meetingbot> Meeting started Tue Dec 15 15:01:02 2020 UTC.  The chair is jnewbery. Information about MeetBot at https://bitcoin.jonasschnelli.ch/ircmeetings.
15:01:02 <core-meetingbot> Available commands: action commands idea info link nick
15:01:09 <jnewbery> #bitcoin -core-dev Meeting: achow101 aj amiti ariard bluematt cfields Chris_Stewart_5 digi_james dongcarl elichai2 emilengler fanquake fjahr gleb gmaxwell gwillen hebasto instagibbs jamesob jb55 jeremyrubin jl2012 jnewbery jonasschnelli jonatack jtimon kallewoof kanzure kvaciral lightlike luke-jr maaku marcofalke meshcollider michagogo moneyball morcos nehan NicolasDorier paveljanik petertodd
15:01:12 <gleb> Hi
15:01:15 <jnewbery> phantomcircuit promag provoostenator ryanofsky sdaftuar sipa vasild wumpus
15:01:22 <jonatack> hi
15:01:30 <ariard> hi
15:01:42 <aj> hey
15:01:44 <ajonas> Hi
15:01:52 <jnewbery> hi all. Welcome to the last p2p meeting of 2020!
15:01:58 <fanquake> hi
15:02:25 <jnewbery> We have one proposed topic: Review of the P2P Review Process (ariard). Before we get into that, does anyone have any proposed topics that they want to add?
15:03:23 <jnewbery> ok, onto our one topic
15:03:33 <jnewbery> #topic Review of the P2P Review Process (ariard)
15:03:33 <core-meetingbot> topic: Review of the P2P Review Process (ariard)
15:03:39 <ariard> hi
15:04:18 <ariard> so as it's the last p2p meeting of 2020, it's a great opportunity to review our p2p review process
15:05:05 <ariard> so I just have an opening question and let's discuss from it
15:05:27 <ariard> which PR reviews stand-out as productive and which were productive ?
15:05:37 <ariard> is there anything we can learn from these examples
15:05:50 <ariard> I did attach some past PRs on the wiki page : https://github.com/bitcoin-core/bitcoin-devwiki/wiki/P2P-IRC-meetings
15:06:23 <ariard> let's start by a roundtable, can anyone tell one PR review liked on the past year :) ?
15:06:45 <gleb> I think the topic is worthy but it’s much broader then what we can discuss here in tea time, although gathering initial thoughts might work
15:07:00 <gleb> in real time*
15:07:20 <ariard> gleb: that's fine start by sharing your initial thoughts
15:07:33 <MarcoFalke> So I think the review process hasn't changed fundamentally in the past years. We made some minor changes to ACKs (allowing them to be more verbose, Approach, Concept, ...), also there is a REVIEWERS file where people can sign up for notifications if their watched file changes...
15:08:17 <MarcoFalke> Though, based on the blog post by ajonas many raised a concern that review isn't focussed enough.
15:08:18 <gleb> I haven’t really noticed REVIEWERS file in action yet, although I liked the concept
15:08:36 <MarcoFalke> I see that too, escpecially on larger pulls.
15:08:41 <kanzure> there was a recent twitter survey asking whether reviews feel different lately. an actual non-twitter survey might be helpful.
15:08:58 <ariard> MarcoFalke: which blog post? didn't read it
15:09:01 <gleb> kanzure: ack but asking the right question matters
15:09:16 <jonatack> ariard: in general, i can think of many outstanding reviews (and there are some outstanding reviewers). Coverage of great reviews was actually proposed one year ago for Bitcoin Optech: https://github.com/bitcoinops/bitcoinops.github.io/issues/301
15:09:17 <MarcoFalke> ariard: kanzure: ajonas did a writeup on a 2019 survey
15:09:30 <MarcoFalke> was shared in the last general meeting
15:09:38 <ariard> ah this one, okay
15:09:54 <ajonas> https://adamjonas.com/bitcoin/coredev/retro/coredev-2019-retro/
15:10:34 <MarcoFalke> So I think it would be good if there was a signal where a contributor could simply express interest in doing a review (at a later time)
15:10:58 <jonatack> ariard: I have a short exposition on this topic that I wrote in reply to your question yesterday, can present it here after the meeting or maybe a as twitter thread
15:11:19 <ariard> jonatack: yes I feel sometimes it's hard to have a clear overview of what's the blocker for the PR (waiting author, dependency, moar-concept-ack, metrics, ...)
15:11:19 <MarcoFalke> Then, it would be easier for maintainers to gather how much review interest there is and based on that ping people at around the same time to start digging into the review
15:12:30 <ariard> MarcoFalke: agree, 19858 is a good example, it's a PR with a lot of context and you want to avoid wasting review time if your the only one doing a review for the coming month
15:12:38 <MarcoFalke> It could be as simple as saying "Concept ACK (willing to upgrade to review ACK)"
15:12:38 <ariard> a recent good example
15:12:43 <MarcoFalke> #19858 
15:12:47 <gribble> https://github.com/bitcoin/bitcoin/issues/19858 | Periodically make block-relay connections and sync headers by sdaftuar · Pull Request #19858 · bitcoin/bitcoin · GitHub
15:12:51 <jnewbery> MarcoFalke: for myself, if I've concept ACKed a PR, then it almost always means that I intend to review the code later (and would be receptive to the author/maintainer pinging me if I hadn't)
15:13:34 <aj> personally, i'm bothered by https://github.com/bitcoin/bitcoin/pull/20599#issuecomment-741631081 ; my opinion is that we should be thoroughly reviewing all p2p changes because there's potential for interactions, so going easy on "boring" PRs seems very unsafe. beyond that, there's a lot of not very beneficial refactoring PRs, that are also causing extra dev and review rework on PRs that change the
15:13:34 <aj> code's behaviour
15:13:37 <MarcoFalke> jnewbery: I think we can't assume that generally. Often I say Concept ACK, but I mean "Concept ACK, but I am not confident in reviewing this or I don't have the motivation"
15:14:56 <MarcoFalke> ariard: Good example. The pull had 4 ACKs at some point, but was merged with less. I think it was a bit unclear to maintainers if more people are interested in testing/reviewing or if the review motivation has been used up already on that pull
15:15:40 <ariard> aj: agree and you have to spend a lot of time just to have confidence you have you don't have interactions, even slight ones
15:16:25 <ajonas> I think encouraging opinions often and early is the main goal of a concept/approach ACK. People may have varying interest in following up as the PR moves into a lower-level kind of review.
15:16:32 <jnewbery> MarcoFalke: I think that's fair. My default (concept ACK implies a later review ACK and explicitly say "I won't review this later" if you don't intend to review later) is perhaps the opposite from other people.
15:16:40 <ariard> I do feel sometimes stack small refactor in bigger PR, even with bigger diff might be actually to review, because less cognitive spreading to reload the code model each time
15:17:45 <ariard> MarcoFalke: yes I think that's a good initiative for maintainers to ping/engage past reviewers if they're still interested to review  again
15:18:04 <MarcoFalke> aj: Also a good point. I think it would be easier for maintainers to know how much resistance there is if NACKs or ~mehs were thrown out more agressively
15:18:16 <aj> ariard: i think limiting refactors to ones that are immediately useful as part of either implementing a feature, fixing a bug or improving automated tests would be a huge improvement. (or perhaps as followups to a PR that's hit its preserve-acks-no-more-nits limit)
15:18:43 <MarcoFalke> I have the feeling that many shy away from a NACK or -0, but that shouldn't be
15:18:44 <jonatack> ariard: i agree (and do that) but bigger PRs scare off reviewers and it doesn't always work out. knowing how much to put in a PR and how much to leave out is the hardest part for me TBH
15:19:17 <jnewbery> aj: 20599 was thoroughly reviewed. It had 5 ACKs
15:20:39 <ariard> aj: yes what we might have to weight more carefully is the "immediately" I feel for some features you will have to stack multiple refactors before to have the ground ready for new stuff, e.g mempool
15:20:56 <aj> ariard: immediately == different commits in the same PR
15:21:15 <aj> ariard: or, like taproot, commits pulled out of a large PR that's already open and available for review
15:21:36 <ariard> aj: okay but not preparatory-PR-for-some-future-feature, like sdaftuar did for motivating workspace in mempool a back while?
15:22:14 <aj> ariard: the package relay stuff? it had a concurrent PR implementing packages via orphans?
15:22:54 <ariard> aj: this one https://github.com/bitcoin/bitcoin/pull/16400
15:23:16 <ariard> which was a substantial diff
15:23:21 <aj> ariard: yeah, it had #16401
15:23:26 <gribble> https://github.com/bitcoin/bitcoin/issues/16401 | Add package acceptance logic to mempool by sdaftuar · Pull Request #16401 · bitcoin/bitcoin · GitHub
15:24:12 <ariard> jonatack: yeah I've a preference for a bit more substantial PR but it's so much tied to everyone review workflow
15:24:29 <ariard> and past experience with a part of the codebase
15:24:29 <jonatack> MarcoFalke: one thing it might be helpful to have signals from maintainers about, is if follow-ups are desired for the review comments that weren't taken or for missing coverage, e.g. 19858 would be an example
15:25:00 <jonatack> ariard: yes
15:26:22 <ariard> aj: my point is sometimes it can be hard to justify one-individual-refactor but they make sense when you zoom out or point to following PR
15:26:41 <ariard> like if we move towards isolating block-relay from tx-relay, it won't happen with one big PR?
15:26:43 <MarcoFalke> jonatack: As a maintainer I try to keep those decicions up to the reviewers/other contributors. I don't want to "rule" the project too much.
15:26:57 <jonatack> MarcoFalke: that's fair
15:27:47 <ariard> jonatack: I do feel we could track follow-ups better, most of the time it's not they're relevant, it's just to much changes for a PR or the PR is already super mature
15:27:56 <ariard> GH doesn't have a follow-ups pinning board?
15:27:57 <jonatack> ariard: I also like PRs that do things well, even if that makes them more substantial...not sure if I would have said the same a year ago, though
15:28:40 <ariard> jonatack: agree to have them strong test coverage, but when it's code style or slight features changes the list of follow-ups might be infinite?
15:28:42 <gleb> ariard: explicitly motivating refactors with some future work would help.
15:28:48 <aj> ariard: if you can point to a following PR, include the refactor in that PR
15:30:36 <ariard> gleb: yes it sounds tied to context-tracking, maybe we could improve it with the wiki? it's widely used to advertise state of the ongoing work around i2p?
15:31:21 <jonatack> ariard: yes, in practice i mostly leave it to the PR author to follow-up or not, there are always other priorities to do and review
15:31:33 <aj> ariard: isn't creating an issue the obvious thing to do if a PR needs followups and you can't just create the followup PR straight away?
15:31:36 <jonatack> that's probably what most do
15:31:38 <gleb> ariard: I think even just mentioning it in the first PR comment would help. Wiki stuff may be too much.
15:32:57 <ariard> jonatack: right I think it's the job of the reviewer to clearly say when comments are blockers or nice-to-follow-ups
15:33:15 <gleb> aj: I think creating issues for follow-ups might be a good idea.
15:34:31 <ariard> aj: well GH issue have the concern they're great to discuss but not really to sum up the state of a discussion IMO?
15:34:57 <ariard> specially for contributors onboarding on the state of an ongoing work and willingly to participate
15:35:38 <jonatack> an issue to centralise the tracking of the effects of 19858 seems like a good idea, for example
15:35:39 <aj> ariard: edit the first comment to maintain a summary?
15:35:52 <ariard> what people feeling about prepatory document like jamesob did for assumeutxo a while back ?
15:35:59 <jonatack> resource usage, peer rotation, the bug aj reported, etc.
15:36:22 <jnewbery> I personally wish that we'd not need so many follow-up PRs and just get things right the first time
15:36:31 <jonatack> jnewbery: agreed
15:37:05 <jnewbery> The attitude of "we know that there are problems with this but we'll merge it now and fix it up later" doesn't seem right for this project
15:37:24 <vasild> hi
15:37:27 <ariard> jnewbery: ideally but getting the "things right", each contributor has a different opinion because different priorities/concerns?
15:37:44 <jamesob> hi
15:37:48 <jonatack> jnewbery: that was unique to this project for me, it seems related to the review bottleneck / preservation of review
15:38:41 <ariard> aj: yes sound a best practice, and lighter than pinning stuff in wiki
15:39:31 <MarcoFalke> I think if something is moving in the wrong direction, we should hold back on merging it, but if there are issues unrelated or tangential to the change that may have existed previously or may be controversial to change, it is up to the pull author to address them or not
15:39:51 <jonatack> new people take a long time to become experienced reviewers, and it the meantime the increased activity structurally increases the bottleneck
15:41:43 <ariard> that kind of prepatory document: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal, I think amiti did the same for mempools reworks, and IMO it's pretty cool to get the rational of some proposed changes, including contra opinions
15:42:23 <aj> jnewbery: imo review time is the most constrained resource and we should be optimising for it; a big change and a small followup is better than multiple reworks of a big change; likewise having a big change merged quicker, so that it doesn't end up conflicting with other changes, thus requiring further re-review
15:43:31 <MarcoFalke> Adding a new commit to a pull that has reviews might better be done in a new pull, because it doesn't invalidate existing review . Though, if one of the commits in the pull is moving in the wrong direction, it might be better to fix it up if everyone agrees.
15:43:52 <ariard> MarcoFalke: obviously, if it's introducing flagrant bugs or security concerns we should hold it, but when it's code structure where people have different tolerances harder to come to a consensus...
15:44:15 <jnewbery> I think #16702 is a very good example of people saying "let's merge this now and fix in follow-ups". There have been at least 8 PRs to fix up the under-reviewed changes, and there are still outstanding bugs over a year later
15:44:18 <gribble> https://github.com/bitcoin/bitcoin/issues/16702 | p2p: supplying and using asmap to improve IP bucketing in addrman by naumenkogs · Pull Request #16702 · bitcoin/bitcoin · GitHub
15:44:36 <jonatack> aj: agree, this all relates back to that constraint
15:44:44 <jnewbery> That's not saving review time. It would have been much more efficient to just review it properly the first time round
15:45:01 <jonatack> so if we can move to loosen it, we can do better in the direction jnewbery is pointing out
15:45:54 <gleb> My intuitiion was that follow-ups are often less important than the core part of the PR, that's why focusing on the core part made sense.
15:46:58 <luke-jr> jnewbery: +1
15:47:44 <jonatack> i also think that pairing 2 complementary people to work on some PRs together, pre-review, could be beneficial
15:48:23 <jonatack> on an ad-hoc, informal basis, but worth exploring
15:48:58 <jnewbery> it was under-reviewed and broke peers.dat serialization for two releases. Reviewers and the PR author were urging the maintainers to merge now and fix issues in follow-ups. I don't understand why there was such a rush to get it merged.
15:49:03 <vasild> jonatack: +1
15:49:24 <ariard> jonatack: +1, specially on the complementarity
15:49:53 <MarcoFalke> jnewbery: I also agree, but I think that means that reviewers should be shy in giving out ACKs and comfortable in giving out NACKs
15:49:57 <MarcoFalke> Another example is #19569
15:50:02 <gribble> https://github.com/bitcoin/bitcoin/issues/19569 | Enable fetching of orphan parents from wtxid peers by sipa · Pull Request #19569 · bitcoin/bitcoin · GitHub
15:50:14 <MarcoFalke> It was ACKed despite a known remote crasher bug
15:50:18 <MarcoFalke> (and merged)
15:50:24 <luke-jr> surprised explicit feerates hasn't been mentioned XD
15:51:08 <aj> MarcoFalke: huh? the remote crasher bug was known before it was merged?
15:51:30 <MarcoFalke> aj: Oh it wasn't? Maybe I missed that
15:52:02 <MarcoFalke> I presumed it was based on this comment: https://github.com/bitcoin/bitcoin/pull/19569#discussion_r462887483
15:52:08 <ariard> there is also a point when some PRs are approaching the upper bound of review ability like #19988, at least at some point you feel you won't provide anymore review value
15:52:11 <gribble> https://github.com/bitcoin/bitcoin/issues/19988 | Overhaul transaction request logic by sipa · Pull Request #19988 · bitcoin/bitcoin · GitHub
15:52:47 <jnewbery> I thought it was possible that there was a remote crash bug, but hadn't identified how to exploit it
15:53:16 <aj> MarcoFalke: i thought it was "here's a cleanup we should do in a followup" ... "oops, that cleanup fixes a crash"
15:53:24 <jnewbery> I offered a fix to make it safer, and the response was "let's fix it separately"
15:53:33 <MarcoFalke> ariard: I think it is still good to review, and maybe clarify that it is a "weaker" review
15:54:41 <ariard> jnewbery: when you feel a PR is under-reviewed but still conceptually agree with it, I think a worthy contribution it's to point out the PR doesn't meet project standards on test coverage, code style, documentation and point to good past examples?
15:55:24 <luke-jr> ariard: by the time it matters, it's merged :/
15:55:34 <jnewbery> ariard: those suggestions are often dismissed as 'pointless refactors' or 'nits'
15:55:56 <ariard> luke-jr: I mean when the PR isn't merged yet but you don't have the time to review it now but still want to inform about your opinion?
15:56:26 <luke-jr> ariard: we'd be posting "this is underreviewed" constantly, because we don't know when some merger might not recognise it?
15:57:14 <gleb> It's hard for me to imagine how one can call a suggestion to improve the safety a "pointless refactor". One is a behavior change, the other is not.
15:57:59 <ariard> jnewbery: IMO increasing project standards is something you do over time and not overnight, so yes you might have to constantly make the point until other contributors share them?
15:58:01 <vasild> Is anybody interested in discussing I2P connectivity?
15:58:44 <jonatack> vasild: sure
15:58:48 <ariard> luke-jr: yeah andGH sucks to clearly gather comment or identify ACK/NACKs, it could be its own feature, not regular comments?
15:58:51 <jonatack> (after the topic)
15:59:00 <vasild> ok, lets discuss after the meeting
15:59:25 <MarcoFalke> luke-jr: I think a good metric to see if something is close to merge is to look at the existing ACKs. If there is worry that something gets merged "underreviewed" with ACKs, you best leave a comment saying that
16:00:07 <jnewbery> ok, that's time folks
16:00:21 <ariard> thanks for participating :)
16:00:34 <MarcoFalke> ariard: Thanks for the topic suggestion
16:00:47 <jonatack> ariard, you asked yesterday how to qualify if the review bottleneck is improving
16:00:48 <jnewbery> Reminder: that was the last p2p meeting this year. We'll meet again on 12-Jan-2021
16:01:03 <jnewbery> #endmeeting