Proposed feature: weekly and monthly review coverage

[jonatack]

Opening an issue to follow-up on #296 (review):

It may be good here to increasingly put spotlight on the importance of review, which I've found tends to be in practice very much under-appreciated outside the small circle of developers working on the protocol who describe it as most needed and lacking. The larger world seems to value and reward making PRs, BIP proposals, mailing list emails and StackExchange answers. Which is fine, but review needs recognition and encouragement too if we want to see more of it, particularly from industry sponsors and the larger ecosystem. This is even more true for deeper review and research on the harder issues that matter and require a heavy commitment of time. It seems to me that Optech is well-positioned to provide that perspective to those who don't follow the daily interactions on GitHub.

For the reasons mentioned, and because much interesting discussion happens on GitHub:

I'd like to propose a section that regularly covers highlights of the past week's interesting review comments and discussions, similar to the coverage of mailing list discussions (and sometimes IRC discussions).

In addition, quality reviews on difficult or high-priority PRs of interest could be cited as well as a monthly ranking of the top ten reviewers of the past month. Not only might these be interesting for readers, they may have the benefit of helping people who are doing outstanding and/or frequent review and whose names don't come up often in the more visible channels of merges, mail lists, BIPs, and SE.

Finally, I believe the review club notes and discussions can at times be a useful resource for readers (cf Dave's CPFP carve-out/ancestor fee mining notes or my recent notes on P2P network attacks and on mitigating uninitialized values) and mentioned/linked to when helpful in the newsletters and the topics (or added to the topics).

This feature would need to be written (or have frequent input) by someone who spends a lot of time following reviews on the relevant GitHub repositories. Dave is already doing a great job following the merges, mailing lists, and IRC; it might be (probably is) too much additional time commitment to place on him.

Thoughts?

[bitschmidty]

@jonatack Thanks for the idea!

Do you think it would work as a monthly segment? Similar to stack exchange and client changes segments?

Also, is it something you would want to contribute to?

[jnewbery]

I'm definitely happy for the newsletter to highlight good review in Bitcoin protocol projects, but I'm not sure how exactly we could make it relevant and interesting for our readers. @jonatack - can you write an example Review Highlights section to show what you think it would look like? Perhaps we could do this in a google doc to try out a few ideas.

Do you think it would work as a monthly segment? Similar to stack exchange and client changes segments?

Monthly seems more appropriate to me than weekly. Stack Exchange + Client Changes + Review Highlights is three monthly features. Add one more and we have one special feature per week.

a monthly ranking of the top ten reviewers of the past month.

I'm a pretty strong NACK on this idea. How do you propose ranking reviewers? Number of comments? Does one "fix this spelling" review comment equal one "you've broken consensus" comment? If not, how do you objectively rank the value of a reviewer/comment? Using commit counts as a way of gamifying code contributions is a bad practice. We shouldn't try to spread that practice to review.

quality reviews on difficult or high-priority PRs of interest could be cited

This, however, is an excellent idea. We should definitely highlight and celebrate excellent review.

the review club notes and discussions can at times be a useful resource for readers [...] and mentioned/linked to when helpful in the newsletters and the topics (or added to the topics).

ACK!

[jonatack]

Yes, I agree with the above.

Also, is it something you would want to contribute to?

Thanks, Mike. I think I could do it well but am out of runway at the moment. (I was a full-time freelance SWE and tech lead for many years for large companies and startups, and work always came on its own. Bitcoin... seems very different.) So I have to focus on paid work to get back on my feet.

[harding]

I'm definitely happy for the newsletter to highlight good review in Bitcoin protocol projects, but I'm not sure how exactly we could make it relevant and interesting for our readers.

I would also worry about a section whose content would all basically be:

• Alice found a problem in PR12345 and Bob fixed it.
• Charlie found a problem in PR67890 and Danielle fixed it.
• Edmond...

Maybe it wouldn't be as bad as I expect, which is something a trial attempt could tell us. Or maybe there's a better way to glorify the process of review or praise the people doing. Two ideas:

1. Probably everyone who does review has interesting stories about it. (For example, I quickly wrote up one of my; see below.) We could work with reviewers to tell those stories and help show how reviewing can be fun and rewarding.

2. More broadly than emphasizing review, but tying into other goals for Optech that @adamjonas and others have advocated for, we could include a monthly (maybe eventually weekly) profile of a Bitcoin or LN contributor that highlights their major accomplishments and the work they usually do (such as reviewing, testing, writing documentation, providing support, or performing other work that neither we nor any other Bitcoin publication currently emphasizes). I assume we could use the section to bring extra attention to promising contributors who are looking for funding.

---

Sample review story

David Harding made an interesting discovery about the first version of the Bitcoin software while reviewing Bitcoin Core PR#FIXME, which added RPC support for BIP158 compact block filters. Starting his review, Harding retrieved the filters for the first few blocks just to see what they looked like:

$ bitcoin-cli getblockfilter $( bitcoin-cli getblockhash 1 )
{
  "filter": "018c3b10",
  "header": "c1ebf18c6a505faa315283a1fd2ee9dc277491be69265856ca1e036161d04038"
}

$ bitcoin-cli getblockfilter $( bitcoin-cli getblockhash 2 )
{
  "filter": "018c78d8",
  "header": "5afaa44e5b2791939311edd62b3299ab7b3bf66fcb98d78338db94d25cea4deb"
}

Although Harding had been involved in [previous discussions][] about BIP158 and knew that the data in a compact block filter was supposed represent the scriptPubKeys for all of the outputs spent or created in the block, he wasn't quite sure how they were formatted. He knew he could've looked it up, but he wanted to try guessing (or, perhaps, he was just too lazy to read the specification). Since all of the first 100 blocks on the chain can only have one transaction[1], Harding needed to skip ahead to a block with an additional transaction in order to get more data on the filter format. Harding just happened to recall that the first Bitcoin transaction, which Hal Finney said was a donation to him from Nakamoto, was a part of block 170:

$ bitcoin-cli getblockfilter $( bitcoin-cli getblockhash 170 )
{
  "filter": "0357e49590040c79b0",
  "header": "349eaecc8bb7793c9f3c28e78df6675ef904515e9a310e4532785aeb45526090"
}

It was immediately evident that the filter got longer, which made sense since there were more scriptPubKeys in or referenced by this block. After looking at a few other blocks, Harding also became convinced that the filter string began with a compactSize uint indicating the number of elements in the filter (e.g. 01 in block 1; 03 in block 170). But, if that was the case, he thought something was wrong with the filter for block 170 and many of the other early block filters. Consider the number of spent or created scriptPubKeys you'd expect to find in that block:

1. The output scriptPubKey for the coinbase transaction paying the miner of block 170
2. The scriptPubKey of the output Nakamoto was spending
3. The output scriptPubKey paying Finney
4. The output scriptPubKey returning change to Nakamoto

There should be four scriptPubKeys[2], but the filter was only identifying three. After some fruitless research on his own, Harding asked for help on the #bitcoin-core-dev IRC channel. Pieter Wuille (sipa) reminded Harding that duplicate scriptPubKeys are not added to the filter:

<sipa> harding: there can be overlaps between them?
<harding> sipa: indeed there can, but AFAICT there's not for block 170.
<harding> sipa: I take that back.
<harding> sipa: there is overlap, block170 tx1 vout1 sends its change back to the spending address.
<echeveria> that's unusual. I don't think bitcoin.exe supported that.
<harding> echeveria: me neither; that's why I discounted it.
<harding> sipa: thanks!

This was surprising. The first user-to-user transaction in Bitcoin reused a public key despite section 10 ("Privacy") of the previously-published [Bitcoin paper][] warning advising, "As an additional firewall, a new key pair should be used for each transaction to keep them from being linked to a common owner."

Further research by Harding found the [code][] in Bitcoin 0.1 that reused keys, and scans of the block chain show that most very early user transactions do reuse keys. Eventually, the Bitcoin wallet code was upgraded not to reuse keys. Why did Nakamoto's code reuse keys when his paper from two months earlier warned against it? We don't know.

In the end, Harding's review of the BIP158 RPC interface didn't find any problems---it just revealed a bit of old Bitcoin trivia.

[1] Exercise for the reader: explain why it's impossible for any of the first hundred blocks on the chain to have more than one transaction. Bonus exercise, explain why it's very unlikely that blocks 100 to 120 would have more than one transaction.

[2] "There... are... four... scriptPubKeys!" ---Jean-Luc Picard

[jonatack]

Nice. Reading everyone's thoughts is helping to crystallise my own. I'm realising that what I had in mind was a bit different:

A brief weekly section to cover highlights of the past week's insightful review comments & discussions. The best of the metadata in the repository... say, 1-3 items, with 1-2 sentences and a link for each.

This would be:

• easy to test initially
• relatively low in time commitment to do and sustain for anyone who is paying attention to the PRs already
• not onerous to review
• complementary to any larger monthly profiles or features described above

Of course, could trial it monthly or fortnightly in the beginning and see how relevant it is to Optech readers and how much really good material there is to share. A week may prove to be too short.

What's in it for readers? Sure, sometimes a reviewer does something different from the others and finds a surprising result, regression, or technical catch. Occasionally reviews are of brilliant depth or quality and deserve more eyes on them. But on a larger level, some review comments and discussions are insightful, some provide history/context, some pull back the curtain on rarely-expressed perspectives or processes, some are noteworthy advice. They are public, thus shareable, yet seen by few. I've collected a fair number of these nuggets for my own notes. It's about curation: There is hidden gold in the review comments and discussions (just as there are on irc, mail lists, reddit and SE); the idea is to reveal it.

In short, this section would ideally be brief, insightful, interesting, useful.

[shesek]

Further research by Harding found the [code][] in Bitcoin 0.1 that reused keys, and scans of the block chain show that most very early user transactions do reuse keys. Eventually, the Bitcoin wallet code was upgraded not to reuse keys. Why did Nakamoto's code reuse keys when his paper from two months earlier warned against it? We don't know.

I also noticed that some time ago while researching the history of address reuse. It seems like the initial release reused keys for change, but did automatically create unique per-source-IP bitcoin addresses for pay-to-IP payments and allowed creating new addresses through the gui (also mentioned by satoshi here).

So it seems to me like satoshi did mostly try to follow up on his earlier warning and gave some thought to avoiding address reuse, just didn't do it completely.

[jonatack]

can you write an example Review Highlights section to show what you think it would look like?

Maybe it wouldn't be as bad as I expect, which is something a trial attempt could tell us.

Yes, this seems best. I'm not sure if it would be as good as I expect either :)

[practicalswift]

It may be good here to increasingly put spotlight on the importance of review, which I've found tends to be in practice very much under-appreciated outside the small circle of developers working on the protocol who describe it as most needed and lacking. The larger world seems to value and reward making PRs, BIP proposals, mailing list emails and StackExchange answers. Which is fine, but review needs recognition and encouragement too if we want to see more of it, particularly from industry sponsors and the larger ecosystem. This is even more true for deeper review and research on the harder issues that matter and require a heavy commitment of time. It seems to me that Optech is well-positioned to provide that perspective to those who don't follow the daily interactions on GitHub.

I agree with the general observation that time consuming review work seems under-appreciated relative other forms of contribution (such as adding new features, etc.).

Another thing that have surprised me if I compare to other projects I've contributed to is how under-appreciated practical vulnerability research appears to be in the Bitcoin Core project relative to more theoretical vulnerability research.

As an example it seems like an externally motivated vulnerability researcher would be better off writing a paper about say a highly hypothetical eclipse attack with "state-level pre-conditions" than writing code that would demonstrate an existing pre-condition-free vulnerability that could be used as part of an actual eclipse attack :)

I worry much more about practical low-level attacks so I think the relative lack of low-level vulnerability research is a bit problematic. For internally motivated vulnerability researchers that is not a problem, but I think we should try to attract researchers who are externally motivated too. Currently those are probably better off writing papers or chasing bug bounties in forks/altcoins :)

[jnewbery]

We've started doing a monthly PR Review Club write-up. I think that addresses this request.