How to review PRs submitted to Bitcoin Core

Author: Jon Atack

Source: https://jonatack.github.io/articles/how-to-review-pull-requests-in-bitcoin-core

introduction

Reviewing and testing are probably the best ways to get started contributing code to Bitcoin Core.

Extensive review and testing are frequently mentioned by long-time Bitcoin Core developers because:

• Resource bottlenecks, and
• It is the best and most effective way to learn, start contributing code, and build up your legacy in the community.

Before you begin

This guide is based on these documents:

1. " Contributing to Bitcoin Core: Personal Experience ," by John Newbery (2017)
2. " Understanding the Technical Aspects of Bitcoin ," by Pierre Rochard (2018).
3. Hardcore workshop video / transcription / slides , from Jeremy Rubin (2018).

the term

ACK and NACK : Their definitions and origins can be found here and here .

Nit: A tiny problem that usually doesn't cause blockages.

PR : Short for " pull request ," sometimes also called "merge request." It is a proposal to change code or documentation in a source code repository.

WIP is an abbreviation for " work in progress ".

General Education

As a newcomer, our goal is to try to add value with a friendly and humble attitude, while learning as much as possible. (Although these goals aren't only relevant to newcomers; or rather, this process has no end.)

A good approach is not to treat it as something you control, but rather to ask yourself: "How can I best serve everyone?"

One of the most challenging issues for new contributors is the breadth of the codebase and the technical complexity surrounding it.

Be aware that there are things you don't know. Long-term developers have years of experience and professional backgrounds. The community has built a deep collective wealth of knowledge and experience. Remember, your new idea may have already been mentioned and considered several times before.

Please remember that contributors and maintainers have limited time and energy—be cautious and respectful when asking for help. The goal is to give more than you ask, help more than you hinder, and speed things up.

Try to figure things out yourself as much as possible, and at least fully respect other people's time.

Follow the #bitcoin-core-dev live chat room and the bitcoin-dev mailing list.

More noteworthy IRC channels can be found here .

You need to invest a lot of time before you even begin:

• Understanding the contribution process and guidelines involves more than just reading the documentation in the codebase (especially the " Contribution Guidelines ," " Developer Notes ," and " Productivity Notes ") (and more broadly, everything in the documentation and test codebase ). It also requires paying attention to interactions in the #bitcoin-core-dev IRC channel and the ongoing code reviews in the codebase .
• Get to know regular contributors: what they do, what they like, what they want, and how they receive feedback.

Many newcomers immediately open PRs, but this only adds one more PR to the hundreds waiting for valuable review. It's much better to review existing PRs, understand what types of PRs are more helpful, and gradually gain a comprehensive understanding of the bigger picture.

A useful rule of thumb is: whenever you are about to create a PR, review 5 to 15 PRs first.

panoramic

A big picture is far more important than nit, spelling, and code style.

A comprehensive review process has several different levels: "Will this change affect what behaviors?" or "Is this change safe?" are different from "Is this a good idea?" Answering the latter question requires more context, which you will gradually learn. Don't let it stop you from thinking about these questions, and don't give up on reviewing at this level.

Steps to improve your understanding of panoramas:

• Complete the Chaincode Labs learning guide
• Consider applying for and taking the highly recommended Chaincode Labs online course (this program also serves as a search engine for new developers looking for Bitcoin jobs and open-source grants).
• Study Bitcoin upgrade proposals (usually referred to as "BIPs" in singular and abbreviated form) and frequently review them.
• Subscribe to the Bitcoin Optech Weekly and use their topics page as a resource at hand.
• Complete " Learning Bitcoin from the Command Line "

Pursue quality over quantity, and strike a balance between in-depth work and rapid results.

Writing documentation is important. For example, it should include a summary of how each component works and interacts, clear and accurate code documentation, instructions on whether a function has a sign and Doxygen documentation , test logs (including both info and debug ), and so on.

Test coverage is also important. Don't hesitate to improve and write any missing unit tests , functional tests , and fuzz tests .

Be a humble and friendly contributor. Help move PRs forward by reviewing them, suggesting tests and useful fixes, proposing code rebases, and even considering taking over after a PR has been dormant for months. In short, let's help each other!

NIT

Try to avoid getting overly hung up on the nit, minor details, and code style in a PR, especially those that are still labeled "WIP"; and don't rush to nitpick when a PR has just been created and the authors are still seeking conceptual and methodological ACKs (such as when they are still seeking general consensus).

Long-term contributors have reported that such activities are off-putting; moreover, doing so depletes your social capital within the project. Try to understand what kind of review people need and when it's necessary.

The best time to comment on any nit is after the PR has received enough ACKs (i.e., consensus) on the concept/method, but before the PR closes and receives "tested ACK". As Pieter Wuille said in IRC : "I feel that the most frustrating thing for a PR is to get a lot of comments on details/nits/code style and still not know if the PR is a good concept."

When giving advice on nit and coding style, it should be friendly, relaxed, and encouraging—for example, "Feel free to ignore it," or "If you happen to change the code, you can adjust it accordingly," etc.

Please remember that no one is obligated to take your review comments into account; if the author believes your comments have gone beyond the scope of the changes and therefore replies that they do not wish to adopt them, that is perfectly acceptable (especially when your comments are all about nits).

improve

When you are able to, increase the difficulty and priority of the PRs you review.

The quality of reviews is far more important than the quantity. You can learn and add more value by providing in-depth, high-quality reviews for high-priority and more difficult PRs. These PRs might intimidate people and stagnate for months, their passion worn down by a lack of high-quality reviews, nitpicking code style, and rebase comments. Properly reviewing them is a true service to Bitcoin.

The improvement process takes time; nothing can replace the years of investment in understanding the background, paying attention to the code , reporting issues, making PRs , the #bitcoin-core-dev IRC channel, and the bitcoin-dev mailing list.

Before embarking on a review, a useful question is: "What is most needed at this stage?" Answering this question requires experience and accumulated background knowledge, but its value lies in determining how to add maximum value with minimal time. Depending on the complexity and criticality of the change, and the stage the PR has already passed through in the review process, a review might involve skimming the code and applying extensive background knowledge in a relevant code comment at a key location, rather than running a full review that includes debugging, development, testing, and reviewing every commit. However, in the vast majority of cases, performing a proper, comprehensive review is best and adds the most value.

One step at a time

Exclude self-evaluation and desires. Don't let things become a matter of personal feelings; instead, push things forward.

When in doubt, assume that the other person has good intentions.

Be patient with people and results.

Public praise; private criticism, but it should be accompanied by encouragement.

Continue to offer help. Strive every day.

Everything is easier said than done. Forgive yourself, and forgive others.

Please remember: Before creating a PR, review 5 to 15 PRs (or process/test 5 to 15 issue reports).

Finally, understand the value of your contributions from people with different backgrounds and experience levels, not just your colleagues or acquaintances. Contact new friends (directly via IRC messaging is fine) and ask them what help they need. You can ask for help occasionally, but don't treat it as a right. Give more, take less.

Technical details

Don't trust, verify. Minimize your reliance on GitHub for your review process. Use the GitHub website only for metadata purposes, such as reading comments and adding your own—the work of reviewing commits and code should be done in your local environment.

Fetch code for local machine

Therefore, the review process begins by pulling a PR branch for your computer, allowing you to compile and review it locally. Different methods exist depending on your ideas, needs, disk space, and internet bandwidth. Here are just a few examples:

1. Use git checkout pr/<number> to pull a PR from the remote repository. As this concise gist article says, you can modify it to suit your needs.
2. My git configuration's [remote "origin"] section: fetch = +refs/pull/*/head:refs/remotes/origin/pr/*
3. Bitcoin Core contributor Luke Dashjr's version: "To avoid all merge branches, configure the origin-pull remote as": fetch = +refs/pull/*/head:refs/remotes/origin-pull/*/head
4. Bitcoin Core documentation: Easily reference PRs using refspecs
5. Using pull/<number>/head (contributor branch) and pull/<number>/merge (merge into the master branch), GitHub exposes PRs as branches of the upstream codebase, for example, git fetch origin pull/17283/head && git checkout FETCH_HEAD . That said, I tend to rely on GitHub as little as possible.

You can test a PR on the contributor branch or on the master branch that has merged the changes. Testing the latter is very useful for checking if anything merged into the master branch since the last PR commit has broken the changes.

Then you can start compiling and testing locally while simultaneously reading the code. You should be able to proficiently compile Bitcoin Core from source code and then run unit and functional tests , as you'll need to test many PRs. Therefore, a Bitcoin Core productivity guide is irreplaceable.

Read and learn about Bitcoin Core's Developer Notes .

Comparison tools

While compilation and testing are still running, start reviewing each code commit in your local environment using comparison tools that can highlight differences, such as gitk , meld , meld for macOS , GNU ediff for Emacs, vimdiff or vim-diffconflicts for Vim, opendiff on macOS, and diffoscope (here is also a guide to using comparison tools ).

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

Git Grep

Become proficient in using git grep to search code repositories. You'll continue using it until you find the material you need in the repository. Run git grep --help in your command-line environment for assistance.

If you're unsure where to start

Read the code, read the PR comments, and then go back and reread both. Find the parts you don't understand, and then figure them out. Repeat this process continuously.

Once everything starts to become clear, run bitcoind on regtest/testnet (or on the mainnet for a small fee), and then track and search the relevant logs (run bitcoin-cli help logging to get the different bitcoind log categories and how to turn them on/off).

You might be able to add some custom logging logic, such as LogPrintf or assert ; adding them to someone else's code is a privilege (to understand why, run git grep -ni logprintf or git grep asset in the codebase).

Run the relevant functional tests and review the debug logs. Verify that the way they fail on the master branch matches expectations. Then, return to the PR branch, reverse or modify the new tests to make them fail, and understand why.

Perhaps C++'s gdb or Python's pdb (or adding import pdb:pdb.set_trace() ` to any functional test code) can provide a view of the breakpoints. Evaluate. Run the RPC command.

Check if the PR has ignored any calling sites, header files, or declarations.

Try refactoring the code to a better state and explore why it's not working. Be prepared for it to take twice as long as expected. Yes, that's the job.

You might be able to run strace ( man page strace ) to trace system calls and signals.

Depending on the content of the changes, contributing benchmarks, memory analytics/valgrind, or flame graphs to the PR review can sometimes be very helpful, even decisive.

Technical Resources

I've compiled a document containing various technical notes that I frequently refer to while developing Bitcoin Core, and it's located here: jonatack/bitcoin-development/notes.txt . There are also other useful resources in the codebase containing these notes. Another excellent resource is fanquake/core-review .

debug

Two excellent gist documents explaining how to debug Bitcoin Core:

• " Debugging Bitcoin Core ", by Fabian Jahr
• " Deep Dive into and Debug Bitcoin Core Using GDB or LLDB ", by Angel Leon

Add missing tests

Even though you're reviewing the code, writing your own tests can help you understand how the code works and verify the changes. Furthermore, if they add useful coverage, you can suggest to the author that these tests be included in a PR. Proposing automated tests is a very useful way to start contributing. Authors will appreciate it when someone reviews the code and provides additional tests. Here's an example .

From panoramic to nit

Please remember that overall picture is far more important than NIT, spelling, and code style. Please reread the "NIT" section above. During the review process, try to avoid commenting on these things, even if you have nothing else to comment on. I know it's hard—I've succumbed to it many times myself—but there are better alternatives:

Question

As a reviewer (without requiring specialized knowledge of the code), one of the best things you can do is ask questions . PR authors are usually happy to discuss their work and see interest in it. So, spend 20 minutes or so looking at the changes, identifying the most confusing or unexpected parts, and then politely ask these questions in the PR comments (or in the #bitcoin-core-dev channel). Others may also be confused by the same issues, and it can then be better clarified and documented. This way, you both learn something and help make the project more understandable (thanks to Russ Yanofsky for this paragraph).

Peer review

Make sure you have learned and understood Bitcoin Core'speer review process . This process is frequently updated , so review it regularly.

" ACK " ( origin ) is typically used after an auditor's description of how they reviewed and manually tested the changes. As a new contributor, it is recommended to provide a more detailed description of what you did and thought in your review comments to demonstrate your understanding of the change.

A "Concept ACK" means the reviewer acknowledges and agrees with the goals of the change, but (but hasn't yet) admitted to reviewing or testing the code. This can be a valuable signal for PR authors, letting them know that the PR is valuable and moving in the right direction. Conversely, a "Concept NACK" indicates disagreement with the goals of the PR.

"Approach ACK" goes a step further than conceptual ACK, meaning that it agrees with both the goals of the PR and the methods used to achieve that change. "NACK" on the other hand indicates agreement with the goals but disagreement with the methods used to achieve them.

Reviewers sometimes comment "Code Review ACK" to indicate that the code looks good, but they haven't tested the changes or formed an opinion on the concept. Adding more context would be better: "Code Review ACK HEAD , not clear on the concept's purpose yet, I will verify x, y, z," etc.

Other ACK variants include "tACK" or "tested ACK", which indicates that the ACK has been tested, and "utACK" or "untested ACK", which indicates that the ACK has not been tested.

Manually testing new features or reporting issues is welcome. Comments like, "Here are my test results and methods," are very helpful, especially with an "ACK" at the end.

Some pull requests may be difficult to test or acknowledge due to their complexity, context, or lack of a testing and mocking framework. For example, if you thoroughly review the code and leave a comment like, "The code looks correct to me, but I'm not confident enough to give an ACK for how it works," that's also a very valuable contribution.

Other helpful comments include, for example, "verified the move-only part" for comments containing "move-only"; and "struggling to figure out if changing X could break Y, but couldn't come up with any results (would this really happen?)," and so on.

ACK with submission hash

When sending an ACK, indicate the status of the code you are reviewing by appending the hash of HEAD commit (or the commit you are reviewing). The trustless, correct approach is to use a hash from your local branch, not from the GitHub webpage. This ensures that you are ACKing specific changes unless your local tools are compromised. This is also useful when a force push has occurred and the linked older commit has been lost on GitHub.

A complete ACK should look like this: "ACK fa2f991 , I compiled, ran the tests, manually tested by running X/Y/Z, and reviewed the code. It looks good, and I agree that it can be merged."

The current Bitcoin Core merge script copies the first paragraph of every ACK comment related to HEAD commit (at the moment of the merge) into the merged commit. So remember, anything you write in it will be copied by the merge script and become a permanent part of git history.

A complex or higher-risk PR may require at least 3 to 4 experienced backers before it can be merged.

APACHE voting system

Bitcoin Core moderators often use the Apache voting system in the comments. Here's one example .

Be more tolerant of others

Review the code, not the contributors themselves and their comments.

When you disagree, state your point once and for all, and then move forward. Here's an example . Don't overwhelm the comments with multiple posts, and don't intimidate or overreact. Be patient, and avoid being aggressive or bullying. Remember, the most important thing may not be the issue at hand, but your relationship with other contributors.

As a new contributor, be careful when giving a NACK. It assumes you lack understanding of the context. If you must give a NACK, provide good reasoning. Here's an example .

Submit using OpenTimeStamp signature

Some Bitcoin contributors sign ACKs and attach an OpenTimeStamp (Open timestamp). While this is beyond the scope of this article, using a Git plugin for OpenTimeStamp to sign your commits is quite straightforward.

Collapsible comments

After a while, you'll notice that contributors sometimes use collapsible comments for moderation. That's cool , you might wonder, how is that done ? It uses the HTML ` details tag. Here's a usage guide .

Acknowledgments

Thanks to Steve Lee (moneyball) and Michael Folkson for reviewing this article and providing their suggestions.

By highlighting Bitcoin Core developers who deserve our gratitude, this article includes comments observed on GitHub and IRC: Wladimir van der Laan, Marco Falke, Pieter Wuille, Gregory Maxwell, Anthony Towns, and Russ Yanofsky.

For years, I was disillusioned by the influence of BDFL (Benevolent Dictator for Life) on programming languages ​​and open-source projects. Wladimir van der Laan's long and humble service to Bitcoin has rekindled my interest in returning to open-source projects.

Finally, I would like to express my sincere gratitude to all Bitcoin Core contributors for their patience in reviewing my submissions, especially John Newbery, Marco Falke, João Barbosa, practicalswift, Gregory Sanders, Jonas Schnelli, Pieter Wuille, and Wladimir van der Laan. I would also like to thank Adam Jonas and John Newbery for their initial guidance and suggestions.

(over)