Page MenuHomePhabricator

[backport#17681] wallet: Keep inactive seeds after sethdseed and derive keys from them as needed
ClosedPublic

Authored by majcosta on Feb 9 2021, 19:35.

Details

Summary

1ed52fbb4d81f7b7634fd4fb6d1d00e1478129dc Remove IBD check in sethdseed (Andrew Chow)
b1810a145a601a8064e4094350cfb6ddafbdb4d8 Test that keys from inactive seeds are generated (Andrew Chow)
c93082ece40b1c72f05b3e2085c022c09eaa4d65 Generate new keys for inactive seeds after marking used (Andrew Chow)
45f2f6a0e8514a0438a87554400bf73cbb90707f Determine inactive HD seeds from key metadata and track them in LegacyScriptPubKeyMan (Andrew Chow)
b59b4504abf96cec860badfed2ac793ae5d40ced have GenerateNewKey and DeriveNewChildKey take a CHDChain as an argument (Andrew Chow)

Pull request description:

Largely implements the suggestion from https://github.com/bitcoin/bitcoin/pull/17484#issuecomment-560845316.

After `sethdseed` is called, the CHDChain for the old seed is kept in the wallet. It is kept on the file as a new `inactivehdseed` record and in memory in a map `m_inactive_hd_seeds`. In `LegacyScriptPubKeyMan::MarkUnusedAddresses` we check each used key's metadata for whether it was derived from an inactive seed. If it is, we then check to see how many keys after that key were derived from the inactive seed. If that number does not match the keypool parameter, we derive more keys from the inactive seed until it does match. This way we won't miss transactions belonging to keys outside of the range of the keypool initially.

The indexes and internal-ness of a key is gotten by checking it's key origin data.

Because of this change, we no longer need to wait for IBD to finish before `sethdseed` can work so that check is also removed.

A test case for this is added as well which fails on master.

Backport of Core PR17681

Test Plan
CC=clang CXX=clang++ cmake .. -GNinja -DENABLE_SANITIZERS=thread
ninja all check check-functional

cmake .. -GNinja -DCMAKE_BUILD_TYPE=Debug
ninja all check check-functional

Event Timeline

majcosta requested review of this revision.Feb 9 2021, 19:35

Tail of the build log:

[330/498] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[331/498] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/pubkey.cpp.o
[332/498] Building CXX object src/test/CMakeFiles/testutil.dir/util/str.cpp.o
[333/498] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/consensus/tx_check.cpp.o
[334/498] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[335/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[336/498] Building CXX object src/CMakeFiles/util.dir/util/time.cpp.o
[337/498] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[338/498] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[339/498] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[340/498] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[341/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[342/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[343/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[344/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[345/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[346/498] Building CXX object src/CMakeFiles/util.dir/util/system.cpp.o
[347/498] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[348/498] Linking CXX static library src/libutil.a
[349/498] Linking CXX static library src/librpcclient.a
[350/498] Linking CXX static library src/libbitcoinconsensus.a
[351/498] Linking CXX static library src/libscript.a
[352/498] Linking CXX static library src/libcommon.a
[353/498] Linking CXX shared library src/libbitcoinconsensus.so.0.23.0
[354/498] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[355/498] Linking CXX executable src/bitcoin-cli
[356/498] Linking CXX executable src/bitcoin-tx
[357/498] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqabstractnotifier.cpp.o
[358/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.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/scriptpubkeyman.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 -Wgnu -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wthread-safety -Wrange-loop-analysis -Wredundant-decls -Wunreachable-code-loop-increment -Wformat-security -Wredundant-move -Wshadow -Wshadow-field -Wno-unused-parameter -Wno-implicit-fallthrough -pthread -std=gnu++17 -MD -MT src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o -c ../../src/wallet/scriptpubkeyman.cpp
/work/abc-ci-builds/build-clang-tidy/../../src/wallet/scriptpubkeyman.cpp:265:30: error: statement should be inside braces [readability-braces-around-statements,-warnings-as-errors]
    if (m_storage.IsLocked()) return false;
                             ^
                              {
3567 warnings generated.
Suppressed 3566 warnings (3566 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
[359/498] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[360/498] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[361/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[362/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[363/498] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqnotificationinterface.cpp.o
[364/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[365/498] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[366/498] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[367/498] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqpublishnotifier.cpp.o
[368/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[369/498] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqrpc.cpp.o
[370/498] Linking CXX static library src/zmq/libzmq.a
[371/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[372/498] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[373/498] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[374/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[375/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[376/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[377/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1

Failed tests logs:

====== Bitcoin ABC functional tests: p2p_timeouts.py ======

------- Stdout: -------
2021-02-09T19:41:58.293000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20210209_194102/p2p_timeouts_322
2021-02-09T19:42:05.436000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 124, in main
    self.run_test()
  File "/work/test/functional/p2p_timeouts.py", line 91, in run_test
    assert not no_send_node.is_connected
  File "/usr/lib/python3.7/contextlib.py", line 119, in __exit__
    next(self.gen)
  File "/work/test/functional/test_framework/test_node.py", line 499, in assert_debug_log
    str(expected_msgs), print_log))
  File "/work/test/functional/test_framework/test_node.py", line 205, in _raise_assertion_error
    raise AssertionError(self._node_msg(msg))
AssertionError: [node 0] Expected messages "['version handshake timeout from 0', 'socket no message in first 3 seconds, 1 0 from 1', 'socket no message in first 3 seconds, 0 0 from 2']" does not partially match log:

 - 


2021-02-09T19:42:05.487000Z TestFramework (INFO): Stopping nodes
2021-02-09T19:42:06.140000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20210209_194102/p2p_timeouts_322
2021-02-09T19:42:06.140000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20210209_194102/p2p_timeouts_322/test_framework.log
2021-02-09T19:42:06.140000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20210209_194102/p2p_timeouts_322' to consolidate all logs

Each failure log is accessible here:
Bitcoin ABC functional tests: p2p_timeouts.py

PiRK requested changes to this revision.Feb 10 2021, 06:51
PiRK added a subscriber: PiRK.
PiRK added inline comments.
src/wallet/scriptpubkeyman.cpp
265 ↗(On Diff #27624)

braces

test/functional/wallet_hd.py
234 ↗(On Diff #27624)

Move comment to its own line (x2)

This revision now requires changes to proceed.Feb 10 2021, 06:51
test/functional/wallet_hd.py
265 ↗(On Diff #27624)

split comment line on two lines

This revision is now accepted and ready to land.Feb 10 2021, 19:02