Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions docs/lnd/code_contribution_guidelines.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
1. [Overview](#overview)
2. [Minimum Recommended Skillset](#minimum-recommended-skillset)
3. [Required Reading](#required-reading)
4. [Development Practices](#development-practices)
4. [Substantial contributions only](#substantial-contributions-only)
5. [Development Practices](#development-practices)
1. [Share Early, Share Often](#share-early-share-often)
1. [Testing](#testing)
1. [Code Documentation and Commenting](#code-documentation-and-commenting)
Expand All @@ -13,12 +14,12 @@
1. [Pointing to Remote Dependent Branches in Go Modules](#pointing-to-remote-dependent-branches-in-go-modules)
1. [Use of Log Levels](#use-of-log-levels)
1. [Use of Golang submodules](#use-of-golang-submodules)
5. [Code Approval Process](#code-approval-process)
6. [Code Approval Process](#code-approval-process)
1. [Code Review](#code-review)
1. [Rework Code (if needed)](#rework-code-if-needed)
1. [Acceptance](#acceptance)
1. [Review Bot](#review-bot)
6. [Contribution Standards](#contribution-standards)
7. [Contribution Standards](#contribution-standards)
1. [Contribution Checklist](#contribution-checklist)
1. [Licensing of Contributions](#licensing-of-contributions)

Expand Down Expand Up @@ -109,8 +110,16 @@ If you are an honest user that wants to contribute to this project, please
consider that every pull request takes precious time from the maintainers to
review and consider the impact of changes. Time that could be spent writing
features or fixing bugs.
If you really want to contribute, consider reviewing and testing other users'
pull requests instead. Or add value to the project by writing unit tests.
If you really want to contribute, [consider reviewing and testing other users'
pull requests instead](review.md).
First-time reviewer friendly [pull requests can be found
here](https://github.com/lightningnetwork/lnd/pulls?q=is%3Aopen+is%3Apr+label%3A%22good+first+review%22).
Once you are familiar with the project's code style, testing and review
procedure, your own pull requests will likely require less guidance and fewer
maintainer review cycles, resulting in potentially faster merges.
Also, consider increasing the test coverage of the code by writing more unit
tests first, which is also a very valuable way to contribute and learn more
about the code base.

# Development Practices

Expand Down
6 changes: 6 additions & 0 deletions docs/lnd/release-notes/release-notes-0.19.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,9 @@ the on going rate we'll permit.
flag](https://github.com/lightningnetwork/lnd/pull/9606/) for future
compatibility.

* [Establish a base DB version even if it is not yet
tracked](https://github.com/lightningnetwork/lnd/pull/9647).

## Code Health

* A code refactor that [moves all the graph related DB code out of the
Expand All @@ -480,6 +483,9 @@ the on going rate we'll permit.
inputs spending logic in the sweeper so it can properly handle missing inputs
and recover from restart.

* A code refactor to [use maps.Copy instead of manually copying map
elements](https://github.com/lightningnetwork/lnd/pull/9630).


## Tooling and Documentation

Expand Down
139 changes: 139 additions & 0 deletions docs/lnd/review.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
# 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](https://github.com/lightningnetwork/lnd/labels/good%20first%20review).
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](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally)
for an easy shortcut).
- Compile the code. See [INSTALL.md](INSTALL.md) and [MAKEFILE.md](MAKEFILE.md)
for inputs on how to do that.
- Run the unit tests of the changed areas. See the [`unit` section of
MAKEFILE.md](MAKEFILE.md#unit) 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](https://github.com/guggero/rt) 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](code_contribution_guidelines.md)
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?