Last updated: 12 July 2020
How to Review Pull Requests in Bitcoin Core

BEFORE YOU BEGIN

This guide builds on the foundation laid by these presentations:

  1. Contributing to Bitcoin Core, a personal account by John Newbery (2017)
  2. A Gentle Introduction to Bitcoin Core Development by Jimmy Song (2017)
  3. Understanding the Technical Side of Bitcoin by Pierre Rochard (2018)
  4. A hardCORE workout / transcript / slides by Jeremy Rubin (2018)

INTRODUCTION

"Reviewing is the most effective way you can contribute as a new contributor (and also will teach you much more about the code than opening PRs)"

— John Newbery, Bitcoin Core PR Review meeting, September 4, 2019 at 13:52

Reviewing and testing can be the best ways to begin contributing to Bitcoin Core.

Experienced review and testing are regularly cited by long-term Bitcoin Core developers as both

  • resource bottlenecks, and
  • the best and most helpful way to learn and begin contributing and earning reputation in the community.

TERMINOLOGY

ACK, NACK: Definitions and origins here and here.

Nit: A trivial, often non-blocking issue.

PR: An acronym for pull request, sometimes called a merge request. A proposed change to the code or documentation in the source code repository.

WIP: An acronym for work in progress.

GENERAL

As a newcomer, the goal is to try to add value, with friendliness and humility, while learning as much as possible.

A good approach is to make it not about you, but rather "How can I best serve?"

One of the most challenging aspects facing new contributors is the breadth of the codebase and the complexity of the technologies surrounding it.

Be aware of what you don’t know; long-term contributors have years of experience and context. The community has built up a deep collective wealth of knowledge and experience. Keep in mind that your new ideas may have already been proposed or considered several times in the past.

Remember that contributor and maintainer resources are limited — ask for them carefully and respectfully. The goal is to try to give more than you take, to help more than hinder, while getting up to speed.

Try to figure things out on your own, at least sufficiently to respect others' time.

Follow the #bitcoin-core-dev IRC channel and the bitcoin-dev mailing list.

More IRC channels to follow can be found here.

Before jumping in, take plenty of time to:

Participate in the excellent Bitcoin Core PR Review Club weekly meetings on IRC. The club was launched in May 2019 by John Newbery. You'll benefit the most if you prepare for each meeting in advance by studying, building, and testing the PR under review — and then actually reviewing it! Go through the notes, questions, and logs of the previous meetings; there is gold in there.

Many newcomers begin by opening pull requests that add to the hundreds already awaiting valuable review. In many cases, it may be better to begin by reviewing the existing pull requests and starting to understand what kind of pull requests and review are most helpful, while slowly gaining the big picture.

The big picture

The big picture is more important than nits, spelling, or code style.

There are different levels of big picture review: "does this change affect behavior?" or "is this safe?" are different from "is this a good idea?" The latter may require more context, which you will slowly gain with time. Don't let that stop you from thinking about these questions and striving to review on that level.

Steps to improve understanding of the big picture:

Aim for quality over quantity and a balance between deep work and quick wins.

Documentation is important, e.g. high-level documentation of how things work and interact, clear and accurate code docs, whether a function has a good description and Doxygen documentation, test logging (both info and debug), and so on.

Test coverage is important; don't hesitate to improve or write any missing unit, functional, or fuzz tests.

Be a contributor. Help PRs move forward by reviewing, proposing tests or fixes in a helpful way, proposing to rebase, or even offering to take over the PR after months of silence. In short, help each other!

Nits

Try to avoid overly commenting in PRs about nits, minutiae and code style, particularly with PRs labeled as WIP, or when a PR has just been filed and the PR author is mainly looking for Concept ACKs and Approach ACKs, e.g. general consensus, not nitpicking.

Long-term contributors report that activity like this repels them, and it can diminish your social capital on the project. Try to understand what kind of review is needed and when to do what.

The best time for any nit comments is after the Concept/Approach ACKs and consensus on the PR, and before the PR is finalised and has tested ACKs. As Pieter Wuille wrote on IRC: "I think the most disrupting thing to a PR is getting a multitude of low level/nits/code style comments, while it's very unclear if a PR is desirable as a concept."

Give nits and style advice in a friendly, light, enabling way — as in, feel free to ignore, feel free to adjust if you happen to rebase, etc.

Keep in mind that no one is forced to take your review comments into account; it's perfectly fine for the author to reply that they don't want to do something if they feel it is outside the scope of the change, especially if your comment is nitpicky.

Scale up

When you can, scale up the difficulty and priority of the PRs you review.

You can add more value and learn more by taking the time to do deep, quality review of the high-priority PRs and the more difficult PRs. These PRs tend to intimidate people and can stagnate for months, killing their author's motivation with death by a thousand cuts from lack of quality review, nitpicking/code style bikeshedding, and rebase hell. Reviewing them well provides a true service to Bitcoin.

The process of ramping up takes time; nothing can substitute for months and years invested in gathering context and understanding from following the code, issues, pull requests, #bitcoin-core-dev IRC channel, and the bitcoin-dev mailing list.

Step by step

Keep ego and hopes out of the process. Don't take things personally and keep moving forward.

When in doubt, assume good intentions.

Be patient with people and outcomes.

Praise publicly; criticize privately and in an encouraging way.

Persistence helps. Work on it every day.

These are all much easier said than done. Be forgiving with yourself and others.

John Newbery:

  • A good rule of thumb is to review 5-15 PRs for each PR you make
  • Start small
  • Have a plan, don't spread yourself too thin
  • Sharpen your tools
  • Ask for and offer help, like rebasing for people or adding test cases
  • People are generous with their time because they care
  • Contribute by understanding what others want and being respectful

TECHNICAL SPECIFICS

Don't trust, verify. Minimise dependance on GitHub in your review process. Use the GitHub website only for the GitHub metadata, e.g. reading comments and adding your own comments — not for reviewing the commits and code, which you should do in your local environment.

Pull down the code locally

Therefore, a review begins by pulling the PR branch down to your computer to build and review locally. There are different ways to do it depending on what you want, your needs, disk space, internet bandwidth, etc. Here are a few:

  1. Pulling down remote PRs with git checkout pr/<number> as described in this nice little gist which can be modified to suit your needs.
  2. My gitconfig [remote "origin"] section: fetch = +refs/pull/*/head:refs/remotes/origin/pr/*
  3. Bitcoin Core contributor Luke Dashjr's version: "To avoid all the merge branches, configure the origin-pull remote as": fetch = +refs/pull/*/head:refs/remotes/origin-pull/*/head
  4. Bitcoin Core documentation for referencing PRs easily with refspecs.
  5. GitHub exposes PRs as branches on the upstream repository with pull/<number>/head (contributor branch) and pull/<number>/merge (merged into master), e.g. git fetch origin pull/17283/head && git checkout FETCH_HEAD. That said, I prefer to depend as little as possible on GitHub.

You can test a PR either on the contributor's branch or with the changes merged on top of master. Testing the latter can be useful to see if anything merged into master since the last PR commit breaks the changes.

Next, launch the build and tests while you begin reading the code locally. You'll need to become comfortable with compiling Bitcoin Core from source and running the unit tests and functional tests since you will need to do it for many of the PRs you test. For this, the Bitcoin Core productivity notes are indispensable.

Read and know the Bitcoin Core developer notes.

Diff tools

While the build and tests are running, begin reviewing each commit separately in your local environment using a diff tool like gitk, meld, meld for macOS, GNU ediff for Emacs, vimdiff or vim-diffconflicts for Vim, opendiff on macOS, or diffoscope (here are some diffoscope usage tips).

If you use gitk and like dark mode, I recommend Dracula for gitk.

Git grepping

Become adept at searching the repository with git grep. You'll use it constantly to find stuff in the codebase. Run git grep --help on the command line for help or information.

If you're not sure where to start

Read the code, read the PR comments, then re-read both. Find something that doesn't make sense and try to figure it out. Repeat.

Once it all starts to make sense, run bitcoind on regtest, testnet (or on mainnet with very small amounts), and tail or search through the relevant logs (run bitcoin-cli help logging for the various bitcoind logging categories and how to toggle them on/off).

Maybe add some custom logging, LogPrintfs, or asserts; it's always a privilege to add these into other people's code (to see how, run git grep -ni logprintf or git grep assert in the repository).

Run the relevant functional tests and look through the debug logs. Verify that they fail in the expected way on master. Back in the PR branch, inverse or change the new tests to make them break and understand why.

Maybe add C++ gdb or Python pdb breakpoints (or add import pdb; pdb.set_trace() anywhere in the functional test code). Examine values. Run RPC commands.

Check if any call sites, headers or declarations have been overlooked in the PR.

Try refactoring the code to be better or prettier, and discover why that doesn't work. Expect it to take twice as long as you planned it to. Yes, it's work.

Maybe run strace (man page strace) to trace system calls and signals.

Depending on the changes, contributing benchmarks, memory profiling/valgrind or flame graphs to a PR review can sometimes be very useful, and even decisive.

Technical resources

I maintain a document of various technical notes for myself, that I refer to often when working on Bitcoin Core, here: jonatack/bitcoin-development/notes.txt. There is also useful stuff in the repository where those notes are located. Another great repository of resources is fanquake/core-review.

Debugging

Two good gists about debugging Bitcoin Core:

Add missing tests

While you're reviewing, writing tests yourself can help you understand the behaviour and verify the changes, and if they add useful coverage you can propose them to the author for inclusion in the PR. Proposing automated tests is a really helpful way to start contributing. Authors appreciate it when someone reviews their PR and provides additional tests. Here's an example.

Big picture > nits

Remember, the big picture is much more important than nits, spelling, or code style. Re-read the Nits section above. Try to avoid commenting on these while reviewing, even if you have no other comments to make. I know, it's hard — I've done it too many times — but there's a better alternative:

Ask questions

A good thing you can do as a reviewer without specialised knowledge of the code is ask questions. A PR author is usually happy to discuss their work or see interest in it. So, spend 20 minutes or so looking at a change, find the thing that seems most confusing or surprising, and ask about it politely in the PR comments or on the #bitcoin-core-dev IRC channel. Chances are other people wonder about the same thing and it could be clarified or documented better. In this way you can learn and help make the project more accessible, too. (Credit for this paragraph: Russ Yanofsky).

Peer review

Be sure to learn and understand the Bitcoin Core peer review process. The process is often updated, so refer back to it frequently.

An ACK (origin) is generally followed by a description of how the reviewer did the review and any manual testing. As a new contributor, it's advisable to be even more verbose in review comments to provide context on what you did and thought through during the review and show that you understood the change.

Concept ACK means the reviewer acknowledges and agrees with the goal of the change, but is not (yet) confirming they've looked at the code or tested it. This can be a valuable signal to a PR author to let them know that the PR has merit and is headed in the right direction. As a corollary, a Concept NACK would indicate disagreement with the goal.

Approach ACK is a step further than Concept ACK and means agreement with both the goal and the approach used in implementing the change. Approach NACK would therefore indicate agreement with the goal but not the approach.

Reviewers sometimes comment "Code review ACK" to communicate that the code looks good but they haven't tested the changes or don't have an opinion on the concept yet. It's good to add more context to clarify: "Code review ACK HEAD, unsure about the concept, I'd like to verify x, y, z, etc."

Other ACK variants that are sometimes used are "tACK" or "tested ACK", and "utACK" or "untested ACK".

Manual testing of new features and reported issues is always welcome. A comment that is really helpful in review: "Here's what I tested and my methodology", particularly to back up an ACK.

Some PRs can be difficult to test or ACK due to complexity, context, or possibly a lack of tests or simulation framework. That shouldn't discourage you from reviewing. For example, if you've reviewed the code thoroughly, a comment like "the code looks correct to me, but I don't feel confident enough about behaviour to give an ACK" is a perfectly helpful contribution.

Examples of other useful comments could be "verified move-only" if the PR includes move-only commits, or "thought hard about how change X could break Y but didn't find any (or could this scenario happen?)", etc.

When giving an ACK, specify the commits reviewed by appending the commit hash of the HEAD commit. The trustless way to do this is to use the hash from your local checkout of the branch and not from the GitHub web page. That way, unless your local tools are compromised, you ensure you are ACKing the exact changes. This is also useful when a force push happens and links to old commits are lost on GitHub.

A full ACK could look like: "ACK fa2f991, I built, ran tests, tested manually by doing X/Y/Z and reviewed the code and it looks OK, I agree it can be merged."

The Bitcoin Core merge script currently copies into the merge commit the first paragraph of each ACK review pertaining to the HEAD commit at the time of merge. Keep in mind that anything you write there that is copied by the merge script will be in git history forever.

A complex PR usually requires at least 3-4 experienced ACKs before merging.

Apache voting system

Bitcoin Core reviewers frequently use the Apache voting system in their comments. Here is an example.

Go easy on the people

Review the code, not the contributor or their comments.

When you disagree, state your point of view once and move on. Don't flood the comments section, browbeat others or overreact. Remember that the most important thing is probably not the issue being discussed, but your relationship with the other contributors.

As a new contributor, be cautious with giving a NACK. Assume by default that you might lack understanding or context. If you do NACK, provide good reasoning. Here's one example.

Signing commits with Opentimestamps

Some Bitcoin Core contributors sign and OpenTimestamp their ACKs. While that is beyond the scope of this document, it is surprisingly trivial to sign your commits using the OpenTimestamps Git Integration.

Collapsible comments

After a while you'll notice that contributors sometimes review using collapsible comments. Cool, you may think, how do I do that? It's done using HTML details tags. Here's how.

CREDITS

A special thank you to John Newbery for launching the Bitcoin Core PR Reviews Club and to the long-term contributors who participated so far: Andrew Chow, Anthony Towns, Bryan Bishop, Dave Harding, Gregory Sanders, James O'Beirne, Karl-Johan Alm, Marco Falke, Mark "Murch" Erhardt, Michael Ford, Mike Schmidt, Pieter Wuille, Samuel Dobson, and Wladimir van der Laan.

Thanks to Steve Lee (moneyball) and Michael Folkson for reviewing this write-up and their suggestions.

This article includes observed comments on GitHub and IRC by the following Bitcoin Core contributors/maintainers who deserve to be credited: Wladimir van der Laan, Marco Falke, Pieter Wuille, Gregory Maxwell, Anthony Towns, and Russ Yanofsky.

Over the years I had become disillusioned by the central influence of BDFLs in programming languages and open source projects. Wladimir van der Laan's long-standing humble service to Bitcoin sparked the possibility to me of perhaps doing the same.

Finally, a big thank you to the Bitcoin Core contributors for their patience with my review attempts so far, notably John Newbery, Marco Falke, João Barbosa, practicalswift, Gregory Sanders, Jonas Schnelli, Pieter Wuille and Wladimir van der Laan, and to Adam Jonas and John Newbery for their guidance and advice from the start.

Cheers,

Jon Atack