doc: Declare BLOCK_VALID_HEADER reserved #14862

1. 
   MarcoFalke commented at 9:59 PM on December 3, 2018: member

   BLOCK_VALID_HEADER was never used and the comment is confusing to me in several ways:

   • It claims "version ok". However, without the previous header, it is not possible to check the validity of the version since the height needs to be known (c.f. BIP 90)
   • It claims "hash satisfies claimed PoW". While it is possible to check against the claimed PoW, it is not possible without the previous header to check that the claimed PoW is itself valid.
   • It claims "1 <= vtx count <= max". However, with the header alone and current consensus rules, the number of transactions is unknown.
2. MarcoFalke added the label Docs on Dec 3, 2018
3. 
   meshcollider commented at 10:52 PM on December 3, 2018: contributor

   Related #9440, @sipa introduced it in #1677

4. 
   MarcoFalke commented at 11:05 PM on December 3, 2018: member

   I could imagine the status "flag" to be useful for some edge-case scenarios in the future, but it can easily be renamed back when needed. (And amended with documentation that matches the future implementation)

5. ryanofsky approved
6. 
   ryanofsky commented at 6:24 PM on December 10, 2018: member

   utACK fae3d8907b6cdb4844689a5b47a2c7a99112b23b since code change seems obviously fine. And if other reviewers think this is good, I'm fine with it too.

   But I do not think this change is an improvement in its current form. Calling a flag "unused" and then going and using it in a bitmask seems misleading, and stretches the definition of "unused". Maybe say something like "reserved" instead or clarify exactly what you mean by "unused". Or if it's possible to really avoid using the flag at all and maybe deleting it, that would be seem better.

7. MarcoFalke force-pushed on Dec 10, 2018
8. 
   MarcoFalke commented at 6:43 PM on December 10, 2018: member

   I think keeping a placeholder makes it easier to make use of the enum value in the future and easier to understand why the first used enum value is 2 right now. So I used your suggestion of "reserved".

9. in src/chain.h:129 in fab72b99fd outdated

    125 | @@ -126,8 +126,8 @@ enum BlockStatus: uint32_t {
    126 |      //! Unused.
    127 |      BLOCK_VALID_UNKNOWN      =    0,
    128 |  
    129 | -    //! Parsed, version ok, hash satisfies claimed PoW, 1 <= vtx count <= max, timestamp not in future
    130 | -    BLOCK_VALID_HEADER       =    1,
    131 | +    //! Unused (was BLOCK_VALID_HEADER).
   

   ---

   ---

   

   ryanofsky commented at 7:39 PM on December 10, 2018:

   Can you say "reserved" instead of "unused" here? Also in the commit description and the PR description? Sorry for being so literal, but if I see a comment telling me that a constant is unused, I really expect it to be unused in the sense that changing it to a different value would have no effect.

   ---

   

   MarcoFalke commented at 11:59 AM on August 5, 2019:

   Done

10. fanquake requested review from sipa on Mar 2, 2019
11. fanquake renamed this:
    [doc] chain: Declare BLOCK_VALID_HEADER unused
    doc: Declare BLOCK_VALID_HEADER unused
    on Aug 4, 2019
12. 
    fanquake commented at 6:26 AM on August 4, 2019: member

    Concept ACK @MarcoFalke Did you want to follow up and address @ryanofsky's comment?

13. MarcoFalke renamed this:
    doc: Declare BLOCK_VALID_HEADER unused
    doc: Declare BLOCK_VALID_HEADER reserved
    on Aug 5, 2019
14. [doc] chain: Declare BLOCK_VALID_HEADER reserved fa0b910486
15. MarcoFalke force-pushed on Aug 5, 2019
16. fanquake requested review from ryanofsky on Aug 6, 2019
17. ryanofsky approved
18. 
    ryanofsky commented at 4:30 PM on August 9, 2019: member

    ACK fa0b910486e9aada077fe47e1201dcb3bd523c87

19. 
    egp commented at 1:49 PM on August 28, 2019: none

    LGTM

20. 
    sipa commented at 7:08 PM on August 29, 2019: member

    ACK fa0b910486e9aada077fe47e1201dcb3bd523c87

    The BLOCK_VALID_* values predate headers-first sync, and I probably had the possibility of more granular validation in mind at the time.

21. fanquake referenced this in commit 74da99e010 on Aug 30, 2019
22. fanquake merged this on Aug 30, 2019
23. fanquake closed this on Aug 30, 2019

    ---

24. sidhujag referenced this in commit e28e7850f7 on Aug 30, 2019
25. MarcoFalke deleted the branch on Sep 3, 2019
26. jasonbcox referenced this in commit eba575e95d on Oct 15, 2020
27. PastaPastaPasta referenced this in commit 298e267b5f on Jun 27, 2021
28. PastaPastaPasta referenced this in commit 09ff25cad6 on Jun 28, 2021
29. PastaPastaPasta referenced this in commit ad5a03a78f on Jun 29, 2021
30. PastaPastaPasta referenced this in commit 089cc64031 on Jul 1, 2021
31. PastaPastaPasta referenced this in commit 68db3c1127 on Jul 1, 2021
32. PastaPastaPasta referenced this in commit bac0b0900d on Jul 12, 2021
33. PastaPastaPasta referenced this in commit 04d768d09a on Jul 13, 2021
34. MarcoFalke locked this on Dec 16, 2021

Contributors

MarcoFalkemeshcolliderryanofskyfanquakeegpsipa

Review Requested
sipa