doc: emphasize that Assume always evaluates #31528

1. 
   l0rinc commented at 1:40 pm on December 18, 2024: contributor

   We’ve been surprised multiple times by Assume doing heavy computations in release:

   • #31178 (comment)
   • #31490 (review)

   Since Assume is a macro, it could have been written differently to avoid parameter evaluation (similarly to LogDebug, which doesn’t evaluate the parameters).

2. doc: emphasize that `Assume` always evaluates

   We've been surprised multiple times by `Assume` doing heavy computations in `release`:
   * https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2448867990
   * https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1887765453
   
   Since `Assume` is a macro, it could have been written differently to avoid parameter evaluation (similarly to `LogDebug`, which doesn't evaluate the parameters).
   
   Co-Authored-By: Anthony Towns <aj@erisian.com.au>

   fe49ecea66
3. 
   DrahtBot commented at 1:40 pm on December 18, 2024: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31528.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	sipa

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

4. DrahtBot added the label Docs on Dec 18, 2024
5. 
   sipa commented at 1:54 pm on December 18, 2024: member

   I don’t think the commentary in the commit message is necessary or useful. I disagree with it, but even if that wasn’t the case, if you believe the semantics of Assume should be different, you should be changing it rather than adding a comment that it could have been done differently!

   ACK on the code changes.

6. in src/util/check.h:96 in fe49ecea66

   92@@ -93,6 +93,7 @@ constexpr T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] con
   93  * - For fatal errors, use Assert().
   94  * - For non-fatal errors in interactive sessions (e.g. RPC or command line
   95  *   interfaces), CHECK_NONFATAL() might be more appropriate.
   96+ *   Note that the assumption is always fully evaluated - even in non-debug builds.
   

   ---

   ---

   

   maflcko commented at 1:54 pm on December 18, 2024:

   This is part of “identity function”. A function can not return the value that was passed in without evaluating it. (Having a different return type, depending on build type would also make it harder to use the function).

   If the docs are worthy to change, it would be good to either change none of them, or all of them in this file for other identity functions as well.

7. 
   l0rinc commented at 3:31 pm on December 18, 2024: contributor

   Seems this change is more controversial than anticipated, thanks for the comments, will close it for now.
8. l0rinc closed this on Dec 18, 2024

   ---

9. l0rinc deleted the branch on Dec 18, 2024

Contributors
l0rinc DrahtBot sipa maflcko

Labels
Docs