log: Fix UB with bench on genesis block #15283

1. 
   instagibbs commented at 1:06 pm on January 29, 2019: member

   During the loading of the genesis block, the bench print lines in ConnectTip divide by zero due to early return in ConnectBlock.
2. Fix UB with bench on genesis block ec30a79f1c
3. fanquake added the label Validation on Jan 29, 2019
4. in src/validation.cpp:2448 in ec30a79f1c

   2444@@ -2445,6 +2445,7 @@ bool CChainState::ConnectTip(CValidationState& state, const CChainParams& chainp
   2445             return error("%s: ConnectBlock %s failed, %s", __func__, pindexNew->GetBlockHash().ToString(), FormatStateMessage(state));
   2446         }
   2447         nTime3 = GetTimeMicros(); nTimeConnectTotal += nTime3 - nTime2;
   2448+        assert(nBlocksTotal > 0);
   

   ---

   ---

   

   promag commented at 2:56 pm on January 29, 2019:

   I’d avoid this, I think it is noise as there is already a “divide by nBlocksTotal” which implies this assertion.

   ---

   

   instagibbs commented at 3:34 pm on January 29, 2019:

   Not a fan of silently committing UB.

   ---

   

   promag commented at 4:26 pm on January 29, 2019:

   There are 11 occurrences of / nBlocksTotal.
5. 
   promag commented at 2:56 pm on January 29, 2019: member

   Funny that DisconnectTip doesn’t decrement nBlocksTotal 👀

   ACK ec30a79, nBlocksTotal is only used in logging. @fanquake I don’t think this should have validation label.

6. 
   DrahtBot commented at 3:45 pm on January 29, 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:

   • #17037 (Testschains: Many regtests with different genesis and default datadir by jtimon)

   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.

7. MarcoFalke renamed this:
   Fix UB with bench on genesis block
   log: Fix UB with bench on genesis block
   on Jan 29, 2019
8. MarcoFalke added the label Utils/log/libs on Jan 29, 2019
9. 
   MarcoFalke commented at 5:19 pm on January 29, 2019: member

   ACK
10. 
    practicalswift commented at 10:34 am on January 30, 2019: contributor

    This is already fixed in #14239 which was submitted back in September and is scheduled for 0.18.0 :-) @promag @MarcoFalke @instagibbs Would you mind reviewing #14239?
11. 
    instagibbs commented at 10:38 am on January 30, 2019: member

    @practicalswift sure
12. MarcoFalke added this to the milestone 0.18.0 on Jan 30, 2019
13. 
    practicalswift commented at 8:23 am on January 31, 2019: contributor

    Close in favour of #14239?
14. 
    laanwj commented at 12:05 pm on January 31, 2019: member

    Yes this exact change is included in #14239. Unless the rest of that PR is controversial and this part really needs to be factored out, it’s probably better to focus on getting that one merged.
15. 
    instagibbs commented at 12:22 pm on January 31, 2019: member

    SGTM. Closing.
16. instagibbs closed this on Jan 31, 2019

    ---

17. 
    practicalswift commented at 12:39 pm on January 31, 2019: contributor

    @laanwj PR #14239 is highly uncontroversial so I don’t think that should be a problem :-)
18. 
    instagibbs commented at 7:24 pm on February 20, 2020: member

    @practicalswift you ended up closing that PR ;P d’oh
19. 
    practicalswift commented at 7:51 pm on February 20, 2020: contributor

    @instagibbs Please re-open this PR :)
20. 
    practicalswift commented at 7:51 pm on February 20, 2020: contributor

    ACK ec30a79f1c430cc7fbda37e5d747b0b31b262fa5
21. instagibbs reopened this on Feb 20, 2020

    ---

22. 
    jonatack commented at 8:00 pm on February 20, 2020: member

    Concept ACK
23. 
    jonatack commented at 3:07 pm on February 21, 2020: member

    Ok, this PR doesn’t resolve all the UBSan issues that #17208 does, only the one involving nBlocksTotal.

    As has been noted several times previously, nBlocksTotal appears to be used solely for bench logging purposes and it seems to me the approach in #17208 of initializing it to 1 instead of 0 is more straightforward to review and robust against regression; this comment by @gmaxwell concurs with that.

24. 
    practicalswift commented at 3:55 pm on February 21, 2020: contributor

    @jonatack Feel free to attack the remaining ones – I’d be glad to review :)
25. 
    sipa commented at 5:35 pm on March 17, 2020: member

    utACK ec30a79f1c430cc7fbda37e5d747b0b31b262fa5
26. MarcoFalke merged this on Mar 17, 2020
27. MarcoFalke closed this on Mar 17, 2020

    ---

28. 
    practicalswift commented at 6:41 pm on March 17, 2020: contributor

    Nice to see this one merged!

    Anyone (perhaps fellow UBSan-fan @jonatack?) who would like to volunteer to get rid of the remaining UBSan suppressions? If so #17208 can be used as a starting point :)

29. sidhujag referenced this in commit ceb70cb60a on Mar 18, 2020
30. MarkLTZ referenced this in commit 8823f65197 on Apr 7, 2020
31. MarkLTZ referenced this in commit 0a1571d657 on Apr 7, 2020
32. sidhujag referenced this in commit 6fe4268440 on Nov 10, 2020
33. MarcoFalke referenced this in commit 99fcc2b477 on Nov 13, 2020
34. sidhujag referenced this in commit 6964401ae7 on Nov 13, 2020
35. deadalnix referenced this in commit ad59559de1 on Jan 9, 2021
36. ftrader referenced this in commit 52c26df386 on Apr 14, 2021
37. DrahtBot locked this on Feb 15, 2022

Contributors
instagibbs promag DrahtBot MarcoFalke practicalswift laanwj jonatack sipa

Labels
Validation Utils/log/libs

Milestone
0.18.0