What should I include in the description of a Bitcoin Core pull request?

Question

What should I include in the description of a pull request (PR) that I open to the Bitcoin Core repo to help reviewers quickly get up to speed and efficiently review and test the PR?

Answer 1

There is currently some (limited) guidance in CONTRIBUTING.md.

The title of the pull request should be prefixed by the component or area that the pull request affects

e.g. consensus: Add new opcode for BIP-XXXX OP_CHECKAWESOMESIG

The body of the pull request should contain sufficient description of what the patch does, and even more importantly, why, with justification and reasoning. You should include references to any discussions (for example, other issues or mailing list discussions).

It is difficult to explain exactly what should be in the PR description because it will depend very much on what kind of pull request it is. (The description for a new pull request should not contain any @ mentions though.) A small documentation PR will just need a sentence or two explaining the change(s) and the rationale for making the change(s). At the other extreme a consensus change will need a long, detailed description explaining the historical context, linking to prior discussions (e.g. BIPs, mailing list) and convincing reviewers that there is community consensus for this change.

However, the PR author should seek to make the PR as easily reviewable for reviewers as possible. Ideally reviewers should be able to understand what the PR is trying to do and why from the PR description and not have to explore chains of PRs to get the required context. Also pointers on what the reviewer should look out for (especially if there are specific security considerations) or guidance on how the reviewer could test the PR may also prove useful. If it is easily reviewable it is more likely to be reviewed.

fanquake supplied some additional guidance in this PR comment:

The PR description (and commit message) could include: What is being changed and why is it equivalent to previous checks? When did this change? e.g. Qt 5.12 isn't the only version of Qt we build against, and our minimum supported is still 5.9.x. If the version of Qt being built against is irrelevant to this change, why is that the case? Link to relevant upstream changes if applicable.

How did you verify / how should reviewers verify that this is a no-op? e.g. "Given that this is a change to a configure check, the binaries produced before and after this change should be identical. You could show that to be the case, maybe using build-for-compare.py."

Adding more information not only means that more contributors can, or will be more likely to review, but any security related change should be better summarized than "some changes happened in a dependency header".

The PR author should also consider making the PR easily testable for new/less experienced reviewers especially if it isn't a complex PR that requires a vast amount of contextual understanding.

A good example of a PR description for less experienced reviewers is this one from Jon Atack. It lays out exactly what a reviewer needs to do to verify that the functionality works as described including the exact commands a reviewer can run.

Testing and review welcome! How to test:

• to run the full live dashboard (on Linux): $ `watch --interval 1
• to run the full dashboard: $ ./src/bitcoin-cli -netinfo 4
• to see the help: $ ./src/bitcoin-cli -netinfo help
• to see the help summary: $ ./src/bitcoin-cli -help | grep -A4 netinfo

There is additional guidance from Jon Atack on contributing pull requests to Bitcoin Core here.