mirror of
https://github.com/btcsuite/btcd.git
synced 2024-11-19 01:40:07 +01:00
320 lines
14 KiB
Markdown
320 lines
14 KiB
Markdown
# Code contribution guidelines
|
||
|
||
Developing cryptocurrencies is an exciting endeavor that touches a wide variety
|
||
of areas such as wire protocols, peer-to-peer networking, databases,
|
||
cryptography, language interpretation (transaction scripts), RPC, and
|
||
websockets. They also represent a radical shift to the current fiscal system
|
||
and as a result provide an opportunity to help reshape the entire financial
|
||
system. There are few projects that offer this level of diversity and impact
|
||
all in one code base.
|
||
|
||
However, as exciting as it is, one must keep in mind that cryptocurrencies
|
||
represent real money and introducing bugs and security vulnerabilities can have
|
||
far more dire consequences than in typical projects where having a small bug is
|
||
minimal by comparison. In the world of cryptocurrencies, even the smallest bug
|
||
in the wrong area can cost people a significant amount of money. For this
|
||
reason, the btcd suite has a formalized and rigorous development process which
|
||
is outlined on this page.
|
||
|
||
We highly encourage code contributions, however it is imperative that you adhere
|
||
to the guidelines established on this page.
|
||
|
||
## Minimum Recommended Skillset
|
||
|
||
The following list is a set of core competencies that we recommend you possess
|
||
before you really start attempting to contribute code to the project. These are
|
||
not hard requirements as we will gladly accept code contributions as long as
|
||
they follow the guidelines set forth on this page. That said, if you don't have
|
||
the following basic qualifications you will likely find it quite difficult to
|
||
contribute.
|
||
|
||
- A reasonable understanding of bitcoin at a high level (see the
|
||
[Required Reading](#ReqReading) section for the original white paper)
|
||
- Experience in some type of C-like language
|
||
- An understanding of data structures and their performance implications
|
||
- Familiarity with unit testing
|
||
- Debugging experience
|
||
- Ability to understand not only the area you are making a change in, but also
|
||
the code your change relies on, and the code which relies on your changed code
|
||
|
||
Building on top of those core competencies, the recommended skill set largely
|
||
depends on the specific areas you are looking to contribute to. For example,
|
||
if you wish to contribute to the cryptography code, you should have a good
|
||
understanding of the various aspects involved with cryptography such as the
|
||
security and performance implications.
|
||
|
||
## Required Reading
|
||
|
||
- [Effective Go](http://golang.org/doc/effective_go.html) - The entire btcd
|
||
suite follows the guidelines in this document. For your code to be accepted,
|
||
it must follow the guidelines therein.
|
||
- [Original Satoshi Whitepaper](http://www.google.com/url?sa=t&rct=j&q=&esrc=s&source=web&cd=1&cad=rja&ved=0CCkQFjAA&url=http%3A%2F%2Fbitcoin.org%2Fbitcoin.pdf&ei=os3VUuH8G4SlsASV74GoAg&usg=AFQjCNEipPLigou_1MfB7DQjXCNdlylrBg&sig2=FaHDuT5z36GMWDEnybDJLg&bvm=bv.59378465,d.b2I) - This is the white paper that started it all. Having a solid
|
||
foundation to build on will make the code much more comprehensible.
|
||
|
||
## Development Practices
|
||
|
||
Developers are expected to work in their own trees and submit pull requests when
|
||
they feel their feature or bug fix is ready for integration into the master
|
||
branch.
|
||
|
||
## Share Early, Share Often
|
||
|
||
We firmly believe in the share early, share often approach. The basic premise
|
||
of the approach is to announce your plans **before** you start work, and once
|
||
you have started working, craft your changes into a stream of small and easily
|
||
reviewable commits.
|
||
|
||
This approach has several benefits:
|
||
|
||
- Announcing your plans to work on a feature **before** you begin work avoids
|
||
duplicate work
|
||
- It permits discussions which can help you achieve your goals in a way that is
|
||
consistent with the existing architecture
|
||
- It minimizes the chances of you spending time and energy on a change that
|
||
might not fit with the consensus of the community or existing architecture and
|
||
potentially be rejected as a result
|
||
- Incremental development helps ensure you are on the right track with regards
|
||
to the rest of the community
|
||
- The quicker your changes are merged to master, the less time you will need to
|
||
spend rebasing and otherwise trying to keep up with the main code base
|
||
|
||
## Testing
|
||
|
||
One of the major design goals of all core btcd packages is to aim for complete
|
||
test coverage. This is financial software so bugs and regressions can cost
|
||
people real money. For this reason every effort must be taken to ensure the
|
||
code is as accurate and bug-free as possible. Thorough testing is a good way to
|
||
help achieve that goal.
|
||
|
||
Unless a new feature you submit is completely trivial, it will probably be
|
||
rejected unless it is also accompanied by adequate test coverage for both
|
||
positive and negative conditions. That is to say, the tests must ensure your
|
||
code works correctly when it is fed correct data as well as incorrect data
|
||
(error paths).
|
||
|
||
Go provides an excellent test framework that makes writing test code and
|
||
checking coverage statistics straight forward. For more information about the
|
||
test coverage tools, see the [golang cover blog post](http://blog.golang.org/cover).
|
||
|
||
A quick summary of test practices follows:
|
||
|
||
- All new code should be accompanied by tests that ensure the code behaves
|
||
correctly when given expected values, and, perhaps even more importantly, that
|
||
it handles errors gracefully
|
||
- When you fix a bug, it should be accompanied by tests which exercise the bug
|
||
to both prove it has been resolved and to prevent future regressions
|
||
|
||
## Code Documentation and Commenting
|
||
|
||
- At a minimum every function must be commented with its intended purpose and
|
||
any assumptions that it makes
|
||
- Function comments must always begin with the name of the function per
|
||
[Effective Go](http://golang.org/doc/effective_go.html)
|
||
- Function comments should be complete sentences since they allow a wide
|
||
variety of automated presentations such as [go.dev](https://go.dev)
|
||
- The general rule of thumb is to look at it as if you were completely
|
||
unfamiliar with the code and ask yourself, would this give me enough
|
||
information to understand what this function does and how I'd probably want
|
||
to use it?
|
||
- Exported functions should also include detailed information the caller of the
|
||
function will likely need to know and/or understand:
|
||
|
||
**WRONG**
|
||
|
||
```Go
|
||
// convert a compact uint32 to big.Int
|
||
func CompactToBig(compact uint32) *big.Int {
|
||
```
|
||
|
||
**RIGHT**
|
||
|
||
```Go
|
||
// CompactToBig converts a compact representation of a whole number N to a
|
||
// big integer. The representation is similar to IEEE754 floating point
|
||
// numbers.
|
||
//
|
||
// Like IEEE754 floating point, there are three basic components: the sign,
|
||
// the exponent, and the mantissa. They are broken out as follows:
|
||
//
|
||
// * the most significant 8 bits represent the unsigned base 256 exponent
|
||
// * bit 23 (the 24th bit) represents the sign bit
|
||
// * the least significant 23 bits represent the mantissa
|
||
//
|
||
// -------------------------------------------------
|
||
// | Exponent | Sign | Mantissa |
|
||
// -------------------------------------------------
|
||
// | 8 bits [31-24] | 1 bit [23] | 23 bits [22-00] |
|
||
// -------------------------------------------------
|
||
//
|
||
// The formula to calculate N is:
|
||
// N = (-1^sign) * mantissa * 256^(exponent-3)
|
||
//
|
||
// This compact form is only used in bitcoin to encode unsigned 256-bit numbers
|
||
// which represent difficulty targets, thus there really is not a need for a
|
||
// sign bit, but it is implemented here to stay consistent with bitcoind.
|
||
func CompactToBig(compact uint32) *big.Int {
|
||
```
|
||
|
||
- Comments in the body of the code are highly encouraged, but they should
|
||
explain the intention of the code as opposed to just calling out the
|
||
obvious
|
||
|
||
**WRONG**
|
||
|
||
```Go
|
||
// return err if amt is less than 5460
|
||
if amt < 5460 {
|
||
return err
|
||
}
|
||
```
|
||
|
||
**RIGHT**
|
||
|
||
```Go
|
||
// Treat transactions with amounts less than the amount which is considered dust
|
||
// as non-standard.
|
||
if amt < 5460 {
|
||
return err
|
||
}
|
||
```
|
||
|
||
**NOTE:** The above should really use a constant as opposed to a magic number,
|
||
but it was left as a magic number to show how much of a difference a good
|
||
comment can make.
|
||
|
||
## Model Git Commit Messages
|
||
|
||
This project prefers to keep a clean commit history with well-formed commit
|
||
messages. This section illustrates a model commit message and provides a bit
|
||
of background for it. This content was originally created by Tim Pope and made
|
||
available on his website, however that website is no longer active, so it is
|
||
being provided here.
|
||
|
||
Here’s a model Git commit message:
|
||
|
||
```text
|
||
Short (50 chars or less) summary of changes
|
||
|
||
More detailed explanatory text, if necessary. Wrap it to about 72
|
||
characters or so. In some contexts, the first line is treated as the
|
||
subject of an email and the rest of the text as the body. The blank
|
||
line separating the summary from the body is critical (unless you omit
|
||
the body entirely); tools like rebase can get confused if you run the
|
||
two together.
|
||
|
||
Write your commit message in the present tense: "Fix bug" and not "Fixed
|
||
bug." This convention matches up with commit messages generated by
|
||
commands like git merge and git revert.
|
||
|
||
Further paragraphs come after blank lines.
|
||
|
||
- Bullet points are okay, too
|
||
- Typically a hyphen or asterisk is used for the bullet, preceded by a
|
||
single space, with blank lines in between, but conventions vary here
|
||
- Use a hanging indent
|
||
```
|
||
|
||
Prefix the summary with the subsystem/package when possible. Many other
|
||
projects make use of the code and this makes it easier for them to tell when
|
||
something they're using has changed. Have a look at [past
|
||
commits](https://github.com/btcsuite/btcd/commits/master) for examples of
|
||
commit messages.
|
||
|
||
Here are some of the reasons why wrapping your commit messages to 72 columns is
|
||
a good thing.
|
||
|
||
- git log doesn’t do any special wrapping of the commit messages. With the
|
||
default pager of less -S, this means your paragraphs flow far off the edge
|
||
of the screen, making them difficult to read. On an 80 column terminal, if we
|
||
subtract 4 columns for the indent on the left and 4 more for symmetry on the
|
||
right, we’re left with 72 columns.
|
||
- git format-patch --stdout converts a series of commits to a series of emails,
|
||
using the messages for the message body. Good email netiquette dictates we
|
||
wrap our plain text emails such that there’s room for a few levels of nested
|
||
reply indicators without overflow in an 80 column terminal.
|
||
|
||
## Code Approval Process
|
||
|
||
This section describes the code approval process that is used for code
|
||
contributions. This is how to get your changes into btcd.
|
||
|
||
## Code Review
|
||
|
||
All code which is submitted will need to be reviewed before inclusion into the
|
||
master branch. This process is performed by the project maintainers and usually
|
||
other committers who are interested in the area you are working in as well.
|
||
|
||
## Code Review Timeframe
|
||
|
||
The timeframe for a code review will vary greatly depending on factors such as
|
||
the number of other pull requests which need to be reviewed, the size and
|
||
complexity of the contribution, how well you followed the guidelines presented
|
||
on this page, and how easy it is for the reviewers to digest your commits. For
|
||
example, if you make one monolithic commit that makes sweeping changes to things
|
||
in multiple subsystems, it will obviously take much longer to review. You will
|
||
also likely be asked to split the commit into several smaller, and hence more
|
||
manageable, commits.
|
||
|
||
Keeping the above in mind, most small changes will be reviewed within a few
|
||
days, while large or far reaching changes may take weeks. This is a good reason
|
||
to stick with the [Share Early, Share Often](#ShareOften) development practice
|
||
outlined above.
|
||
|
||
## What is the review looking for?
|
||
|
||
The review is mainly ensuring the code follows the [Development Practices](#DevelopmentPractices)
|
||
and [Code Contribution Standards](#Standards). However, there are a few other
|
||
checks which are generally performed as follows:
|
||
|
||
- The code is stable and has no stability or security concerns
|
||
- The code is properly using existing APIs and generally fits well into the
|
||
overall architecture
|
||
- The change is not something which is deemed inappropriate by community
|
||
consensus
|
||
|
||
## Rework Code (if needed)
|
||
|
||
After the code review, the change will be accepted immediately if no issues are
|
||
found. If there are any concerns or questions, you will be provided with
|
||
feedback along with the next steps needed to get your contribution merged with
|
||
master. In certain cases the code reviewer(s) or interested committers may help
|
||
you rework the code, but generally you will simply be given feedback for you to
|
||
make the necessary changes.
|
||
|
||
This process will continue until the code is finally accepted.
|
||
|
||
## Acceptance
|
||
|
||
Once your code is accepted, it will be integrated with the master branch.
|
||
Typically it will be rebased and fast-forward merged to master as we prefer to
|
||
keep a clean commit history over a tangled weave of merge commits. However,
|
||
regardless of the specific merge method used, the code will be integrated with
|
||
the master branch and the pull request will be closed.
|
||
|
||
Rejoice as you will now be listed as a [contributor](https://github.com/btcsuite/btcd/graphs/contributors)!
|
||
|
||
## Contribution Standards
|
||
|
||
## Contribution Checklist
|
||
|
||
- [ ] All changes are Go version 1.17 compliant
|
||
- [ ] The code being submitted is commented according to the
|
||
[Code Documentation and Commenting](#CodeDocumentation) section
|
||
- [ ] For new code: Code is accompanied by tests which exercise both
|
||
the positive and negative (error paths) conditions (if applicable)
|
||
- [ ] For bug fixes: Code is accompanied by new tests which trigger
|
||
the bug being fixed to prevent regressions
|
||
- [ ] Any new logging statements use an appropriate subsystem and
|
||
logging level
|
||
- [ ] Code has been formatted with `go fmt`
|
||
- [ ] Running `go test` does not fail any tests
|
||
- [ ] Running `go vet` does not report any issues
|
||
- [ ] Running [golint](https://github.com/golang/lint) does not
|
||
report any **new** issues that did not already exist
|
||
|
||
## Licensing of Contributions
|
||
|
||
All contributions must be licensed with the
|
||
[ISC license](https://github.com/btcsuite/btcd/blob/master/LICENSE). This is
|
||
the same license as all of the code in the btcd suite.
|