Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 7 additions & 9 deletions CONTRIBUTING.md
Expand Up @@ -10,9 +10,10 @@ First, in terms of structure, there is no particular concept of "Bitcoin Core
developers" in the sense of privileged people. Open source often naturally
revolves around a meritocracy where contributors earn trust from the developer
community over time. Nevertheless, some hierarchy is necessary for practical
purposes. As such, there are repository maintainers who are responsible for
merging pull requests, the [release cycle](/doc/release-process.md), and
moderation.
purposes. As such, there are repository [maintainers](/MAINTAINERS.md) who are
responsible for merging pull requests, the [release
cycle](/doc/release-process.md), and moderation. See
[MAINTAINERS.md](/MAINTAINERS.md) for more information on maintainers.

Getting Started
---------------
Expand Down Expand Up @@ -293,12 +294,6 @@ The following applies to code changes to the Bitcoin Core project (and related
projects such as libsecp256k1), and is not to be confused with overall Bitcoin
Network Protocol consensus changes.

Whether a pull request is merged into Bitcoin Core rests with the project merge
maintainers.

Maintainers will take into consideration if a patch is in line with the general
principles of the project; meets the minimum standards for inclusion; and will
judge the general consensus of contributors.

In general, all pull requests must:

Expand All @@ -319,6 +314,9 @@ be different, one should be prepared to expend more time and effort than for
other kinds of patches because of increased peer review and consensus building
requirements.

Maintainers fulfil the role of evaluating PRs having met the above conditions
and merging, as detailed in [MAINTAINERS.md](/MAINTAINERS.md).


### Peer Review

Expand Down
161 changes: 161 additions & 0 deletions MAINTAINERS.md
@@ -0,0 +1,161 @@
# Maintainers

This document serves to clarify the responsibilities of a maintainer as well as
serve as a wiki for maintainers to link to / include resources.


## Current Maintainers

The current maintainers are:

| Name | Focus Areas |
| ------------- | ------------- |
| @laanwj | General |
| @MarcoFalke | General, tests, QA |
| @fanquake | General, tooling, build system |
| @hebasto | UI/UX |
| @achow101 | Wallet, PSBTs |
| @glozow | General, transaction relay and mempool policy |



## Regular Contributors, Scoped Maintainers, General Maintainers, and Lead Maintainers

The title of Maintainer in Bitcoin is more of a "janitorial responsibility"
than a position of privilege. However, the role does include permissions on the
repository, and connotes a general sense of stewardship over the project.
Comment on lines +25 to +26
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



Largely, the role of maintainers is to decide to merge Pull Requests.

Whether a pull request is merged into Bitcoin Core rests with the project merge
maintainers.

Maintainers will take into consideration if a patch is in line with the general
principles of the project, meets the minimum standards for inclusion, and will
judge the general consensus of contributors. The review process that
establishes general consensus is detailed in
[CONTRIBUTING.mg](/CONTRIBUTING.md).

Thus maintainers role is not directly elevated above that of regular
contributors, as maintainers serve mostly as arbiters of if changes are
acceptable to the contributors to the project in aggregate.

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.
Comment on lines +44 to +54
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Historically, one Bitcoin's maintainers was a designated "Lead Maintainer",
which was most recently @laanwj. However, [in a blog
post](https://laanwj.github.io/2021/01/21/decentralize.html), @laanwj stated
that they wanted to take more of a background role as a maintainer and have not
"passed the baton" to a successor as a Lead Maintainer, and instead focused on
decentralizing the maintainership ecosystem. As such, while the unofficial
title of Lead Maintainer still is held by @laanwj, it should be considered a
bug-not-feature if it confers any additional responsibility or privilege over
other General Maintainers, and may be explicitly deprecated in the future.

## Self Merges

Typically, a PR authored by a maintainer will be merged by a different
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

maintainer. However, given that there may not be another maintainer who
regularly works on the section of code or is familiar, self-merge is not
forbidden. However, as a guard, self merges should usually only happen if the
following criteria has been met:

1. The PR has been reviewed and ACK'd by other regular contributors familiar
with that section of the code.
1. There are no NACKs being overruled on that PR.
1. The PR is at least 1 week old, to permit time for review.
1. The maintainer should communicate intent to self-merge 48 hours before doing
so. (Regular contributors may also indicate if they think a PR can be merged by commenting "Ready for Merge").


It is preferred that maintainers self-merge as opposed to privately allying
with another maintainer to avoid the self-merge decorum. This promotes
transparency and minimizes bias in the review process. Tracking self-merges
also helps the project demonstrate need for additional maintainers in areas where they might be frequent, which can help solve the issues around self-merges.


These rules aren't hard and fast, but are considered a best practice to avoid
self-merge as a convenience.



## Nomination and Selection of Maintainers

Being a Maintainer is not an honor (but it may be honorable to serve as one).
Many regular contributors who are capable or experienced enough choose not to
be a Maintainer. Similarly, just because a regular contributor has reached a
certain level of experience does not mean that they "deserve" to be a maintainer.
Copy link
Contributor

@flack flack Jul 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. what a maintainer is / does
  2. how someone became a maintainer

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Maintainers are added when there is a demonstrated need. For there to be a new
maintainer, first an issue should be created with a "Call for Maintainer" (C4M)
by one of the existing maintainers or regular contributors. C4Ms should specify
if the call is for a General Maintainer, or for a Scoped Maintainer. If the
role is for a Scoped Maintainer, the C4M should describe the need for a new
maintainer, and link to some Pull Requests (or other material) that would be
relevant for such a maintainer to be responsible for.

Following the C4M, anyone may respond to the issue with a nomination for the
position and a description of their qualifications. Self-nomination is
encouraged. ACKs/co-signs of other nominations are also encouraged so that the
maintainers may get a sense of developer sentiment towards nominees.
Nominations must be accepted by the nominee. After 2 weeks from the date of
issue creation, assuming there is at least 1 nomination, the maintainers may
select one or more candidate from the nominee pool at their discretion.


The selected candidate should create a PR for adding their key to the
repository and to the above table (see
https://github.com/bitcoin/bitcoin/pull/23798 for example). During / Before
the PR, regular contributors are encouraged to correspond with candidate(s)
publicly about any concerns or questions. Regular contributors are also
encouraged to raise any concerns about the maintainers selection of candidate
against other nominated candidates not selected.

At the discretion of the maintainers, and no sooner than 2 weeks after the PR
is opened, the candidate(s) PR may be merged.


## Maintainer Prioritizations

While the maintainers are ultimately not accountable to any particular roadmap,
nor can make regular contributors focus on one area or another, practically the
views and prioritizations of maintainers have an impact on what gets reviewed,
merged, and the general direction of the project.


However, to discourage people from looking to maintainers for any roadmap,
maintainers are not required to share what they prioritize. Publicizing or
listing in the project documentation maintainer's personal prioritizations
might serve to discourage other contributors from working on activities outside
the maintainer's areas of interests, reducing the diversity of thought in the
project. To avoid this centralization effect around the maintainers, it is best
that the maintainers project neutrality with respect to any particular personal
goal for the project.

Maintainers are not forbidden from producing a document detailing their
prioritizations, e.g., on their personal blog. But it will not be considered to
be a post in their capacity as a maintainer.

## Maintainers "Moving On"

Maintainers may choose to cease being a maintainer at any time without notice.
Where possible, maintainers should try to gracefully transition out of their
role, until a new maintainer has been selected to replace them (if needed).

## Maintainer Removal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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


There is no public or defined process to remove a maintainer.

If a maintainer should be removed, individuals should either remark publicly or
confer with other maintainers privately to suggest their removal.