refactor: Pass interpreter flags as uint32_t instead of signed int #22232
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fa25e11 to
fa480cd
Compare
|
Concept ACK |
�[0;39;49m�[1;34;49mtest/sigopcount_tests.cpp(26): Entering test suite "sigopcount_tests"
�[0;39;49m�[1;34;49mtest/sigopcount_tests.cpp(108): Entering test case "GetTxSigOpCost"
test/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)
#0 0x5567920a3a53 in sigopcount_tests::GetTxSigOpCost::test_method() test/sigopcount_tests.cpp:167:9
#1 0x5567920a1ee5 in sigopcount_tests::GetTxSigOpCost_invoker() test/sigopcount_tests.cpp:108:1
#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
#3 0x7f1708c4f521 (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x31521)
#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)
#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)
#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)
#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)
#8 0x7f1708c5524c (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x3724c)
#9 0x7f1708c55769 (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x37769)
#10 0x7f1708c55769 (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x37769)
#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)
#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)
#13 0x5567917de70d in main /usr/include/boost/test/unit_test.hpp:64:12
#14 0x7f170827b564 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28564)
#15 0x55679172fa7d in _start (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin+0x1bdfa7d)
SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change test/sigopcount_tests.cpp:167:9 in
make[3]: *** [Makefile:18040: test/sigopcount_tests.cpp.test] Error 1 |
fa480cd to
faa0591
Compare
They are cast to unsigned anyway when calling VerifyScript, bitcoinconsensus_verify_script*, or CountWitnessSigOps.
faa0591 to
fa621ed
Compare
|
Fixed by pinning the underlying type to uint32_t |
theStack
approved these changes
Jun 16, 2021
There was a problem hiding this comment.
Concept and code review ACK fa621ed
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>)
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. |
|
Concept ACK |
kristapsk
approved these changes
Jun 17, 2021
jonatack
reviewed
Jun 23, 2021
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jul 23, 2021
hebasto
pushed a commit
to hebasto/bitcoin
that referenced
this pull request
Aug 7, 2021
They are cast to unsigned anyway when calling VerifyScript, bitcoinconsensus_verify_script*, or CountWitnessSigOps. Github-Pull: bitcoin#22232 Rebased-From: fa621ed
Merged
|
UPDATE: #22629 (comment) |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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