Add 64-bit overflow check to CScriptNum
AbandonedPublic
Actions

Authored by tobias_ruck on Jul 28 2021, 04:44.

Details

Reviewers

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Summary

This allows increasing the script integer size to 63-bit + signed bit down the line.

It uses __builtin_saddll_overflow and __builtin_ssubll_overflow to check if an overflow occurred if available, and falls back to a manual overflow check otherwise.

Test Plan

ninja check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

check_cscriptnum_overflow

Lint

Lint Errors

Severity	Location	Code	Message
Error	src/test/scriptnum63bit.h:	BOOST_DEPENDENCY	A new dependency has been introduced

Unit

No Test Coverage

Build Status

Buildable 16263
Build 32393: Build Diff	build-clang · build-diff · build-without-wallet · lint-circular-dependencies · build-debug · build-clang-tidy
Build 32392: arc lint + arc unit

Event Timeline

tobias_ruck created this revision.Jul 28 2021, 04:44

Owners added a reviewer: Restricted Owners Package.Jul 28 2021, 04:44

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 28 2021, 04:44

tobias_ruck requested review of this revision.Jul 28 2021, 04:44

Harbormaster completed remote builds in B16262: Diff 29320.Jul 28 2021, 04:44

Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Tail of the build log:

[0m[1;30mwallet_multiwallet.py                   | ○ Skipped | 0 s
[0m[1;30mwallet_multiwallet.py --usecli          | ○ Skipped | 0 s
[0m[1;30mwallet_reorgsrestore.py                 | ○ Skipped | 0 s
[0m[1;30mwallet_resendwallettransactions.py      | ○ Skipped | 0 s
[0m[1;30mwallet_txn_clone.py                     | ○ Skipped | 0 s
[0m[1;30mwallet_txn_clone.py --mineblock         | ○ Skipped | 0 s
[0m[1;30mwallet_txn_doublespend.py               | ○ Skipped | 0 s
[0m[1;30mwallet_txn_doublespend.py --mineblock   | ○ Skipped | 0 s
[0m[1;30mwallet_watchonly.py                     | ○ Skipped | 0 s
[0m[1;30mwallet_watchonly.py --usecli            | ○ Skipped | 0 s
[0m[1;30mwallet_zapwallettxes.py                 | ○ Skipped | 0 s
[0m[1m
ALL                                     | ✓ Passed  | 428 s (accumulated) 
[0mRuntime: 86 s

----------------------------------------------------------------------
Ran 5 tests in 0.025s

OK

[154/413] Running avalanche test suite
PASSED: avalanche test suite
[162/413] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.005s

OK
[164/413] cd /work/contrib/devtools/chainparams && /usr/bin/python3.7 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[179/413] Running seeder test suite
PASSED: seeder test suite
[182/413] Running pow test suite
PASSED: pow test suite
[188/413] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[189/413] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/script_tests.cpp.o
In file included from /usr/include/boost/test/unit_test.hpp:19,
                 from ../../src/test/script_tests.cpp:30:
../../src/test/script_tests.cpp: In member function ‘void script_tests::script_build::test_method()’:
../../src/test/script_tests.cpp:540:22: note: variable tracking size limit exceeded with -fvar-tracking-assignments, retrying without
 BOOST_AUTO_TEST_CASE(script_build) {
                      ^~~~~~~~~~~~
[406/413] bitcoin: testing scriptnum63bit_tests
FAILED: src/test/CMakeFiles/check-bitcoin-scriptnum63bit_tests 
cd /work/abc-ci-builds/build-without-wallet/src/test && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-without-wallet/test/junit && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-without-wallet/test/log && /usr/bin/cmake -E env /work/cmake/utils/log-and-print-on-failure.sh /work/abc-ci-builds/build-without-wallet/test/log/bitcoin-scriptnum63bit_tests.log /work/abc-ci-builds/build-without-wallet/src/test/test_bitcoin --run_test=scriptnum63bit_tests --logger=HRF,message:JUNIT,message,bitcoin-scriptnum63bit_tests.xml --catch_system_errors=no
Running 1 test case...
../../src/test/scriptnum63bit_tests.cpp(47): error: in "scriptnum63bit_tests/scriptnum_63_bit_arithmetic_test": 9223372036854775807 - -1 overflowed
../../src/test/scriptnum63bit_tests.cpp(77): error: in "scriptnum63bit_tests/scriptnum_63_bit_arithmetic_test": 9223372036854775807 -= -1 overflowed
../../src/test/scriptnum63bit_tests.cpp(47): error: in "scriptnum63bit_tests/scriptnum_63_bit_arithmetic_test": 1 - -9223372036854775807 overflowed
../../src/test/scriptnum63bit_tests.cpp(77): error: in "scriptnum63bit_tests/scriptnum_63_bit_arithmetic_test": 1 -= -9223372036854775807 overflowed

*** 4 failures are detected in the test module "Bitcoin ABC unit tests"
[410/413] Running utility command for check-bitcoin-coins_tests
ninja: build stopped: cannot make progress due to previous errors.
Build build-without-wallet failed with exit code 1

Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

Tail of the build log:

[0m[0;32mwallet_import_rescan.py                 | ✓ Passed  | 14 s
[0m[0;32mwallet_import_with_label.py             | ✓ Passed  | 1 s
[0m[0;32mwallet_importdescriptors.py             | ✓ Passed  | 8 s
[0m[0;32mwallet_importmulti.py                   | ✓ Passed  | 6 s
[0m[0;32mwallet_importprunedfunds.py             | ✓ Passed  | 3 s
[0m[0;32mwallet_keypool.py                       | ✓ Passed  | 3 s
[0m[0;32mwallet_keypool_topup.py                 | ✓ Passed  | 5 s
[0m[0;32mwallet_labels.py                        | ✓ Passed  | 3 s
[0m[0;32mwallet_listreceivedby.py                | ✓ Passed  | 51 s
[0m[0;32mwallet_listsinceblock.py                | ✓ Passed  | 4 s
[0m[0;32mwallet_listtransactions.py              | ✓ Passed  | 6 s
[0m[0;32mwallet_multiwallet.py                   | ✓ Passed  | 16 s
[0m[0;32mwallet_multiwallet.py --usecli          | ✓ Passed  | 17 s
[0m[0;32mwallet_reorgsrestore.py                 | ✓ Passed  | 6 s
[0m[0;32mwallet_resendwallettransactions.py      | ✓ Passed  | 10 s
[0m[0;32mwallet_txn_clone.py                     | ✓ Passed  | 3 s
[0m[0;32mwallet_txn_clone.py --mineblock         | ✓ Passed  | 4 s
[0m[0;32mwallet_txn_doublespend.py               | ✓ Passed  | 3 s
[0m[0;32mwallet_txn_doublespend.py --mineblock   | ✓ Passed  | 4 s
[0m[0;32mwallet_watchonly.py                     | ✓ Passed  | 1 s
[0m[0;32mwallet_watchonly.py --usecli            | ✓ Passed  | 1 s
[0m[0;32mwallet_zapwallettxes.py                 | ✓ Passed  | 7 s
[0m[1m
ALL                                     | ✓ Passed  | 1373 s (accumulated) 
[0mRuntime: 276 s

----------------------------------------------------------------------
Ran 5 tests in 0.002s

OK

[173/453] Running avalanche test suite
PASSED: avalanche test suite
[192/453] Running seeder test suite
PASSED: seeder test suite
[194/453] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.005s

OK
[195/453] cd /work/contrib/devtools/chainparams && /usr/bin/python3.7 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[367/453] Running secp256k1 test suite
PASSED: secp256k1 test suite
[369/453] bitcoin: testing scriptnum63bit_tests
FAILED: src/test/CMakeFiles/check-bitcoin-scriptnum63bit_tests 
cd /work/abc-ci-builds/build-debug/src/test && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-debug/test/junit && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-debug/test/log && /usr/bin/cmake -E env /work/cmake/utils/log-and-print-on-failure.sh /work/abc-ci-builds/build-debug/test/log/bitcoin-scriptnum63bit_tests.log /work/abc-ci-builds/build-debug/src/test/test_bitcoin --run_test=scriptnum63bit_tests --logger=HRF,message:JUNIT,message,bitcoin-scriptnum63bit_tests.xml --catch_system_errors=no
Aborted (core dumped)
[424/453] Running pow test suite
PASSED: pow test suite
[446/453] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[450/453] Running utility command for check-bitcoin-coins_tests
ninja: build stopped: cannot make progress due to previous errors.
Build build-debug failed with exit code 1

Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Tail of the build log:

[0m[0;32mrpc_psbt.py                             | ✓ Passed  | 29 s
[0m[0;32mrpc_rawtransaction.py                   | ✓ Passed  | 13 s
[0m[0;32mrpc_scantxoutset.py                     | ✓ Passed  | 3 s
[0m[0;32mrpc_setban.py                           | ✓ Passed  | 2 s
[0m[0;32mrpc_signmessage.py                      | ✓ Passed  | 1 s
[0m[0;32mrpc_signrawtransaction.py               | ✓ Passed  | 2 s
[0m[0;32mrpc_txoutproof.py                       | ✓ Passed  | 2 s
[0m[0;32mrpc_uptime.py                           | ✓ Passed  | 1 s
[0m[0;32mrpc_users.py                            | ✓ Passed  | 5 s
[0m[0;32mrpc_whitelist.py                        | ✓ Passed  | 1 s
[0m[0;32mtool_wallet.py                          | ✓ Passed  | 4 s
[0m[0;32mwallet_abandonconflict.py               | ✓ Passed  | 7 s
[0m[0;32mwallet_address_types.py                 | ✓ Passed  | 13 s
[0m[0;32mwallet_avoidreuse.py                    | ✓ Passed  | 4 s
[0m[0;32mwallet_backup.py                        | ✓ Passed  | 31 s
[0m[0;32mwallet_balance.py                       | ✓ Passed  | 9 s
[0m[0;32mwallet_basic.py                         | ✓ Passed  | 27 s
[0m[0;32mwallet_coinbase_category.py             | ✓ Passed  | 1 s
[0m[0;32mwallet_create_tx.py                     | ✓ Passed  | 6 s
[0m[0;32mwallet_createwallet.py                  | ✓ Passed  | 2 s
[0m[0;32mwallet_createwallet.py --usecli         | ✓ Passed  | 3 s
[0m[0;32mwallet_descriptor.py                    | ✓ Passed  | 8 s
[0m[0;32mwallet_disable.py                       | ✓ Passed  | 1 s
[0m[0;32mwallet_dump.py                          | ✓ Passed  | 5 s
[0m[0;32mwallet_encryption.py                    | ✓ Passed  | 5 s
[0m[0;32mwallet_groups.py                        | ✓ Passed  | 42 s
[0m[0;32mwallet_hd.py                            | ✓ Passed  | 6 s
[0m[0;32mwallet_import_rescan.py                 | ✓ Passed  | 5 s
[0m[0;32mwallet_import_with_label.py             | ✓ Passed  | 1 s
[0m[0;32mwallet_importdescriptors.py             | ✓ Passed  | 4 s
[0m[0;32mwallet_importmulti.py                   | ✓ Passed  | 3 s
[0m[0;32mwallet_importprunedfunds.py             | ✓ Passed  | 2 s
[0m[0;32mwallet_keypool.py                       | ✓ Passed  | 3 s
[0m[0;32mwallet_keypool_topup.py                 | ✓ Passed  | 2 s
[0m[0;32mwallet_labels.py                        | ✓ Passed  | 1 s
[0m[0;32mwallet_listreceivedby.py                | ✓ Passed  | 24 s
[0m[0;32mwallet_listsinceblock.py                | ✓ Passed  | 4 s
[0m[0;32mwallet_listtransactions.py              | ✓ Passed  | 17 s
[0m[0;32mwallet_multiwallet.py                   | ✓ Passed  | 11 s
[0m[0;32mwallet_multiwallet.py --usecli          | ✓ Passed  | 13 s
[0m[0;32mwallet_reorgsrestore.py                 | ✓ Passed  | 3 s
[0m[0;32mwallet_resendwallettransactions.py      | ✓ Passed  | 20 s
[0m[0;32mwallet_txn_clone.py                     | ✓ Passed  | 2 s
[0m[0;32mwallet_txn_clone.py --mineblock         | ✓ Passed  | 3 s
[0m[0;32mwallet_txn_doublespend.py               | ✓ Passed  | 1 s
[0m[0;32mwallet_txn_doublespend.py --mineblock   | ✓ Passed  | 3 s
[0m[0;32mwallet_watchonly.py                     | ✓ Passed  | 1 s
[0m[0;32mwallet_watchonly.py --usecli            | ✓ Passed  | 1 s
[0m[0;32mwallet_zapwallettxes.py                 | ✓ Passed  | 4 s
[0m[1m
ALL                                     | ✓ Passed  | 973 s (accumulated) 
[0mRuntime: 195 s

----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1

Fix off-by-one error in overflow_add_int64 and overflow_sub_int64 (doesn't seem to appear on Darwin/clang)

Harbormaster completed remote builds in B16263: Diff 29322.Jul 28 2021, 05:54

Add multiprecision/cpp_int.hpp Boost dependency (Lord have mercy)

Harbormaster completed remote builds in B16264: Diff 29323.Jul 28 2021, 06:21

deadalnix requested changes to this revision.Aug 4 2021, 21:43

deadalnix added inline comments.

src/config/CMakeLists.txt
66 ↗	(On Diff #29323)	Why is that needed?
src/script/script.h
276 ↗	(On Diff #29323)	This is clearly a new error condition that is being added to existing code. If you modify existing consensus code, please do not mix this with additional changes. Do them standalone.