doc: Add CODEOWNERS file to automatically nominate PR reviewers #18949

This PR brings back and builds on #17094. GitHub uses a CODEOWNERS magic file to automatically add tagged contributors to the "Reviewers" list for a PR.

The goal of this is to make use of GitHub's suggested reviewers feature and not to confer ownership or give veto power to specific people. It would be better if this file could be named CODEREVIEWERS, but alas, that wouldn't work. The idea of a NAGFILE was proposed in Bitcoin Core Dev meeting in 2018. While this GitHub implementation has some complications, it's a step towards realizing the promise of automating "reviewing begging" and (hopefully) positively impacting the review process as a whole.

Of secondary value, this file can serve as documentation for who the maintainers are and who it might be smart to check with for certain areas of code/features (i.e., fuzzing, PSBT, and Bech32) -- this is helpful information for new contributors.

• The first commit is taken from #17094
• The second commit adds comments and expands the list of reviewers based on the suggestions and comments from that PR
• ~The third WIP commit~ This commit also uses the doc dir as an example of granular assignments based on lines of codes ~contributed~ written and/or general engagement with the project. (If interested, here is a report for most lines of code per author for each file). The pro of this level of detail is that the best reviewer is more likely to be nominated. The con is that it will create churn as files are renamed, new files are added, or reviewers want to be added or removed.

Some open questions:

• How often should this file be changed?
• What level of history does one need have on the project before being added to this file? When does it make sense to remove a reviewer?
• These review notifications can cause a lot of noise and automatically subscribes the requested reviewer to the thread. A GitHub Team based approach would allow for adding or removing reviewers without modifying this file; however, this comes along with its own set of problems, including granting write access. Other projects have used bots to sidestep this.

I think what @MarcoFalke suggested here: #17094 (comment) is a good idea, this can be a way also for contributors to subscribe to PRs touching specific parts of the code, that way they can know if something might conflict with their PRs or they can know when there's something they feel comfortable reviewing.

I do wonder though how it will look like when each folder might have a long list of people, Although if it's a problem we maybe could use githubs teams feature, and create teams like validation, crypto, p2p, wallet etc. and subscribe those teams and then add people to the teams accordingly, but maybe that's an overkill.

Could be worth trying but I'll defer to the maintainers. IIUC this would be a guideline/suggestion and not a limiter, but I'm not sure LOC "contributed" is a great metric. Review time spent on the codebase and willingness to invest time reviewing a particular PR may both be factors as well, but which would not be accounted for. Also, PRs can be improved by repeated rounds of reviewing but in the end only the PR author is taken into account in the LOC stats. It may be good for these factors to be stated if this file is intended to be a reference for new contributors. A final question is if the updating and maintainance effort would be worth the value added.

("contributed" in quotes also because sometimes a refactoring or code move changes the git history to look like the latest committer wrote the code, but I digress... this all just means that active updating and judgement might be important)

Code review ACK cfd2792a375025b05508fe954e5c0cdfcf41a666. This looks great, and very thoughtfully implemented. I'd like to be added (in this PR or after) as a reviewer for src/interfaces/, and probably a few other places too like src/util/settings/.

On the concept, I think it's a potentially promising experiment. We'll have to see how it works and how people change the file and use the feature to match their preferences. Maybe it will even encourage less monolithic code organization and breaking up giant files like wallet.cpp into modules that people want to receive review requests for. In past projects, I've found systems for tracking review requests very useful, not just as a guide for what to review next, but also for requesting reviewers I might not have thought of, and for steering new requests away from people who are already overloaded with too many existing ones

Concept ACK, I think that's nice for newcomers, specially with regards to less visible part of the project like build or doc. Though I do think that name CODEOWNERS is pretty deceiving.

I do like Elichai idea of notifications list to subscribe by labels.

Concept ACK

We can't be sure how this will work out long term but I think this is worth a try. My biggest concern with this would be that a reviewer that is added to a PR might be a bottleneck. There just needs to be clarity that maintainers don't need to wait for their reviews when something is RFM and if the automatically added reviewers don't have the time or feel they are not the right person they remove themselves. So good job on clarifying what this is and what it isn't!

Concept ACK

Concept ACK, I think there is nothing to lose trying this out for a few weeks or months and keeping it if it works.

Slightly stronger Concept ACK, if my name was removed from the list, since I will get notifications either way.

Thank you for the review and your suggestions. I've made most of the corrections from the comments. @jonatack

I'm not sure LOC "contributed" is a great metric

I've substituted "written" for "contributed" in the commit message and PR description. The purpose of this proposal is to encourage peer review, so it's an error on my part if my wording misses the mark. I agree LOC may not be a great metric and will lead to some poor initial tagging, but it's an easy way to get up and running. Open to other metrics or suggestions as well. @ryanofsky I've added you to the file for the requested files/dirs (and thanks for the encouragement). @ariard I'm missing where the suggestion is made to subscribe by labels. I don't think that's possible without a bot, but GitHub teams come native to the feature. @MarcoFalke Happy to remove anyone that requests it. The unit and functional test suites will probably have to be broken down into more granular assignments for others to pick it up since you were assigned the whole framework.

Code review ACK cfd604a1a263e80f88596c043c41dc6b15dd4c9a. Changes since previous review: names, dates, places, things. So much

Slightly stronger Concept ACK, if my name was removed from the list, since I will get notifications either way.

Same. I already get emails for everything that happens on this repo. So can probably skip this if it's going to add duplicates.

Slightly stronger Concept ACK, if my name was removed from the list, since I will get notifications either way.

Same. I already get emails for everything that happens on this repo. So can probably skip this if it's going to add duplicates.

I'd be really surprised if github started sending duplicate notifications. I think listing names here could not just be useful for automated emails, but also for project documentation, to give people an idea of who they could ping if they need help with an issue or pull request. Of course if you don't want to be contacted or receive notifications your name shouldn't be here, but it sounds like that's a different issue?

I'd be really surprised if github started sending duplicate notifications

I thought this was the case last time I received a review request, but the behaviour may have changed. I've created a repo here if someone wants to open a PR and test the behaviour.

but also for project documentation, to give people an idea of who they could ping if they need help with an issue or pull request.

In that case also put me down for symbol/security-check scripts/tests and glibc compat.

I thought this was the case last time I received a review request, but the behaviour may have changed.

Looks like this is still the case; you'll get an email for the PR, and then a followup with the review request (thanks @elichai):

<img width="716" alt="emails" src="https://user-images.githubusercontent.com/863730/82414598-df9d4d00-9aa9-11ea-84f5-4d26ba54390d.png">

However I think I care about this less now. Feel free to add me for the files I mentioned above.

ACK 375dcd15566213c4d58e05a9738c8b5b1360a0d1

@MarcoFalke removed from test framework in 8666f65 @fanquake additions in 375dcd1

Code review ACK 375dcd15566213c4d58e05a9738c8b5b1360a0d1. Various tweaks since last

I am thinking about merging this next week, but I am wondering if there are any outstanding questions or problems that need to be solved first?

Concept ACK. Worth trying to see what the real effects will be. If it goes well, I could see more files or parts of the codebase added to the list.

Potential positives

• may stimulate review or accelerate it
• may provide info for newer people on who they can ask for help

Potential negatives

• may concentrate requests on fewer people
• the info for newer people will be incomplete if some don't wish to be on the list
• adds process (PRs to add/remove people?)

Since tagged reviewers included in this file will receive notifications, I'd feel more comfortable if everyone listed opted-in before merging so that there aren't any surprises. Namely @sipsorcery, @practicalswift, @meshcollider, @achow101, and @sipa should have a chance to say no.

Reflected in 92f83731d76c0adc78f815aecac52eb0a7fcde91, I reached out to some folks out-of-band and made their requested adjustments. I also squashed the last commit since the concept has, for the most part, been ACKed.

ACK 92f83731d76c0adc78f815aecac52eb0a7fcde91

ACK 92f83731d76c0adc78f815aecac52eb0a7fcde91 :)

ACK 92f83731d76c0adc78f815aecac52eb0a7fcde91.

ACK 92f83731d76c0adc78f815aecac52eb0a7fcde91

ACK 92f83731d76c0adc78f815aecac52eb0a7fcde91

@achow101 @meshcollider @sipa Progress here is currently blocked unless you select one of the following options:

• Remove your name for now
• Keep your name in for now

@adamjonas If you have to retouch, feel free to add me to these files (if not too granular and at your discretion):

LOC touched:

• src/bitcoin-cli.cpp
• test/functional/feature_asmap.py
• test/functional/interface_bitcoin_cli.py
• test/functional/tool_wallet.py

(I was tempted to propose these also, as I would not mind being notified, but would prefer to wait until others are added to them first. Posting this as a memo:

• src/net.h/cpp
• src/net_processing.h/cpp
• src/protocol.h/cpp
• src/txmempool.h/cpp
• src/validation.h/cpp
• src/validationinterface.h/cpp)

How does GitHub behave without this file? Does adding it lose some kind of automated suggestion of reviewers?

@luke-jr This file adds automated reviewer suggestions. Without this file, the status quo remains the same.

Touched-up with additional files for @jonatack and adding @achow101 to the wallet as discussed over IRC.

Still looking for the OK from @jonasschnelli, @sipa, and @meshcollider.

If you have to retouch I would be interested in adding these:

# Coinstats
/src/node/coinstats.* [@fjahr](/bitcoin-bitcoin/contributor/fjahr/)

# Index
/src/index/ [@fjahr](/bitcoin-bitcoin/contributor/fjahr/)

Hi sorry I missed the pings, I think this will make a lot of noise but I would be happy to give it a trial run at least, and remove it later if it is overwhelming. So concept ACK from me.

@meshcollider if it's all the same, I'd rather remove you if you think it could lead to problems. I'm sure this file will require tweaks but the hope is that it adds more signal than noise. I've done the same for @jonasschnelli.

Other changes in 79042955f59bf56bc93aa93b6b69d3979a89b174 are adding @fjahr to the files he requested and @harding to the docs folder (excluding release notes).

ACK (tested the pattern notifying me using git ls-files)

Any chance I can jump on coins.*, txdb.*, and dbwrapper.* (unless it'll invalidate too many ACKs)?

Cool, thanks. ACK https://github.com/bitcoin/bitcoin/pull/18949/commits/2e2cf25ce98c4e1bca4647a2ab54cf5f38167703 from me.

Rebased, added sipa/ jameob, and removed redundant practicalswift line in the latest set of touch-ups.

re-crACK 2e2cf25ce98c4e1bca4647a2ab54cf5f38167703

https://github.com/bitcoin/bitcoin/compare/2e2cf25..a06eb03 adds src/script/script.* and groups the consensus files as requested by @sipa.

I think this is ready to go in now. Should be trivial to edit/revert if it turns out to be not useful.

@MarcoFalke, do you think it makes sense to add a reference to this in CONTRIBUTING.md? I could also include the rules for inclusion that you proposed.

Before baking this into other documentation, let's try it out first and see whether it causes any issues.