refactor: Pass interpreter flags as uint32

MarcoFalke commented at 2:59 pm on June 13, 2021: member

The flags are cast to unsigned in the interpreter anyway, so avoid the confusion (and fuzz crashes) by just passing them as unsigned from the beginning.

Also, the flags are often inverted bit-wise with the ~ operator, which also works on signed integers, but might cause confusion as the sign bit is flipped.

Fixes #22233

MarcoFalke force-pushed on Jun 13, 2021

DrahtBot added the label Refactoring on Jun 13, 2021

practicalswift commented at 4:51 pm on June 13, 2021: contributor

Concept ACK

fanquake commented at 2:16 am on June 14, 2021: member

Sanitizer CI failure:

 0�[0;39;49m�[1;34;49mtest/sigopcount_tests.cpp(26): Entering test suite "sigopcount_tests"
 1�[0;39;49m�[1;34;49mtest/sigopcount_tests.cpp(108): Entering test case "GetTxSigOpCost"
 2test/sigopcount_tests.cpp:167:9: runtime error: implicit conversion from type 'int' of value -2049 (32-bit, signed) to type 'unsigned int' changed the value to 4294965247 (32-bit, unsigned)
 3    [#0](/bitcoin-bitcoin/0/) 0x5567920a3a53 in sigopcount_tests::GetTxSigOpCost::test_method() test/sigopcount_tests.cpp:167:9
 4    [#1](/bitcoin-bitcoin/1/) 0x5567920a1ee5 in sigopcount_tests::GetTxSigOpCost_invoker() test/sigopcount_tests.cpp:108:1
 5    [#2](/bitcoin-bitcoin/2/) 0x556791886eff in boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /usr/include/boost/function/function_template.hpp:117:11
 6    [#3](/bitcoin-bitcoin/3/) 0x7f1708c4f521  (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x31521)
 7    [#4](/bitcoin-bitcoin/4/) 0x7f1708c4dcc4 in boost::execution_monitor::catch_signals(boost::function<int ()> const&) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x2fcc4)
 8    [#5](/bitcoin-bitcoin/5/) 0x7f1708c4dd47 in boost::execution_monitor::execute(boost::function<int ()> const&) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x2fd47)
 9    [#6](/bitcoin-bitcoin/6/) 0x7f1708c4ddf4 in boost::execution_monitor::vexecute(boost::function<void ()> const&) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x2fdf4)
10    [#7](/bitcoin-bitcoin/7/) 0x7f1708c7cca5 in boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned long) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x5eca5)
11    [#8](/bitcoin-bitcoin/8/) 0x7f1708c5524c  (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x3724c)
12    [#9](/bitcoin-bitcoin/9/) 0x7f1708c55769  (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x37769)
13    [#10](/bitcoin-bitcoin/10/) 0x7f1708c55769  (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x37769)
14    [#11](/bitcoin-bitcoin/11/) 0x7f1708c595bf in boost::unit_test::framework::run(unsigned long, bool) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x3b5bf)
15    [#12](/bitcoin-bitcoin/12/) 0x7f1708c7b81d in boost::unit_test::unit_test_main(bool (*)(), int, char**) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x5d81d)
16    [#13](/bitcoin-bitcoin/13/) 0x5567917de70d in main /usr/include/boost/test/unit_test.hpp:64:12
17    [#14](/bitcoin-bitcoin/14/) 0x7f170827b564 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28564)
18    [#15](/bitcoin-bitcoin/15/) 0x55679172fa7d in _start (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin+0x1bdfa7d)
19
20SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change test/sigopcount_tests.cpp:167:9 in 
21make[3]: *** [Makefile:18040: test/sigopcount_tests.cpp.test] Error 1

MarcoFalke force-pushed on Jun 14, 2021

refactor: Pass script verify flags as uint32_t

They are cast to unsigned anyway when calling VerifyScript,
bitcoinconsensus_verify_script*, or CountWitnessSigOps.

fa621ededd

MarcoFalke force-pushed on Jun 14, 2021

MarcoFalke commented at 7:20 am on June 14, 2021: member

Fixed by pinning the underlying type to uint32_t

theStack approved

theStack commented at 3:54 pm on June 16, 2021: member

Concept and code review ACK fa621ededdfe31a200b77a8787de7e3d2e667aec

IMHO there is really not much point in ever using signed datatypes for flags (though e.g. in CheckFinalTx, the flags for the nSequence/nLockTime locks have a special meaning if negative 👀… could probably be replaced by std::optional<uint32_t>)

MarcoFalke commented at 4:17 pm on June 16, 2021: member

though e.g. in CheckFinalTx, the flags for the nSequence/nLockTime locks have a special meaning if negative eyes… could probably be replaced by std::optional<uint32_t>

Agree, though no sanitizer complains about them right now. Also, those aren’t script verify flags in the same enum, so should be done in a separate pull request.