Reviewing is not only the most effective way to contribute as a new contributor to Bitcoin, it is also an important way to influence consensus on changes. Just as our daily choices and actions are the best way we can influence things, our review choices matter profoundly and cumulatively to Bitcoin.
The Current State of Affairs
Several hundred pull requests are open at any given moment in Bitcoin Core waiting for review and testing, and the stack continues to grow higher — 280 in November 2019, 380 at the end of April 2020, and 400 at the start of June 2020, when the number of PRs in the blockers list also reached a record high number.
Review is currently not keeping pace with PRs. The same is true for open issues, which generally keep growing in number, outside of occasional exemplary efforts to weed and close stale ones.
Valuable, high-priority PRs can be seen sitting for months or even years without enough reviewers, subjecting their authors to frustration, discouragement, and rebase hell until they abandon working on them.
This is not a criticism of the maintainers. They're doing a great job. They need help: more review and testing.
The number of people doing good, consistent review and testing is surprisingly low. On any given day, we see a handful of regular contributors reviewing on a daily basis. Of these, fewer still have the situation and predisposition to tackle review of the harder or larger PRs. That leaves us with very few consistent reviewers present most days.
When I describe this situation to people, even to established ones in the Bitcoin space, they tend to be astonished at how few people the system is depending on.
This state of affairs creates several problems:
- High-priority, larger, or harder-to-review PRs can go for months (and even years) without sufficient review.
- Regular PRs that provide good but non-urgent improvements or prevention can sometimes sit for months without review, or until an issue they address causes pain from a bug, regression, or vulnerability.
- Any PR of non-immediate urgency beyond a certain size, number of commits, or level of complexity is a candidate for needing to be separated into bite-sized PRs in order to attract reviewers, which multiplies the number of PRs. While small focused PRs can be good, and separating large PRs into logical small ones can improve the code, some changes are better done in one go.
- PR authors may be incentivized to open more trivial PRs that have a better chance of being reviewed and merged within a reasonable time (until the author tires of making this sort of contribution and aspires to greater things, before being discouraged again).
- Things are often abandoned or never quite finished. Some contributors leave or take long periods of absence from the project out of frustration. Other people shake it off but feel recurring discouragement. Yes, it's open source, but I hope we can improve the situation.
- Managing context, relevance, and being humble and kind can make a big difference. Bitcoin Core code review is a social process. Having PRs reviewed without being too annoying (hopefully by helping others and doing consistent review yourself) can be an art.
- Nevertheless, the project and its reviewers run a continual risk of mal-investment — scarce review resources going to poorly done, annoying, insufficiently researched, or even dangerous changes, or ones that add technical debt — rather than catching a regression or vulnerability, fixing issues, improving performance, increasing test coverage, finishing ongoing projects, doing long-term work, and other priorities.
- Above all, not enough contributors put the project first, do consistent review, and follow the rule of thumb of 5 to 15 reviews or issues handled per PR opened.
What to Review
Given this state of affairs, which PRs we review with our limited time is an important choice, and one that might be under-appreciated.
The PRs we review represent:
- what areas we contribute to
- which project priorities matter to us
- what kind of PRs we incentivize
- ultimately, which behavior we reward
With this in mind, I try to pay more attention to selecting the best PRs to review. It's a juggling act, to be sure, and guilt-driven review is also a real (and valid) driver. Nevertheless, here are my criteria:
- What: choosing harder, high-value PRs over new, easy or trivial ones (knowing that the latter will see review anyway) unless they are useful, interesting, or an area I know well.
- Who: rewarding people who do consistent or outstanding review/issue testing or who have been kind and selfless.
- It's also generally more enjoyable to review people who respond well or at least reasonably nicely to your review feedback. Kindness is free!
- If a PR seems to be suffering from lack of review or author motivation and needs a nudge forward, that's even more reason to review it. A simple nudge can get the ball rolling and be surprisingly powerful!
I also try to avoid reviewing these sorts of PRs:
- PRs that are unfinished, or lacking sufficient research, or that don't build cleanly, or the tests don't run.
- PRs that are poorly done.
- PRs whose goals or approach I don't agree with or that seem dangerous (another option is to comment on it or NACK as a last resort).
- PRs where bikeshedding, arguing, or politics/signaling/tribalism is taking place; these PRs attract a lot of attention anyway.
- PRs where there is already a crowd of reviewers, unless it's an area I know well and can add value (and the PR doesn't fall into one of the above categories).
Sure, writing code and opening PRs can be fun and superficially newsworthy. But it needs to be accompanied by a project-sustainable level of reviewing and testing or it becomes immature and short-sighted. A balance here seems important. Reviewing, issue testing, release candidate testing, and building guix/gitian signatures are vital to Bitcoin and not optional to the health of the project. We need to reward selfless review and testing, kindness, and helping others.
If you are a newer or less-experienced contributor, please don't let that be an excuse or discourage you from testing issues, release candidates, and reviewing. These activities will teach you more and be of higher value to the project — and to you — than just opening PRs.
Read through the recently-active PRs and issues and review or test ones that are somewhat accessible — ideally at the edge of your current reach — and ramp up gradually to the harder and important high-priority ones.
As Steve Lee of Spiral recently wrote: "If literally all a developer did was review hard but important PRs it'd be incredibly valuable." It would be wonderful if more open source contributors and sponsors/donors shared this view.
What To Do
This is open source; it's ad hoc and free-flowing. These ideas are proposed more as guidelines than as rules; as goals to think about and consider.
Contributing is not only about commits. It would be great to get away from the idea of telling people that contributing equals opening PRs. Contributing is much more than this and it could be reflected in what we say and do.
Continue to do more reviewing and issue testing than adding PRs to the backlog. Reviewing 5 to 15 PRs (or testing 5-15 issues) for every PR you open is a good range. Communicate that norm to people.
Before and after opening a PR in a particular domain, review the other open PRs in the same domain. Presumably you have or could use more domain expertise. Gather and share it by reviewing and testing the related PRs.
Find a way to reward people who are doing consistent review or testing. It's a harder and less-celebrated path. These people need support. Review their work. Help them. Donate Bitcoin to them.