Restore warning for individual unknown version bits, as well as unknown version schemas #15861

1. 
   luke-jr commented at 6:01 pm on April 20, 2019: member

   Prior to #15471, we had a warning for 50% of versions being unexpected as a whole. Overt ASICBoost abuses broke that, so it was removed.

   This restores the warning, but only looks for 50% of each version bit independently, or 50% with an unknown version schema. Hopefully this shouldn’t get triggered by (or at least can be more practically avoided by) ASICBoost stuff.

   Additionally, it changes the phrasing of the warning to reflect the uncertainty of the cause, and avoid searches from turning up “not to worry” responses (that were given to the now-removed warning).

2. 
   DrahtBot commented at 8:08 pm on April 20, 2019: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #19898 (log: print unexpected version warning in validation log category by n-thumann)

   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.

3. fanquake added the label Validation on Apr 20, 2019
4. 
   gmaxwell commented at 6:16 pm on April 21, 2019: contributor

   I think I’d rather (or additionally) ignore some bits for warning… the issue with bit-limiting it is that it could still false trigger.
5. 
   luke-jr commented at 8:07 pm on April 21, 2019: member

   Only if a majority of miners violate the protocol. To set aside bits for ASICBoost is a protocol change, that hasn’t had community support demonstrated for. Developers alone don’t get to make that call.
6. 
   MarcoFalke commented at 12:49 pm on April 22, 2019: member

   Tend to NACK. Seems a lot of code for a feature that has no use case.
7. 
   gmaxwell commented at 3:09 pm on April 22, 2019: contributor

   NAK. Doesn’t actually fix the thing it intends to fix.
8. 
   luke-jr commented at 5:51 pm on April 22, 2019: member

   The use case is to provide a proper warning that the protocol rules may be changing and the user should consider upgrading (either to the new rules, or a rule-blocking fork).

   NAK. Doesn’t actually fix the thing it intends to fix.

   If there’s a bug in it, report the bug…

9. 
   MarcoFalke commented at 6:16 pm on April 22, 2019: member

   If a future softfork is activated though versionbits on a single bit in less than 100 blocks, you might as well leave away signalling altogether. For other versionbits-activated (i.e. BIP-9 compatible) softforks we already warn appropriately before your change.
10. 
    luke-jr commented at 8:32 pm on April 22, 2019: member

    BIP 9 does not require the expected 95% for the other warning, and we currently lack any warning for different schemas.
11. 
    MarcoFalke commented at 8:58 pm on April 22, 2019: member

    currently lack any warning for different schemas

    Indeed. And your code doens’t offer a warning for any kind of schema either. So I’d prefer if we just embraced that fact and avoided any softfork deployments that are incompatible with the existing BIP9 warning mechanism.

12. 
    luke-jr commented at 9:15 pm on April 22, 2019: member

    Yes, it does… And again, the other warning doesn’t work for all potential BIP 9 deployments either (they can use a lower threshold). On top of that, it’s generally accepted that BIP 9 was a failure, and it’s pretty likely future deployments will use other mechanisms.
13. 
    Seccour commented at 8:57 am on April 25, 2019: none

    Concept ACK. Miners not using the version numbers as intended should not be a reason to remove the warning.

    Only if a majority of miners violate the protocol. To set aside bits for ASICBoost is a protocol change, that hasn't had community support demonstrated for. Developers alone don't get to make that call.

    Setting aside bits for ASICBoost should have community support. I agree with Luke on that one.

14. 
    jnewbery commented at 2:49 pm on April 25, 2019: member

    Concept NACK for the reasons @MarcoFalke gives
15. luke-jr force-pushed on May 2, 2019
16. DrahtBot added the label Needs rebase on Jul 24, 2019
17. luke-jr force-pushed on Oct 25, 2019
18. DrahtBot removed the label Needs rebase on Oct 25, 2019
19. 
    luke-jr commented at 6:44 pm on October 25, 2019: member

    Rebased
20. 
    MarcoFalke commented at 6:47 pm on October 25, 2019: member

    Rebased

    Why? This has three NACKs and one Concept ACK, so at the least it seems controversial.

21. 
    luke-jr commented at 6:53 pm on October 25, 2019: member

    The NACKs are all based on the false claims, so should be ignored (or substantiated).
22. 
    luke-jr commented at 7:03 pm on October 25, 2019: member

    Per CONTRIBUTING.md:

    NACK needs to include a rationale why the change is not worthwhile. NACKs without accompanying reasoning may be disregarded.

23. 
    MarcoFalke commented at 7:03 pm on October 25, 2019: member

    We are talking about future events, so there is no clear true or false. My point is that we should build protocols that encourage safe use with little surprises. For example, using a threshold that is not recommended in BIP 9 is surprising in my eyes and might even lead to consensus deployment issues. Any BIP or code change that explicitly makes use of that or implicitly encourages it will be NACKed by me for exactly that reasoning.
24. 
    luke-jr commented at 9:13 pm on October 25, 2019: member

    You said it “has no use case”, which it clearly does. You also claimed it “doesn’t offer a warning for any kind of schema either”, which is also false, as it has a check for different version schemas. @gmaxwell said it doesn’t fix the issue intended (lack of a warning), which again, it clearly does.

    Neither of these deal with future events, just present realities (in which the claims are false).

25. DrahtBot added the label Needs rebase on May 13, 2020
26. luke-jr force-pushed on May 17, 2020
27. DrahtBot removed the label Needs rebase on May 17, 2020
28. DrahtBot added the label Needs rebase on Jun 10, 2020
29. 
    rebroad commented at 5:11 am on August 20, 2020: contributor

    @luke-jr If it “clearly” has a use case then why would someone be suggesting it does not? It seems it is clearly not clear to them. Also, “a check for different version schemas” does not seem equal to “a warning for any kind of version schema”, so I am not sure you have rebutted the basis for the NACK here yet.

    Seems so close to understanding being reached here…. just a little more effort from both sides..?

30. luke-jr force-pushed on Aug 20, 2020
31. 
    luke-jr commented at 6:19 pm on August 20, 2020: member

    Rebased
32. DrahtBot removed the label Needs rebase on Aug 20, 2020
33. DrahtBot added the label Needs rebase on Sep 29, 2020
34. 
    DrahtBot commented at 2:16 pm on September 29, 2020: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

35. luke-jr force-pushed on Dec 3, 2020
36. Restore warning for individual unknown version bits, as well as unknown version schemas f016cd420d
37. luke-jr force-pushed on Dec 15, 2020
38. 
    DrahtBot commented at 11:22 am on December 15, 2021: member

    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
39. 
    MarcoFalke commented at 7:22 pm on December 15, 2021: member

    Closing for now due to inactivity for more than 12 months. It can be reopened any time, just let us know with a comment either here or on IRC.
40. MarcoFalke closed this on Dec 15, 2021

    ---

41. DrahtBot locked this on Dec 15, 2022

Contributors
luke-jr DrahtBot gmaxwell MarcoFalke Seccour jnewbery rebroad

Labels
Validation Needs rebase