< bitcoin-git>
bitcoin/master 33d6337 Jonas Schnelli: Merge bitcoin-core/gui#115: Replace "Hide tray icon" option with positive ...
< jonasschnelli>
yanmaani: GetBlocksDir() in system.cpp
< yanmaani>
ah, thanks
< jnewbery>
andytoshi: if you're mining to the wallet's address, you could call generate(), get the block hash, fetch the coinbase txid, and then wait_until() the wallet's listunspent includes that transaction output
< wumpus>
luke-jr: qthreadpoolthread is used with qthreadpool, which is used by qconcurrent (high level concurrency framework), neither of which we AFAIK currently use... even then, 64 seems excessive, but i guess they didn't really think about the scenario with so many cores. They should all be constantly sleeping so I guess it's not much overhead besides some memory
< wumpus>
using QRunnable for some things sounds interesting
< jnewbery>
We won't have our normal meeting in two weeks' time (29 Dec), so this is the last p2p meeting of the year. The next will be on 12 Jan 2021.
< jnewbery>
hi all. Welcome to the last p2p meeting of 2020!
< fanquake>
hi
< 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?
< jnewbery>
ok, onto our one topic
< jnewbery>
#topic Review of the P2P Review Process (ariard)
< core-meetingbot>
topic: Review of the P2P Review Process (ariard)
< ariard>
hi
< ariard>
so as it's the last p2p meeting of 2020, it's a great opportunity to review our p2p review process
< ariard>
so I just have an opening question and let's discuss from it
< ariard>
which PR reviews stand-out as productive and which were productive ?
< ariard>
is there anything we can learn from these examples
< ariard>
let's start by a roundtable, can anyone tell one PR review liked on the past year :) ?
< 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
< gleb>
in real time*
< ariard>
gleb: that's fine start by sharing your initial thoughts
< 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...
< MarcoFalke>
Though, based on the blog post by ajonas many raised a concern that review isn't focussed enough.
< gleb>
I haven’t really noticed REVIEWERS file in action yet, although I liked the concept
< MarcoFalke>
I see that too, escpecially on larger pulls.
< kanzure>
there was a recent twitter survey asking whether reviews feel different lately. an actual non-twitter survey might be helpful.
< ariard>
MarcoFalke: which blog post? didn't read it
< gleb>
kanzure: ack but asking the right question matters
< 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
< MarcoFalke>
ariard: kanzure: ajonas did a writeup on a 2019 survey
< MarcoFalke>
was shared in the last general meeting
< 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)
< 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
< 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, ...)
< 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
< 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
< MarcoFalke>
It could be as simple as saying "Concept ACK (willing to upgrade to review ACK)"
< 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)
< 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
< aj>
code's behaviour
< 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"
< 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
< 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
< 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.
< 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.
< 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
< 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
< 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
< 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)
< MarcoFalke>
I have the feeling that many shy away from a NACK or -0, but that shouldn't be
< 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
< jnewbery>
aj: 20599 was thoroughly reviewed. It had 5 ACKs
< 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
< aj>
ariard: immediately == different commits in the same PR
< aj>
ariard: or, like taproot, commits pulled out of a large PR that's already open and available for review
< ariard>
aj: okay but not preparatory-PR-for-some-future-feature, like sdaftuar did for motivating workspace in mempool a back while?
< aj>
ariard: the package relay stuff? it had a concurrent PR implementing packages via orphans?
< ariard>
jonatack: yeah I've a preference for a bit more substantial PR but it's so much tied to everyone review workflow
< ariard>
and past experience with a part of the codebase
< 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
< jonatack>
ariard: yes
< 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
< ariard>
like if we move towards isolating block-relay from tx-relay, it won't happen with one big PR?
< 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.
< jonatack>
MarcoFalke: that's fair
< 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
< ariard>
GH doesn't have a follow-ups pinning board?
< 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
< 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?
< gleb>
ariard: explicitly motivating refactors with some future work would help.
< aj>
ariard: if you can point to a following PR, include the refactor in that PR
< 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?
< 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
< 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?
< jonatack>
that's probably what most do
< gleb>
ariard: I think even just mentioning it in the first PR comment would help. Wiki stuff may be too much.
< ariard>
jonatack: right I think it's the job of the reviewer to clearly say when comments are blockers or nice-to-follow-ups
< gleb>
aj: I think creating issues for follow-ups might be a good idea.
< 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?
< ariard>
specially for contributors onboarding on the state of an ongoing work and willingly to participate
< jonatack>
an issue to centralise the tracking of the effects of 19858 seems like a good idea, for example
< aj>
ariard: edit the first comment to maintain a summary?
< ariard>
what people feeling about prepatory document like jamesob did for assumeutxo a while back ?
< jonatack>
resource usage, peer rotation, the bug aj reported, etc.
< jnewbery>
I personally wish that we'd not need so many follow-up PRs and just get things right the first time
< jonatack>
jnewbery: agreed
< 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
< vasild>
hi
< ariard>
jnewbery: ideally but getting the "things right", each contributor has a different opinion because different priorities/concerns?
< jamesob>
hi
< jonatack>
jnewbery: that was unique to this project for me, it seems related to the review bottleneck / preservation of review
< ariard>
aj: yes sound a best practice, and lighter than pinning stuff in wiki
< 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
< jonatack>
new people take a long time to become experienced reviewers, and it the meantime the increased activity structurally increases the bottleneck
< 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
< 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.
< 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...
< 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
< 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
< jonatack>
aj: agree, this all relates back to that constraint
< jnewbery>
That's not saving review time. It would have been much more efficient to just review it properly the first time round
< jonatack>
so if we can move to loosen it, we can do better in the direction jnewbery is pointing out
< 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.
< luke-jr>
jnewbery: +1
< jonatack>
i also think that pairing 2 complementary people to work on some PRs together, pre-review, could be beneficial
< jonatack>
on an ad-hoc, informal basis, but worth exploring
< 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.
< vasild>
jonatack: +1
< ariard>
jonatack: +1, specially on the complementarity
< 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
< 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
< jnewbery>
I thought it was possible that there was a remote crash bug, but hadn't identified how to exploit it
< aj>
MarcoFalke: i thought it was "here's a cleanup we should do in a followup" ... "oops, that cleanup fixes a crash"
< jnewbery>
I offered a fix to make it safer, and the response was "let's fix it separately"
< MarcoFalke>
ariard: I think it is still good to review, and maybe clarify that it is a "weaker" review
< 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?
< luke-jr>
ariard: by the time it matters, it's merged :/
< jnewbery>
ariard: those suggestions are often dismissed as 'pointless refactors' or 'nits'
< 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?
< luke-jr>
ariard: we'd be posting "this is underreviewed" constantly, because we don't know when some merger might not recognise it?
< 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.
< 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?
< vasild>
Is anybody interested in discussing I2P connectivity?
< jonatack>
vasild: sure
< ariard>
luke-jr: yeah andGH sucks to clearly gather comment or identify ACK/NACKs, it could be its own feature, not regular comments?
< jonatack>
(after the topic)
< vasild>
ok, lets discuss after the meeting
< 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
< jnewbery>
ok, that's time folks
< ariard>
thanks for participating :)
< MarcoFalke>
ariard: Thanks for the topic suggestion
< jonatack>
ariard, you asked yesterday how to qualify if the review bottleneck is improving
< jnewbery>
Reminder: that was the last p2p meeting this year. We'll meet again on 12-Jan-2021
< jonatack>
ariard: 2 reasonable rough proxies might be: open issues and pulls, and distribution of funding
< luke-jr>
MarcoFalke: that might work
< jonatack>
Open pull requests saw new records during the past year
< jonatack>
reaching a peak in late May or early June iirc and staying roughly stable at that level since
< jonatack>
Funding-wise, 2020 was a banner year, and yet at the same time a half-dozen good reviewers faced funding cuts
< ariard>
jonatack: I'm a bit skeptical sometimes about pure quantiative metrics like "open issues" obviously they're likely a good sign but doesn't indicate if the underlying discussion are better, sane, productive, ...
< jonatack>
ariard: yes, I can't say if experienced and consistent review/testing are decreasing
< jonatack>
or are steady or increasing but just not keeping up with the increased activity
< luke-jr>
FWIW, I think the number of upstream bugs I find rebasing Knots has been increasing
< jonatack>
It takes time to develop experienced reviewers on this project
< luke-jr>
but it's not something I actively try to count, and perception is hard
< jonatack>
so structurally it makes sense that more people participating
< jonatack>
would initially cause a review crunch
< jonatack>
though testing, on the other hand, can be done by relatively new people
< ariard>
luke-jr: that's not good sign for sure
< luke-jr>
jonatack: review, even though perhaps not experienced, is also a good way for newcomers to learn
< jonatack>
so, there may to be a misalignment of incentives
< jonatack>
that encourages people to not spend too much time reviewing and testing
< jonatack>
e.g. I think most would agree that commits/merges, research, mailing list posts, and interviews/podcasts/outreach are more newsworthy and garner far more accolades than review and testing
< ariard>
I agree you don't have to be experienced to review, and the review you're doing as a newcomer are likely to be really different and complementary
< jonatack>
It's also fair to say that, with a few exceptions, new funding principally goes to those with specific project proposals
< jonatack>
projects which are, naturally, prioritized by the grantees over reviewing and testing, as they will be evaluated on them
< jonatack>
If then, funding, fame and glory don’t generally go to new people doing review and testing...
< jonatack>
it may be fair to say that incentives are not really aligned if it is a bottleneck and we need more of it.
< MarcoFalke>
I think review can also be done by new people. Obvioulsy they need more time to reach the same conclusions, but the only precondition is to have a working brain and common sense. It is not like anyone is born with a review badge on your shoulder.
< MarcoFalke>
*their
< jonatack>
Are review and testing then somewhat altruistic, partly done for swapping favors and keeping promises, and partly done for the greater good?
< jonatack>
Is there, like open source software, an element of tragedy of the commons?
< jonatack>
If yes to any of the above, I suppose it makes perfect sense if it remains the bottleneck...
< jonatack>
...and a bit like sex: more talk than actually doing it!
< jonatack>
fin
< luke-jr>
jonatack: one way to make funding help in this area might be to have it based on bug bounties instead ofsimple grants
< ariard>
jonatack: agree on the newsworthy thing, bitcoin medias don't talk about a great ongoing review but maybe we should keep educating them?
< luke-jr>
eg, reward people for finding concrete bugs in either merged or PR code
< luke-jr>
(this probably has other downsides ofc, like higher overhead)
< andytoshi>
jnewbery: oh, good idea about just waiting on listunspent (or even getbalance)
< jnewbery>
I was originally going to suggest getbalance, but I thought listunspent would allow you to wait for the exact block, and you might not know exactly what balance you're waiting for
< bitcoin-git>
bitcoin/master fa86217 MarcoFalke: doc: Move add relay comment in net to correct place
< bitcoin-git>
bitcoin/master a35a346 MarcoFalke: Merge #20653: doc: Move addr relay comment in net to correct place
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #20653: doc: Move addr relay comment in net to correct place (master...2012-docNetAddrRelay) https://github.com/bitcoin/bitcoin/pull/20653
< bitcoin-git>
[bitcoin] sipa opened pull request #20661: Only relay torv3 addresses to addrv2-capable peers (master...202012_torv2_relay_only_addrv2) https://github.com/bitcoin/bitcoin/pull/20661
< bitcoin-git>
bitcoin/master 8bb40d5 MarcoFalke: Merge #20560: fuzz: Link all targets once
< jonatack>
luke-jr: sure, istm grants are essential, and donations and bounties might be complementary (if they can surmount the overhead) but probably aren't enough on their own for what needs to be done and for developer sustainability.
< provoostenator>
wumpus: it's the same machine that I use for DNS seed crawling. Perhaps its incessant hammering of the Tor network is causing the flakiness.
< provoostenator>
I might move the Signet seed node elsewhere.
< provoostenator>
Indeed curling the blockstream explorer v3 URL is also unresponsive.
< provoostenator>
(from that machine)
< dhruvm>
jonatack: Your comment about grant-seekers prioritizing projects over testing/review sounds right. One potential way to affect change: Core devs could get together and let it be known that they will give letters of recommendation for grant committees and will overweight review and testing.
< wumpus>
provoostenator: i've seen this problem as well with more or less busy tor services, usually restarting the daemon works, at least for a while
< provoostenator>
I just moved it to a different machine, it should be reachable now
< wumpus>
oh it succeeds now !
< provoostenator>
Hopefully it stays that way. This new machine doesn't do antying tor-intensive.
< bitcoin-git>
bitcoin/master faf2c6e MarcoFalke: cirrus: Schedule one task with paid credits for faster CI feedback
< bitcoin-git>
bitcoin/master c434e2c Wladimir J. van der Laan: Merge #20615: cirrus: Schedule one task with paid credits for faster CI fe...
< bitcoin-git>
[bitcoin] laanwj merged pull request #20615: cirrus: Schedule one task with paid credits for faster CI feedback (master...2012-ciFasterFeedback) https://github.com/bitcoin/bitcoin/pull/20615
< jonatack>
dhruvm: sure, sgtm. partly i reckon that even when grantors eyes don't glaze over at the importance of review & testing (and understanding of that does indeed seem to be improving from what I hear), evaluating grantee performance on that remains a challenge for them as they have to rely on peer feedback. It might be a bit easier for them to evaluate the delivery of a specific project, and
< jonatack>
to sell/market it internally to stakeholders to justify continued open source grant funding.
< jonatack>
dhruvm: that said, it's only speculation on my part. my grantor specifically asked that i keep reviewing if supported--they get it.
< wumpus>
even if the eventual goal is do deliver a project, reviewing other people's changes (especially to the part of the code which is their interest) will likely make them a better contributor in the first place
< jonatack>
yess
< dhruvm>
yeah - so the core of the issue is that it's hard for the grantors to evaluate performance. that is something core devs can develop a system around?
< dhruvm>
kinda like peer performance reviews at companies
< dhruvm>
they face similar challenges with hiring for example. engineers don't like interviewing but it is crucial in a fast growth situation.
< dhruvm>
i've had jobs where contribution to hiring was as important as new launches for those reviews
< jonatack>
dhruvm: hiring: yes, in my previous life i sometimes brought the dev team for the 6-12 month long projects, every project was like a band reunion
< jonatack>
dhruvm: evaluation does seem to be a key issue, and there are some initiatives afoot to work on this
< bitcoin-git>
[gui] luke-jr opened pull request #153: GUI: Define MAX_DIGITS_BTC for magic number in BitcoinUnits::format (master...const_max_digits) https://github.com/bitcoin-core/gui/pull/153
< instagibbs>
apologies if this gets double-posted, my irc setup being wonky: I can say from a bit of experience in the "grant business" that there is a lot of pressure(rightly so) to not influence "what" a person is working on, and that "inability to evaluate a reviewer" has not been a reason to deny grants(IIRC no one actually applied where their top line was "review"). in other words, if someone wants to get a grant to do review, they have to
< instagibbs>
submit an application :)
< instagibbs>
if we have a dearth of review because of *funding* this is fixable. I suspect not.
< instagibbs>
s/fixable/easily fixable/
< luke-jr>
instagibbs: …
< instagibbs>
luke-jr, !!!
< luke-jr>
instagibbs: well, it might not add review, but it could keep from losing my reviewing if I got funding <.<
< luke-jr>
I hate saying it like that, but I can't keep doing this unfunded forever XD
< instagibbs>
(Can only speak to the one grant program I'm involved in)
< instagibbs>
I suspect in general it's "we need to motivate coders to become reviewers" and "we need new contributors"
< instagibbs>
Also, people can still bug me for review, I'm not on IRC much anymore(life stuff) but if you bug me via email I will likely do it :)