@@ -6,27 +6,28 @@ welcome to contribute towards development in the form of peer review, testing
6
6
and patches. This document explains the practical process and guidelines for
7
7
contributing.
8
8
9
- Firstly in terms of structure, there is no particular concept of "Core
9
+ First, in terms of structure, there is no particular concept of "Bitcoin Core
10
10
developers" in the sense of privileged people. Open source often naturally
11
- revolves around meritocracy where longer term contributors gain more trust from
12
- the developer community. However, some hierarchy is necessary for practical
13
- purposes. As such there are repository "maintainers" who are responsible for
14
- merging pull requests as well as a "lead maintainer" who is responsible for the
15
- release cycle, overall merging, moderation and appointment of maintainers.
11
+ revolves around a meritocracy where contributors earn trust from the developer
12
+ community over time. Nevertheless, some hierarchy is necessary for practical
13
+ purposes. As such, there are repository "maintainers" who are responsible for
14
+ merging pull requests, as well as a "lead maintainer" who is responsible for the
15
+ release cycle as well as overall merging, moderation and appointment of
16
+ maintainers.
16
17
17
18
Getting Started
18
19
---------------
19
20
20
21
New contributors are very welcome and needed.
21
22
22
- Reviewing and testing is the most effective way you can contribute as a new
23
- contributor, and it also will teach you much more about the code and process
24
- than opening PRs . Please refer to the section [ peer review] ( #peer-review ) later
25
- in this document .
23
+ Reviewing and testing is highly valued and the most effective way you can contribute
24
+ as a new contributor. It also will teach you much more about the code and
25
+ process than opening pull requests . Please refer to the [ peer review] ( #peer-review )
26
+ section below .
26
27
27
28
Before you start contributing, familiarize yourself with the Bitcoin Core build
28
29
system and tests. Refer to the documentation in the repository on how to build
29
- Bitcoin Core and how to run the unit and functional tests.
30
+ Bitcoin Core and how to run the unit tests, functional tests, and fuzz tests.
30
31
31
32
There are many open issues of varying difficulty waiting to be fixed.
32
33
If you're looking for somewhere to start contributing, check out the
@@ -62,7 +63,7 @@ history logs can be found
62
63
on [ http://www.erisian.com.au/bitcoin-core-dev/ ] ( http://www.erisian.com.au/bitcoin-core-dev/ )
63
64
and [ http://gnusha.org/bitcoin-core-dev/ ] ( http://gnusha.org/bitcoin-core-dev/ ) .
64
65
65
- Discussion about code base improvements happens in GitHub issues and on pull
66
+ Discussion about codebase improvements happens in GitHub issues and pull
66
67
requests.
67
68
68
69
The developer
@@ -75,7 +76,7 @@ Contributor Workflow
75
76
--------------------
76
77
77
78
The codebase is maintained using the "contributor workflow" where everyone
78
- without exception contributes patch proposals using "pull requests". This
79
+ without exception contributes patch proposals using "pull requests" (PRs) . This
79
80
facilitates social contribution, easy testing and peer review.
80
81
81
82
To contribute a patch, the workflow is as follows:
@@ -113,6 +114,9 @@ In general, [commits should be atomic](https://en.wikipedia.org/wiki/Atomic_comm
113
114
and diffs should be easy to read. For this reason, do not mix any formatting
114
115
fixes or code moves with actual code changes.
115
116
117
+ Make sure each individual commit is hygienic: that it builds successfully on its
118
+ own without warnings, errors, regressions, or test failures.
119
+
116
120
Commit messages should be verbose by default consisting of a short subject line
117
121
(50 chars max), a blank line and detailed explanatory text as separate
118
122
paragraph(s), unless the title alone is self-explanatory (like "Corrected typo
@@ -124,7 +128,7 @@ If a particular commit references another issue, please add the reference. For
124
128
example: ` refs #1234 ` or ` fixes #4321 ` . Using the ` fixes ` or ` closes ` keywords
125
129
will cause the corresponding issue to be closed when the pull request is merged.
126
130
127
- Commit messages should never contain any ` @ ` mentions.
131
+ Commit messages should never contain any ` @ ` mentions (usernames prefixed with "@") .
128
132
129
133
Please refer to the [ Git manual] ( https://git-scm.com/doc ) for more information
130
134
about Git.
@@ -158,10 +162,16 @@ Examples:
158
162
qt: Add feed bump button
159
163
log: Fix typo in log message
160
164
161
- The body of the pull request should contain enough description about what the
162
- patch does together with any justification/reasoning. You should include
163
- references to any discussions (for example other tickets or mailing list
164
- discussions).
165
+ The body of the pull request should contain sufficient description of * what* the
166
+ patch does, and even more importantly, * why* , with justification and reasoning.
167
+ You should include references to any discussions (for example, other issues or
168
+ mailing list discussions).
169
+
170
+ The description for a new pull request should not contain any ` @ ` mentions. The
171
+ PR description will be included in the commit message when the PR is merged and
172
+ any users mentioned in the description will be annoyingly notified each time a
173
+ fork of Bitcoin Core copies the merge. Instead, make any username mentions in a
174
+ subsequent comment to the PR.
165
175
166
176
### Translation changes
167
177
@@ -197,13 +207,13 @@ before it will be merged. The basic squashing workflow is shown below.
197
207
# Save and quit.
198
208
git push -f # (force push to GitHub)
199
209
200
- Please update the resulting commit message if needed. It should read as a
201
- coherent message. In most cases, this means that you should not just list the
202
- interim commits.
210
+ Please update the resulting commit message, if needed. It should read as a
211
+ coherent message. In most cases, this means not just listing the interim
212
+ commits.
203
213
204
- If you have problems with squashing ( or other workflows with ` git ` ) , you can
205
- alternatively enable "Allow edits from maintainers" in the right GitHub
206
- sidebar and ask for help in the pull request.
214
+ If you have problems with squashing or other git workflows , you can enable
215
+ "Allow edits from maintainers" in the right-hand sidebar of the GitHub web
216
+ interface and ask for help in the pull request.
207
217
208
218
Please refrain from creating several pull requests for the same change.
209
219
Use the pull request that is already open (or was created earlier) to amend
@@ -287,8 +297,8 @@ In general, all pull requests must:
287
297
288
298
- Have a clear use case, fix a demonstrable bug or serve the greater good of
289
299
the project (for example refactoring for modularisation);
290
- - Be well peer reviewed;
291
- - Have unit tests and functional tests where appropriate;
300
+ - Be well peer- reviewed;
301
+ - Have unit tests, functional tests, and fuzz tests, where appropriate;
292
302
- Follow code style guidelines ([ C++] ( doc/developer-notes.md ) , [ functional tests] ( test/functional/README.md ) );
293
303
- Not break the existing test suite;
294
304
- Where bugs are fixed, where possible, there should be unit tests
@@ -315,7 +325,7 @@ spread out over GitHub, mailing list and IRC discussions).
315
325
#### Conceptual Review
316
326
317
327
A review can be a conceptual review, where the reviewer leaves a comment
318
- * ` Concept (N)ACK ` , meaning "I do (not) agree in the general goal of this pull
328
+ * ` Concept (N)ACK ` , meaning "I do (not) agree with the general goal of this pull
319
329
request",
320
330
* ` Approach (N)ACK ` , meaning ` Concept ACK ` , but "I do (not) agree with the
321
331
approach of this change".
@@ -325,30 +335,28 @@ NACKs without accompanying reasoning may be disregarded.
325
335
326
336
#### Code Review
327
337
328
- After conceptual agreement on the change, code review can be provided. It is
329
- starting with ` ACK BRANCH_COMMIT ` , where ` BRANCH_COMMIT ` is the top of the
330
- topic branch. The review is followed by a description of how the reviewer did
331
- the review. The following
332
- language is used within pull-request comments:
338
+ After conceptual agreement on the change, code review can be provided. A review
339
+ begins with ` ACK BRANCH_COMMIT ` , where ` BRANCH_COMMIT ` is the top of the PR
340
+ branch, followed by a description of how the reviewer did the review. The
341
+ following language is used within pull request comments:
333
342
334
- - "I have tested the code", involving
335
- change-specific manual testing in addition to running the unit and functional
336
- tests, and in case it is not obvious how the manual testing was done, it should
337
- be described;
343
+ - "I have tested the code", involving change-specific manual testing in
344
+ addition to running the unit, functional, or fuzz tests, and in case it is
345
+ not obvious how the manual testing was done, it should be described;
338
346
- "I have not tested the code, but I have reviewed it and it looks
339
347
OK, I agree it can be merged";
340
- - Nit refers to trivial, often non-blocking issues .
348
+ - A "nit" refers to a trivial, often non-blocking issue .
341
349
342
350
Project maintainers reserve the right to weigh the opinions of peer reviewers
343
- using common sense judgement and also may weight based on meritocracy: Those
344
- that have demonstrated a deeper commitment and understanding towards the project
345
- (over time) or have clear domain expertise may naturally have more weight, as
346
- one would expect in all walks of life.
351
+ using common sense judgement and may also weigh based on merit. Reviewers that
352
+ have demonstrated a deeper commitment and understanding of the project over time
353
+ or who have clear domain expertise may naturally have more weight, as one would
354
+ expect in all walks of life.
347
355
348
- Where a patch set affects consensus critical code, the bar will be set much
356
+ Where a patch set affects consensus- critical code, the bar will be much
349
357
higher in terms of discussion and peer review requirements, keeping in mind that
350
358
mistakes could be very costly to the wider community. This includes refactoring
351
- of consensus critical code.
359
+ of consensus- critical code.
352
360
353
361
Where a patch set proposes to change the Bitcoin consensus, it must have been
354
362
discussed extensively on the mailing list and IRC, be accompanied by a widely
@@ -365,7 +373,7 @@ about:
365
373
366
374
- It may be because of a feature freeze due to an upcoming release. During this time,
367
375
only bug fixes are taken into consideration. If your pull request is a new feature,
368
- it will not be prioritized until the release is over . Wait for release.
376
+ it will not be prioritized until after the release. Wait for the release.
369
377
- It may be because the changes you are suggesting do not appeal to people. Rather than
370
378
nits and critique, which require effort and means they care enough to spend time on your
371
379
contribution, thundering silence is a good sign of widespread (mild) dislike of a given change
@@ -375,16 +383,18 @@ about:
375
383
[ developer notes] ( doc/developer-notes.md ) , is dangerous or insecure, is messily written, etc.
376
384
Identify and address any of the issues you find. Then ask e.g. on IRC if someone could give
377
385
their opinion on the concept itself.
378
- - It may be because your code is too complex for all but a few people. And those people
386
+ - It may be because your code is too complex for all but a few people, and those people
379
387
may not have realized your pull request even exists. A great way to find people who
380
388
are qualified and care about the code you are touching is the
381
389
[ Git Blame feature] ( https://help.github.com/articles/tracing-changes-in-a-file/ ) . Simply
382
- find the person touching the code you are touching before you and see if you can find
383
- them and give them a nudge. Don't be incessant about the nudging though.
390
+ look up who last modified the code you are changing and see if you can find
391
+ them and give them a nudge. Don't be incessant about the nudging, though.
384
392
- Finally, if all else fails, ask on IRC or elsewhere for someone to give your pull request
385
- a look. If you think you've been waiting an unreasonably long amount of time (month+) for
386
- no particular reason (few lines changed, etc), this is totally fine. Try to return the favor
387
- when someone else is asking for feedback on their code, and universe balances out.
393
+ a look. If you think you've been waiting for an unreasonably long time (say,
394
+ more than a month) for no particular reason (a few lines changed, etc.),
395
+ this is totally fine. Try to return the favor when someone else is asking
396
+ for feedback on their code, and the universe balances out.
397
+ - Remember that the best thing you can do while waiting is give review to others!
388
398
389
399
390
400
Backporting
@@ -393,11 +403,11 @@ Backporting
393
403
Security and bug fixes can be backported from ` master ` to release
394
404
branches.
395
405
If the backport is non-trivial, it may be appropriate to open an
396
- additional PR, to backport the change, only after the original PR
406
+ additional PR to backport the change, but only after the original PR
397
407
has been merged.
398
408
Otherwise, backports will be done in batches and
399
409
the maintainers will use the proper ` Needs backport (...) ` labels
400
- when needed (the original author does not need to worry).
410
+ when needed (the original author does not need to worry about it ).
401
411
402
412
A backport should contain the following metadata in the commit body:
403
413
0 commit comments