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

• 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.

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

E.g. this seems like a meaningful case:

if (!file) {
    LogPrintf("Warning: Could not open blocks file %s\n", path.string());
} else {
    LogPrintf("Importing blocks file %s...\n", path.string());
    if (!LoadExternalBlockFile(chainparams, file)) {
        LogPrintf("Warning: No blocks loaded from %s\n", path.string());
    }
}

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

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.

@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 :-)

Rebased! Please review :-)

Updated NODISCARD to work also with Visual C++ :-)

Rebased! :-)

Rebased! :-)

Rebased! :-)