Page MenuHomePhabricator

[backport#15024] Allow specific private keys to be derived from descriptor
ClosedPublic

Authored by majcosta on Oct 7 2020, 17:33.

Details

Summary

53b7de629d3d9281dc6f8fa10e32c4cdec59c140 Add test for dumping the private key imported from descriptor (MeshCollider)
2857bc4a64cc8dc7914bc984ac878397ac64292d Extend importmulti descriptor tests (MeshCollider)
81a884bbd0dbee108d11776794d9627ca07504aa Import private keys from descriptor with importmulti if provided (MeshCollider)
a4d1bd1a29be2dcc5e00c63b6b41916b1c466de0 Add private key derivation functions to descriptors (MeshCollider)

Pull request description:

~This is based on #14491, review the last 3 commits only.~

Currently, descriptors have an Expand() function which returns public keys and scripts for a specific index of a ranged descriptor. But the private key for a specific index is not given. This allows private keys for specific indices to be derived. This also allows those keys to be imported through the `importmulti` RPC rather than having to provide them separately.

Backport of Core PR15024

Test Plan
ninja all check check-functional

afaict, either this functionality is unavailable for legacy addresses or Core didn't add tests aside from sh(wpkh()) descriptors

Event Timeline

majcosta requested review of this revision.Oct 7 2020, 17:33

[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:

[300/482] Building CXX object src/CMakeFiles/common.dir/protocol.cpp.o
[301/482] Building CXX object src/CMakeFiles/common.dir/outputtype.cpp.o
[302/482] Building CXX object src/CMakeFiles/common.dir/policy/policy.cpp.o
[303/482] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/arith_uint256.cpp.o
[304/482] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/primitives/transaction.cpp.o
[305/482] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/hash.cpp.o
[306/482] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/uint256.cpp.o
[307/482] Building CXX object src/CMakeFiles/bitcoin-cli.dir/bitcoin-cli.cpp.o
[308/482] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/util/strencodings.cpp.o
[309/482] Building C object src/secp256k1/CMakeFiles/sign-bench.dir/src/bench_sign.c.o
[310/482] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/pubkey.cpp.o
[311/482] Building C object src/secp256k1/CMakeFiles/verify-bench.dir/src/bench_verify.c.o
[312/482] Building C object src/secp256k1/CMakeFiles/recover-bench.dir/src/bench_recover.c.o
[313/482] Building C object src/secp256k1/CMakeFiles/internal-bench.dir/src/bench_internal.c.o
[314/482] Building C object src/secp256k1/CMakeFiles/ecmult-bench.dir/src/bench_ecmult.c.o
[315/482] Building CXX object src/CMakeFiles/common.dir/rpc/rawtransaction_util.cpp.o
[316/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[317/482] Building CXX object src/CMakeFiles/common.dir/rpc/util.cpp.o
[318/482] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/consensus/tx_check.cpp.o
[319/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/str.cpp.o
[320/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[321/482] Building CXX object src/CMakeFiles/script.dir/script/standard.cpp.o
[322/482] Building CXX object src/CMakeFiles/common.dir/psbt.cpp.o
[323/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[324/482] Building CXX object src/CMakeFiles/script.dir/script/sign.cpp.o
[325/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[326/482] Building C object src/secp256k1/CMakeFiles/secp256k1.dir/src/secp256k1.c.o
[327/482] Linking C static library src/secp256k1/libsecp256k1.a
[328/482] Linking C executable src/secp256k1/ecmult-bench
[329/482] Linking C executable src/secp256k1/internal-bench
[330/482] Building CXX object src/CMakeFiles/bitcoin-wallet.dir/bitcoin-wallet.cpp.o
[331/482] Linking C executable src/secp256k1/sign-bench
[332/482] Linking C executable src/secp256k1/verify-bench
[333/482] Linking C executable src/secp256k1/recover-bench
[334/482] Building CXX object src/CMakeFiles/script.dir/script/signingprovider.cpp.o
[335/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[336/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[337/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[338/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[339/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[340/482] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[341/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[342/482] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
[343/482] Building CXX object src/seeder/CMakeFiles/seeder.dir/dns.cpp.o
[344/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[345/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/psbtwallet.cpp.o
[346/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[347/482] Building CXX object src/seeder/CMakeFiles/seeder.dir/bitcoin.cpp.o
[348/482] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[349/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[350/482] Building CXX object src/seeder/CMakeFiles/seeder.dir/db.cpp.o
[351/482] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[352/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[353/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[354/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[355/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[356/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[357/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
deadalnix requested changes to this revision.Oct 7 2020, 18:32
deadalnix added a subscriber: deadalnix.

You also need to adapt the integration test from the PR to BCH.

src/script/descriptor.cpp
404 ↗(On Diff #24352)

braces

614 ↗(On Diff #24352)

dito

src/script/descriptor.h
84 ↗(On Diff #24352)

layout

This revision now requires changes to proceed.Oct 7 2020, 18:32
PiRK added inline comments.
src/script/descriptor.cpp
617 ↗(On Diff #24352)

Note that PR16609 fixes something here, related to changes made in PR14934 (D7806)

m_script_arg -> m_subdescriptor_arg

Failed tests logs:

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

------- Stdout: -------
2020-11-11T23:26:20.992000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20201111_232329/wallet_importmulti_592
2020-11-11T23:26:21.260000Z TestFramework (INFO): Mining blocks...
2020-11-11T23:26:21.273000Z TestFramework (INFO): Should import an address
2020-11-11T23:26:21.289000Z TestFramework (INFO): Should not import an invalid address
2020-11-11T23:26:21.289000Z TestFramework (INFO): Should import a scriptPubKey with internal flag
2020-11-11T23:26:21.299000Z TestFramework (INFO): Should not allow a label to be specified when internal is true
2020-11-11T23:26:21.307000Z TestFramework (INFO): Should not import a nonstandard scriptPubKey without internal flag
2020-11-11T23:26:21.315000Z TestFramework (INFO): Should import an address with public key
2020-11-11T23:26:21.328000Z TestFramework (INFO): Should import a scriptPubKey with internal and with public key
2020-11-11T23:26:21.340000Z TestFramework (INFO): Should not import a nonstandard scriptPubKey without internal and with public key
2020-11-11T23:26:21.350000Z TestFramework (INFO): Should import an address with private key
2020-11-11T23:26:21.362000Z TestFramework (INFO): Should not import an address with private key if is already imported
2020-11-11T23:26:21.363000Z TestFramework (INFO): Should import an address with private key and with watchonly
2020-11-11T23:26:21.376000Z TestFramework (INFO): Should import a scriptPubKey with internal and with private key
2020-11-11T23:26:21.387000Z TestFramework (INFO): Should not import a nonstandard scriptPubKey without internal and with private key
2020-11-11T23:26:21.568000Z TestFramework (INFO): Should import a p2sh
2020-11-11T23:26:21.864000Z TestFramework (INFO): Should import a p2sh with respective redeem script
2020-11-11T23:26:22.309000Z TestFramework (INFO): Should import a p2sh with respective redeem script and private keys
2020-11-11T23:26:22.927000Z TestFramework (INFO): Should import a p2sh with respective redeem script and private keys
2020-11-11T23:26:23.010000Z TestFramework (INFO): Should not import an address with the wrong public key as non-solvable
2020-11-11T23:26:23.103000Z TestFramework (INFO): Should import a scriptPubKey with internal and with a wrong public key as non-solvable
2020-11-11T23:26:23.198000Z TestFramework (INFO): Should import an address with a wrong private key as non-solvable
2020-11-11T23:26:23.290000Z TestFramework (INFO): Should import a scriptPubKey with internal and with a wrong private key as non-solvable
2020-11-11T23:26:23.381000Z TestFramework (INFO): Should replace previously saved watch only timestamp.
2020-11-11T23:26:23.985000Z TestFramework (INFO): Should throw on invalid or missing timestamp values
2020-11-11T23:26:23.996000Z TestFramework (INFO): Should fail to import a p2pkh address from descriptor with no checksum
2020-11-11T23:26:24.036000Z TestFramework (INFO): Should import a descriptor with a WIF private key as spendable
2020-11-11T23:26:24.161000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 120, in main
    self.run_test()
  File "/work/test/functional/wallet_importmulti.py", line 519, in run_test
    privkey = self.nodes[1].dumpprivkey(address)
  File "/work/test/functional/test_framework/coverage.py", line 48, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/work/test/functional/test_framework/authproxy.py", line 159, in __call__
    raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Address does not refer to a key (-3)
2020-11-11T23:26:24.212000Z TestFramework (INFO): Stopping nodes
2020-11-11T23:26:24.465000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20201111_232329/wallet_importmulti_592
2020-11-11T23:26:24.465000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20201111_232329/wallet_importmulti_592/test_framework.log
2020-11-11T23:26:24.465000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20201111_232329/wallet_importmulti_592' to consolidate all logs

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

Failed tests logs:

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

------- Stdout: -------
2020-11-11T23:25:19.187000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20201111_232304/wallet_importmulti_133
2020-11-11T23:25:19.452000Z TestFramework (INFO): Mining blocks...
2020-11-11T23:25:19.460000Z TestFramework (INFO): Should import an address
2020-11-11T23:25:19.470000Z TestFramework (INFO): Should not import an invalid address
2020-11-11T23:25:19.471000Z TestFramework (INFO): Should import a scriptPubKey with internal flag
2020-11-11T23:25:19.480000Z TestFramework (INFO): Should not allow a label to be specified when internal is true
2020-11-11T23:25:19.487000Z TestFramework (INFO): Should not import a nonstandard scriptPubKey without internal flag
2020-11-11T23:25:19.495000Z TestFramework (INFO): Should import an address with public key
2020-11-11T23:25:19.508000Z TestFramework (INFO): Should import a scriptPubKey with internal and with public key
2020-11-11T23:25:19.520000Z TestFramework (INFO): Should not import a nonstandard scriptPubKey without internal and with public key
2020-11-11T23:25:19.527000Z TestFramework (INFO): Should import an address with private key
2020-11-11T23:25:19.536000Z TestFramework (INFO): Should not import an address with private key if is already imported
2020-11-11T23:25:19.536000Z TestFramework (INFO): Should import an address with private key and with watchonly
2020-11-11T23:25:19.546000Z TestFramework (INFO): Should import a scriptPubKey with internal and with private key
2020-11-11T23:25:19.554000Z TestFramework (INFO): Should not import a nonstandard scriptPubKey without internal and with private key
2020-11-11T23:25:19.615000Z TestFramework (INFO): Should import a p2sh
2020-11-11T23:25:19.711000Z TestFramework (INFO): Should import a p2sh with respective redeem script
2020-11-11T23:25:19.844000Z TestFramework (INFO): Should import a p2sh with respective redeem script and private keys
2020-11-11T23:25:20.026000Z TestFramework (INFO): Should import a p2sh with respective redeem script and private keys
2020-11-11T23:25:20.058000Z TestFramework (INFO): Should not import an address with the wrong public key as non-solvable
2020-11-11T23:25:20.095000Z TestFramework (INFO): Should import a scriptPubKey with internal and with a wrong public key as non-solvable
2020-11-11T23:25:20.136000Z TestFramework (INFO): Should import an address with a wrong private key as non-solvable
2020-11-11T23:25:20.184000Z TestFramework (INFO): Should import a scriptPubKey with internal and with a wrong private key as non-solvable
2020-11-11T23:25:20.222000Z TestFramework (INFO): Should replace previously saved watch only timestamp.
2020-11-11T23:25:20.760000Z TestFramework (INFO): Should throw on invalid or missing timestamp values
2020-11-11T23:25:20.765000Z TestFramework (INFO): Should fail to import a p2pkh address from descriptor with no checksum
2020-11-11T23:25:20.770000Z TestFramework (INFO): Should import a descriptor with a WIF private key as spendable
2020-11-11T23:25:20.800000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 120, in main
    self.run_test()
  File "/work/test/functional/wallet_importmulti.py", line 519, in run_test
    privkey = self.nodes[1].dumpprivkey(address)
  File "/work/test/functional/test_framework/coverage.py", line 48, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/work/test/functional/test_framework/authproxy.py", line 159, in __call__
    raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Address does not refer to a key (-3)
2020-11-11T23:25:20.850000Z TestFramework (INFO): Stopping nodes
2020-11-11T23:25:21.002000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20201111_232304/wallet_importmulti_133
2020-11-11T23:25:21.002000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20201111_232304/wallet_importmulti_133/test_framework.log
2020-11-11T23:25:21.002000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20201111_232304/wallet_importmulti_133' to consolidate all logs

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

turns out importmulti'ing a sh(pkh( does not work on core either, so I changed the test to import a pkh( descriptor with private keys

interestingly, the sh(wpkh( test case in the original PR does not work on a build of current Bitcoin Core, importing as watchonly as well.

scratch that

Note: our test is slightly different than Core's in the sense that the address that is passed to dumpprivkey in their version is the 'outer' address of the sh(wpkh(_)) and in ours we pass the embedded address in sh(pkh(_))

Tested both cases with a single WIF key on Core (since we don't implement segwit) and dumpprivkey returns the same result for both

rebase, used CashAddr for the addresses

This revision is now accepted and ready to land.Nov 12 2020, 20:44