Page MenuHomePhabricator
Differential D7804

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

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

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rABC5815006534e5: [backport#15024] Allow specific private keys to be derived from descriptor
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

Diff Detail

Event Timeline

majcosta created this revision.Oct 7 2020, 17:33
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 7 2020, 17:33
majcosta requested review of this revision.Oct 7 2020, 17:33
teamcity edited the summary of this revision. (Show Details)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.

Harbormaster completed remote builds in B13245: Diff 24352.Oct 7 2020, 17:36
teamcity added a comment.Oct 7 2020, 17:36
Build Bitcoin ABC Diffs / Diff Testing (build-clang-tidy) failed.

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 a subscriber: PiRK.Oct 13 2020, 12:22
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

PiRK mentioned this in D8175: Add private key derivation functions to descriptors.Oct 29 2020, 10:37
PiRK mentioned this in D8359: Add test for descriptor ranges in wallet_importmulti.py.Nov 11 2020, 05:13
majcosta updated this revision to Diff 25669.Nov 11 2020, 23:18

rebasing

majcosta planned changes to this revision.Nov 11 2020, 23:18
Harbormaster completed remote builds in B13991: Diff 25669.Nov 11 2020, 23:27
teamcity added a comment.Nov 11 2020, 23:27
Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

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

teamcity added a comment.Nov 11 2020, 23:27
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

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

majcosta updated this revision to Diff 25670.EditedNov 12 2020, 00:21

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.

majcosta updated this revision to Diff 25671.Nov 12 2020, 00:29

fix a missing newline

Harbormaster completed remote builds in B13992: Diff 25670.Nov 12 2020, 00:33
Harbormaster completed remote builds in B13993: Diff 25671.Nov 12 2020, 00:41
majcosta planned changes to this revision.Nov 12 2020, 00:46

scratch that

majcosta updated this revision to Diff 25672.Nov 12 2020, 00:53

now it's right

Harbormaster completed remote builds in B13994: Diff 25672.Nov 12 2020, 01:01
majcosta added a comment.Nov 12 2020, 01:27

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

majcosta planned changes to this revision.Nov 12 2020, 14:57

rebasing onto D8359

majcosta updated this revision to Diff 25690.Nov 12 2020, 17:24

rebase, used CashAddr for the addresses

majcosta updated this revision to Diff 25691.Nov 12 2020, 17:32

fix typo

Harbormaster completed remote builds in B14007: Diff 25690.Nov 12 2020, 17:35
Harbormaster completed remote builds in B14008: Diff 25691.Nov 12 2020, 17:42
majcosta edited the test plan for this revision. (Show Details)Nov 12 2020, 18:10
deadalnix accepted this revision.Nov 12 2020, 20:44
This revision is now accepted and ready to land.Nov 12 2020, 20:44
Closed by commit rABC5815006534e5: [backport#15024] Allow specific private keys to be derived from descriptor (authored by Wladimir J. van der Laan <laanwj@gmail.com>, committed by majcosta). · Explain WhyNov 12 2020, 20:50
This revision was automatically updated to reflect the committed changes.
majcosta added a commit: rABC5815006534e5: [backport#15024] Allow specific private keys to be derived from descriptor.

Revision Contents
Changeset List

PathSize
src/
script/
11 lines
61 lines
wallet/
8 lines
test/
functional/
41 lines

Diff 25690

View Options

src/script/descriptor.h

Loading...
View Options

src/script/descriptor.cpp

Loading...
View Options

src/wallet/rpcdump.cpp

Loading...
View Options

test/functional/wallet_importmulti.py

Loading...