[WIP] Add a MAINTAINERS.md file and desribe the role of maintainer more directly

JeremyRubin commented at 0:29 am on July 7, 2022: contributor

As a follow up to the conversation in IRC around maintainers, I have begun drafting a document to better define the roles and procedures around adding a maintainer.

I’ve tried my best to document what is currently done, as well as inputting some of what was discussed in IRC. I also had to “fill in” a little bit of procedure on things I couldn’t find reference for, but very open to feedback if I’ve missed the mark somehow in those areas.

I’ve tried largely to keep it informal, with “shoulds” and “mays” aplently, but over time we might change these to be required behaviors/procedures.

JeremyRubin force-pushed on Jul 7, 2022

in MAINTAINERS.md:17 in c18b9bca89 outdated

12+------------------
13+|   @achow101  | wallet, PSBTs |
14+|   @fanquake | general, tooling, build system |
15+|   @hebasto | UI/UX |
16+|   @laanwj | general |
17+|   @marcofalke | general |

luke-jr commented at 0:39 am on July 7, 2022:

IIRC, Marco’s focus was tests/QA?

JeremyRubin commented at 1:00 am on July 7, 2022:

ah i see – I wasn’t quite sure about this section so I mostly guessed. @MarcoFalke is that accurate? would it be better to be general + tests/QA, or just tests/QA?

in MAINTAINERS.md:69 in c18b9bca89 outdated

64+if it confers any additional responsibility or privilege over other General
65+Maintainers, and may be explicitly deprecated in the future.
66+
67+## Self Merges
68+
69+It is discouraged for maintainers to merge their own work. However, given that

luke-jr commented at 0:41 am on July 7, 2022:

Why is it discouraged? The same criteria should apply…

JeremyRubin commented at 1:03 am on July 7, 2022:

I think there are a number of reasons for it to be discouraged, but I agree the same criteria ultimately applies: it has to be good code for Bitcoin ;)

I think there are a couple practical concerns, one being that on the bubble self merges might be less well reviewed (e.g. is the self merge happening because no one else is qualified to review it?) and also a centralizing issue whereby maintainers might be much more “productive” than regular contributors because they favour their own work over the work of others.

unknown commented at 9:10 am on July 7, 2022:

Why self merges should be discouraged?

Reviewers can find better approach, bugs etc. in code because a developer who writes code may not be able to review own code without bias. Similarly, other maintainer merging the pull request would take the responsibility to make changes, will be less biased and go through everything before merging.
If other maintainer who merges code did not already review the pull request, it will be helpful in getting another review and find new issues, bugs etc. that may go unnoticed.

Example: If laanwj decides to merge #24362 instead of achow101 there will be another review, no bias and maybe new issues, bugs etc.

Only positive of allowing self merges is merge/week or merge/month is not affected even if less active maintainers.

josibake commented at 3:57 pm on July 7, 2022:

Why is it discouraged? The same criteria should apply…

+1 , a merge is a merge. every criticism of a self-merge that i have seen so far really boils down to “merging a PR that has not received sufficient review or consensus.” preventing self-merges (or in this case creating somewhat arbitrary hoops to jump through) does not address the root problem and instead creates a false sense of security that non-self-merged PR’s are somehow better or less risky

unknown commented at 5:00 pm on July 7, 2022:

every criticism of a self-merge that i have seen so far really boils down to “merging a PR that has not received sufficient review or consensus

No, it is different from merging a less reviewed pull request. I have shared 2 reasons above your comment:

Bias: Alice and Bob are maintainers. Alice opens a pull request which is reviewed by others. Maintainers are humans and assessment of the reviews for this pull request can be different by Alice and Bob. Alice will have some bias to get it merged with 3 ACKs and Bob will look at all the comments, reviews, nits, possibility of bugs etc. before merging.
1 Extra review before merge: In some cases it adds another review before merge by another maintainer. More reviews should never be a problem.

achow101 commented at 6:10 pm on July 7, 2022:

While bias is a concern, it doesn’t go away if self merges are disallowed, and in fact, becomes opaque. If Alice and Bob are maintainers, and self merges are not allowed, Alice informs Bob that they think the PR is ready to be merged. This induces a bias in Bob which has the same problems as the bias of Alice merging her own PR, but perhaps less strongly. It is likely that Bob trusts Alice’s judgejent as she is a maintainer. However it is possible to be completely opaque to observers that there was bias because the communication from Alice to Bob may not be in a place that is public or publicly logged. With a self merge, it is at least completely evident that there is some bias.

This also exists with regular contributors who post “ready for merge”.

I think it is more preferable to keep things as transparent as possible rather than to reduce bias here. Review can be done after merges, and transparency allows such reviewers to keep any potential bias in mind.

JeremyRubin commented at 6:51 pm on July 7, 2022:

the section is explicitly not disallowing self merges, it’s discouraging them and permitting them as long as they check some boxes around providing sufficient time for third party review.

MarcoFalke commented at 6:45 am on July 8, 2022:

Regardless of how “weakly” they are discouraged, they will decrease transparency, as the slightest discouragement will encourage maintainers to cooperate on trading merges privately.

unknown commented at 1:47 pm on July 15, 2022:

Transparency isn’t affected because everyone can see who merged the pull request. Private communications cannot be stopped and there is no way to verify if they already affect pull requests. Things that are written publicly and merging the pull requests is important. If someone wants to take the responsibility of merging a pull request it doesn’t matter if there was a private trade involved. If everything is checked before merging without bias it should be okay else its risking reputation.

michaelfolkson commented at 2:22 pm on July 15, 2022:

@1440000bytes: For a new contributor (not a long term contributor and certainly not a maintainer) you have extremely opinionated views and express them with presumed certainty. I’d dial these back if I were you. You are not an authority on this pull request. Questions and challenges are fine to improve your own understanding but lecturing maintainers on what is and isn’t true as a new contributor verges on the bizarre. Maintainers know what has worked well and what hasn’t in the past and have a much better understanding than you on the trade-offs for suggested changes.

unknown commented at 3:45 pm on July 15, 2022:

@michaelfolkson

I’d dial these back if I were you.

Do not expect every contributor in this repository or on GitHub to follow your rules.

You are not an authority on this pull request.

I am a reviewer in this pull request writing my thoughts and responding to comments.

Questions and challenges are fine to improve your own understanding but lecturing maintainers on what is and isn’t true as a new contributor verges on the bizarre.

There are no lectures or truths being shared. Opinions of different people about self merges.

Maintainers know what has worked well and what hasn’t in the past and have a much better understanding than you on the trade-offs for suggested changes.

I do not need your approval in every pull request to write something and this isn’t the first time you are trying to bully new contributors. Hopefully you would learn something and comment on things being discussed.

ghost commented at 3:13 am on July 7, 2022: none

Concept ACK

in MAINTAINERS.md:104 in c18b9bca89 outdated

 99+
100+Following the C4M, anyone may respond to the issue with a nomination for the
101+position and a description of their qualifications. Self-nomination is
102+encouraged. Nominations must be accepted. After 2 weeks from the date of issue
103+creation, assuming there is at least 1 nomination, the maintainers may select
104+one or more candidate from the nominee pool at their discretion. The selected

unknown commented at 3:21 am on July 7, 2022:

0encouraged. Nominations must be accepted. After 2 weeks from the date of issue
1creation, assuming there is at least 1 nomination, the maintainers, contributors
2can discuss and vote for one candidate in an IRC meeting. The selected

I think contributors should be allowed to share their choice of candidate else process would remain almost the same or result in controversies once pull request is opened to add key.

JeremyRubin commented at 4:15 pm on July 7, 2022:

good point – maybe I should note that contribs should ack / co-sign nominations?

I think the only issue is that maintainers do need to ultimately decide who is the candidate.]

I don’t think voting should happen in an IRC meeting so I won’t take this exact suggestion (IRC is ’no decisions’).

unknown commented at 5:29 pm on July 7, 2022:

good point – maybe I should note that contribs should ack / co-sign nominations?

Yes

I think the only issue is that maintainers do need to ultimately decide who is the candidate.

If only maintainers decide the candidate there won’t be any major change in the process. Nominations and Call for maintainers will just become a formality.

in MAINTAINERS.md:11 in c18b9bca89 outdated

 6+
 7+## Current Maintainers
 8+
 9+The current maintainers are:
10+
11+| name | focus areas |

MarcoFalke commented at 6:05 am on July 7, 2022:

Not sure how many duplicate lists we need. At least this could remove https://github.com/bitcoin/bitcoin/blob/aeab1b42e67cc8146bfc7d127d15633bd652fe60/REVIEWERS#L20 ?

MarcoFalke commented at 6:14 am on July 7, 2022:

I am not sure about the purpose of focus areas. Not only does the list of maintainers change, but also their focus area, so are they required to open a pull request to change their focus and wait for approval by others to change their interests?

JeremyRubin commented at 4:15 pm on July 7, 2022:

Sure – happy to make an edit to remove that. Better that this live in one place…

instagibbs commented at 1:28 pm on August 12, 2022:

I think as long as maintainers know what other focus other maintainers have currently, they can point people in the right direction. i.e., I don’t think it’s required for this PR

JeremyRubin commented at 5:23 pm on August 13, 2022:

this is made clear in the following part of the document:

The role of the Maintainer is currently evolving with respect to “Scoped Maintainers”. The concept of a scoped maintainer is that it is helpful to add maintainers who are subject matter experts in a particular area of the code, but perhaps not in other areas. As such, maintainers with an explicit scope typically will avoid merging PRs in areas outside their scope, but may do so on occasion. As an individual maintainer’s experience levels in the project increase, they may change their focus from scoped to general, but this is not currently an explicit designation (all maintainers should be considered to be general at this time). As a rough measure, scoped maintainers should avoid “stepping on the toes” of work that is being guided by another maintainer or contributor.

I’m not sure if the “review bar” for updating documentation is the same as for modifying code, but I would assume it would be OK for maintainers to self-merge if they are keeping it up to date. As the document describes, this is just documenting what a maintainer’s focus is, not restricting it.

in MAINTAINERS.md:20 in c18b9bca89 outdated

15+|   @hebasto | UI/UX |
16+|   @laanwj | general |
17+|   @marcofalke | general |
18+|   @sipa | general, peer-to-peer, consensus, cryptography |
19+
20+See [REVIEWERS](/REVIEWERS) for more information on specific focus areas of

MarcoFalke commented at 6:08 am on July 7, 2022:

Pretty sure that file was a failure. It isn’t updated (including stale entries and lacking recent contributors), and when I tried to use it to notify reviewers, the notifications were largely ignored.

JeremyRubin commented at 4:15 pm on July 7, 2022:

Ok – perhaps we should delete it then?

adamjonas commented at 6:18 pm on July 7, 2022:

As the one that was initially pushing this, it’s time to admit defeat.

JeremyRubin commented at 7:12 pm on July 18, 2022:

removed

in MAINTAINERS.md:34 in c18b9bca89 outdated

29+
30+
31+Largely, the role of maintainers is to decide to merge Pull Requests. This
32+process is described in [CONTRIBUTING.md](/CONTRIBUTING.md), but as an excerpt:
33+
34+> Whether a pull request is merged into Bitcoin Core rests with the project merge

MarcoFalke commented at 6:15 am on July 7, 2022:

Instead of cross-quoting I’d just move the relevant sections into this document, or modify the sections there, avoiding duplicates.

JeremyRubin commented at 4:17 pm on July 7, 2022:

Ok – I can move the section, that just felt a bit more invasive because that section is generally useful to have in contributors, but maybe I can change to a link from CONTRIBUTORS.md to MAINTAINERS.md?

in MAINTAINERS.md:65 in c18b9bca89 outdated

60+that they wanted to take more of a background role as a maintainer and have not
61+"passed the baton" to a successor as a Lead Maintainer, and instead focused on
62+decentralizing the maintainership ecosystem. As such, while the title of Lead
63+Maintainer still is held by @laanwj, it should be considered a bug-not-feature
64+if it confers any additional responsibility or privilege over other General
65+Maintainers, and may be explicitly deprecated in the future.

MarcoFalke commented at 6:22 am on July 7, 2022:

For reference, the only mention of “lead maintainer” in this repo was about the release cycle. However, there is no technical requirement that a single maintainer does the release cycle. In fact in large parts in can be done by any contributor, and the final steps of the release process are currently done by 4 different maintainers collectively, which is why I have removed the mention in https://github.com/bitcoin/bitcoin/commit/8abe79aedd0ba129e0fd3bcd971e8733d22fb3c4

JeremyRubin commented at 4:32 pm on July 7, 2022:

Interesting I can’t actually find a reference for @laanwj ever becoming Lead Maintainer – it seems when Gavin handed off, he explicitly said that there would not be a Lead Maintainer, just a Core Maintainer.

Wladimir van der Laan has been paid to work on Bitcoin Core full-time for several months now– again, thanks to all of you Foundation members for stepping up and helping to fund core development– and has been doing a fantastic job. He has agreed to take over for me as the “Bitcoin Core Maintainer.”

ref: https://web.archive.org/web/20140915022516/https://bitcoinfoundation.org/2014/04/bitcoin-core-maintainer-wladimir-van-der-laan/ which is clarified here https://youtu.be/Y6hZhdZuct0 that there is not a lead maintainer around the 3 min mark.

Was there a point where we adopted the Lead Maintainer title? Is it something we should formally retire at this point?

MarcoFalke commented at 4:41 pm on July 7, 2022:

Was there a point where we adopted the Lead Maintainer title? Is it something we should formally retire at this point?

I don’t know an answer to either question. All I know is that the phrase I removed was added in commit 06d92d71a2ef414543940fd6ef1080176eb455bb

JeremyRubin commented at 5:22 pm on July 7, 2022:

gotcha – it looks like #6718 was the PR that was drafted by @btcdrak and @laanwj and it did recieve ACKs at that point from relevant parties, but perhaps that was an oversight v.s. intended?

in MAINTAINERS.md:78 in c18b9bca89 outdated

73+
74+1. The PR has been reviewed and ACK'd by other regular contributors familiar
75+   with that section of the code.
76+1. There are no NACKs being overruled on that PR.
77+1. The PR is at least 1 week old, to permit time for review.
78+1. The maintainer should communicate intent to self-merge 48 hours before doing so

MarcoFalke commented at 7:03 am on July 7, 2022:

I don’t see a reason why self-merges should be treated any different than regular merges. In fact, if you do treat them differently, you will likely achieve the opposite of what you want to achieve. If there is a requirement for maintainers to ask other maintainers for a favour to “take a look” at a pull request to avoid a self-merge, I’d be worried that this will decrease transparency and instead create an incentive for maintainers to swap merges of each other’s pulls.

See also #25524 (comment) and #25524 (comment)

It might be better if you stated your concern/goal and then work on a solution that is better suited. For example, if your concern is that maintainers are too productive, I think it would be more helpful to find more maintainers to share their workload.

unknown commented at 9:11 am on July 7, 2022:

I have shared two reasons why self merges should be discouraged: #25560 (review)

JeremyRubin commented at 4:34 pm on July 7, 2022:

It’s not clear if this document is that place to state concerns, or to just state the policy as a result of the concerns.

in MAINTAINERS.md:132 in c18b9bca89 outdated

127+1. What went well in the last release cycle
128+1. What is making good progress for this release cycle
129+1. How the maintainer intends to prioritize outstanding
130+   PRs/issues/features/projects in the next release cycle
131+1. Projects which could use participation from other contributors
132+1. Longer term focuses beyond the next release

MarcoFalke commented at 7:15 am on July 7, 2022:

I don’t see how this could be of any help. If you want to find out what went well in the past, you can read the release notes. If you want maintainers to prioritize something, it likely won’t work. (Assume I prioritized $project, but then no one reviewed it, am I supposed to merge it without review?)

Don’t get me wrong, I am not holding anyone back from sharing their interests and motivations or making a personal roadmap, but in my eyes this has nothing to do with maintainers. In fact, if you force contributors that are maintainers to come up with a roadmap, I could see that this will discourage contributors that are non-maintainers from sharing their goals, since it doesn’t align with the “official roadmap”.

vasild commented at 9:43 am on July 7, 2022:

In my experience, both from corporate projects and open source projects, including Bitcoin Core, this would be mostly bureaucracy stuff with very little or no real impact. Are maintainers doing this now? If not, then why impose that on them with this document?

Also, it gives somehow the false impression that maintainers are deciding there the project is going. In reality contributors do by opening PRs and reviewing them.

vasild commented at 10:11 am on July 7, 2022:

An example from Bitcoin Core of a similar thing that stalled: https://github.com/bitcoin-core/bitcoin-devwiki/wiki/P2P-Current-Priorities/

mzumsande commented at 2:06 pm on July 7, 2022:

agree, this would push maintainers into a more active project management/leadership role which would only lead to frustration:

by maintainers who will be made responsible by contributors if their projects don’t move forward due to lack of review or approval, getting blamed for “prioritizing” them not enough in their roadmap posts
by contributors who would feel less a part of a largely self-organizing community without central project management

I think that even if individual maintainers wanted to do these kind of posts, they should make them in the role of a contributor, not maintainer.

So I would prefer not to have this section.

josibake commented at 4:12 pm on July 7, 2022:

+1 to the other commenters, the only thing id add:

practically the views and prioritizations of maintainers have an impact on what gets reviewed, merged, and the general direction of the project.

this seems like a bug-not-a-feature. if anything, we should be thinking of ways to minimize this, not formalize it.

JeremyRubin commented at 4:37 pm on July 7, 2022:

I don’t see how this could be of any help. If you want to find out what went well in the past, you can read the release notes. If you want maintainers to prioritize something, it likely won’t work. (Assume I prioritized $project, but then no one reviewed it, am I supposed to merge it without review?)

Reading release notes is not really a valid summary of what went well, since release notes don’t contain any information on e.g., what went according to expectations, what complexities came up, status of ongoing projects.

Don’t get me wrong, I am not holding anyone back from sharing their interests and motivations or making a personal roadmap, but in my eyes this has nothing to do with maintainers. In fact, if you force contributors that are maintainers to come up with a roadmap, I could see that this will discourage contributors that are non-maintainers from sharing their goals, since it doesn’t align with the “official roadmap”.

There is a section in the document encouraging regular contributors to share their prioritization documents listed alongside the maintainers.

JeremyRubin commented at 4:48 pm on July 7, 2022:

In my experience, both from corporate projects and open source projects, including Bitcoin Core, this would be mostly bureaucracy stuff with very little or no real impact. Are maintainers doing this now? If not, then why impose that on them with this document?

Also, it gives somehow the false impression that maintainers are deciding there the project is going. In reality contributors do by opening PRs and reviewing them.

It’s something that’s been done historically by past maintainers, but I think it’s never been codified as a requirement.

The point of imposing it as such is that maintainers already do have these prioritizations and plans, but they live in their heads and aren’t shared publicly with opportunity for comment.

Having maintainers regularly write up their thoughts on what they think is possible / prioritized and how they will spend their time leads to better utilization of all contributors time, as well as helps contributors understand if the projects they are working on are unlikely to recieve ‘support’ from maintainers (which might open the door to proposing a new maintainer with that scope).

MarcoFalke commented at 5:00 pm on July 7, 2022:

if the projects they are working on are unlikely to recieve ‘support’ from maintainers (which might open the door to proposing a new maintainer with that scope).

(edit: your meaning anyone’s)

If your project doesn’t receive any review, then an encouragement by maintainers is likely not going to magically give it reviewers. If a project isn’t merged despite review, then it may have outstanding issues or controversies. Assigning a new maintainer for the sole purpose of merging your project isn’t magically going to resolve them.

JeremyRubin commented at 5:27 pm on July 7, 2022:

do you mind de-personalizing this? i don’t presently have any work i think would be benefitted by the addition of a maintainer.

There are many contributors who are not maintainers who have long term projects that seem to be somewhat stalled out, even though there are other individual who might be qualified to serve as a scoped maintainer to help (or that individual could as well). I think often times the issue in it of itself is getting sufficient review, and maintainers with scopes help coordinate review. And there are PRs where there is sufficient review, but no maintainer has a strong interest that stall out. These are circumstances where adding a maintainer would help.

achow101 commented at 6:15 pm on July 7, 2022:

I really don’t think that we should have maintainers be required to share what they prioritize, particularly in long form writing in a permanent location. It will have an affect on the direction of the project and lead to those things being perceived as roadmaps. This will discourage contributors from pursuing their own projects that may conflict with whatever maintainers are prioritizing and generally reduce the diversity of thought in the project. I think that will have a centralizing effect around the maintainers that we want to avoid.

MarcoFalke commented at 8:37 am on July 8, 2022:

These are circumstances where adding a maintainer would help.

Yes, I agree that currently adding more maintainers is probably beneficial to the project. Especially if making someone maintainer “tricks” them into spending time on code review in the areas they are interested in. Moreover, when there are multiple maintainers for the same module, it might naturally increase the accountability and cross-checks, something that you’d rather see fixed by having them “check some boxes”.

in MAINTAINERS.md:155 in c18b9bca89 outdated

150+#### @hebasto
151+#### @laanwj
152+#### @marcofalke
153+#### @sipa
154+
155+Non maintainers may also share similar documents detailing their work here:

MarcoFalke commented at 7:16 am on July 7, 2022:

I don’t think we want to use this repository as a world-writeable blog for anyone (or a link collection to such)

JeremyRubin commented at 4:51 pm on July 7, 2022:

I think it’s an overstatement to call it a world writable blog: we could maintain these in a docs folder if we want them comitted / reviewed.

Alternatively, they could live somewhere else, but I think it’s important that you can somehow discover where these live from the section detailing they exist. Otherwise it’s just “there exists this informaiton somewhere find it via google”?

MarcoFalke commented at 7:19 am on July 7, 2022: member

Concept ACK on tidying up the documentation, but I am not entirely convinced that your suggested processes are going to achieve the goals you want to achieve.

in MAINTAINERS.md:173 in c18b9bca89 outdated

168+believe the maintainer or their keys may be compromised -- this removal can be
169+done temporarily by any single maintainer, unless there are only three
170+remaining maintainers (in which case removal requires 2, a majority), but
171+should be unanimously decided if permanent removal. While imperfect, this
172+procedure ensures that access can be revoked quickly in the event of a
173+compromise, while preventing takeover by a single dev.

vasild commented at 9:49 am on July 7, 2022:

preventing takeover by a single dev

But it allows takeover by 2 cooperating maintainers. These hard rules have the downside that they make it clear to adversaries what exactly has to be done in order to f**k up the project.

JeremyRubin commented at 4:53 pm on July 7, 2022:

Well right now we “allow”, afaict, takeover by a single dev it’s just not clearly stated.

Is security through obscurity preferred?

MarcoFalke commented at 5:02 pm on July 7, 2022:

Any single GitHub/Microsoft employee with enough permissions is “allowed” to “take over” the project.

in MAINTAINERS.md:195 in c18b9bca89 outdated

190+that were rebuffed or ignored. For example, if the average PR time to merge is
191+3 months, and the bottleneck seems to be that the maintainer only merges PRs
192+once a month, contributors might reasonably request that the maintainer merge
193+PRs at least once a week if there is work that is ready to go. Should the
194+maintainer refuse, contributors might ask that they step down so that a more
195+active maintainer might step in. Or, if the issue is too few maintainers for

vasild commented at 10:05 am on July 7, 2022:

So a maintainer may be removed for not merging PRs frequently enough!? This sounds like a gross misunderstanding to me. Also, it would achieve the opposite effect - without that maintainer even less PRs will be merged.

How could a single maintainer be a bottleneck? Nothing prevents other maintainers from merging a PR. It is not like a given maintainer has an exclusive “only I merge PRs matching a given criteria”.

JeremyRubin commented at 4:56 pm on July 7, 2022:

The section you’re referrencing basically says the opposite of what you’re saying, or is at least intended to.

Requests for removal are a last resort, not a first resort. Contributors might ask if the maintainer can spend more time, they might propose adding a new maintainer. Or, they might request replacing the maintainer if there is someone who can spend more time on it.

in MAINTAINERS.md:156 in c18b9bca89 outdated

159+
160+Maintainers may choose to cease being a maintainer at any time without notice.
161+Where possible, maintainers should try to gracefully transition out of their
162+role, until a new maintainer has been selected to replace them (if needed).
163+
164+## Maintainer Removal

vasild commented at 10:07 am on July 7, 2022:

I think the whole section below should be reduced to just “the best judgement of other maintainers and contributors apply” or something like that.

josibake commented at 4:16 pm on July 7, 2022:

+1, adding more rules and processes here seems to be increasing the attack surface. in many cases, having less formalized rules makes the process more resilient

in MAINTAINERS.md:26 in c18b9bca89 outdated

23+
24+## Regular Contributors, Scoped Maintainers, General Maintainers, and Lead Maintainers
25+
26+The title of Maintainer in Bitcoin is more of a "janitorial responsibility"
27+than a position of privilege. However, the role does include permissions on the
28+repository, and connotes a general sense of stewardship over the project.

josibake commented at 3:31 pm on July 7, 2022:

However, the role does include permissions on the repository, and connotes a general sense of stewardship over the project.

I would prefer leaving it at “janitorial.” stewardship implies a more active role in leading and setting the direction of the project, which is not the responsibility of a maintainer.

JeremyRubin commented at 5:34 pm on July 7, 2022:

Going from bitcoincore.org,

Project maintainers have commit access and are responsible for merging patches from contributors. They perform a > janitorial role merging patches that the team agrees should be merged. They also act as a final check to ensure that > patches are safe and in line with the project goals. The maintainers’ role is by agreement of project contributors.

I read “ensuring patches are in line with the project goals” as being stewardship, given that it requires setting or having some project goals (I don’t know if we have them written down anywhere?).

josibake commented at 6:33 pm on July 7, 2022:

i dont think that sentence implies the maintainers are setting the project goals (ergo stewarding the project), which is why i prefer restricting the verbiage to janitorial.

MarcoFalke commented at 6:35 am on July 8, 2022:

I also don’t think that this sentence means the project sets the goal. Maybe we should remove documentation that can be interpreted to mean both one thing and the opposite of that thing?

vasild commented at 8:17 am on July 8, 2022:

I agree with @josibake and @MarcoFalke. I interpret “ensure that patches are … in line with the project goals” in a sense that maintainers will not merge a patch, no matter how many ACKs it has if it e.g. turns Bitcoin Core into a 3D computer game, or Word processor, or an online dating app - something that is gross offtopic.

in MAINTAINERS.md:54 in c18b9bca89 outdated

50+occasion. As an individual maintainer's experience levels in the project
51+increase, they may change their focus from scoped to general, but this is not
52+currently an explicit designation (all maintainers should be considered to be
53+general at this time). As a rough measure, scoped maintainers should avoid
54+"stepping on the toes" of work that is being guided by another maintainer or
55+contributor.

josibake commented at 3:48 pm on July 7, 2022:

is this section needed? i think it would be sufficient to document that some individuals may elect to focus on a particular area of the codebase due to having subject matter expertise in that area but are able to review and merge PRs from any section of the codebase.

im also not sure what is meant by “stepping on the toes.” do you have an example of this?

JeremyRubin commented at 5:38 pm on July 7, 2022:

steppind on toes: It was a term used by others in the IRC meeting – I read it to mean that a maintainer is merging PRs that another maintainer would consider their area of expertise, and would disrupt merging of other work that maintainer might think has higher priority. E.g., not-achow merging a refactoring PR in wallet which conflicts with other PRs that achow is planning to merge soon / has indicated are close to ready.

I think the section is needed, and it is the documentation “that some individuals may elect to focus on a particular area of the codebase due to having subject matter expertise in that area but are able to review and merge PRs from any section of the codebase.”

josibake commented at 6:42 pm on July 7, 2022:

sorry, i could have phrased that better: i do think there should be a section explaining what a scoped maintainer is but i think it would be less confusing if it were reduced to something along the lines of “Individuals may elect to focus on a particular area of the codebase due to having subject matter expertise in that area but are able to review and merge PRs from any section of the codebase.”

given that there doesn’t currently exist a formalized distinction between general and scoped maintainer, the full paragraph seems a bit premature for documentation. the reality is, regardless of what a maintainer’s stated focus is, they are able to review and merge any PR in the codebase. i think the documentation should clearly state the reality and if we later decided on a more formalized distinction, the document can be updated.

in MAINTAINERS.md:98 in c18b9bca89 outdated

85+## Nomination and Selection of Maintainers
86+
87+Being a Maintainer is not an honor (but it may be honorable to serve as one).
88+Many regular contributors who are capable or experienced enough choose not to
89+be a Maintainer. Similarly, just because a regular contributor has reached a
90+certain level of experience does not mean that they "deserve" to be a maintainer.

flack commented at 11:32 am on July 14, 2022:

Can this whole paragraph be removed? It reads more like an editorial, and all the actual info is in the following paragraphs anyway.

edit: Removed probably inappropriate attempt at humor

JeremyRubin commented at 7:28 pm on July 18, 2022:

my litmus test is if it is helpful for someone to read this if they are trying to understand

what a maintainer is / does
how someone became a maintainer

is the document improved, for someone with little context into the project , whithout this section?

flack commented at 7:51 pm on July 18, 2022:

I mean strictly speaking this paragraph doesn’t explain what a maintainer is, just what a maintainer is not (and who did not become a maintainer). I guess what the paragraph is trying to convey is that it isn’t some career goal that you obtain when you reach Bitcoin programmer level 35. Which is fine to mention, but at least I wouldn’t open the section with that. And if at all possible I would avoid words like “honour” in a technical role description (maybe replace with “achievement award” or similar?), but maybe that’s just me.

JeremyRubin commented at 6:02 pm on August 13, 2022:

it took me a while to find this (I blame british/american spelling differences). This is the reason why I included the bit about honor/honour in particular.

https://www.coindesk.com/markets/2014/04/08/gavin-andresen-steps-down-as-bitcoins-lead-developer/

“I had noticed that Gavin had been much less active in the Github project lately, but I did not expect him to step down as lead developer. But it makes sense, there is a lot more theoretical work with regard to Bitcoin and cryptocurrency in general making it a full-time job just to keep up with that (and giving talks, travelling, and such). On the other hand, I have been effectively the maintainer of the code for quite a while. In practice nothing changes.” He added that it was “an honour to be entrusted with the role” and said the positive responses he had received were a reminder that his work was appreciated in the community: “it’s easy to forget that sometimes with all the trolling and meanness going on around Bitcoin!”

I felt it is important to clarify that it’s not the type of honor that is like an award, even though there is honor in it.

flack commented at 6:37 pm on August 13, 2022:

To me, “there is honor in it” sounds like something a politician would say to get young people to do something stupid (like invade some foreign country), but I guess we were just socialised differently. But before I stop beating that dead horse, let me point out that the quote you posted directly contradicts your proposed text:

He added that it was “an honour to be entrusted with the role”

vs.

Being a Maintainer is not an honor

(and doing something honorably is not the same thing as feeling honored to be entrusted with something. But again, I’ll stop now, our usage of/associations with that word are just different I guess)

JeremyRubin commented at 6:45 pm on August 13, 2022:

it’s fine to discuss and i’m interested to meaningfully incorporate your feedback.

perhaps one of:

Being a Maintainer is not a bestowed honor (but it may be honorable to serve as one).

Being a Maintainer is not an honorific title (but it may be honorable to serve as one).

Being a Maintainer is not an “honorable status” (but it may be honorable to serve as one).

Being a Maintainer is not an “honorable appointment” (but it may be honorable to serve as one).

Paired with one of:

(but it may be honorable to serve as one).

.

, not to be confused with the honorability of serving in this role which is beyond the scope of this document.

bitcoin deleted a comment on Jul 14, 2022

in MAINTAINERS.md:18 in c18b9bca89 outdated

13+|   @achow101  | wallet, PSBTs |
14+|   @fanquake | general, tooling, build system |
15+|   @hebasto | UI/UX |
16+|   @laanwj | general |
17+|   @marcofalke | general |
18+|   @sipa | general, peer-to-peer, consensus, cryptography |

kristapsk commented at 2:17 pm on July 15, 2022:

Should @sipa still be in this list? #25568 https://twitter.com/pwuille/status/1545159642447335424

JeremyRubin commented at 7:28 pm on July 18, 2022:

removed

murchandamus commented at 2:44 pm on July 15, 2022: contributor

It’s not clear to me what expected benefit this document is aiming to achieve. It feels that this proposes to add more bureaucracy and formality to processes that are orthogonal to the issues in practice. It seems more likely to me that this document would become a basis for arguments rather than substantially benefit anyone or improve processes.

MarcoFalke commented at 2:51 pm on July 15, 2022: member

@Xekyo Conceptually, I think there can be minor fixups to the existing docs to clarify some stuff that was discussed in the IRC meeting that motivated this pull requests.

On this pull request: I am wondering if the author is still working on this? For now, it might be best to not provide further review feedback (to avoid piling up comments that are unlikely to be addressed) and wait for the current feedback to be addressed first.

kristapsk commented at 3:52 pm on July 15, 2022: contributor

It’s not clear to me what expected benefit this document is aiming to achieve.

IMO having public list of all Bitcoin Core maintainers (people with commit access to the repo) somewhere is very important and good thing. Don’t think we had something like this always up to date anywhere before? (some time ago somebody asked me this question, about all maintainers, and I couldn’t find complete list)

ghost commented at 4:01 pm on July 15, 2022: none

It’s not clear to me what expected benefit this document is aiming to achieve.

IMO having public list of all Bitcoin Core maintainers (people with commit access to the repo) somewhere is very important and good thing. Don’t think we had something like this always up to date anywhere before? (some time ago somebody asked me this question, about all maintainers, and I couldn’t find complete list)

List of maintainers, a better process to decide new maintainers and clarity on a few things that were discussed recently in IRC meeting and https://github.com/bitcoin/bitcoin/pull/25524

JeremyRubin commented at 7:44 pm on July 18, 2022: contributor

attempted to incorporate feedback, prioritizing that from extant maintainers.

in MAINTAINERS.md:18 in 3cfadc3273 outdated

13+|   @achow101  | wallet, PSBTs |
14+|   @fanquake | general, tooling, build system |
15+|   @hebasto | UI/UX |
16+|   @laanwj | general |
17+|   @marcofalke | general, tests, QA |
18+|   @glozow | general, transaction relay and mempool policy |

unknown commented at 8:25 pm on July 18, 2022:

Fixed the table markdown and changed the order of maintainers according to dates

0| Name  | Focus Areas |
1| ------------- | ------------- |
2| [@laanwj](/bitcoin-bitcoin/contributor/laanwj/) | General  |
3| [@MarcoFalke](/bitcoin-bitcoin/contributor/marcofalke/)   | General, tests, QA  |
4| [@fanquake](/bitcoin-bitcoin/contributor/fanquake/)   | General, tooling, build system  |
5| [@hebasto](/bitcoin-bitcoin/contributor/hebasto/)    | UI/UX  |
6| [@achow101](/bitcoin-bitcoin/contributor/achow101/)    | Wallet, PSBTs  |
7| [@glozow](/bitcoin-bitcoin/contributor/glozow/)     | General, transaction relay and mempool policy |

fanquake added the label Docs on Jul 19, 2022

murchandamus commented at 5:52 pm on August 2, 2022: contributor

It’s not clear to me what expected benefit this document is aiming to achieve.

IMO having public list of all Bitcoin Core maintainers (people with commit access to the repo) somewhere is very important and good thing. Don’t think we had something like this always up to date anywhere before? @kristapsk: A list of maintainers is here already: https://github.com/bitcoin/bitcoin/blob/aeab1b42e67cc8146bfc7d127d15633bd652fe60/REVIEWERS#L20

ghost commented at 9:57 pm on August 2, 2022: none

It was removed in #25613 and based on PR description scope of this PR appears to be more than just a list

in MAINTAINERS.md:35 in 3cfadc3273 outdated

30+
31+Whether a pull request is merged into Bitcoin Core rests with the project merge
32+maintainers.
33+
34+Maintainers will take into consideration if a patch is in line with the general
35+principles of the project; meets the minimum standards for inclusion; and will

instagibbs commented at 1:30 pm on August 12, 2022:

grammar micronit for later: you probably don’t want ; in this sentence.

in MAINTAINERS.md:68 in 3cfadc3273 outdated

63+bug-not-feature if it confers any additional responsibility or privilege over
64+other General Maintainers, and may be explicitly deprecated in the future.
65+
66+## Self Merges
67+
68+Typically, a PR authored by a maintainer will be merged by a different

instagibbs commented at 1:34 pm on August 12, 2022:

This is new policy, which probably would have to be iterated on a few times to make sure it strikes the right balance of accountability and light-weight process.

instagibbs commented at 1:35 pm on August 12, 2022: member

concept ACK on a document like this, but it’s probably gonna have to be split up between descriptive(what is ostensibly followed today) and prescriptive(more aspirational, such as self-merge section), so we can debate the prescriptive portion a bit more.

MarcoFalke commented at 2:11 pm on August 12, 2022: member

Are you still working on this to address feedback?

JeremyRubin commented at 4:26 pm on August 12, 2022: contributor

@MarcoFalke i’m not sure what you mean; the most recent non-formatting feedback was from 3 hours ago, and before that, all other feedback had been meaningfully addressed / responded to.

MarcoFalke commented at 7:44 am on August 13, 2022: member

For example my comment from July 7th wasn’t addressed (among others): #25560 (review) , also the CI is failing.

michaelfolkson commented at 10:08 am on August 13, 2022: contributor

@Xekyo Conceptually, I think there can be minor fixups to the existing docs to clarify some stuff that was discussed in the IRC meeting that motivated this pull requests.

There seems to be a general desire from numerous contributors and maintainers including Marco above to reduce the scope of the changes in this PR and instead just to complete minor fixups. There are various reasons for this but they include not wanting to impose greater maintenance burden (i.e. requiring regular changes to this doc as things change e.g. maintainer focus) and not wanting to tie maintainers up in processes that not only take extra time but also potentially make things worse than the status quo (a maintainer needing to auto merge another maintainer’s PRs to get round a restriction that a maintainer can’t merge their own PR no matter how much review it has).

I strongly believe that when there are controversies or differences of viewpoint that there should be open discussion (e.g. on IRC) and maintainers should be expected to engage with that discussion (i.e. if they merge something that informed contributors don’t think should have been merged or merge something prematurely without sufficient review). But I don’t believe in tying them up in counterproductive processes and trying to catch them out because they didn’t follow a long list of processes imposed by this doc.

Hence I’m a Concept ACK on minor fixups as Marco says but if the PR author disagrees and thinks more expansive changes are required I’m an Approach NACK on that. I don’t want to speak on behalf of others but that seems to be the general sentiment from most other contributors’ comments too.

I also don’t want to engage in a flame fight on this so I won’t be commenting on this PR again. Just wanted to pop up and say my 2 cents as I’m not sure this is heading in a productive direction.

ghost commented at 3:48 pm on August 13, 2022: none

@Xekyo Conceptually, I think there can be minor fixups to the existing docs to clarify some stuff that was discussed in the IRC meeting that motivated this pull requests. There seems to be a general desire from numerous contributors and maintainers including Marco above to reduce the scope of the changes in this PR and instead just to complete minor fixups.

There is no maintainers.md file right now in bitcoin core master branch. I could not find any other doc file that could be used to add things discussed in IRC and #25524 with minor fixups.

TL;DR of IRC

19:28 i think it would make sense to draft a Job Description of what the appointment is for and scoping, and have people ack that when it exists. 19:34 so I think as a project we should communicate more clearly about what exactly is being done and expected

TL;DR about changes in process from PR 25524

There have been a number of months since the last maintainer was added (achow101) and plenty of opportunity to discuss changing the process for adding maintainers (whether they should have to commit to one-to-one meetings with certain contributors or not etc).

#25524 (comment)

Surely there are imperfections (of process documentation and otherwise), but I highly doubt that refusing glozow as a maintainer is going to fix them.

#25524 (comment)

regarding the NACKs, it seems they are less about glozow and more about larger, difficult-to-address concerns: having multiple maintainers funded by the same entity could be bad the process of adding maintainers could be better concerns about age, social circles, etc of maintainers

#25524 (comment)

I would say it’s better to think maintenance as scoped, or at least to know who merges what well-defined. Not only between maintainers (to avoid merges not meeting the usual review bar) but also in the eyes of the contributors to know whom should be the default interlocutor in case of questions on such or such subsystem. As already echoed, I also believe it would be great to have any consensus change to have a review/merge process of its own (where maybe the merge could be counter-signed by a N-of-M of the maintainers).

#25524 (comment)

JeremyRubin commented at 5:30 pm on August 13, 2022: contributor

For example my comment from July 7th wasn’t addressed (among others): #25560 (comment) , also the CI is failing.

I wasn’t sure that comment needed a direct response as the question was already answered in the text immediately following, I’ve responded to that comment nevertheless. #25560 (comment) was intended to note that I incorporated the majority of the actionable feedback and the document was ready for re-review.

w.r.t. the failing build, it was a lint error that I intended to fix on a subsequent patch, which I’ve done now. I didn’t think that a lint error was the blocking/bottleneck for a documentation change which still needed more buy-in before merge, but perhaps you think the document is closer to ready for merge than I appreciated. Apologies if I misinterpreted, let me know if I should remover WIP.

JeremyRubin commented at 5:45 pm on August 13, 2022: contributor

@instagibbs that seems reasonable; i tried to synthesize a bit the desires people have around things like that but maybe consensus can be reached on a pared down version of this document while debate continues on a more expansive document.

JeremyRubin force-pushed on Aug 13, 2022

Add a minimal MAINTAINERS.md file and desribe the current role of maintainer more directly, to be clarified/defined in further PRs. 87fb345c13

DrahtBot commented at 4:50 am on August 14, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

MarcoFalke commented at 1:31 pm on August 15, 2022: member

The pull request still has [WIP] in the title (which forbids a merge). If you yourself aren’t confident this is rfm, even after all the feedback (and apparently you addressing it), then I think it is too early to ask for review? Same goes for #25839. Or are you asking for Concept ACKs at this point?

JeremyRubin commented at 3:53 pm on August 15, 2022: contributor

I don’t think the normal review structure applies in this case, since it is a project organization change v.s. a code change.

The point of being WIP is that if there is more feedback than it is very happy to be integrated in. WIP isn’t an issue of “confidence”, it’s an issue of process, and it’s not certain to me by what process a document would be approved or by who. E.g., is this something to get a quorum of some sort of contributors to weigh in on? All of the current maintainers? Since I began this document, we’ve added 1 contributor and removed 2, so clearly it’s a moving target. Perhaps, as the maintainer engaging on this PR, you could shed light on what you think the “merge criteria” are on this, and I’d be happy to appropriately remove WIP. In terms of confidence I think that #25839 is mergeable (although should get some review to check it) as per @instagibbs feedback that the bits that are nor purely descriptive ought to be debated, that version is more or less descriptive (I think).

Update processes in MAINTAINERS.md 2e5f28046d

JeremyRubin force-pushed on Aug 15, 2022

ghost commented at 7:35 pm on August 20, 2022: none

I have no strong opinion on this pull request anymore or a mild NACK because of the following points about process that I wanted to improve:

It does not include anything about funding of maintainers, privacy and security.
Call for maintainers is possible without this doc and there would be no change.
Self merging would not stop even if mentioned in this doc. I review every pull request merged in bitcoin core and will mention if I see something wrong with a self merge.
Nobody stopped anyone from sharing their opinion in IRC channel or GitHub in the recent meeting about adding another maintainer.

MarcoFalke commented at 6:45 am on August 29, 2022: member

Closing for now. Let’s focus on #25839 first and then reopen this one.

MarcoFalke closed this on Aug 29, 2022

bitcoin locked this on Aug 29, 2023

[WIP] Add a MAINTAINERS.md file and desribe the role of maintainer more directly #25560

TL;DR of IRC

TL;DR about changes in process from PR 25524

Conflicts