Page MenuHomePhabricator

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

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

Details

Reviewers
deadalnix
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

Event Timeline

Owners added a reviewer: Restricted Owners Package.Sep 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.

Snippet of first build failure:

wallet_createwallet.py                  | ○ Skipped | 0 s
wallet_createwallet.py --usecli         | ○ Skipped | 0 s
wallet_dump.py                          | ○ Skipped | 0 s
wallet_encryption.py                    | ○ Skipped | 0 s
wallet_groups.py                        | ○ Skipped | 0 s
wallet_hd.py                            | ○ Skipped | 0 s
wallet_import_rescan.py                 | ○ Skipped | 0 s
wallet_import_with_label.py             | ○ Skipped | 0 s
wallet_importmulti.py                   | ○ Skipped | 0 s
wallet_importprunedfunds.py             | ○ Skipped | 0 s
wallet_keypool.py                       | ○ Skipped | 0 s
wallet_keypool_topup.py                 | ○ Skipped | 0 s
wallet_labels.py                        | ○ Skipped | 0 s
wallet_listreceivedby.py                | ○ Skipped | 0 s
wallet_listsinceblock.py                | ○ Skipped | 0 s
wallet_listtransactions.py              | ○ Skipped | 0 s
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  | 259 s (accumulated) 
Runtime: 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

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

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

Snippet of first build failure:

rpc_getblockstats.py                    | ✓ Passed  | 1 s
rpc_getchaintips.py                     | ✓ Passed  | 2 s
rpc_help.py                             | ✓ Passed  | 1 s
rpc_invalidateblock.py                  | ✓ Passed  | 6 s
rpc_misc.py                             | ✓ Passed  | 1 s
rpc_named_arguments.py                  | ✓ Passed  | 1 s
rpc_net.py                              | ✓ Passed  | 1 s
rpc_preciousblock.py                    | ✓ Passed  | 1 s
rpc_psbt.py                             | ✓ Passed  | 20 s
rpc_scantxoutset.py                     | ✓ Passed  | 4 s
rpc_setban.py                           | ✓ Passed  | 2 s
rpc_signmessage.py                      | ✓ Passed  | 1 s
rpc_signrawtransaction.py               | ✓ Passed  | 1 s
rpc_txoutproof.py                       | ✓ Passed  | 2 s
rpc_uptime.py                           | ✓ Passed  | 1 s
rpc_users.py                            | ✓ Passed  | 2 s
rpc_whitelist.py                        | ✓ Passed  | 1 s
tool_wallet.py                          | ✓ Passed  | 3 s
wallet_abandonconflict.py               | ✓ Passed  | 10 s
wallet_address_types.py                 | ✓ Passed  | 15 s
wallet_avoidreuse.py                    | ✓ Passed  | 3 s
wallet_backup.py                        | ✓ Passed  | 39 s
wallet_balance.py                       | ✓ Passed  | 6 s
wallet_basic.py                         | ✓ Passed  | 23 s
wallet_coinbase_category.py             | ✓ Passed  | 1 s
wallet_create_tx.py                     | ✓ Passed  | 5 s
wallet_createwallet.py                  | ✓ Passed  | 2 s
wallet_createwallet.py --usecli         | ✓ Passed  | 3 s
wallet_disable.py                       | ✓ Passed  | 1 s
wallet_dump.py                          | ✓ Passed  | 2 s
wallet_encryption.py                    | ✓ Passed  | 5 s
wallet_groups.py                        | ✓ Passed  | 12 s
wallet_hd.py                            | ✓ Passed  | 5 s
wallet_import_rescan.py                 | ✓ Passed  | 4 s
wallet_import_with_label.py             | ✓ Passed  | 1 s
wallet_importmulti.py                   | ✓ Passed  | 3 s
wallet_importprunedfunds.py             | ✓ Passed  | 2 s
wallet_keypool.py                       | ✓ Passed  | 3 s
wallet_keypool_topup.py                 | ✓ Passed  | 3 s
wallet_labels.py                        | ✓ Passed  | 1 s
wallet_listreceivedby.py                | ✓ Passed  | 12 s
wallet_listsinceblock.py                | ✓ Passed  | 3 s
wallet_listtransactions.py              | ✓ Passed  | 8 s
wallet_multiwallet.py                   | ✓ Passed  | 12 s
wallet_multiwallet.py --usecli          | ✓ Passed  | 13 s
wallet_reorgsrestore.py                 | ✓ Passed  | 3 s
wallet_resendwallettransactions.py      | ✓ Passed  | 7 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  | 3 s

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

deadalnix requested changes to this revision.Sep 24 2020, 16:46
deadalnix added a subscriber: deadalnix.

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.

remove one more test case testing CExt{Pub,}Key (de)serialization methods (otherwise unused)

This revision is now accepted and ready to land.Sep 25 2020, 13:41