validation: Document where we are intentionally ignoring bool return values from validation related functions #13864

1. 
   practicalswift commented at 2:25 pm on August 3, 2018: contributor

   • Document where we are intentionally ignoring the return value from bool returning functions from validation.{cpp,h} that modify state
   • Mark validation functions whose bool return value should not be discarded in the general case with NODISCARD (expands to [[nodiscard]] or __attribute__((warn_unused_result)) depending on availability)

   This will help us identify code where we’re we incorrectly assume that a certain function always succeeds in performing its state modification.

2. 
   Empact commented at 3:50 pm on August 3, 2018: member

   Should we not update the callers to react and log warnings on failures?

   E.g. this seems like a meaningful case:

   0if (!file) {
   1    LogPrintf("Warning: Could not open blocks file %s\n", path.string());
   2} else {
   3    LogPrintf("Importing blocks file %s...\n", path.string());
   4    if (!LoadExternalBlockFile(chainparams, file)) {
   5        LogPrintf("Warning: No blocks loaded from %s\n", path.string());
   6    }
   7}
   

3. 
   DrahtBot commented at 4:38 pm on August 3, 2018: 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:

   • #14624 (Some simple improvements to the RNG code by sipa)
   • #14605 (Return of the Banman by dongcarl)
   • #14224 (Document intentional and unintentional unsigned integer overflows (wraparounds) using annotations by practicalswift)
   • #14194 (Annotate unused parameters with [[maybe_unused]] by practicalswift)
   • #14193 (validation: Add missing mempool locks by MarcoFalke)
   • #13937 (Track best-possible-headers (TheBlueMatt) by Sjors)
   • #13868 (Remove unused fScriptChecks parameter from CheckInputs by Empact)
   • #13815 (util: Add [[nodiscard]] to all {Decode,Parse}… functions returning bool by practicalswift)
   • #13804 (WIP: Transaction Pool Layer by MarcoFalke)

   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. laanwj added the label Docs on Aug 7, 2018
5. laanwj added the label Refactoring on Aug 7, 2018
6. DrahtBot added the label Needs rebase on Aug 25, 2018
7. practicalswift force-pushed on Aug 25, 2018
8. 
   practicalswift commented at 11:05 pm on August 25, 2018: contributor

   @Empact Increased logging should perhaps be a subject for another PR. Personally I’m not convinced that these cases are worth logging for – nobody seems to have been missing these log messages so far and we should be careful to decrease the signal to noise in our logging. It is already very verbose :-)
9. 
   practicalswift commented at 11:05 pm on August 25, 2018: contributor

   Rebased! Please review :-)
10. practicalswift force-pushed on Aug 25, 2018
11. DrahtBot removed the label Needs rebase on Aug 26, 2018
12. practicalswift force-pushed on Aug 27, 2018
13. 
    practicalswift commented at 5:14 pm on August 27, 2018: contributor

    Updated NODISCARD to work also with Visual C++ :-)
14. DrahtBot added the label Needs rebase on Sep 20, 2018
15. practicalswift force-pushed on Sep 21, 2018
16. 
    practicalswift commented at 9:13 am on September 21, 2018: contributor

    Rebased! :-)
17. DrahtBot removed the label Needs rebase on Sep 21, 2018
18. practicalswift force-pushed on Sep 21, 2018
19. DrahtBot added the label Needs rebase on Oct 24, 2018
20. practicalswift force-pushed on Oct 24, 2018
21. 
    practicalswift commented at 8:44 am on October 24, 2018: contributor

    Rebased! :-)
22. DrahtBot removed the label Needs rebase on Oct 24, 2018
23. DrahtBot added the label Needs rebase on Oct 27, 2018
24. Remove unused return value from LoadExternalBlockFile(...) d47f0b1faf
25. Remove unused return value from LoadMempool(...) 4e8b3a19dc
26. Document where we are intentionally ignoring the return value from bool returning functions from validation.{cpp,h} that modify state da395b9500
27. Add #define NODISCARD b272348d80
28. Mark functions whose bool return value should not be discarded with NODISCARD 71e5ce54a9
29. 
    practicalswift commented at 4:33 pm on October 28, 2018: contributor

    Rebased! :-)
30. practicalswift force-pushed on Oct 28, 2018
31. DrahtBot removed the label Needs rebase on Oct 28, 2018
32. practicalswift closed this on Nov 12, 2018

    ---

33. practicalswift deleted the branch on Apr 10, 2021
34. DrahtBot locked this on Aug 16, 2022

Contributors
practicalswift Empact DrahtBot

Labels
Refactoring Docs