[backport#17537] wallet: Cleanup and move opportunistic and superfluous TopUp()s
ClosedPublic
Actions

Authored by majcosta on Oct 1 2020, 17:10.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC34b1f1fca4aa: [backport#17537] wallet: Cleanup and move opportunistic and superfluous TopUp()s

Summary

6e77a7b65cda1b46ce42f0c99ca91562255aeb28 keypool: Add comment about TopUp and when to use it (Andrew Chow)
ea50e34b287e0da0806c1116bb55ade730e8ff6c keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination (Andrew Chow)
bb2c8ce23c9d7ba8d0e5538243e07218443c85b4 keypool: Remove superfluous topup from CWallet::GetNewChangeDestination (Andrew Chow)

Pull request description:

The TopUp() in CWallet::GetNewChangeDestination is unnecessary as currently m_spk_man calls TopUp further down the call stack inside LegacyScriptPubKeyMan::ReserveKeyFromKeyPool (called by LegacyScriptPubKeyMan::GetReservedDestination). This also lets us prepare for future changes with multiple ScriptPubKeyMans in the wallet.
An opportunistic TopUp() is moved from LegacyScriptPubKeyMan::GetNewDestination to CWallet::GetNewDestination.
Another opportunistic TopUp() is moved from LegacyScriptPubKeyMan::ReserveKeyFromKeyPool

Moving opportunistic TopUps ensures that ScriptPubKeyMans will always be topped up before requesting Destinations from them as we cannot always rely on future ScriptPubKeyMan implementaions topping up internally.

See also: https://github.com/bitcoin/bitcoin/pull/17373#discussion_r348598174

Backport of Core PR17537

Test Plan

ninja all check check-functional

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

majcosta created this revision.Oct 1 2020, 17:10

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 1 2020, 17:10

majcosta requested review of this revision.Oct 1 2020, 17:10

[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 B13130: Diff 24158.Oct 1 2020, 17:17

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

Snippet of first build failure:

[319/482] Building CXX object src/CMakeFiles/util.dir/util/error.cpp.o
[320/482] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/util/strencodings.cpp.o
[321/482] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/pubkey.cpp.o
[322/482] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/primitives/transaction.cpp.o
[323/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[324/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[325/482] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[326/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[327/482] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
[328/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/db.cpp.o 
/usr/bin/cmake -E __run_co_compile --launcher=/usr/bin/ccache --tidy="/usr/bin/clang-tidy-8;-warnings-as-errors=*" --source=../../src/wallet/db.cpp -- /usr/bin/clang++  -DBOOST_AC_USE_STD_ATOMIC -DBOOST_SP_USE_STD_ATOMIC -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -isystem /usr/include/jemalloc -g -O2 -fPIC -fvisibility=hidden   -fstack-protector-all -Wstack-protector -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wthread-safety-analysis -Wshadow -Wshadow-field -Wrange-loop-analysis -Wredundant-decls -Wformat-security -Wredundant-move -Wno-unused-parameter -Wno-implicit-fallthrough -pthread -std=gnu++14 -MD -MT src/wallet/CMakeFiles/wallet.dir/db.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/db.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/db.cpp.o -c ../../src/wallet/db.cpp
/work/abc-ci-builds/build-clang-tidy/../../src/wallet/db.cpp:869:30: error: statement should be inside braces [readability-braces-around-statements,-warnings-as-errors]
                if (!fMockDb) dbenv->lsn_reset(strFile.c_str(), 0);
                             ^
                              {
2458 warnings generated.
Suppressed 2457 warnings (2457 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning treated as error
[329/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[330/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[331/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[332/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[333/482] Building CXX object src/CMakeFiles/util.dir/util/system.cpp.o
[334/482] Linking CXX static library src/libutil.a
[335/482] Linking CXX static library src/libscript.a
[336/482] Linking CXX static library src/libcommon.a
[337/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[338/482] Linking CXX static library src/librpcclient.a
[339/482] Linking CXX static library src/libbitcoinconsensus.a
[340/482] Linking CXX shared library src/libbitcoinconsensus.so.0.22.4
[341/482] Linking CXX executable src/bitcoin-cli
[342/482] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[343/482] Linking CXX executable src/bitcoin-tx
[344/482] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqabstractnotifier.cpp.o
[345/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[346/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[347/482] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[348/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[349/482] Building CXX object src/seeder/CMakeFiles/seeder.dir/dns.cpp.o
[350/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[351/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/psbtwallet.cpp.o
[352/482] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqnotificationinterface.cpp.o
[353/482] Building CXX object src/seeder/CMakeFiles/seeder.dir/db.cpp.o
[354/482] Building CXX object src/seeder/CMakeFiles/seeder.dir/bitcoin.cpp.o
[355/482] Linking CXX static library src/seeder/libseeder.a
[356/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[357/482] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqrpc.cpp.o
[358/482] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[359/482] Linking CXX executable src/seeder/bitcoin-seeder
[360/482] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqpublishnotifier.cpp.o
[361/482] Linking CXX static library src/zmq/libzmq.a
[362/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[363/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[364/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[365/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[366/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
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-clang) failed.

Snippet of first build failure:

[348/409] Running utility command for check-bitcoin-settings_tests
[349/409] bitcoin: testing checkdatasig_tests
[350/409] bitcoin: testing timedata_tests
[351/409] bitcoin: testing streams_tests
[352/409] Running utility command for check-bitcoin-checkdatasig_tests
[353/409] Running utility command for check-bitcoin-timedata_tests
[354/409] Running utility command for check-bitcoin-streams_tests
[355/409] bitcoin: testing uint256_tests
[356/409] bitcoin: testing checkqueue_tests
[357/409] Running utility command for check-bitcoin-uint256_tests
[358/409] Running utility command for check-bitcoin-checkqueue_tests
[359/409] bitcoin: testing walletdb_tests
[360/409] bitcoin: testing sigencoding_tests
[361/409] bitcoin: testing serialize_tests
[362/409] Running utility command for check-bitcoin-walletdb_tests
[363/409] Running utility command for check-bitcoin-sigencoding_tests
[364/409] Running utility command for check-bitcoin-serialize_tests
[365/409] bitcoin: testing undo_tests
[366/409] Running utility command for check-bitcoin-undo_tests
[367/409] bitcoin: testing util_threadnames_tests
[368/409] bitcoin: testing radix_tests
[369/409] Running utility command for check-bitcoin-util_threadnames_tests
[370/409] bitcoin: testing script_standard_tests
[371/409] Running utility command for check-bitcoin-radix_tests
[372/409] bitcoin: testing blockcheck_tests
[373/409] Running utility command for check-bitcoin-script_standard_tests
[374/409] Running utility command for check-bitcoin-blockcheck_tests
[375/409] bitcoin: testing crypto_tests
[376/409] Running utility command for check-bitcoin-crypto_tests
[377/409] bitcoin: testing blockstatus_tests
[378/409] bitcoin: testing ismine_tests
[379/409] bitcoin: testing cashaddr_tests
[380/409] Running utility command for check-bitcoin-ismine_tests
[381/409] Running utility command for check-bitcoin-blockstatus_tests
[382/409] Running utility command for check-bitcoin-cashaddr_tests
[383/409] bitcoin: testing versionbits_tests
[384/409] bitcoin: testing script_tests
[385/409] bitcoin: testing getarg_tests
[386/409] Running utility command for check-bitcoin-versionbits_tests
[387/409] Running utility command for check-bitcoin-getarg_tests
[388/409] bitcoin: testing bswap_tests
[389/409] Running utility command for check-bitcoin-script_tests
[390/409] Running utility command for check-bitcoin-bswap_tests
[391/409] bitcoin: testing validation_block_tests
[392/409] Running utility command for check-bitcoin-validation_block_tests
[393/409] bitcoin: testing validation_tests
[394/409] Running utility command for check-bitcoin-validation_tests
[395/409] bitcoin: testing skiplist_tests
[396/409] Running utility command for check-bitcoin-skiplist_tests
[397/409] bitcoin: testing util_tests
[398/409] Running utility command for check-bitcoin-util_tests
[399/409] bitcoin: testing cuckoocache_tests
[400/409] Running utility command for check-bitcoin-cuckoocache_tests
[401/409] bitcoin: testing op_reversebytes_tests
[402/409] Running utility command for check-bitcoin-op_reversebytes_tests
[403/409] bitcoin: testing coins_tests
[404/409] Running utility command for check-bitcoin-coins_tests
[405/409] bitcoin: testing transaction_tests
[406/409] Running utility command for check-bitcoin-transaction_tests
Build build-clang timed out after 1200.0s

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

Snippet of first build failure:

[0m[0;32mrpc_getblockfilter.py                   | ✓ Passed  | 1 s
[0m[0;32mrpc_getblockstats.py                    | ✓ Passed  | 1 s
[0m[0;32mrpc_getchaintips.py                     | ✓ Passed  | 3 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  | 22 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  | 15 s
[0m[0;32mwallet_address_types.py                 | ✓ Passed  | 13 s
[0m[0;32mwallet_avoidreuse.py                    | ✓ Passed  | 4 s
[0m[0;32mwallet_backup.py                        | ✓ Passed  | 48 s
[0m[0;32mwallet_balance.py                       | ✓ Passed  | 16 s
[0m[0;32mwallet_basic.py                         | ✓ Passed  | 21 s
[0m[0;32mwallet_coinbase_category.py             | ✓ Passed  | 1 s
[0m[0;32mwallet_create_tx.py                     | ✓ Passed  | 10 s
[0m[0;32mwallet_createwallet.py                  | ✓ Passed  | 2 s
[0m[0;32mwallet_createwallet.py --usecli         | ✓ Passed  | 3 s
[0m[0;32mwallet_disable.py                       | ✓ Passed  | 0 s
[0m[0;32mwallet_dump.py                          | ✓ Passed  | 2 s
[0m[0;32mwallet_encryption.py                    | ✓ Passed  | 5 s
[0m[0;32mwallet_groups.py                        | ✓ Passed  | 8 s
[0m[0;32mwallet_hd.py                            | ✓ Passed  | 5 s
[0m[0;32mwallet_import_rescan.py                 | ✓ Passed  | 3 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  | 2 s
[0m[0;32mwallet_listreceivedby.py                | ✓ Passed  | 10 s
[0m[0;32mwallet_listsinceblock.py                | ✓ Passed  | 3 s
[0m[0;32mwallet_listtransactions.py              | ✓ Passed  | 9 s
[0m[0;32mwallet_multiwallet.py                   | ✓ Passed  | 12 s
[0m[0;32mwallet_multiwallet.py --usecli          | ✓ Passed  | 14 s
[0m[0;32mwallet_reorgsrestore.py                 | ✓ Passed  | 3 s
[0m[0;32mwallet_resendwallettransactions.py      | ✓ Passed  | 17 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  | 2 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  | 693 s (accumulated) 
[0mRuntime: 139 s

Build build-diff timed out after 1200.0s

Fabien accepted this revision.Oct 1 2020, 18:57

This revision is now accepted and ready to land.Oct 1 2020, 18:57

This revision was landed with ongoing or failed builds.Oct 1 2020, 20:29

Closed by commit rABC34b1f1fca4aa: [backport#17537] wallet: Cleanup and move opportunistic and superfluous TopUp()s (authored by fanquake <fanquake@gmail.com>, committed by majcosta). · Explain Why

This revision was automatically updated to reflect the committed changes.

majcosta added a commit: rABC34b1f1fca4aa: [backport#17537] wallet: Cleanup and move opportunistic and superfluous TopUp()s.