Together we review series

[stratospher]

Reviewing PRs is the best way to contribute to Bitcoin Core. We pick a few easy PRs and review them together on the call.
The aim of this session is only to show participants that reviewing PRs isn't intimidating.

Next Sessions

Session 5 on 22 Feb 2024. At Bitshala Disord.

We'll be reviewing PR 27114 which deals with whitelisting manually added peer connections.

Previous Sessions

session	date	PR
Session #1	21 Dec 2023	refactor: share and use GenerateRandomKey helper #28455
Session #2	4 Jan 2024	test: Use test framework utils in functional tests #28528
Session #3	25 Jan 2024	test: p2p: check disconnect due to lack of desirable service flags #29279
Session #4	8 Feb 2024	rpc: parse legacy pubkeys consistently with specific error messages #28336

PRs to review

How many PRs do you think we can cover in our 1 hour call? :) Feel free to add suggestions.

1. https://github.com/bitcoin/bitcoin/pull/29122

Reference

This is an excellent resource capturing the essence of reviewing - https://jonatack.github.io/articles/how-to-review-pull-requests-in-bitcoin-core!

[stratospher]

Session #1: 21 Dec 2023

Reviewed refactor: share and use GenerateRandomKey helper #28455 during the call.

• covered basics of how to review PRs
• thoughts involved when reviewing refactors
• why this refactor is useful

[stratospher]

Session #2: 4 Jan 2024

Reviewed test: Use test framework utils in functional tests #28528 during the call.

Left review comments:

1. test: Use test framework utils in functional tests bitcoin/bitcoin#28528 (comment)
2. test: Use test framework utils in functional tests bitcoin/bitcoin#28528 (comment)

• covered basics of how to review PRs
• gathering past context to judge usefulness of change

[stratospher]

Session #3: 25 Jan 2024

Reviewed test: p2p: check disconnect due to lack of desirable service flags #29279 during the call.

Left review comments:

1. test: p2p: check disconnect due to lack of desirable service flags bitcoin/bitcoin#29279 (review)

Overview:

• discussed version handshake - see question 5 in [Part 1/2: Introduction] Functional tests for v2 P2P encryption #24748 #47
• went through different connection types - Outbound vs Inbound connections, Different types of outbound connections like feeler connections, addrfetch connections, manual connections, outbound full relay connections. See link for more details.
• examined version message contents - https://en.bitcoin.it/wiki/Protocol_documentation#version
• basics of service flags
  

[stratospher]

Session #4: 8 Feb 2024

Reviewed rpc: parse legacy pubkeys consistently with specific error messages #28336 during the call.

Left review comments:

1. rpc: parse legacy pubkeys consistently with specific error messages bitcoin/bitcoin#28336 (review)

Overview:

• what are public keys
• types of public keys - compressed, uncompressed and hybrid
• examined HexToPubKey function
• discussed subtrees - https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees