PR Review Checklist

EN 3 min read

Compiling a Playbook for Quality PR Reviews

bitcoin open source review pr
Table of Contents

Mindset

  • “If all a developer did was review hard but important PRs it’d be incredibly valuable”
  • “I want this to get merged asap! Just as much as the author!”
  • “How can I best serve?”
  • “review 5-15 PRs for each PR you open”
  • Quality of review is much more important than quantity
  • very few consistent reviewers present most days.
  • “simple nudge can get the ball rolling”

PR selection

  • Prefer a less reviewed PR (more repo value and less competition)
  • Check if the PR really is useful at this point in time
  • Do I already have context? - Ideally at the edge of your current reach
  • Deep, quality review of priority or harder PRs over new, easy or trivial ones
  • What areas we contribute to, which project priorities matter to us
  • Reward people who do consistent review/issue testing
  • Avoid PR unfinished, insufficient research, don’t build cleanly, tests don’t run
  • What kind of change is this: consensus / policy / wallet / relay / RPC / GUI / build / refactor / tests / docs?
  • Is this standalone or part of a series?
  • What are the prerequisites / dependencies / non-goals?

Initiating a review

  • What is most needed here at this time?
  • Create a note with a list of unresolved questions
  • Define the scope of your review
  • Scope the timeblock with potential extensions

Core loop

  • Load branch locally
  • Read the commits
  • Read the PR comments
  • Perform analysis:
    • Context Driven Analysis:
      • Copy the PR paste it on claude
      • Read the surrounding code
      • Find nits fix friendly suggestions
      • Check if any call sites, headers or declarations have been overlooked in the PR
      • Try refactoring the code to be better or prettier
      • “How change X could break Y”
      • Review naming, comments, docs, and explainability
    • Execution Driven Analysis
      • Find one concrete entry point into the codepath
      • Run with regtest
      • Run with testnet
      • Run a debugger with breakpoints: C++ gdb or Python pdb
    • Logs Driven Analysis
      • Add some custom logging
      • Check logs bitcoin-cli help logging
      • Contribute benchmarks, memory profiling/valgrind or flame graphs`
    • Test Driven Analysis
      • Run the tests
      • Verify that tests fail in the expected way in master
      • Break the test
      • Ask LLM to generate a script to run this change
      • Improve or write any missing unit, functional or fuzz test
  • Find something that doesn’t make sense and try to figure it out
  • Repeat

Networking

  • Ask questions - thing that seems most confusing or surprising
  • Proposing to rebase or a commit
  • Reach out to new and different people (direct IRC messages work well)
  • Take over the PR after months of silence
  • Consider testing release candidates

Outputs

  • Be verbose about what you did during review
  • Status: ACK HEAD, Code review ACK, tACK, utACK, NACK, Concept ACK, Approach ACK
  • “Here’s what I tested and my methodology”, particularly to back up an ACK
  • Uncertain is still valuable: looks correct, don’t feel confident enough to ACK
  • ACKing: “ACK fa2f991, I built, ran tests, tested manually by doing X/Y/Z and reviewed the code and it looks OK, I agree it can be merged.”
  • Collapsible comments
  • Say exactly what I did and did not verify
  • Explain what is a blocker
  • Note follow-up ideas

REFERENCES

More Posts