Avoid unused locals for CScript::IsWitnessProgram #12938

1. 
   Empact commented at 4:21 AM on April 11, 2018: member

   For those cases where the version and program are not useful.

2. Empact force-pushed on Apr 11, 2018
3. 
   practicalswift commented at 10:56 AM on April 11, 2018: contributor

   utACK b9f0169bcce3ca7c9d0646ddda5bf264062be87b

   Nice simplification

4. Empact renamed this:
   Overload CScript::IsWitnessProgram with a test-only version
   Avoid unused locals for CScript::IsWitnessProgram
   on Apr 12, 2018
5. in src/script/script.cpp:229 in b9f0169bcc outdated

    226 | @@ -227,6 +227,14 @@ bool CScript::IsWitnessProgram(int& version, std::vector<unsigned char>& program
    227 |          return false;
    228 |      }
    229 |      if ((size_t)((*this)[1] + 2) == this->size()) {
   

   ---

   ---

   

   jimpo commented at 1:40 AM on April 13, 2018:

   Might as well return this directly.

   ---

   

   Empact commented at 10:04 AM on April 13, 2018:

   I like that style too, but held back for the sake of simplifying review. Here's my choice if I'm to simplify the method:

   static constexpr bool IsOP_N(const unsigned char op)
   {
       return (op == OP_0 || (op >= OP_1 && op <= OP_16));
   }
   
   // A witness program is any valid CScript that consists of a 1-byte push opcode
   // followed by a data push between 2 and 40 bytes.
   bool CScript::IsWitnessProgram() const
   {
       return ((this->size() >= 4 && this->size() <= 42) &&
               IsOP_N((*this)[0]) &&
               (size_t)((*this)[1] + 2) == this->size());
   }
   

   Incidentally, this matches the style of IsPayToWitnessScriptHash and IsPayToScriptHash, above. Thoughts?

   ---

   

   promag commented at 10:42 AM on April 13, 2018:

   @Empact looks good, I'd ACK that.

6. jimpo approved
7. 
   jimpo commented at 1:41 AM on April 13, 2018: contributor

   utACK b9f0169

8. 
   promag commented at 10:42 AM on April 13, 2018: member

   utACK b9f0169.

9. meshcollider added the label Refactoring on Apr 16, 2018
10. fanquake requested review from sipa on Apr 16, 2018
11. Empact force-pushed on Jun 1, 2018
12. 
    Empact commented at 12:13 PM on June 1, 2018: member

    Rebased, squashed, and updated to take advantage of IsSmallInteger from #13194

13. Empact force-pushed on Jun 1, 2018
14. Empact force-pushed on Jun 1, 2018
15. Empact force-pushed on Jun 1, 2018
16. 
    sdaftuar commented at 2:17 PM on June 1, 2018: member

    I am generally a concept NACK on changes like this; IMO the review burden for refactoring consensus-critical code typically outweighs the benefits.

17. Overload CScript::IsWitnessProgram with a test-only version

    For those cases where the version and program are not useful.

    943f3b78d6
18. Empact force-pushed on Jun 1, 2018
19. 
    laanwj commented at 5:44 PM on June 24, 2018: member

    I am generally a concept NACK on changes like this; IMO the review burden for refactoring consensus-critical code typically outweighs the benefits.

    Agree, seems to risky, closing.

20. laanwj closed this on Jun 24, 2018

    ---

21. Empact deleted the branch on Jul 2, 2018
22. MarcoFalke locked this on Sep 8, 2021

Contributors

Empactpracticalswiftjimpopromagsdaftuarlaanwj

Review Requested
sipa