RFC: Introduce "Risk NACK" Review Feedback #35643

Motivation

Bitcoin Core reviewers currently have a well-understood vocabulary for expressing opposition to a pull request.

At a high level:

• Concept NACK indicates opposition to the underlying idea or goal of a PR.
• Approach NACK indicates agreement with the goal but opposition to the proposed implementation or design.

In practice, both Concept NACKs and Approach NACKs are generally treated as strong negative review feedback. While they are not formal vetoes, maintainers typically expect substantive concerns raised in a NACK to be addressed before a PR is merged.

However, there is currently no established way to express a different kind of feedback:

"I have not identified a specific bug, design flaw, or requested change. However, based on my current understanding of the PR and the affected code, I believe the risks outweigh the benefits and would not currently prioritize additional review effort on it. If other reviewers disagree regarding the benefits and choose to continue pursuing the change, I believe the risks involved should require a higher review bar before merging."

Today, reviewers holding this type of concern are faced with three unsatisfactory options:

1. Leave a NACK, which may be interpreted as a blocking objection requiring a direct response.
2. Leave a general comment expressing concern, which may not clearly communicate that they believe the PR, if pursued, should face a higher merge threshold.
3. Provide no review feedback at all, because their concern is about risk, uncertainty, or review effort rather than a concrete objection to the concept or approach. This results in a potentially useful review signal being lost.

This proposal introduces a dedicated term for this type of feedback.

Proposal

Introduce the term Risk NACK.

A Risk NACK is intended for situations where a reviewer has concerns about the overall risk/review-effort tradeoff of a change, but is not necessarily objecting to the proposed end state itself.

Scope and Examples

Examples where a Risk NACK might be appropriate:

• A large refactor with modest expected benefits.
• A change touching security-sensitive code where the benefits do not appear to justify the review effort required to gain confidence in it.
• A proposal involving substantial code churn for relatively small expected gains.
• A change where the reviewer is unconvinced that the expected benefits justify spending additional review resources, but acknowledges that other reviewers may reasonably disagree.

Examples where a Concept NACK or Approach NACK would be more appropriate:

• The reviewer believes the feature itself should not exist.
• The reviewer believes the resulting design is unnecessarily complex.
• The reviewer believes the resulting code will create an unreasonable maintenance burden.
• The reviewer identifies a security, correctness, or reliability concern.
• The reviewer believes the implementation approach is flawed.

A useful rule of thumb is:

If a genie could make the change safely, correctly, and with perfect review confidence, and the reviewer would still object to the result, a Concept NACK or Approach NACK is generally more appropriate. If the reviewer would accept the result in that genie-assisted world but objects to the real-world risk/review tradeoff, a Risk NACK may be appropriate.

Interpretation and Caveats

A Risk NACK would contribute negative review signal without constituting a formal veto or blocking objection. Unlike a Concept NACK or Approach NACK, a Risk NACK would not imply that the reviewer has identified a specific problem that must be addressed before merge. Rather, it communicates concern about the overall risk/benefit tradeoff of a change, and may indicate that the reviewer believes the PR should receive stronger justification, broader support, or additional review before being merged.

To be useful, a Risk NACK should be based on enough familiarity with the PR or affected area to explain the concern, and should include a rationale describing the perceived risks, the expected benefits, and why the reviewer believes the tradeoff is unfavorable. The point is not to make negative feedback easier to give without justification, but to make a particular kind of negative feedback easier to express accurately.

The exact effect of a Risk NACK would remain a matter of judgment rather than a formal rule. The primary goal of this proposal is to provide clearer vocabulary for communicating concerns.

A note on context: the proposal above is mine, though I iterated on the wording quite a bit before posting it.

Some examples that got me thinking about this type of feedback are #35569 and #32532. In those discussions, I found it useful to distinguish concerns about the risks involved in making a change from concerns about the desirability of the resulting end state.

To be clear, I am not claiming that any reviews in those PRs are incorrect or mischaracterized, or that a "Risk NACK" would change any outcomes there. I mention them only because they helped me think about the distinction this proposal is trying to capture.

I also do not expect the primary benefit of this proposal to be in actively discussed PRs such as those. I think the more likely benefit would be in cases where reviewers have concerns about a PR's overall risk/benefit tradeoff, but are not primarily objecting to the concept or approach itself. In those situations, a Risk NACK could provide a clearer way to communicate that concern and surface review signal that might otherwise remain unexpressed.

Risk NACK. This further codifies responses for code review in a way that might compromise creative and free review by encouraging a limited vocabulary or language. To some extent, such encouragement has already happened without this proposal, so it cannot be solely attributed to this. If the idea is to help with counting responses from reviewers that have been posted, I think that is OK for an at a glance check but folks really should just read the comments and figure it out for themselves.

Depending on what the underlying goals are here, your goal might also be achievable by making a document that shows examples of excellent review (of different kinds and objections), including concise but also verbose reviews spanning security, general code change risk, fork risk, backwards/forwards compatibility, etc...

I think there shouldn't be too much bureaucracy and strict rules in reviews and we should encourage creativity and out-of-box thinking.

A Concept NACK already encompasses different types of conceptual disagreement (including a Risk NACK). Reviewers are already required to provide a rationale for NACKs, so they should feel free (and encouraged) to type Risk NACK or Risk Concept NACK with their rationale. At least DrahtBot already supports that and will correctly map Risk NACK or Risk Concept NACK to a conceptual disagreement in the table. See https://github.com/maflcko/DrahtBot/commit/a438157b2b81b86d791c15bb6cbbd24af9deead5

Examples where a Risk NACK might be appropriate: ...

I think an Approach NACK conceptually already covers all these examples, i.e. the concept/idea may be useful, but the way to get there is no good. As stated by earlier reviewers, useful review already provides additional context on a (N)ACK, so this can easily be expressed. So I'm Concept NACK on formalizing this idea, I think people should just use Approach NACK.

re: kanzure #35643 (comment)

This further codifies responses for code review in a way that might compromise creative and free review by encouraging a limited vocabulary or language.

I know this is not the point you are making, but this actually describes a reason I think it would be good to use the "Risk NACK" label.

Right now if a reviewer thinks a change is too risky given its benefits, they are constrained by our limited "Concept NACK" "Approach NACK" vocabulary to choose one of these labels if they want to express clear opposition to the PR. "Risk NACK" would expand the vocabulary a little so opposition can be expressed more clearly if is mainly based on the risk of the PR, not the concept or the approach.

If the idea is to help with counting responses from reviewers that have been posted, I think that is OK for an at a glance check but folks really should just read the comments and figure it out for themselves.

That could be a benefit but it's not the intended benefit. Main intended benefit is just providing clarity. Practically, I also think this could be a less harsh way of pushing back on PRs that are risky and not worth review costs.

re: maflcko #35643 (comment)

Glad to know DrahtBot already supports this!

This post is not setting up any bureaucracy. It is saying, reviewers, if you feel that a PR is risky and not do not feel concept NACKing accurately describes your feelings about a PR because:

• You think the concept is perfectly ok
• You don't want to imply to the PR author their idea is bad
• You want to make a more objective claim about risks of the PR, instead of a more subjective claim about the benefits of the PR.
• You want to send a clear signal to maintainers (not just a comment that will get lost in a long thread) that when they are weighing risks of the PR vs review, that your assessment is risks are high.

In any of these cases, you can label your review with "Risk NACK". This way the review can just describe where you see the risks without needing an extra paragraph to disambiguate your intent or express an opinion on the concept or idea behind the PR if risk is your primacy concern.

re: stickies-v #35643 (comment)

I think when people see "Approach NACK" they think there is some problem with the implementation or design of whatever feature/bugfix/refactoring is being implemented. Obviously, if your approach NACK is substantive, you can describe how this isn't the case, and actually, you are less concerned about the implementation in the PR and more concerned about the implementation of the PR. Or you can use "Risk NACK" to characterize the narrower concern from the beginning.

(Also, I think all reviewers should use terms they like, and I'm not sure what it could mean to concept NACK this RFC, but I plan to close the github issue after a few days whenever there aren't more comments.)

Glad to know DrahtBot already supports this!

This post is not setting up any bureaucracy. It is saying, reviewers, [... can ...] review with "Risk NACK".

Ok, I see. My point is that this is already possible today (no one is holding anyone back from typing Risk NACK). Seems fine to use it more often, but I think this issue can be closed, or are you asking to add it to contrib .md?

Also, I think all reviewers should use terms they like, and I'm not sure what it could mean to concept NACK this RFC

Absolutely, I meant to say "Concept NACK on formalizing this idea" (have now edited my post), because I think it is insufficiently distinct to warrant a separate category. I absolutely agree that not doing something because it is too risky is a very valid concern and people should be encouraged to voice it.

Also, I think all reviewers should use terms they like

Yes, very much so. I've used "Priority NACK" a few times, and I think it's good that people experiment with language and process.

I think when people see "Approach NACK" they think there is some problem with the implementation or design of whatever feature/bugfix/refactoring is being implemented.

A concept can be too risky, and an approach can be too risky. People should call out whichever it is. If a concept is good, but the code changes required are too risky, then that is an approach/implementation issue. That is not necessarily the author's fault, e.g. maybe it touches too much ~orthogonal code that is currently not tested well enough (but may change in the future), maybe the code changes are much less invasive with a new c++ version, ... Approach NACK doesn't require a better approach to exist.

I think "priority" and "risk" are somewhat orthogonal -- you can reduce risk of a change by doing additional work (eg 35569 introduces a bitcoin-tidy check to make the copious mechanical changes actually be mechanical), but for things that aren't a priority, adding more work, and potentially more review burden, that doesn't seem like a good approach. Extensively debating topics that aren't a priority also doesn't seem like a good approach though.

Another example is #33759 (comment) (vs #33755 or #33882). I find "Concept/Approach NACK because [there is this risk/this isn't worth the review time/...]" sensible, and I think "risk" can sometimes result more from the concept and other times the approach, so the distinction still seems useful.

(By "Approach NACK" I generally mean "this PR needs big changes before it makes sense and there's a good chance that would be better done in a new PR, but I think those changes would be possible" and by "Concept NACK" I generally mean "I don't think there's any way of achieving this change or something like it that would make sense")

One thing to perhaps consider is the ability to request a "concept decision" -- for 33759, things worked fine and discussion moved to other issues/PRs without much hassle; for #35569 discussion is more hostile, and continuing to argue there seems like a waste of everyone's energy and time: it may be better to have a short poll amongst contributors with a result of either "concept is approved, nacks are overriden" or "PR needs to pass these objective tests" or "PR is closed, contributor encouraged to maintain their branch demonstrating the wins/reducing the costs, and perhaps try again either when some of the nackers are convinced, or in 6-12 months". If the answer is "concept is approved" that gives the opportunity to move on to code review without continuing to go round in circles debating the topic; if the PR is closed, that at least moves the debate out of the repo to twitter and podcasts. Requires making decisions, though.