Page MenuHomePhabricator

Add 64-bit overflow check to CScriptNum
AbandonedPublic

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

Details

Reviewers
deadalnix
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
SeverityLocationCodeMessage
Errorsrc/test/scriptnum63bit.h:BOOST_DEPENDENCYA new dependency has been introduced
Unit
No Test Coverage
Build Status
Buildable 16263
Build 32393: Build Diffbuild-clang · build-diff · build-without-wallet · lint-circular-dependencies · build-debug · build-clang-tidy
Build 32392: arc lint + arc unit

Event Timeline

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

Tail of the build log:

wallet_multiwallet.py                   | ○ Skipped | 0 s
wallet_multiwallet.py --usecli          | ○ Skipped | 0 s
wallet_reorgsrestore.py                 | ○ Skipped | 0 s
wallet_resendwallettransactions.py      | ○ Skipped | 0 s
wallet_txn_clone.py                     | ○ Skipped | 0 s
wallet_txn_clone.py --mineblock         | ○ Skipped | 0 s
wallet_txn_doublespend.py               | ○ Skipped | 0 s
wallet_txn_doublespend.py --mineblock   | ○ Skipped | 0 s
wallet_watchonly.py                     | ○ Skipped | 0 s
wallet_watchonly.py --usecli            | ○ Skipped | 0 s
wallet_zapwallettxes.py                 | ○ Skipped | 0 s

ALL                                     | ✓ Passed  | 428 s (accumulated) 
Runtime: 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

Tail of the build log:

wallet_import_rescan.py                 | ✓ Passed  | 14 s
wallet_import_with_label.py             | ✓ Passed  | 1 s
wallet_importdescriptors.py             | ✓ Passed  | 8 s
wallet_importmulti.py                   | ✓ Passed  | 6 s
wallet_importprunedfunds.py             | ✓ Passed  | 3 s
wallet_keypool.py                       | ✓ Passed  | 3 s
wallet_keypool_topup.py                 | ✓ Passed  | 5 s
wallet_labels.py                        | ✓ Passed  | 3 s
wallet_listreceivedby.py                | ✓ Passed  | 51 s
wallet_listsinceblock.py                | ✓ Passed  | 4 s
wallet_listtransactions.py              | ✓ Passed  | 6 s
wallet_multiwallet.py                   | ✓ Passed  | 16 s
wallet_multiwallet.py --usecli          | ✓ Passed  | 17 s
wallet_reorgsrestore.py                 | ✓ Passed  | 6 s
wallet_resendwallettransactions.py      | ✓ Passed  | 10 s
wallet_txn_clone.py                     | ✓ Passed  | 3 s
wallet_txn_clone.py --mineblock         | ✓ Passed  | 4 s
wallet_txn_doublespend.py               | ✓ Passed  | 3 s
wallet_txn_doublespend.py --mineblock   | ✓ Passed  | 4 s
wallet_watchonly.py                     | ✓ Passed  | 1 s
wallet_watchonly.py --usecli            | ✓ Passed  | 1 s
wallet_zapwallettxes.py                 | ✓ Passed  | 7 s

ALL                                     | ✓ Passed  | 1373 s (accumulated) 
Runtime: 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

Tail of the build log:

rpc_psbt.py                             | ✓ Passed  | 29 s
rpc_rawtransaction.py                   | ✓ Passed  | 13 s
rpc_scantxoutset.py                     | ✓ Passed  | 3 s
rpc_setban.py                           | ✓ Passed  | 2 s
rpc_signmessage.py                      | ✓ Passed  | 1 s
rpc_signrawtransaction.py               | ✓ Passed  | 2 s
rpc_txoutproof.py                       | ✓ Passed  | 2 s
rpc_uptime.py                           | ✓ Passed  | 1 s
rpc_users.py                            | ✓ Passed  | 5 s
rpc_whitelist.py                        | ✓ Passed  | 1 s
tool_wallet.py                          | ✓ Passed  | 4 s
wallet_abandonconflict.py               | ✓ Passed  | 7 s
wallet_address_types.py                 | ✓ Passed  | 13 s
wallet_avoidreuse.py                    | ✓ Passed  | 4 s
wallet_backup.py                        | ✓ Passed  | 31 s
wallet_balance.py                       | ✓ Passed  | 9 s
wallet_basic.py                         | ✓ Passed  | 27 s
wallet_coinbase_category.py             | ✓ Passed  | 1 s
wallet_create_tx.py                     | ✓ Passed  | 6 s
wallet_createwallet.py                  | ✓ Passed  | 2 s
wallet_createwallet.py --usecli         | ✓ Passed  | 3 s
wallet_descriptor.py                    | ✓ Passed  | 8 s
wallet_disable.py                       | ✓ Passed  | 1 s
wallet_dump.py                          | ✓ Passed  | 5 s
wallet_encryption.py                    | ✓ Passed  | 5 s
wallet_groups.py                        | ✓ Passed  | 42 s
wallet_hd.py                            | ✓ Passed  | 6 s
wallet_import_rescan.py                 | ✓ Passed  | 5 s
wallet_import_with_label.py             | ✓ Passed  | 1 s
wallet_importdescriptors.py             | ✓ Passed  | 4 s
wallet_importmulti.py                   | ✓ Passed  | 3 s
wallet_importprunedfunds.py             | ✓ Passed  | 2 s
wallet_keypool.py                       | ✓ Passed  | 3 s
wallet_keypool_topup.py                 | ✓ Passed  | 2 s
wallet_labels.py                        | ✓ Passed  | 1 s
wallet_listreceivedby.py                | ✓ Passed  | 24 s
wallet_listsinceblock.py                | ✓ Passed  | 4 s
wallet_listtransactions.py              | ✓ Passed  | 17 s
wallet_multiwallet.py                   | ✓ Passed  | 11 s
wallet_multiwallet.py --usecli          | ✓ Passed  | 13 s
wallet_reorgsrestore.py                 | ✓ Passed  | 3 s
wallet_resendwallettransactions.py      | ✓ Passed  | 20 s
wallet_txn_clone.py                     | ✓ Passed  | 2 s
wallet_txn_clone.py --mineblock         | ✓ Passed  | 3 s
wallet_txn_doublespend.py               | ✓ Passed  | 1 s
wallet_txn_doublespend.py --mineblock   | ✓ Passed  | 3 s
wallet_watchonly.py                     | ✓ Passed  | 1 s
wallet_watchonly.py --usecli            | ✓ Passed  | 1 s
wallet_zapwallettxes.py                 | ✓ Passed  | 4 s

ALL                                     | ✓ Passed  | 973 s (accumulated) 
Runtime: 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)

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

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.

This revision now requires changes to proceed.Aug 4 2021, 21:43