/ CONTRIBUTING.md
CONTRIBUTING.md
1 Contributing to Bitcoin Core 2 ============================ 3 4 The Bitcoin Core project operates an open contributor model where anyone is 5 welcome to contribute towards development in the form of peer review, testing 6 and patches. This document explains the practical process and guidelines for 7 contributing. 8 9 First, in terms of structure, there is no particular concept of "Bitcoin Core 10 developers" in the sense of privileged people. Open source often naturally 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, the [release cycle](/doc/release-process.md), and 15 moderation. 16 17 Getting Started 18 --------------- 19 20 New contributors are very welcome and needed. 21 22 Reviewing and testing is highly valued and the most effective way you can contribute 23 as a new contributor. It also will teach you much more about the code and 24 process than opening pull requests. Please refer to the [peer review](#peer-review) 25 section below. 26 27 Before you start contributing, familiarize yourself with the Bitcoin Core build 28 system and tests. Refer to the documentation in the repository on how to build 29 Bitcoin Core and how to run the unit tests, functional tests, and fuzz tests. 30 31 There are many open issues of varying difficulty waiting to be fixed. 32 If you're looking for somewhere to start contributing, check out the 33 [good first issue](https://github.com/bitcoin/bitcoin/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22) 34 list or changes that are 35 [up for grabs](https://github.com/bitcoin/bitcoin/issues?utf8=%E2%9C%93&q=label%3A%22Up+for+grabs%22). 36 Some of them might no longer be applicable. So if you are interested, but 37 unsure, you might want to leave a comment on the issue first. 38 39 You may also participate in the [Bitcoin Core PR Review Club](https://bitcoincore.reviews/). 40 41 ### Good First Issue Label 42 43 The purpose of the `good first issue` label is to highlight which issues are 44 suitable for a new contributor without a deep understanding of the codebase. 45 46 However, good first issues can be solved by anyone. If they remain unsolved 47 for a longer time, a frequent contributor might address them. 48 49 You do not need to request permission to start working on an issue. However, 50 you are encouraged to leave a comment if you are planning to work on it. This 51 will help other contributors monitor which issues are actively being addressed 52 and is also an effective way to request assistance if and when you need it. 53 54 Communication Channels 55 ---------------------- 56 57 Most communication about Bitcoin Core development happens on IRC, in the 58 `#bitcoin-core-dev` channel on Libera Chat. The easiest way to participate on IRC is 59 with the web client, [web.libera.chat](https://web.libera.chat/#bitcoin-core-dev). Chat 60 history logs can be found 61 on [https://www.erisian.com.au/bitcoin-core-dev/](https://www.erisian.com.au/bitcoin-core-dev/) 62 and [https://gnusha.org/bitcoin-core-dev/](https://gnusha.org/bitcoin-core-dev/). 63 64 Discussion about codebase improvements happens in GitHub issues and pull 65 requests. 66 67 The developer 68 [mailing list](https://groups.google.com/g/bitcoindev) 69 should be used to discuss complicated or controversial consensus or P2P protocol changes before working on 70 a patch set. 71 Archives can be found on [https://gnusha.org/pi/bitcoindev/](https://gnusha.org/pi/bitcoindev/). 72 73 74 Contributor Workflow 75 -------------------- 76 77 The codebase is maintained using the "contributor workflow" where everyone 78 without exception contributes patch proposals using "pull requests" (PRs). This 79 facilitates social contribution, easy testing and peer review. 80 81 Pull request authors must fully and confidently understand their own changes 82 and must have tested them. Contributors should mention which tests cover their 83 changes, or include the manual steps they used to confirm the change. 84 Contributors are expected to be prepared to clearly motivate and explain their 85 changes. If there is doubt, the pull request may be closed. 86 Please refer to the [peer review](#peer-review) section below for more details. 87 88 To contribute a patch, the workflow is as follows: 89 90 1. Fork repository ([only for the first time](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/fork-a-repo)) 91 1. Create topic branch 92 1. Commit patches 93 94 For GUI-related issues or pull requests, the https://github.com/bitcoin-core/gui repository should be used. 95 For all other issues and pull requests, the https://github.com/bitcoin/bitcoin node repository should be used. 96 97 The master branch for all monotree repositories is identical. 98 99 As a rule of thumb, everything that only modifies `src/qt` is a GUI-only pull 100 request. However: 101 102 * For global refactoring or other transversal changes the node repository 103 should be used. 104 * For GUI-related build system changes, the node repository should be used 105 because the change needs review by the build systems reviewers. 106 * Changes in `src/interfaces` need to go to the node repository because they 107 might affect other components like the wallet. 108 109 For large GUI changes that include build system and interface changes, it is 110 recommended to first open a pull request against the GUI repository. When there 111 is agreement to proceed with the changes, a pull request with the build system 112 and interfaces changes can be submitted to the node repository. 113 114 The project coding conventions in the [developer notes](doc/developer-notes.md) 115 must be followed. 116 117 ### Committing Patches 118 119 In general, [commits should be atomic](https://en.wikipedia.org/wiki/Atomic_commit#Atomic_commit_convention) 120 and diffs should be easy to read. For this reason, do not mix any formatting 121 fixes or code moves with actual code changes. 122 123 Make sure each individual commit is hygienic: that it builds successfully on its 124 own without warnings, errors, regressions, or test failures. 125 This means tests must be updated in the same commit that changes the behavior. 126 127 Commit messages should be verbose by default consisting of a short subject line 128 (50 chars max), a blank line and detailed explanatory text as separate 129 paragraph(s), unless the title alone is self-explanatory (like "Correct typo 130 in init.cpp") in which case a single title line is sufficient. Commit messages should be 131 helpful to people reading your code in the future, so explain the reasoning for 132 your decisions. Further explanation [here](https://cbea.ms/git-commit/). 133 134 If a particular commit references another issue, please add the reference. For 135 example: `refs #1234` or `fixes #4321`. Using the `fixes` or `closes` keywords 136 will cause the corresponding issue to be closed when the pull request is merged. 137 138 Commit messages should never contain any `@` mentions (usernames prefixed with "@"). 139 140 Please refer to the [Git manual](https://git-scm.com/doc) for more information 141 about Git. 142 143 - Push changes to your fork 144 - Create pull request 145 146 ### Creating the Pull Request 147 148 The title of the pull request should be prefixed by the component or area that 149 the pull request affects. Valid areas as: 150 151 - `consensus` for changes to consensus critical code 152 - `doc` for changes to the documentation 153 - `qt` or `gui` for changes to bitcoin-qt 154 - `log` for changes to log messages 155 - `mining` for changes to the mining code 156 - `net` or `p2p` for changes to the peer-to-peer network code 157 - `refactor` for structural changes that do not change behavior 158 - `rpc`, `rest` or `zmq` for changes to the RPC, REST or ZMQ APIs 159 - `contrib` or `cli` for changes to the scripts and tools 160 - `test`, `qa` or `ci` for changes to the unit tests, QA tests or CI code 161 - `util` or `lib` for changes to the utils or libraries 162 - `wallet` for changes to the wallet code 163 - `build` for changes to CMake 164 - `guix` for changes to the GUIX reproducible builds 165 166 Examples: 167 168 consensus: Add new opcode for BIP-XXXX OP_CHECKAWESOMESIG 169 net: Automatically create onion service, listen on Tor 170 qt: Add feed bump button 171 log: Fix typo in log message 172 173 The body of the pull request should contain sufficient description of *what* the 174 patch does, and even more importantly, *why*, with justification and reasoning. 175 You should include references to any discussions (for example, other issues or 176 mailing list discussions). 177 178 The description for a new pull request should not contain any `@` mentions. The 179 PR description will be included in the commit message when the PR is merged and 180 any users mentioned in the description will be annoyingly notified each time a 181 fork of Bitcoin Core copies the merge. Instead, make any username mentions in a 182 subsequent comment to the PR. 183 184 ### Translation changes 185 186 Note that translations should not be submitted as pull requests. Please see 187 [Translation Process](/doc/translation_process.md) 188 for more information on helping with translations. 189 190 ### Work in Progress Changes and Requests for Comments 191 192 If a pull request is not to be considered for merging (yet), please 193 prefix the title with [WIP] or use [Tasks Lists](https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#task-lists) 194 in the body of the pull request to indicate tasks are pending. 195 196 ### Address Feedback 197 198 At this stage, one should expect comments and review from other contributors. You 199 can add more commits to your pull request by committing them locally and pushing 200 to your fork. 201 202 You are expected to reply to any review comments before your pull request is 203 merged. You may update the code or reject the feedback if you do not agree with 204 it, but you should express so in a reply. If there is outstanding feedback and 205 you are not actively working on it, your pull request may be closed. 206 207 Please refer to the [peer review](#peer-review) section below for more details. 208 209 ### Squashing Commits 210 211 If your pull request contains fixup commits (commits that change the same line of code repeatedly) or too fine-grained 212 commits, you may be asked to [squash](https://git-scm.com/docs/git-rebase#_interactive_mode) your commits 213 before it will be reviewed. The basic squashing workflow is shown below. 214 215 git checkout your_branch_name 216 git rebase -i HEAD~n 217 # n is normally the number of commits in the pull request. 218 # Set commits (except the one in the first line) from 'pick' to 'squash', save and quit. 219 # On the next screen, edit/refine commit messages. 220 # Save and quit. 221 git push -f # (force push to GitHub) 222 223 Please update the resulting commit message, if needed. It should read as a 224 coherent message. In most cases, this means not just listing the interim 225 commits. 226 227 If your change contains a merge commit, the above workflow may not work and you 228 will need to remove the merge commit first. See the next section for details on 229 how to rebase. 230 231 Please refrain from creating several pull requests for the same change. 232 Use the pull request that is already open (or was created earlier) to amend 233 changes. This preserves the discussion and review that happened earlier for 234 the respective change set. 235 236 The length of time required for peer review is unpredictable and will vary from 237 pull request to pull request. 238 239 ### Rebasing Changes 240 241 When a pull request conflicts with the target branch, you may be asked to rebase it on top of the current target branch. 242 243 git fetch https://github.com/bitcoin/bitcoin # Fetch the latest upstream commit 244 git rebase FETCH_HEAD # Rebuild commits on top of the new base 245 246 This project aims to have a clean git history, where code changes are only made in non-merge commits. This simplifies 247 auditability because merge commits can be assumed to not contain arbitrary code changes. Merge commits should be signed, 248 and the resulting git tree hash must be deterministic and reproducible. The script in 249 [/contrib/verify-commits](/contrib/verify-commits) checks that. 250 251 After a rebase, reviewers are encouraged to sign off on the force push. This should be relatively straightforward with 252 the `git range-diff` tool explained in the [productivity 253 notes](/doc/productivity.md#diff-the-diffs-with-git-range-diff). To avoid needless review churn, maintainers will 254 generally merge pull requests that received the most review attention first. 255 256 Pull Request Philosophy 257 ----------------------- 258 259 Patchsets should always be focused. For example, a pull request could add a 260 feature, fix a bug, or refactor code; but not a mixture. Please also avoid super 261 pull requests which attempt to do too much, are overly large, or overly complex 262 as this makes review difficult. 263 264 265 ### Features 266 267 When adding a new feature, thought must be given to the long term technical debt 268 and maintenance that feature may require after inclusion. Before proposing a new 269 feature that will require maintenance, please consider if you are willing to 270 maintain it (including bug fixing). If features get orphaned with no maintainer 271 in the future, they may be removed by the Repository Maintainer. 272 273 274 ### Refactoring 275 276 Refactoring is a necessary part of any software project's evolution. The 277 following guidelines cover refactoring pull requests for the project. 278 279 There are three categories of refactoring: code-only moves, code style fixes, and 280 code refactoring. In general, refactoring pull requests should not mix these 281 three kinds of activities in order to make refactoring pull requests easy to 282 review and uncontroversial. In all cases, refactoring PRs must not change the 283 behaviour of code within the pull request (bugs must be preserved as is). 284 285 Project maintainers aim for a quick turnaround on refactoring pull requests, so 286 where possible keep them short, uncomplex and easy to verify. 287 288 Pull requests that refactor the code should not be made by new contributors. It 289 requires a certain level of experience to know where the code belongs to and to 290 understand the full ramification (including rebase effort of open pull requests). 291 292 Trivial pull requests or pull requests that refactor the code with no clear 293 benefits may be immediately closed by the maintainers to reduce unnecessary 294 workload on reviewing. 295 296 297 "Decision Making" Process 298 ------------------------- 299 300 The following applies to code changes to the Bitcoin Core project (and related 301 projects such as libsecp256k1), and is not to be confused with overall Bitcoin 302 Network Protocol consensus changes. 303 304 Whether a pull request is merged into Bitcoin Core rests with the project merge 305 maintainers. 306 307 Maintainers will take into consideration if a patch is in line with the general 308 principles of the project; meets the minimum standards for inclusion; and will 309 judge the general consensus of contributors. 310 311 In general, all pull requests must: 312 313 - Have a clear use case, fix a demonstrable bug or serve the greater good of 314 the project (for example refactoring for modularisation); 315 - Be well peer-reviewed; 316 - Have unit tests, functional tests, and fuzz tests, where appropriate; 317 - Follow code style guidelines ([C++](doc/developer-notes.md), [functional tests](test/functional/README.md)); 318 - Not break the existing test suite; 319 - Where bugs are fixed, where possible, there should be unit tests 320 demonstrating the bug and also proving the fix. This helps prevent regression. 321 - Change relevant comments and documentation when behaviour of code changes. 322 323 Patches that change Bitcoin consensus rules are considerably more involved than 324 normal because they affect the entire ecosystem and so must be preceded by 325 extensive mailing list discussions and have a numbered BIP. While each case will 326 be different, one should be prepared to expend more time and effort than for 327 other kinds of patches because of increased peer review and consensus building 328 requirements. 329 330 331 ### Peer Review 332 333 Anyone may participate in peer review which is expressed by comments in the pull 334 request. Typically reviewers will review the code for obvious errors, as well as 335 test out the patch set and opine on the technical merits of the patch. Project 336 maintainers take into account the peer review when determining if there is 337 consensus to merge a pull request (remember that discussions may have been 338 spread out over GitHub, mailing list and IRC discussions). 339 340 Code review is a burdensome but important part of the development process, and 341 as such, certain types of pull requests are rejected. In general, if the 342 **improvements** do not warrant the **review effort** required, the PR has a 343 high chance of being rejected. It is up to the PR author to convince the 344 reviewers that the changes warrant the review effort, and if reviewers are 345 "Concept NACK'ing" the PR, the author may need to present arguments and/or do 346 research backing their suggested changes. 347 348 Moreover, if there is reasonable doubt that the pull request author does not 349 fully understand the changes they are submitting themselves, or if it becomes 350 clear that they have not tested the changes on a basic level themselves, the 351 pull request may be closed immediately. 352 353 #### Conceptual Review 354 355 A review can be a conceptual review, where the reviewer leaves a comment 356 * `Concept (N)ACK`, meaning "I do (not) agree with the general goal of this pull 357 request", 358 * `Approach (N)ACK`, meaning `Concept ACK`, but "I do (not) agree with the 359 approach of this change". 360 361 A `NACK` needs to include a rationale why the change is not worthwhile. 362 NACKs without accompanying reasoning may be disregarded. 363 364 #### Code Review 365 366 After conceptual agreement on the change, code review can be provided. A review 367 begins with `ACK BRANCH_COMMIT`, where `BRANCH_COMMIT` is the top of the PR 368 branch, followed by a description of how the reviewer did the review. The 369 following language is used within pull request comments: 370 371 - "I have tested the code", involving change-specific manual testing in 372 addition to running the unit, functional, or fuzz tests, and in case it is 373 not obvious how the manual testing was done, it should be described; 374 - "I have not tested the code, but I have reviewed it and it looks 375 OK, I agree it can be merged"; 376 - A "nit" refers to a trivial, often non-blocking issue. 377 378 Project maintainers reserve the right to weigh the opinions of peer reviewers 379 using common sense judgement and may also weigh based on merit. Reviewers that 380 have demonstrated a deeper commitment and understanding of the project over time 381 or who have clear domain expertise may naturally have more weight, as one would 382 expect in all walks of life. 383 384 Where a patch set affects consensus-critical code, the bar will be much 385 higher in terms of discussion and peer review requirements, keeping in mind that 386 mistakes could be very costly to the wider community. This includes refactoring 387 of consensus-critical code. 388 389 Where a patch set proposes to change the Bitcoin consensus, it must have been 390 discussed extensively on the mailing list and IRC, be accompanied by a widely 391 discussed BIP and have a generally widely perceived technical consensus of being 392 a worthwhile change based on the judgement of the maintainers. 393 394 ### Finding Reviewers 395 396 As most reviewers are themselves developers with their own projects, the review 397 process can be quite lengthy, and some amount of patience is required. If you find 398 that you've been waiting for a pull request to be given attention for several 399 months, there may be a number of reasons for this, some of which you can do something 400 about: 401 402 - It may be because of a feature freeze due to an upcoming release. During this time, 403 only bug fixes are taken into consideration. If your pull request is a new feature, 404 it will not be prioritized until after the release. Wait for the release. 405 - It may be because the changes you are suggesting do not appeal to people. Rather than 406 nits and critique, which require effort and means they care enough to spend time on your 407 contribution, thundering silence is a good sign of widespread (mild) dislike of a given change 408 (because people don't assume *others* won't actually like the proposal). Don't take 409 that personally, though! Instead, take another critical look at what you are suggesting 410 and see if it: changes too much, is too broad, doesn't adhere to the 411 [developer notes](doc/developer-notes.md), is dangerous or insecure, is messily written, etc. 412 Identify and address any of the issues you find. Then ask e.g. on IRC if someone could give 413 their opinion on the concept itself. 414 - It may be because your code is too complex for all but a few people, and those people 415 may not have realized your pull request even exists. A great way to find people who 416 are qualified and care about the code you are touching is the 417 [Git Blame feature](https://docs.github.com/en/repositories/working-with-files/using-files/viewing-and-understanding-files). Simply 418 look up who last modified the code you are changing and see if you can find 419 them and give them a nudge. Don't be incessant about the nudging, though. 420 - Finally, if all else fails, ask on IRC or elsewhere for someone to give your pull request 421 a look. If you think you've been waiting for an unreasonably long time (say, 422 more than a month) for no particular reason (a few lines changed, etc.), 423 this is totally fine. Try to return the favor when someone else is asking 424 for feedback on their code, and the universe balances out. 425 - Remember that the best thing you can do while waiting is give review to others! 426 427 428 Backporting 429 ----------- 430 431 Security and bug fixes can be backported from `master` to release 432 branches. 433 Maintainers will do backports in batches and 434 use the proper `Needs backport (...)` labels 435 when needed (the original author does not need to worry about it). 436 437 A backport should contain the following metadata in the commit body: 438 439 ``` 440 Github-Pull: #<PR number> 441 Rebased-From: <commit hash of the original commit> 442 ``` 443 444 Have a look at [an example backport PR]( 445 https://github.com/bitcoin/bitcoin/pull/16189). 446 447 Also see the [backport.py script]( 448 https://github.com/bitcoin-core/bitcoin-maintainer-tools#backport). 449 450 Copyright 451 --------- 452 453 By contributing to this repository, you agree to license your work under the 454 MIT license unless specified otherwise in `contrib/debian/copyright` or at 455 the top of the file itself. Any work contributed where you are not the original 456 author must contain its license header with the original author(s) and source.