script: Remove magic numbers #6818

pull dajohi wants to merge 1 commits into bitcoin:master from dajohi:script_const changing 3 files +12 −5

dajohi commented at 2:05 PM on October 13, 2015: contributor

This adds two new constants, MAX_OPS_PER_SCRIPT and MAX_PUBKEYS_PER_MULTISIG.
MarcoFalke commented at 3:30 PM on October 13, 2015: member

Concept ACK.
jamesob commented at 6:09 PM on October 13, 2015: member

utACK
jonasschnelli commented at 6:25 PM on October 13, 2015: contributor

utACK
morcos commented at 6:52 PM on October 13, 2015: member

NACK, there is little reward here for the inherent risk in changing consensus code. This is even a compiler warning for comparing signed and unsigned.

paveljanik commented at 7:47 PM on October 13, 2015: contributor

This is a diff between master and this PR build:

+script/interpreter.cpp:276:46: warning: comparison of integers of different signs: 'int' and 'const unsigned int' [-Wsign-compare]
+            if (opcode > OP_16 && ++nOpCount > MAX_OPS_PER_SCRIPT)
+                                  ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~
+script/interpreter.cpp:872:54: warning: comparison of integers of different signs: 'int' and 'const unsigned int' [-Wsign-compare]
+                    if (nKeysCount < 0 || nKeysCount > MAX_PUBKEYS_PER_MULTISIG)
+                                          ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~
+script/interpreter.cpp:875:34: warning: comparison of integers of different signs: 'int' and 'const unsigned int' [-Wsign-compare]
+                    if (nOpCount > MAX_OPS_PER_SCRIPT)
+                        ~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~
+3 warnings generated.

So NACK.

Why don't you use #define instead? This way you could prove it is OK by comparing binaries.

dcousens commented at 9:56 PM on October 13, 2015: contributor

concept/utACK now that it is rebased / fixed.
sipa commented at 12:50 AM on October 14, 2015: member

utACK
in src/script/script.h:None in 6d734d3549 outdated
```
  16 | @@ -17,7 +17,14 @@
  17 |  #include <string>
  18 |  #include <vector>
  19 |  
  20 | -static const unsigned int MAX_SCRIPT_ELEMENT_SIZE = 520; // bytes
  21 | +// Maximum number of non-push operations per script
  22 | +#define MAX_OPS_PER_SCRIPT 201
```
laanwj commented at 10:58 AM on October 15, 2015:

Please don't use defines to declare constants, follow the rest of the code and use static const <type>

dcousens commented at 11:00 AM on October 15, 2015:

They look like int from a quick glance.
laanwj added the label Consensus on Oct 15, 2015

script: Remove magic numbers

This adds two new constants, MAX_OPS_PER_SCRIPT and
MAX_PUBKEYS_PER_MULTISIG.

b48da5c189

dajohi commented at 3:14 PM on October 15, 2015: contributor

updated to use static const int
paveljanik commented at 6:51 PM on October 15, 2015: contributor

ACK
dcousens commented at 9:21 PM on October 15, 2015: contributor

ACK
fanquake commented at 2:33 AM on October 23, 2015: member

utACK
laanwj referenced this in commit 1731bab7ef on Oct 23, 2015
laanwj merged this on Oct 23, 2015
laanwj closed this on Oct 23, 2015
laanwj referenced this in commit 923c5e93a9 on Oct 23, 2015
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels