review.md

Code review

This document aims to explain why code review can be a very valuable form of contribution to any Open Source project. But code review is a very broad term that can mean many things. Good in-depth code review is also a skill that needs to be learned but can be highly valuable in any development team.

Why should new contributors review code first?

Developers interested in contributing to an Open Source project seem to often think that writing code is the bottleneck in those projects. That rarely is the case though. Especially in Bitcoin related projects, where any mistake can have enormous consequences, any code that is to be merged needs to go through a thorough review first (often requiring two or more developers to approve before it can be merged).

Therefore, the actual bottleneck in Bitcoin related Open Source projects is review, not the production of code.

So new contributors that come into a project by opening a PR often take more time away from the maintainers of that project than what their first contribution ends up adding in value. That is especially the case if the contributors don't have a lot of experience with git, GitHub, the target programming language and its formatting requirements or the review and test process as a whole.

If a new contributor instead starts reviewing Pull Requests first, before opening their own ones, they can gain the following advantages:

• They learn about the project, its formal requirements and the programming language used, without anyone's assistance.
• They learn about how to compile, execute and test the project they are aiming to contribute to.
• They see best practices in terms of formatting, naming, commit structure, etc.
• The value added to the project by reviewing, running and testing code is likely positive from day one and requires much less active assistance and time from maintainers.

The maintainers of this project therefore ask all new contributors to review at least a couple of Pull Requests before adding their own.

How do I review code?

Reviewing code in a constructive and helpful way is almost an art form by itself. The following list is definitely not complete and every developer tends to develop their own style when it comes to code review. But the list should have the most basic tasks associated with code review that should be a good entry point for anyone not yet familiar with the process:

• Make sure to pick a Pull Request that fits your level of experience. You might want to pick one that has the "good first review" label. If you're new to the area that you'd like to review, first familiarize yourself with the code, try to understand the architecture, public interfaces, interactions and code coverage. It can be useful to look at previous PRs to get more insight.
• Check out the code of the pull request in your local git (check this guide for an easy shortcut).
• Compile the code. See INSTALL.md and MAKEFILE.md for inputs on how to do that.
• Run the unit tests of the changed areas. See the unit section of MAKEFILE.md on how to run specific packages or test cases directly.
• Run the code on your local machine and attempt to test the changes of the pull request. For any actual Bitcoin or Lightning related functionality, you might want to spin up a regtest local network to test against. You can check out the regtest setup project that might be helpful for that.
• Try to manually test the changes in the PR. What to actually test might differ from PR to PR. For a bug fix you might first want to attempt to reproduce the bug in a version prior to the fix. Then test that the Pull Request actually fixes the bug. Code that introduces new features should be tested by trying to run all the new code paths and try out all new command line flags as well as RPC methods and fields. Making sure that invalid values and combinations lead to proper error messages the user can easily understand is important.
• Keep the logs of your tests as part of your review feedback. If you find a bug, the exact steps to reproduce as well as the local logs will be super helpful to the author of the Pull Request.
• Propose areas where the unit test coverage could be improved (try to propose specific test cases, for example "we should test the behavior with a negative number here", instead of just saying "this area needs more tests").
• Bonus: Write a unit test (or test case) that you can propose to add to the PR.
• Go through the PR commit by commit and line by line
  • Is the flow of the commits easy to understand? Does the order make sense? See the code contribution guidelines for more information on this.
  • Is the granularity of the commit structure easy to understand? Should commits be split up or be combined?
  • What does each line achieve in terms of the overall goal of the PR?
  • What does each line change in the behavior of the code?
  • Are there any potentially unwanted side effects?
  • Are all errors handled appropriately?
  • Is there sufficient logging to trace what happens if a user runs into problems with the code? (e.g. when testing the code, can you find out what code path your execution took just by looking at the trace-level log?)
  • Does the commit message appropriately reflect the changes in that commit? Only relevant for non-self-explanatory commits.
• Is the documentation added by the PR sufficient?
• Bonus: Propose additional documentation as part of the review that can be added to the PR.
• What does the user interface (command line flags, gRPC/REST API) affected by the PR look like? Is it easy to understand for users? Are there any unintended breaking changes for current users of the API being introduced? We attempt to not break existing API users by renaming or changing existing fields.
• Does the CI pass for the PR? If not, can you find out what needs to be changed to make the CI pass? Unfortunately there are still some flakes in the tests, so some steps might fail unrelated to the changes in the Pull Request.

How to submit review feedback

The above checklist is very detailed and extensive. But a review can also be helpful if it only covers some of the steps outlined. The more, the better, of course, but any review is welcome (within reason of course, don't spam pull requests with trivial things like typos/naming or obvious AI-generated review that doesn't add any value).

Just make sure to mention what parts you were focusing on. The following abbreviations/acronyms might be used to indicate partial or specific review:

• Examples:
  • cACK (concept acknowledgement): I only looked at the high-level changes, the approach looks okay to me (but didn't deeply look at the code or tests).
  • tACK (tested acknowledgement): I didn't look at the code, but I compiled, ran and tested the code changes.
  • ACK ("full" acknowledgement): I reviewed all relevant aspects of the PR and I have no objections to merging the code.
  • Request changes: I think we should change the following aspects of the PR before merging: ...
  • NACK (negative acknowledgement): I think we should not proceed with the changes in this PR, because of the following reasons: ...

Then try to give as much detail as you can. And as with every communication in Open Source projects: Always be polite and objective in the feedback. Ask yourself: Would you find the feedback valuable and appropriate if someone else posted it on your PR?