validation: Remove RECENT_CONSENSUS_CHANGE validation result #31269

1. 
   TheCharlatan commented at 12:04 pm on November 11, 2024: contributor

   The *_RECENT_CONSENSUS_CHANGE variants in the validation result enumerations were always unused. They seem to have been kept around speculatively for a soft fork after segwit, however they were never used for taproot either. This points at them not having a clear purpose. Based on the original pull requests’ comments their usage was never entirely clear: #11639 (comment) #15141 (review)

   Since they are part of the validation interface and need to be exposed by the kernel library keeping them around may also be confusing to future users of the library.

2. validation: Remove RECENT_CONSENSUS_CHANGE validation result

   The *_RECENT_CONSENSUS_CHANGE variants in the validation result
   enumerations were always unused. They seem to have been kept around
   speculatively for a soft fork after segwit, however they were never used
   for taproot either. This points at them not having a clear purpose.
   Based on the original pull requests' comments their usage was never
   entirely clear:
   https://github.com/bitcoin/bitcoin/pull/11639#issuecomment-370234133
   https://github.com/bitcoin/bitcoin/pull/15141#discussion_r271039747
   
   Since they are part of the validation interface and need to exposed by
   the kernel library keeping them around may also be confusing to future
   users of the library.

   e80e4c6ff9
3. 
   DrahtBot commented at 12:04 pm on November 11, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31269.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	dergoegge, sipa, ajtowns, naumenkogs
   Concept ACK	instagibbs

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #31112 (Improve parallel script validation error debug logging by sipa)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

4. DrahtBot added the label Validation on Nov 11, 2024
5. dergoegge approved
6. 
   dergoegge commented at 2:17 pm on November 11, 2024: member

   ACK e80e4c6ff91e27d7d40f099a2d7942c29085234c

   We can always re-introduce the concept and actually make use of it if needed. For now it seems better to remove it from the interface.

7. 
   ajtowns commented at 3:52 am on November 13, 2024: contributor

   To be able to differentiate RECENT_CONSENSUS_CHANGE errors, you’d need to revalidate txs up to three times instead of two (did this fail standardness? because of consensus rules? because of recent consensus rules?), which is probably more hassle (it’s marginally easier for an attacker to use up CPU cycles) than it’s worth (non-consensus compatible nodes stay well-connected for longer so are less likely to follow low-work chains). It’s probably better dealt with via things like block-only-peers (even if you relay bad txs, your block-only peers won’t disconnect you), long LOCKED_IN periods (more time to upgrade to new consensus changes), and the general policy of consensus changes only affecting txs that are already non-standard. IMHO anyway.

   Concept ACK.

8. 
   sipa commented at 1:23 pm on November 13, 2024: member

   ACK e80e4c6ff91e27d7d40f099a2d7942c29085234c
9. DrahtBot requested review from ajtowns on Nov 13, 2024
10. 
    instagibbs commented at 2:51 pm on November 13, 2024: member

    @sdaftuar since you’re the one who introduced this flag IIRC, mind weighing in?

    makes sense to me personally, concept ACK

11. 
    sdaftuar commented at 5:28 pm on November 13, 2024: member

    As far as I can tell, I was also in agreement with not needing this RECENT_CONSENSUS_CHANGE enum in our code until it was time to use it! @TheBlueMatt was the one who thought we needed it, and I came around to thinking that there was a potential use case for it in the event of another soft fork.

    But I’m fine with dropping it; if someone decides to rework our net_processing code to take advantage of subtler differences in validation failures, we can reintroduce it at that point.

12. ajtowns approved
13. 
    ajtowns commented at 2:17 am on November 14, 2024: contributor

    ACK e80e4c6ff91e27d7d40f099a2d7942c29085234c
14. DrahtBot requested review from instagibbs on Nov 14, 2024
15. 
    naumenkogs commented at 9:08 am on November 14, 2024: member

    ACK https://github.com/bitcoin/bitcoin/commit/e80e4c6ff91e27d7d40f099a2d7942c29085234c
16. fanquake merged this on Nov 14, 2024
17. fanquake closed this on Nov 14, 2024

    ---

Contributors
TheCharlatan DrahtBot dergoegge ajtowns sipa instagibbs sdaftuar naumenkogs

Review Requested
instagibbs

Labels
Validation