Return-Path: Received: from smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by lists.linuxfoundation.org (Postfix) with ESMTP id 80004C002A for ; Thu, 20 Apr 2023 08:46:12 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 53CDC60BA3 for ; Thu, 20 Apr 2023 08:46:12 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 53CDC60BA3 Authentication-Results: smtp3.osuosl.org; dkim=pass (2048-bit key) header.d=protonmail.com header.i=@protonmail.com header.a=rsa-sha256 header.s=protonmail3 header.b=qdbQd745 X-Virus-Scanned: amavisd-new at osuosl.org X-Spam-Flag: NO X-Spam-Score: -2.102 X-Spam-Level: X-Spam-Status: No, score=-2.102 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001] autolearn=ham autolearn_force=no Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id jzHJ0tILewRw for ; Thu, 20 Apr 2023 08:46:10 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 21DDC60A9F Received: from mail-4322.protonmail.ch (mail-4322.protonmail.ch [185.70.43.22]) by smtp3.osuosl.org (Postfix) with ESMTPS id 21DDC60A9F for ; Thu, 20 Apr 2023 08:46:10 +0000 (UTC) Date: Thu, 20 Apr 2023 08:45:58 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail3; t=1681980367; x=1682239567; bh=UG5/4cCqnsuqaM07418Xo8kKwQeKfrY+Kpt4tNHUfwo=; h=Date:To:From:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector; b=qdbQd745KbQpFeheD+0YuT6tykMW+GL9nvnS4a40ybrUUuAcPWMpZxVk+wnzBSvfy 7QK40gUfHLkyovADdNFQf8xZRSP7YE3PD1vYUadRMIY+YSNXrV77bnZZOcItjnP44M DE6SHS5mLCaD7lDYg2yYohq7VdIh8FWihI0thwhMQj4bohmofDEhaHnsqV20eNerU6 ZVU6PXj4M+E1hsuTTE/2GXVpf1TdEeF0Erxs8hEocaIeQWeX1S5dJ8Wx5TcKd0ZNYO qVj72Um68DliDBRV2tJ/hvSZ2VD01wYV9JFxtXHNDZ5rvIIunZxl6KERAS24sOQeQl rCw12NtfM5r5A== To: Andrew Chow , Bitcoin Protocol Discussion From: Michael Folkson Message-ID: In-Reply-To: References: Feedback-ID: 27732268:user:proton MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Mailman-Approved-At: Thu, 20 Apr 2023 10:40:08 +0000 Subject: Re: [bitcoin-dev] Bitcoin Core maintainers and communication on merge decisions X-BeenThere: bitcoin-dev@lists.linuxfoundation.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Bitcoin Protocol Discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 Apr 2023 08:46:12 -0000 Thanks for this Andrew. > What commentary does there need to be? There doesn't "need" to be explanations about anything. There doesn't "need= " to be any review comments whatsoever from anybody. But a world where revi= ewers explain what they've done to satisfy themselves that a pull request i= s ready to merge and a world where maintainers explain their thought proces= s behind a merge decision is much easier to follow and much more scalable t= han the current black box where people see pull requests being self merged = by a maintainer with no ACKs within a day of it being opened. Most likely t= hese decisions make sense (low risk, unlikely to be reviewed by anybody els= e, blocking other pull requests etc). But more and more people are funded t= o work on Core and increasingly they seem to stick to their own mini projec= ts and not review anybody else's work. Of course you can't put the responsi= bility for this entirely down to maintainers but the black box isn't scalab= le. Maintainers (presumably) have private discussions and so know how best = to spend their review time. Everyone else (especially new contributors) are= playing an uninformed and in the dark lottery with how they spend their re= view time (to the extent that they allocate any). > There are currently 320 open PRs and 366 open issues. I wake up every morning to 150+ email notifications containing everything that went on overnight, and throughout the day, I typically get hundreds more. Indeed. All the more reason for more and higher quality public communicatio= n. If you struggle and you're in those private discussions with other maint= ainers on merge decisions and ready for merge discussions how do you think = everyone is trying to assess how to spend their time? It is even more bewil= dering. As far as I know most of the current maintainers haven't worked on = other projects or in the private sector for a sustained period of time but = the way other projects and businesses function is that those with the most = experience and most responsibilities are able to manage and provide guidanc= e to those with less experience and less responsibilities. I'm sure this go= es on within Brink if you've been anointed but this is supposed to be an op= en source project. If everything is done in private conversations and every= thing other than the code is open source it is pretty much a fa=C3=A7ade. I= t is very hard to make meaningful contributions without getting anointed. T= hose who do get anointed very early in their careers seem especially bad at= hoarding information, refusing to do anything in public and not assisting = those who haven't been anointed. > Things can simply fall through the cracks. > With several long time maintainers stepping away, this may be a factor in PRs taking longer to get merged as the remaining maintainers may be less familiar with the parts of the codebase that were previously maintained by someone else. > Requiring maintainers to have to write explanations for every single merge is simply going to increase the burden on them and increase the rate of burnout and resignations. > We've had too many maintainers step down already. This all points to a a need for additional maintainers (assuming they are s= ufficiently competent and qualified). We had a potential maintainer (Vasil)= come forward (long term contributor, made significant contributions over a= number of years, a qualified reviewer, contributes to a part of the codeba= se that current maintainers aren't very familiar with, ACKed by maintainers= and long term contributors) and it was blocked. How does that make sense? = You seem to want it both ways. We can't ask maintainers to meet higher stan= dards because there's a shortage and they're close to burning out. But ther= e's no need to add a new maintainer. I've responded to the parts I disagree with and see inconsistencies with bu= t generally I thought it was a very thoughtful and informative response so = thank you. Of the current maintainers you seem to me to be one of (if not t= he) most responsive and open to public discussion on the project. I've lear= nt a tonne from your StackExchange posts and Twitch streams that are all pu= blic/open source that you do in addition to your contributions and maintena= nce of Core.=20 Thanks Michael -- Michael Folkson Email: michaelfolkson at protonmail.com Keybase: michaelfolkson PGP: 43ED C999 9F85 1D40 EAF4 9835 92D6 0159 214C FEE3 ------- Original Message ------- On Wednesday, April 19th, 2023 at 22:33, Andrew Chow via bitcoin-dev wrote: > Responses in-line. > Note that the opinions expressed in this email are my own and are not > representative of what other maintainers think or believe. >=20 > On 04/18/2023 08:40 AM, Michael Folkson via bitcoin-dev wrote: >=20 > > Communication has been a challenge on Bitcoin Core for what I can >=20 > tell the entire history of the project. Maintainers merge a pull request > and provide no commentary on why they=E2=80=99ve merged it. >=20 > What commentary does there need to be? > It's self evident that the maintainer believes the code is ready to be > merged, and has observed enough ACKs from contributors that they are > comfortable to do so. > You're welcome to ask for clarification, but frankly, I don't think > having any commentary on merges is going to be helpful or more elaborate > in any way. > Requiring maintainers to have to write explanations for every single > merge is simply going to increase the burden on them and increase the > rate of burnout and resignations. > We've had too many maintainers step down already. > It'll end up being a bunch of boilerplate comments that don't say > anything meaningful. >=20 > There are certainly situations where PRs are merged very quickly or with > otherwise little apparent review. > But, as I said, if you ask a maintainer why it was merged, the answer > will be "I thought it was ready and had enough review". > There may be other reasons that made the maintainer think it was ready > sooner, such as the PR fixes a critical bug or security vulnerability, > but these reasons aren't going to be stated publicly. >=20 > > Maintainers leave a pull request with many ACKs and few (if any) >=20 > NACKs for months and provide no commentary on why they haven't merged it. >=20 > There are currently 320 open PRs and 366 open issues. > I wake up every morning to 150+ email notifications containing > everything that went on overnight, and throughout the day, I typically > get hundreds more. > It's impossible to keep up with everything that goes on throughout the re= po. > ACKs come in sporadically, PRs are updated, reviews are posted, etc. > Often times PRs are not merged simply because the maintainers were not > aware that a PR was ready to be merged. > Things can simply fall through the cracks. >=20 > Of course there are other reasons why something might not be merged, and > these generally fall into the camp of "I don't think it has had enough > review". > It's the maintainer's judgement call to make as to whether something has > been sufficiently reviewed, and part of the judgement call is to > consider the quality and competence of the reviewers. > If a PR had 100 ACKs but all from random people who have never > contributed to the project in any capacity, then it's not going to be > merged because those reviewers would be considered low quality. > It's not just about the numbers, but also about whether the reviewers > are people the maintainers think are familiar enough with an area and > have had a history of thoroughly reviewing PRs. > For example, if a reviewer who primarily works on the mempool reviewed a > PR in the wallet, I would consider their review and ACK with less weight > because they are unlikely to be familiar with the intricacies of the wall= et. > Obviously that changes over time as they make more reviews. > For another example, if I see an ACK from a reviewer who posts reviews > that primarily contain nits on code style and other trivialities, I > would consider that ACK with less weight. >=20 > Furthermore, the maintainers are not necessarily the ones who block a mer= ge. > Part of evaluating if something is ready to be merged is to read the > comments on a PR. > Other frequent contributors may have commented or asked questions that > haven't been resolved yet. > PRs will often not be merged (even if they have ACKs) until a maintainer > deems that those comments and questions have been sufficiently resolved, > typically with the commenter stating in some way that their concerns > were addressed. > In these situations, no commentary from maintainers is given nor > necessary as it should be self evident (by reading the comments) that > something is controversial. > These kinds of comments are not explicit NACKs (so someone who is only > counting (N)ACKs won't see them), but are blocking nonetheless. >=20 > Lastly, personally I like to review every PR before I merge it. > This often means that a PR that might otherwise be ready to be merged > wouldn't be merged by myself as I may not be familiar with that part of > the codebase. > It may also mean that I would require more or specific additional people > to review a PR before I merge it as I would weight my own review less > heavily. > With several long time maintainers stepping away, this may be a factor > in PRs taking longer to get merged as the remaining maintainers may be > less familiar with the parts of the codebase that were previously > maintained by someone else. >=20 > > but a casual observer would have only seen Concept ACKs and ACKs with >=20 > 3 stray NACKs. Many of these casual observers inflated the numbers on > the utxos.org site [4] signalling support for a soft fork activation > attempt. >=20 > Anyone who thinks that maintainers only look at the numbers of (N)ACKs > is delusional. > As I explained above, there is a whole lot more nuance to determining > even just the status of the opinions on a PR, nevermind the code itself. >=20 > In this specific example of a soft fork, there is also consideration of > the opinions outside of the repo itself, such as on this mailing list > and elsewhere that people discuss soft forks. >=20 > On 04/19/2023 11:17 AM, Aymeric Vitte via bitcoin-dev wrote: >=20 > > While some simple changes can allow bitcoin to surpass ethereum, as >=20 > usual, like "Allow several OP_RETURN in one tx and no limited size" > https://github.com/bitcoin/bitcoin/issues/27043 >=20 > > How long it will take remains mysterious >=20 >=20 > No one (maintainers or contributors) is obligated to implement anything. > A feature request not being implemented is because the people who do > open PRs are either not interested in implementing the feature, or are > working on other things that they believe to be higher priority. > If there is a feature that you want, then you will often need to either > to it yourself, or pay someone to do it for you. >=20 > Additionally, a feature may seem like a good idea to you, but there are > often interactions with other things that may end up resulting in it > being rejected or need significant revision, especially for something > which affects transaction relay. >=20 >=20 >=20 > Andrew Chow >=20 > _______________________________________________ > bitcoin-dev mailing list > bitcoin-dev@lists.linuxfoundation.org > https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev