script: Avoid side effect in RHS operand (MISRA-C). Clarify intended logic. #13684

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:avoid-side-effect-in-right-hand-operand-of-a-logical-&&-operator changing 1 files +6 −2
  1. practicalswift commented at 9:47 AM on July 17, 2018: contributor

    Avoid side effect in RHS operand (MISRA-C). Clarify intended logic.

    Fix MISRA-C related warning.

  2. script: Avoid side effect in RHS operand (MISRA-C). Clarify intended logic.
    Fix MISRA-C related warning.
    bdd83ddbea
  3. fanquake requested review from sipa on Jul 17, 2018
  4. Empact commented at 7:29 PM on July 17, 2018: member

    utACK bdd83dd Is this the only MISRA-C warning?

  5. practicalswift commented at 8:53 AM on July 18, 2018: contributor

    @Empact This is the only warning for this specific issue, but there are warnings for other issues – some of which might be worth addressing. I'll look into which ones :-)

  6. laanwj added the label Refactoring on Jul 20, 2018
  7. laanwj commented at 4:03 PM on July 20, 2018: member

    As this is a change in critical consensus code, I don't think this is worth the risk, just to shut up a warning. (as far as I know this is not ub?)

  8. sipa commented at 6:10 PM on July 20, 2018: member

    Code review ACK bdd83ddbea03ce025e19fdb24e0bc120d0f8d8ae. @laanwj Agree, though it also makes things more readable.

  9. practicalswift commented at 7:12 PM on July 20, 2018: contributor

    @laanwj I agree in the general case that we shouldn't refactor consensus critical code just to shut up a warning, but this change is about making the author's intention clear. Making the intention clear is extra important in consensus critical code to avoid future mistakes. It is not obvious for the reader that the side effect here is intentional.

    FWIW, I thought it was a mistake when I first read it :-)

    Should I add an extra comment instead?

  10. practicalswift commented at 7:19 PM on July 20, 2018: contributor

    @laanwj Just to make the rationale clear – this is not UB :-)

  11. sdaftuar commented at 9:02 PM on July 20, 2018: member

    I agree with @laanwj that this is not worth the risk -- a comment would suffice for clarifying the logic IMO.

  12. practicalswift commented at 9:33 PM on July 20, 2018: contributor

    FWIW, both clang++ -S -O and g++ -S -O generate the same assembly for the before/after code.

    Some git history:

    Satoshi introduced this code in 6ff5f718b6a67797b2b3bab8905d607ad216ee21 (2010). @petertodd added a clarifying comment in 214d45b6b9a4f25d7d8bd4e5443fa2bee485353a (2013):

    commit 214d45b6b9a4f25d7d8bd4e5443fa2bee485353a
Author: Peter Todd <pete@petertodd.org>
Date:   Sun Aug 25 12:37:07 2013 -0400

    Document and test OP_RESERVED weirdness

    Seems it was forgotten about when IsPushOnly() and the unittests were
    written. A particular oddity is that OP_RESERVED doesn't count towards
    the >201 opcode limit unlike every other named opcode.

diff --git a/src/script.cpp b/src/script.cpp
index 2df2e9f0d..a3aae42d2 100644
--- a/src/script.cpp
+++ b/src/script.cpp
@@ -327,6 +327,8 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, co
                 return false;
             if (vchPushValue.size() > MAX_SCRIPT_ELEMENT_SIZE)
                 return false;
+
+            // Note how OP_RESERVED does not count towards the opcode limit.
             if (opcode > OP_16 && ++nOpCount > 201)
                 return false;
  13. MarcoFalke commented at 10:04 PM on July 20, 2018: member

    utACK bdd83ddbea03ce025e19fdb24e0bc120d0f8d8ae

  14. MarcoFalke added the label Needs gitian build on Jul 29, 2018
  15. DrahtBot commented at 3:13 AM on July 30, 2018: member

    <!--a722867cd34abeea1fadc8d60700f111-->

    Gitian builds for commit e8ffec69f773e67ae82535afc87164eb8102f05e (master):

    Gitian builds for commit 1c61ee541e75a31f830e99d90bef980942abd551 (master and this pull):

  16. DrahtBot removed the label Needs gitian build on Jul 30, 2018
  17. laanwj commented at 12:00 PM on August 7, 2018: member

    Closing, as there's disagreement whether to do this.

  18. laanwj closed this on Aug 7, 2018
  19. practicalswift deleted the branch on Apr 10, 2021
  20. DrahtBot locked this on Aug 16, 2022
Contributors
practicalswiftEmpactlaanwjsipasdaftuarMarcoFalkeDrahtBot


sipa

Labels
Refactoring
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-16 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me