Remove redundant NDEBUG preprocessor checks #17459

Since commit 7cee85807c4db679003c6659d247a2fe74c2464a ("Add compile time verification of assumptions we're currently making implicitly/tacitly", PR #15391), this check is done in compat/assumptions.h: https://github.com/bitcoin/bitcoin/blob/8237889e8d0fb7542669a9098516c96da91913f0/src/compat/assumptions.h#L13-L18 This header is included by util/system.h, which is in turn included by most .cpp files, including the ones this commit modifies (validation.cpp and net_processing.cpp).

@theStack

I'm all for removing redundancies in the general case, but in this specific case the redundancy is intentional:

Note that the inclusion of assumptions.h could be removed accidentally from net_processing.cpp and validation.cpp without breaking the build.

Thus we cannot assume that assumptions.h will always be included :)

I think of it this way:

• Enforcement: The NDEBUG checks in net_processing.cpp and validation.cpp make sure that we enforce this project wide (since at least one of those files likely will be part of all relevant future builds of the project where this should be enforced).
• Documentation: The NDEBUG check in assumptions.h serves as a documentation of an important assumption we make.

Does that make sense? :)

@practicalswift: Thanks for clarifying! My strong assumption was that in any possible build, one of the dozens .cpp files which currently include the compat/assumptions.h header (which after all says "compile-time verification", implying more than merely documentation) would also do so in the future, triggering the error in case NDEBUG is set. Also it seemed quite fishy to me that the check was even present in two .cpp files, not just one. Looking deeper into it, that seems to have historic reasons: introduced originally by gmaxwell with commit 9b59e3bda8c137bff885db5b1f9150346e36e076, the NDEBUG check was only present in main.cpp. Then later when main.cpp was split up into validation.cpp and net_processing.cpp it got copied over into both files (commits e736772c56a883e2649cc8534dd7857a0718ec56, 76faa3cdfedbd3fc91be4ecfff77fc6dc18134fb by TheBlueMatt; part of PR #9260 with a very entertaining title/description :laughing: ).

Still feels odd to have the exact same preprocessor construct and error string scattered aorund in three places in the same project, but I get your point.

Tend toward NACK. This code can go when the run-time critical assumptions stop using assertions. Then it is fully safe to define NDEBUG. Until then, belts and suspenders. (@MarcoFalke is working on this)

Yeah, if the goal is to make sure the assert can never be disabled by accident, but also avoid these preprocessor checks, you could cherry-pick something like fa1564326499f69eeb5427262ff0f26070f86146, replace the std::abort(); with assert(false), and add the preprocessor check to ./src/util/check.h (a single place).

Though, I'd say it's not much of a goal in itself. Wouldn't be surprised if the preprocessor is the least CPU intensive part of the C++ compilation process.

Closing as something that we can potentially revisit in the future.