refactor: Remove unused CExt{Pub,}Key (de)serialization methods
ClosedPublic
Actions

Authored by PiRK on Sep 24 2020, 16:24.

Details

Reviewers

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

rABC231b37f3ea53: refactor: Remove unused CExt{Pub,}Key (de)serialization methods

Summary

The serialization/deserialization methods for the classes CExtKey and
CExtPubKey were only used in the BIP32 unit tests, where the relevant parts are
removed as well.

Backport of Bitcoin Core PR17212

Test Plan

ninja check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

backport-pr17212

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 12901
Build 25869: Build Diff	build-clang-10 · build-diff · lint-circular-dependencies · build-without-wallet · build-clang-tidy
Build 25868: arc lint + arc unit

Event Timeline

PiRK created this revision.Sep 24 2020, 16:24

Owners added a reviewer: Restricted Owners Package.Sep 24 2020, 16:24

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 24 2020, 16:24

PiRK requested review of this revision.Sep 24 2020, 16:24

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those PRs have been inserted into the summary for reference.

Harbormaster completed remote builds in B12901: Diff 23766.Sep 24 2020, 16:29

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

Snippet of first build failure:

[0m[1;30mwallet_createwallet.py                  | ○ Skipped | 0 s
[0m[1;30mwallet_createwallet.py --usecli         | ○ Skipped | 0 s
[0m[1;30mwallet_dump.py                          | ○ Skipped | 0 s
[0m[1;30mwallet_encryption.py                    | ○ Skipped | 0 s
[0m[1;30mwallet_groups.py                        | ○ Skipped | 0 s
[0m[1;30mwallet_hd.py                            | ○ Skipped | 0 s
[0m[1;30mwallet_import_rescan.py                 | ○ Skipped | 0 s
[0m[1;30mwallet_import_with_label.py             | ○ Skipped | 0 s
[0m[1;30mwallet_importmulti.py                   | ○ Skipped | 0 s
[0m[1;30mwallet_importprunedfunds.py             | ○ Skipped | 0 s
[0m[1;30mwallet_keypool.py                       | ○ Skipped | 0 s
[0m[1;30mwallet_keypool_topup.py                 | ○ Skipped | 0 s
[0m[1;30mwallet_labels.py                        | ○ Skipped | 0 s
[0m[1;30mwallet_listreceivedby.py                | ○ Skipped | 0 s
[0m[1;30mwallet_listsinceblock.py                | ○ Skipped | 0 s
[0m[1;30mwallet_listtransactions.py              | ○ Skipped | 0 s
[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  | 259 s (accumulated) 
[0mRuntime: 52 s

[140/373] Running avalanche test suite
PASSED: avalanche test suite
[142/373] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.005s

OK
[143/373] Running seeder test suite
PASSED: seeder test suite
[144/373] cd /work/contrib/devtools/chainparams && /usr/bin/python3.7 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[158/373] Running pow test suite
PASSED: pow test suite
[166/373] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[168/373] 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:541:22: note: variable tracking size limit exceeded with -fvar-tracking-assignments, retrying without
 BOOST_AUTO_TEST_CASE(script_build) {
                      ^~~~~~~~~~~~
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-clang-10) failed.

Snippet of first build failure:

[146/409] Running utility command for check-avalanche-processor_tests
[147/409] Running avalanche test suite
PASSED: avalanche test suite
[148/409] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/init_tests.cpp.o
[149/409] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.005s

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

OK
[151/409] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/parse_name_tests.cpp.o
[152/409] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/write_name_tests.cpp.o
[153/409] Linking CXX executable src/seeder/test/test-seeder
[154/409] Automatic MOC for target test_bitcoin-qt
[155/409] seeder: testing p2p_messaging_tests
[156/409] Running utility command for check-seeder-p2p_messaging_tests
[157/409] seeder: testing parse_name_tests
[158/409] seeder: testing write_name_tests
[159/409] Running utility command for check-seeder-parse_name_tests
[160/409] Running utility command for check-seeder-write_name_tests
[161/409] Running seeder test suite
PASSED: seeder test suite
[162/409] Test Bitcoin utilities...
[163/409] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/bitcoinaddressvalidatortests.cpp.o
[164/409] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/compattests.cpp.o
[165/409] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_crypto_tests.cpp.o
[166/409] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/ismine_tests.cpp.o
[167/409] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/util.cpp.o
[168/409] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/psbt_wallet_tests.cpp.o
[169/409] pow: testing aserti32d_tests
[170/409] Running utility command for check-pow-aserti32d_tests
[171/409] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_bitcoin-qt_autogen/mocs_compilation.cpp.o
[172/409] Running pow test suite
PASSED: pow test suite
[173/409] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/guiutiltests.cpp.o
[174/409] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/walletdb_tests.cpp.o
[175/409] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/coinselector_tests.cpp.o
[176/409] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/uritests.cpp.o
[177/409] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/apptests.cpp.o
[178/409] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/rpcnestedtests.cpp.o
[179/409] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_main.cpp.o
[180/409] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_tests.cpp.o
[181/409] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/paymentservertests.cpp.o
[182/409] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/__/__/wallet/test/wallet_test_fixture.cpp.o
[183/409] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/addressbooktests.cpp.o
[184/409] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/util_tests.cpp.o
[185/409] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/wallettests.cpp.o
[186/409] Linking CXX executable src/qt/test/test_bitcoin-qt
[187/409] bitcoin-qt: testing test_bitcoin-qt
[188/409] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[189/409] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/script_tests.cpp.o
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-10 failed with exit code 1

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

Snippet of first build failure:

[139/404] Running avalanche test suite
PASSED: avalanche test suite
[140/404] Running utility command for check-pow-daa_tests
[141/404] Automatic MOC for target bitcoin-qt-base
[142/402] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/db_tests.cpp.o
[143/402] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.005s

OK
[144/402] Linking CXX executable src/seeder/test/test-seeder
[145/402] cd /work/contrib/devtools/chainparams && /usr/bin/python3.7 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[146/402] seeder: testing parse_name_tests
[147/402] seeder: testing write_name_tests
[148/402] Running utility command for check-seeder-parse_name_tests
[149/402] Running utility command for check-seeder-write_name_tests
[150/402] seeder: testing p2p_messaging_tests
[151/402] Running utility command for check-seeder-p2p_messaging_tests
[152/402] Running seeder test suite
PASSED: seeder test suite
[153/402] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/init_tests.cpp.o
[154/402] Automatic MOC for target test_bitcoin-qt
[155/402] Test Bitcoin utilities...
[156/402] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/bitcoinaddressvalidatortests.cpp.o
[157/402] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/compattests.cpp.o
[158/402] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_crypto_tests.cpp.o
[159/402] pow: testing aserti32d_tests
[160/402] Running utility command for check-pow-aserti32d_tests
[161/402] Running pow test suite
PASSED: pow test suite
[162/402] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/ismine_tests.cpp.o
[163/402] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/psbt_wallet_tests.cpp.o
[164/402] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/util.cpp.o
[165/402] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/script_tests.cpp.o
[166/402] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/guiutiltests.cpp.o
[167/402] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_bitcoin-qt_autogen/mocs_compilation.cpp.o
[168/402] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/walletdb_tests.cpp.o
[169/402] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/coinselector_tests.cpp.o
[170/402] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_tests.cpp.o
[171/402] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/apptests.cpp.o
[172/402] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/rpcnestedtests.cpp.o
[173/402] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/uritests.cpp.o
[174/402] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_main.cpp.o
[175/402] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/util_tests.cpp.o
[176/402] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/__/__/wallet/test/wallet_test_fixture.cpp.o
[177/402] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/paymentservertests.cpp.o
[178/402] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/addressbooktests.cpp.o
[179/402] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/wallettests.cpp.o
[180/402] Linking CXX executable src/qt/test/test_bitcoin-qt
[181/402] bitcoin-qt: testing test_bitcoin-qt
[182/402] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1

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

Snippet of first build failure:

[0m[0;32mrpc_getblockstats.py                    | ✓ Passed  | 1 s
[0m[0;32mrpc_getchaintips.py                     | ✓ Passed  | 2 s
[0m[0;32mrpc_help.py                             | ✓ Passed  | 1 s
[0m[0;32mrpc_invalidateblock.py                  | ✓ Passed  | 6 s
[0m[0;32mrpc_misc.py                             | ✓ Passed  | 1 s
[0m[0;32mrpc_named_arguments.py                  | ✓ Passed  | 1 s
[0m[0;32mrpc_net.py                              | ✓ Passed  | 1 s
[0m[0;32mrpc_preciousblock.py                    | ✓ Passed  | 1 s
[0m[0;32mrpc_psbt.py                             | ✓ Passed  | 20 s
[0m[0;32mrpc_scantxoutset.py                     | ✓ Passed  | 4 s
[0m[0;32mrpc_setban.py                           | ✓ Passed  | 2 s
[0m[0;32mrpc_signmessage.py                      | ✓ Passed  | 1 s
[0m[0;32mrpc_signrawtransaction.py               | ✓ Passed  | 1 s
[0m[0;32mrpc_txoutproof.py                       | ✓ Passed  | 2 s
[0m[0;32mrpc_uptime.py                           | ✓ Passed  | 1 s
[0m[0;32mrpc_users.py                            | ✓ Passed  | 2 s
[0m[0;32mrpc_whitelist.py                        | ✓ Passed  | 1 s
[0m[0;32mtool_wallet.py                          | ✓ Passed  | 3 s
[0m[0;32mwallet_abandonconflict.py               | ✓ Passed  | 10 s
[0m[0;32mwallet_address_types.py                 | ✓ Passed  | 15 s
[0m[0;32mwallet_avoidreuse.py                    | ✓ Passed  | 3 s
[0m[0;32mwallet_backup.py                        | ✓ Passed  | 39 s
[0m[0;32mwallet_balance.py                       | ✓ Passed  | 6 s
[0m[0;32mwallet_basic.py                         | ✓ Passed  | 23 s
[0m[0;32mwallet_coinbase_category.py             | ✓ Passed  | 1 s
[0m[0;32mwallet_create_tx.py                     | ✓ Passed  | 5 s
[0m[0;32mwallet_createwallet.py                  | ✓ Passed  | 2 s
[0m[0;32mwallet_createwallet.py --usecli         | ✓ Passed  | 3 s
[0m[0;32mwallet_disable.py                       | ✓ Passed  | 1 s
[0m[0;32mwallet_dump.py                          | ✓ Passed  | 2 s
[0m[0;32mwallet_encryption.py                    | ✓ Passed  | 5 s
[0m[0;32mwallet_groups.py                        | ✓ Passed  | 12 s
[0m[0;32mwallet_hd.py                            | ✓ Passed  | 5 s
[0m[0;32mwallet_import_rescan.py                 | ✓ Passed  | 4 s
[0m[0;32mwallet_import_with_label.py             | ✓ Passed  | 1 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  | 3 s
[0m[0;32mwallet_labels.py                        | ✓ Passed  | 1 s
[0m[0;32mwallet_listreceivedby.py                | ✓ Passed  | 12 s
[0m[0;32mwallet_listsinceblock.py                | ✓ Passed  | 3 s
[0m[0;32mwallet_listtransactions.py              | ✓ Passed  | 8 s
[0m[0;32mwallet_multiwallet.py                   | ✓ Passed  | 12 s
[0m[0;32mwallet_multiwallet.py --usecli          | ✓ Passed  | 13 s
[0m[0;32mwallet_reorgsrestore.py                 | ✓ Passed  | 3 s
[0m[0;32mwallet_resendwallettransactions.py      | ✓ Passed  | 7 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  | 3 s
[0m[1m
ALL                                     | ✓ Passed  | 689 s (accumulated) 
[0mRuntime: 139 s

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

I think I missed something. It actually causes an error on my side. Looks like in our codebase these methods are used in serialize.h

Back to you, this seems it's all broken.

This revision now requires changes to proceed.Sep 24 2020, 16:46

We have an extra test that Bitcoin Core does not have: https://reviews.bitcoinabc.org/rABCdaec76faa3e74ac1d14ddb6b65fd40fcbea3d866

If I remove it, the tests pass. As far as I can tell, this test is the only place in the code base where CExtPubKey is serialized/deserialized. But I'm not super confident in my ability to fully untangle C++ code, yet.