Add CONTRIBUTING.md

btcdrak commented at 1:44 pm on September 24, 2015: contributor

Add a CONTRIBUTING.md as per https://github.com/blog/1184-contributing-guidelines

This document was written in collaboration with @laanwj.

paveljanik commented at 1:58 pm on September 24, 2015: contributor

Please add a line or two about Trivial tree?

in CONTRIBUTING.md: in 0ceac70f80 outdated

36+
37+If a pull request is specifically not to be considered for merging (yet) please prefix the title with [WIP] or use [Tasks Lists](https://github.com/blog/1375-task-lists-in-gfm-issues-pulls-comments) in markdown to the body of the pull request to indicate tasks are pending.
38+
39+The body of the pull request should contain enough description about what the patch does together with any justification/reasoning. You should include references to any discussions (for example other tickets or mailing list discussions).
40+
41+  - At this stage one should expect comments and review from other contributors. You can add more commits to your pull request by committing them locally and pushing to your fork until you have satisfied all feedback. If your pull request is accepted for merging, you may be asked by a maintainer to squash and or rebase your commits before it will be merged. The length of time for required for peer review is unpredictable and will vary from patch to patch.

MarcoFalke commented at 1:58 pm on September 24, 2015:

Nit: Remove -.

in CONTRIBUTING.md: in 0ceac70f80 outdated

84+
85+  - ACK means "I have tested the code and I agree it should be merged"
86+  - NACK means "I disagree this should be merged", and must be accompanied by sound technical justification. NACKs without accompanying reasoning may be disregarded.
87+  - utACK means "I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged"
88+  - Concept ACK mean "I agree in the general principle of this pull request"
89+

MarcoFalke commented at 1:59 pm on September 24, 2015:

Nit: Should “Nit” be included in this list?

laanwj commented at 2:01 pm on September 24, 2015: member

ACK, thanks for writing this up

laanwj added the label Docs and Output on Sep 24, 2015

btcdrak force-pushed on Sep 24, 2015

paveljanik commented at 2:05 pm on September 24, 2015: contributor

ACK, very well written!

gavinandresen commented at 2:08 pm on September 24, 2015: contributor

ACK, very nice!

laanwj commented at 2:14 pm on September 24, 2015: member

@paveljanik Re: the trivial tree, maybe @theuni could comment on that. I think it was a great idea, but AFAIK it has not been kept up to date regularly lately, so I ’m not sure what his stance on it is now.

paveljanik commented at 2:16 pm on September 24, 2015: contributor

If it is so, I think that adding [Trivial] to pull request titles is enough to solve this.

in CONTRIBUTING.md: in 86945479e6 outdated

19+
20+In general [commits should be atomic](https://en.wikipedia.org/wiki/Atomic_commit#Atomic_commit_convention) and diff should be easy to read. For this reason do not mix any formatting fixes or code moves with actual code changes.
21+
22+Commit messages should be verbose by default consisting of a short subject line (50 chars max), a blank line and detailed explanatory text as separate paragraph(s); unless the title alone is self-explanatory (like "fixed spelling") then a single title line is sufficient. Commit messages should be helpful to people reading your code in the future, so explain the reasoning for your decisions. Further explanation [here](http://chris.beams.io/posts/git-commit/).
23+
24+If a particular commit references another issue, please add the reference, for example "refs #1234", or "fixes #4321". Using "fixes or closes" keywords will cause the corresponding issue to be closed when the pull request is merged.

jonasschnelli commented at 2:17 pm on September 24, 2015:

I’m not sure if this is a good idea. Git and Github are two separate things. I agree with referencing a fix by fixes [#4321](/bitcoin-bitcoin/4321/) placed in a Github PR description or a Github comment. Placing it in a git commit is not required and not best practice IMO.

MarcoFalke commented at 2:18 pm on September 24, 2015:

+1. I prefer to place it somewhere in the GitHub PR description.

sipa commented at 2:20 pm on September 24, 2015:

Agree, to the extent possible you should be able to read and understand the commits without needing access to the github repository or its PRs/issues.

laanwj commented at 2:34 pm on September 24, 2015:

Absolutely, it must be understandable without referring to them, but please do refer to solved github issues in the commit descriptions. This makes it much easier to keep track of what was solved when when browsing history.

jonasschnelli commented at 2:19 pm on September 24, 2015: contributor

ACK. Nice work!

in CONTRIBUTING.md: in 86945479e6 outdated

 7+
 8+
 9+Contributor Workflow
10+--------------------
11+
12+The codebase is maintained using the “contributor workflow” where everyone without exception contributes patch proposal using “pull requests”. This facilitates social contribution, easy testing and peer review.

fanquake commented at 2:56 pm on September 24, 2015:

s/proposal/proposals

in CONTRIBUTING.md: in 86945479e6 outdated

15+  - Create topic branch
16+  - Commit patches 
17+
18+The project coding conventions in [doc/developer-notes.md](doc/developer-notes.md) must be adhered to.
19+
20+In general [commits should be atomic](https://en.wikipedia.org/wiki/Atomic_commit#Atomic_commit_convention) and diff should be easy to read. For this reason do not mix any formatting fixes or code moves with actual code changes.

fanquake commented at 2:56 pm on September 24, 2015:

s/diff/diffs

in CONTRIBUTING.md: in 86945479e6 outdated

17+
18+The project coding conventions in [doc/developer-notes.md](doc/developer-notes.md) must be adhered to.
19+
20+In general [commits should be atomic](https://en.wikipedia.org/wiki/Atomic_commit#Atomic_commit_convention) and diff should be easy to read. For this reason do not mix any formatting fixes or code moves with actual code changes.
21+
22+Commit messages should be verbose by default consisting of a short subject line (50 chars max), a blank line and detailed explanatory text as separate paragraph(s); unless the title alone is self-explanatory (like "fixed spelling") then a single title line is sufficient. Commit messages should be helpful to people reading your code in the future, so explain the reasoning for your decisions. Further explanation [here](http://chris.beams.io/posts/git-commit/).

fanquake commented at 2:59 pm on September 24, 2015:

Should “fixed spelling” be the example we give as a good simple commit message? Maybe “Correct typo in xx.cpp”, or something similar?

in CONTRIBUTING.md: in 86945479e6 outdated

32+
33+    [P2P] Add ZMQ message support
34+    [Consensus] Add new opcode for BIP-XXXX OP_CHECKAWESOMESIG
35+    [Qt Wallet] Refactor send code
36+
37+If a pull request is specifically not to be considered for merging (yet) please prefix the title with [WIP] or use [Tasks Lists](https://github.com/blog/1375-task-lists-in-gfm-issues-pulls-comments) in markdown to the body of the pull request to indicate tasks are pending.

fanquake commented at 3:01 pm on September 24, 2015:

s/markdown to the /markdown in the/

in CONTRIBUTING.md: in 86945479e6 outdated

36+
37+If a pull request is specifically not to be considered for merging (yet) please prefix the title with [WIP] or use [Tasks Lists](https://github.com/blog/1375-task-lists-in-gfm-issues-pulls-comments) in markdown to the body of the pull request to indicate tasks are pending.
38+
39+The body of the pull request should contain enough description about what the patch does together with any justification/reasoning. You should include references to any discussions (for example other tickets or mailing list discussions).
40+
41+At this stage one should expect comments and review from other contributors. You can add more commits to your pull request by committing them locally and pushing to your fork until you have satisfied all feedback. If your pull request is accepted for merging, you may be asked by a maintainer to squash and or rebase your commits before it will be merged. The length of time for required for peer review is unpredictable and will vary from patch to patch.

fanquake commented at 3:02 pm on September 24, 2015:

s/time for required/time required/

in CONTRIBUTING.md: in 86945479e6 outdated

64+
65+The following applies to code changes to the Bitcoin Core project (and related projects such as secp256k1), and is not to be confused with overall Bitcoin Network Protocol consensus changes.
66+
67+Whether a pull request is merged into Bitcoin Core rests with the project merge maintainers and ultimately the project lead. 
68+
69+Maintainers will take into consideration if a patch is in line with the general principles of the project, that the quality of the patch is at the minimum bar and as well as judging the general consensus of contributors.

fanquake commented at 3:06 pm on September 24, 2015:

Suggest rewriting this slightly *that the quality of the patch meets the minimum standards for inclusion, as well as

in CONTRIBUTING.md: in 86945479e6 outdated

73+  - have a clear use case, fix a demonstrable bug or serve the greater good of the project (for example refactoring for modularisation);
74+  - be well peer reviewed;
75+  - have unit tests and functional tests where appropriate;
76+  - follow code style guidelines;
77+  - not break the existing test suite;
78+  - where bugs are fixed, where possible, there should be unit test demonstrating the bug and also proving the fix. This helps prevent regression;

fanquake commented at 3:07 pm on September 24, 2015:

s/be unit test/be a unit test/

in CONTRIBUTING.md: in 86945479e6 outdated

78+  - where bugs are fixed, where possible, there should be unit test demonstrating the bug and also proving the fix. This helps prevent regression;
79+  - where consensus rules are affected, must be accompanied by a published BIP assigned with a number;
80+
81+###Peer Review
82+
83+Anyone may participate in peer review is expressed by comments in the pull request. Typically reviewers will review the code for obvious errors, as well as test out the patch set and opine on the technical merits of the patch. Project maintainers take into account the peer review when determining if there is consensus to merge a pull request (remember that discussions may have been spread out over github, mailing list and IRC discussions). The following language is used within pull-request comments:

fanquake commented at 3:08 pm on September 24, 2015:

s/peer review is/peer review which is/

fanquake commented at 3:16 pm on September 24, 2015: member

Nice work. Definite improvement on the developer-notes, which were somewhat hidden away. The fact that this is shown as suggested reading to everyone opening a pull request is also good.

in CONTRIBUTING.md: in 7a95562901 outdated

0@@ -0,0 +1,104 @@
1+Contributing to Bitcoin Core
2+============================
3+
4+The Bitcoin Core project operates an open contributor model where anyone is welcome to contribute towards development in the form of peer review, testing and patches. This document explains the practical process and guidelines for contributing.
5+
6+Firstly in terms of structure, there is no particular concept of “Core developers” in the sense of privileged people. Open source often naturally revolves around meritocracy where longer term contributors gain more trust from the developer community. However, some hierarchy is necessary for practical purposes. As such there are repositories “maintainers” who are responsible for merging pull requests as well as a “lead maintainer” who is responsible for overall merging, moderation and appointment of maintainers and the release cycle.

luke-jr commented at 5:45 pm on September 24, 2015:

s/hierarchy/roles/

sipa commented at 7:00 pm on September 24, 2015:

s/repositories/repository/

in CONTRIBUTING.md: in 7a95562901 outdated

30+
31+The title of the pull request should be prefixed by the component or area that the pull request affects. Examples:
32+
33+    [P2P] Add ZMQ message support
34+    [Consensus] Add new opcode for BIP-XXXX OP_CHECKAWESOMESIG
35+    [Qt Wallet] Refactor send code

luke-jr commented at 5:48 pm on September 24, 2015:

Prefer colon format (and seems to be more common):

0p2p: Add ZMQ message support
1consensus: Add new opcode for BIP-XXXX
2qt/wallet: Refactor send code

(also note ZMQ really has nothing to do with p2p..)

in CONTRIBUTING.md: in 7a95562901 outdated

47+Patchsets should always be focused. For example, a pull request could add a feature, fix a bug, or refactor code; but not a mixture. Please also avoid super pull requests which attempt to do too much, are overly large, or overly complex as this makes review difficult.
48+
49+
50+###Features
51+
52+When adding a new feature, thought must be given to the long term technical debt and maintenance that feature may require after inclusion. Before proposing a new feature that will require maintenance, please consider if you are willing to maintain it (including bug fixing). If features get orphaned with no maintainer in the future, they may be removed by the Repository Maintainer.

luke-jr commented at 5:49 pm on September 24, 2015:

Should I reopen TBC support? ;)

in CONTRIBUTING.md: in 7a95562901 outdated

62+
63+
64+"Decision Making" Process
65+-------------------------
66+
67+The following applies to code changes to the Bitcoin Core project (and related projects such as secp256k1), and is not to be confused with overall Bitcoin Network Protocol consensus changes.

luke-jr commented at 5:50 pm on September 24, 2015:

libsecp256k1*

Although I’m not sure this document should apply to it.

in CONTRIBUTING.md: in 7a95562901 outdated

75+  - have a clear use case, fix a demonstrable bug or serve the greater good of the project (for example refactoring for modularisation);
76+  - be well peer reviewed;
77+  - have unit tests and functional tests where appropriate;
78+  - follow code style guidelines;
79+  - not break the existing test suite;
80+  - where bugs are fixed, where possible, there should be unit tests demonstrating the bug and also proving the fix. This helps prevent regression;

luke-jr commented at 5:51 pm on September 24, 2015:

This doesn’t belong under “must”.

in CONTRIBUTING.md: in 7a95562901 outdated

76+  - be well peer reviewed;
77+  - have unit tests and functional tests where appropriate;
78+  - follow code style guidelines;
79+  - not break the existing test suite;
80+  - where bugs are fixed, where possible, there should be unit tests demonstrating the bug and also proving the fix. This helps prevent regression;
81+  - where consensus rules are affected, must be accompanied by a published BIP assigned with a number;

luke-jr commented at 5:52 pm on September 24, 2015:

Also needs non-trivial miner support (for new rules) or community consensus (for removed/changed rules).

laanwj commented at 1:50 pm on September 25, 2015:

absolutely. They are a wholly different animal from normal run-of-the-mill changes, that should be made clear.

in CONTRIBUTING.md: in 7a95562901 outdated

84+###Peer Review
85+
86+Anyone may participate in peer review which is expressed by comments in the pull request. Typically reviewers will review the code for obvious errors, as well as test out the patch set and opine on the technical merits of the patch. Project maintainers take into account the peer review when determining if there is consensus to merge a pull request (remember that discussions may have been spread out over github, mailing list and IRC discussions). The following language is used within pull-request comments:
87+
88+  - ACK means "I have tested the code and I agree it should be merged";
89+  - NACK means "I disagree this should be merged", and must be accompanied by sound technical justification. NACKs without accompanying reasoning may be disregarded;

luke-jr commented at 5:52 pm on September 24, 2015:

Not all good NACK reasons are technical.

laanwj commented at 1:48 pm on September 25, 2015:

I’m sure there are some exceptions, but in general they should aim to be.

btcdrak force-pushed on Sep 24, 2015

in CONTRIBUTING.md: in 73f63e8978 outdated

91+  - Concept ACK means "I agree in the general principle of this pull request";
92+  - Nit refers to trivial, often non-blocking issues.
93+
94+Project maintainers reserve the right to weigh the opinions of peer reviewers using common sense judgement and also may weight based on meritocracy: Those that have demonstrated a deeper commitment and understanding towards the project (over time) or have clear domain expertise may naturally have more weight, as one would expect in all walks of life.
95+
96+Where a patch set affects consensus critical code, the bar will be set much higher in terms of discussion and peer review, remembering mistakes could be very costly to the wider community. This includes refactoring of consensus critical code.

instagibbs commented at 1:56 am on September 25, 2015:

“remembering [that] mistakes” or “keeping in mind that” perhaps?

Good work :)

btcdrak force-pushed on Sep 25, 2015

btcdrak force-pushed on Sep 26, 2015

Add CONTRIBUTING.md 06d92d71a2

btcdrak force-pushed on Sep 26, 2015

laanwj merged this on Sep 26, 2015

laanwj closed this on Sep 26, 2015

laanwj referenced this in commit 2fa417f829 on Sep 26, 2015

MarcoFalke locked this on Sep 8, 2021

Add CONTRIBUTING.md #6718