External signer support - Wallet Box edition

Sjors commented at 9:42 pm on August 4, 2019: member

Big picture overview in this gist.

This PR lets bitcoind call an arbitrary command -signer=<cmd>, e.g. a hardware wallet driver, where it can fetch public keys, ask to display an address, and sign a transaction (using PSBT under the hood).

It’s design to work with https://github.com/bitcoin-core/HWI, which supports multiple hardware wallets. Any command with the same arguments and return values will work. It simplifies the manual procedure described here.

Usage is documented in doc/external-signer.md, which also describes what protocol a different signer binary should conform to.

Use --enable-external-signer to opt in, requires Boost::Process:

0Options used to compile and link:
1  with wallet     = yes
2  with gui / qt   = no
3  external signer = yes

It adds the following RPC methods:

enumeratesigners: asks for a list of signers (e.g. devices) and their master key fingerprint
signerdisplayaddress <address>: asks to display an address

It enhances the following RPC methods:

createwallet: takes an additional external_signer argument and fetches keys from device
send: automatically sends transaction to device and waits

Usage TL&DR:

clone HWI repo somewhere and launch bitcoind -signer=../HWI/hwi.py
check if you can see your hardware device: bitcoin-cli enumeratesigners
create wallet and auto import keys bitcoin-cli createwallet "hww" true true "" true true true
display address on device: bitcoin-cli signerdisplayaddress ...
to spend, use send RPC and approve transaction on device

Prerequisites:

#21127 load wallet flags before everything else
#21182 remove mostly pointless BOOST_PROCESS macro

Potentially useful followups:

GUI support: bitcoin-core/gui#4
bumpfee support
(automatically) verify (a subset of) keys on the device after import, through message signing

DrahtBot commented at 10:16 pm on August 4, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21207 (MOVEONLY: CWallet transaction code out of wallet.cpp/.h by ryanofsky)
#21073 (wallet: check when create wallets for the reserved name “wallets” by Saibato)
#20744 ([POC] Use std::filesystem. Remove Boost Filesystem & System by fanquake)
#20354 (test: Add feature_taproot.py –previous_release by MarcoFalke)
#20191 (wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag by S3RK)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Build system on Aug 4, 2019

DrahtBot added the label Docs on Aug 4, 2019

DrahtBot added the label GUI on Aug 4, 2019

DrahtBot added the label RPC/REST/ZMQ on Aug 4, 2019

DrahtBot added the label Tests on Aug 4, 2019

DrahtBot added the label Utils/log/libs on Aug 4, 2019

DrahtBot added the label Wallet on Aug 4, 2019

in src/wallet/rpcwallet.cpp:2729 in 10b713dc90 outdated

2672@@ -2653,6 +2673,8 @@ static UniValue createwallet(const JSONRPCRequest& request)
2673             {"blank", RPCArg::Type::BOOL, /* default */ "false", "Create a blank wallet. A blank wallet has no keys or HD seed. One can be set using sethdseed."},
2674             {"passphrase", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Encrypt the wallet with this passphrase."},
2675             {"avoid_reuse", RPCArg::Type::BOOL, /* default */ "false", "Keep track of coin reuse, and treat dirty and clean coins differently with privacy considerations in mind."},
2676+            {"descriptors", RPCArg::Type::BOOL, /* default */ "false", "Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation"},
2677+            {"external_signer", RPCArg::Type::BOOL, /* default */ "false", "Use an external signer such as a hardware wallet. Requires -signer to be configured. Wallet creation will fail if keys cannot be fetched. Requires disable_private_keys and descriptors set to true."},

luke-jr commented at 11:24 pm on August 4, 2019:

This list-of-bools thing seems like a terrible idea…

Sjors commented at 9:34 am on August 5, 2019:

Indeed, I was complaining about that when blank was added :-)

in src/wallet/rpcwallet.cpp:4364 in 10b713dc90 outdated

4337+        throw JSONRPCTransactionError(err);
4338+    }
4339+
4340+    if (pwallet->IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) {
4341+        const std::string command = gArgs.GetArg("-signer", DEFAULT_EXTERNAL_SIGNER);
4342+        if (command == "") throw std::runtime_error(std::string(__func__) + ": restart bitcoind with -signer=<cmd>");

luke-jr commented at 11:25 pm on August 4, 2019:

What if there are multiple wallets, with different signers?

IMO -signer needs to be replaced with either a wallet-stored path, or a path provided when the wallet is loaded…

Sjors commented at 9:35 am on August 5, 2019:

Storing signer in the wallet makes sense, but I also think it can wait for a later PR. In practice afaik the only tool that currently works is HWI and it can handle multiple wallets. In the long run however I do hope that wallet manufactures provide their own software that just uses the same commands / responses.

fanquake removed the label Build system on Aug 4, 2019

fanquake removed the label Docs on Aug 4, 2019

fanquake removed the label GUI on Aug 4, 2019

fanquake removed the label Utils/log/libs on Aug 4, 2019

Sjors force-pushed on Aug 5, 2019

DrahtBot added the label Needs rebase on Aug 6, 2019

Sjors force-pushed on Sep 4, 2019

DrahtBot removed the label Needs rebase on Sep 4, 2019

Sjors force-pushed on Sep 4, 2019

DrahtBot added the label Needs rebase on Sep 7, 2019

Relaxo143 commented at 11:45 am on September 12, 2019: none

Can we expect this to be merged and included in 0.19? It’s a really useful and requested feature! Thanks to everyone who is working on it.

Sjors commented at 8:54 am on September 16, 2019: member

@Relaxo143 not a chance; this is still work in progress and there’s several pull requests that need to be reviewed and merged first. There’s also a feature freeze on 0.19.

Sjors force-pushed on Sep 16, 2019

Sjors commented at 4:27 pm on September 16, 2019: member

I dropped the dependency on my new send RPC proposal #16378, in favor of just tweaking sendmany and sendtoaddress. It’s less powerful, but should reduce the review burden once native descriptors are merged.

Sjors force-pushed on Sep 18, 2019

Sjors force-pushed on Oct 10, 2019

Sjors force-pushed on Oct 25, 2019

DrahtBot removed the label Needs rebase on Oct 25, 2019

Sjors force-pushed on Oct 27, 2019

DrahtBot added the label Needs rebase on Oct 29, 2019

Sjors force-pushed on Oct 29, 2019

DrahtBot removed the label Needs rebase on Oct 29, 2019

Sjors force-pushed on Oct 29, 2019

DrahtBot added the label Needs rebase on Oct 30, 2019

Sjors force-pushed on Oct 30, 2019

Sjors force-pushed on Oct 31, 2019

DrahtBot removed the label Needs rebase on Oct 31, 2019

Sjors force-pushed on Oct 31, 2019

DrahtBot added the label Needs rebase on Oct 31, 2019

Sjors force-pushed on Nov 3, 2019

Sjors force-pushed on Nov 7, 2019

DrahtBot removed the label Needs rebase on Nov 7, 2019

DrahtBot added the label Needs rebase on Nov 8, 2019

Sjors force-pushed on Dec 9, 2019

DrahtBot removed the label Needs rebase on Dec 9, 2019

DrahtBot added the label Needs rebase on Dec 16, 2019

Sjors force-pushed on Jan 30, 2020

DrahtBot removed the label Needs rebase on Jan 30, 2020

Sjors force-pushed on Jan 30, 2020

DrahtBot added the label Needs rebase on Feb 4, 2020

in build-aux/m4/ax_boost_process.m4:17 in 0a0d295415 outdated

12+#   requires a preceding call to AX_BOOST_BASE. Further documentation is
13+#   available at <http://randspringer.de/boost/index.html>.
14+#
15+#   This macro calls:
16+#
17+#     AC_SUBST(BOOST_PROCESS_LIB)

luke-jr commented at 5:36 am on February 6, 2020:

I don’t see where we are linking this

in configure.ac:1204 in 0a0d295415 outdated

1080@@ -1075,6 +1081,7 @@ AX_BOOST_SYSTEM
1081 AX_BOOST_FILESYSTEM
1082 AX_BOOST_THREAD
1083 AX_BOOST_CHRONO
1084+AX_BOOST_PROCESS

luke-jr commented at 5:44 am on February 6, 2020:

This seems like it will pull in Boost.Process (and our - in this case dead - wrapper code using it) even with --disable-external-signer.

Maybe not a problem since we plan to use it for other things too?

luke-jr changes_requested

Sjors force-pushed on Feb 21, 2020

Sjors commented at 3:33 pm on February 21, 2020: member

Rebase and big update:

I dropped #15590 (GetAddressType() & IsSegWit()) in favor of GetOutputType()
this PR introduces ExternalSignerScriptPubKeyMan as a subclass of DescriptorScriptPubKeyMan

DrahtBot removed the label Needs rebase on Feb 21, 2020

Sjors force-pushed on Feb 21, 2020

DrahtBot added the label Needs rebase on Feb 25, 2020

Sjors force-pushed on Feb 25, 2020

DrahtBot removed the label Needs rebase on Feb 25, 2020

DrahtBot added the label Needs rebase on Mar 9, 2020

Sjors force-pushed on Mar 23, 2020

Sjors commented at 7:50 pm on March 23, 2020: member

Rebased on the latest #15764.

DrahtBot removed the label Needs rebase on Mar 23, 2020

DrahtBot added the label Needs rebase on Mar 23, 2020

Sjors force-pushed on Mar 27, 2020

DrahtBot removed the label Needs rebase on Mar 27, 2020

Sjors force-pushed on Apr 1, 2020

DrahtBot added the label Needs rebase on Apr 6, 2020

Sjors force-pushed on Apr 22, 2020

DrahtBot removed the label Needs rebase on Apr 22, 2020

Sjors force-pushed on Apr 23, 2020

Sjors force-pushed on Apr 27, 2020

Sjors renamed this:
~~[WIP] External signer support - Wallet Box edition~~
External signer support - Wallet Box edition
on Apr 27, 2020

Sjors commented at 2:08 pm on April 27, 2020: member

I’m marking this ready for review, even though 372eef4 is a placeholder commit (to be fixed in other pull requests). There’s still rough edges, but it does work, as does the GUI followup in #16549.

Sjors force-pushed on Apr 27, 2020

fjahr commented at 2:02 pm on April 28, 2020: member

Concept ACK

DrahtBot added the label Needs rebase on May 1, 2020

Sjors force-pushed on May 7, 2020

DrahtBot removed the label Needs rebase on May 7, 2020

DrahtBot added the label Needs rebase on May 11, 2020

Sjors force-pushed on May 18, 2020

DrahtBot removed the label Needs rebase on May 18, 2020

Sjors force-pushed on May 18, 2020

DrahtBot added the label Needs rebase on May 21, 2020

Sjors force-pushed on May 22, 2020

DrahtBot removed the label Needs rebase on May 22, 2020

DrahtBot added the label Needs rebase on May 27, 2020

Sjors force-pushed on Jun 10, 2020

DrahtBot removed the label Needs rebase on Jun 10, 2020

Sjors force-pushed on Jun 16, 2020

Sjors force-pushed on Jun 18, 2020

Sjors closed this on Jun 18, 2020

Sjors reopened this on Jun 18, 2020

Sjors force-pushed on Jun 19, 2020

DrahtBot added the label Needs rebase on Jun 19, 2020

Sjors force-pushed on Jun 19, 2020

DrahtBot removed the label Needs rebase on Jun 19, 2020

meshcollider added this to the "PRs" column in a project

DrahtBot added the label Needs rebase on Jun 24, 2020

Sjors force-pushed on Jun 25, 2020

DrahtBot removed the label Needs rebase on Jun 25, 2020

DrahtBot added the label Needs rebase on Jun 29, 2020

Sjors force-pushed on Jun 29, 2020

DrahtBot removed the label Needs rebase on Jun 29, 2020

DrahtBot added the label Needs rebase on Jul 2, 2020

Sjors force-pushed on Jul 2, 2020

DrahtBot removed the label Needs rebase on Jul 2, 2020

Sjors force-pushed on Jul 6, 2020

DrahtBot added the label Needs rebase on Jul 11, 2020

Sjors force-pushed on Jul 14, 2020

DrahtBot removed the label Needs rebase on Jul 14, 2020

DrahtBot added the label Needs rebase on Jul 17, 2020

Sjors force-pushed on Jul 18, 2020

DrahtBot removed the label Needs rebase on Jul 18, 2020

DrahtBot added the label Needs rebase on Jul 28, 2020

Sjors force-pushed on Jul 30, 2020

DrahtBot removed the label Needs rebase on Jul 30, 2020

DrahtBot added the label Needs rebase on Jul 30, 2020

Sjors force-pushed on Jul 31, 2020

DrahtBot removed the label Needs rebase on Jul 31, 2020

meshcollider referenced this in commit e4df534c60 on Aug 5, 2020

sidhujag referenced this in commit 06bceb01d7 on Aug 5, 2020

Sjors force-pushed on Aug 6, 2020

DrahtBot added the label Needs rebase on Aug 9, 2020

Sjors force-pushed on Aug 28, 2020

Sjors commented at 6:43 pm on August 28, 2020: member

Rebased and dropped 3c65029b2cd7dc4874055cc55424bb7512071d2b since it’s unused.

DrahtBot removed the label Needs rebase on Aug 28, 2020

DrahtBot added the label Needs rebase on Sep 7, 2020

Sjors force-pushed on Sep 7, 2020

DrahtBot removed the label Needs rebase on Sep 7, 2020

meshcollider referenced this in commit ffaac6e614 on Sep 15, 2020

sidhujag referenced this in commit 2d9d86f6b8 on Sep 15, 2020

DrahtBot added the label Needs rebase on Sep 15, 2020

Sjors force-pushed on Sep 15, 2020

Sjors commented at 2:44 pm on September 15, 2020: member

Rebased on top of the ultimate send RPC #16378; it now uses send instead of sendtoaddress.

DrahtBot removed the label Needs rebase on Sep 15, 2020

Sjors commented at 9:55 am on September 16, 2020: member

Something is causing Travis on ARM64 to bail out right before the functional tests. I already tried a restart.

laanwj added this to the "Blockers" column in a project

DrahtBot added the label Needs rebase on Sep 29, 2020

Sjors force-pushed on Oct 1, 2020

DrahtBot removed the label Needs rebase on Oct 1, 2020

DrahtBot added the label Needs rebase on Oct 7, 2020

Sjors force-pushed on Oct 8, 2020

DrahtBot removed the label Needs rebase on Oct 8, 2020

jojoe30 approved

Sjors force-pushed on Oct 15, 2020

Sjors commented at 9:35 am on October 15, 2020: member

Rebased on Sqlite support #18916

in src/wallet/externalsigner.h:20 in fa453656d0 outdated

15+class ExternalSignerException : public std::runtime_error {
16+public:
17+    using std::runtime_error::runtime_error;
18+};
19+
20+//! Enables interaction with an external signing device or service, such as a

kiminuo commented at 9:13 am on October 20, 2020:

0//! Enables interaction with an external signing device or service, such as

in src/wallet/rpcsigner.h:23 in fa453656d0 outdated

18+class Handler;
19+}
20+
21+Span<const CRPCCommand> GetSignerRPCCommands();
22+
23+#endif // BOOST_VERSION

kiminuo commented at 9:19 am on October 20, 2020:

0#endif // ENABLE_EXTERNAL_SIGNER

DrahtBot added the label Needs rebase on Oct 29, 2020

Sjors force-pushed on Oct 30, 2020

DrahtBot removed the label Needs rebase on Oct 30, 2020

fanquake deleted a comment on Nov 4, 2020

Sjors force-pushed on Nov 17, 2020

promag commented at 2:44 pm on November 20, 2020: member

Nice work, builds on macos and playing around, will try gui branch too.

Sjors force-pushed on Nov 23, 2020

MarcoFalke referenced this in commit 2ee954daae on Nov 23, 2020

Sjors force-pushed on Nov 24, 2020

DrahtBot added the label Needs rebase on Dec 1, 2020

Sjors force-pushed on Dec 4, 2020

DrahtBot removed the label Needs rebase on Dec 4, 2020

DrahtBot added the label Needs rebase on Dec 10, 2020

Sjors force-pushed on Dec 10, 2020

DrahtBot removed the label Needs rebase on Dec 10, 2020

DrahtBot added the label Needs rebase on Dec 17, 2020

Sjors force-pushed on Dec 17, 2020

DrahtBot removed the label Needs rebase on Dec 17, 2020

fanquake deleted a comment on Dec 20, 2020

in src/wallet/scriptpubkeyman.h:520 in 4bd42a528e outdated

516@@ -517,8 +517,6 @@ class LegacySigningProvider : public SigningProvider
517 class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
518 {
519 private:
520-    WalletDescriptor m_wallet_descriptor GUARDED_BY(cs_desc_man);

fjahr commented at 11:07 pm on January 2, 2021:

Is this necessary to be removed?

Sjors commented at 4:14 pm on January 22, 2021:

It’s not removed: I moved it from private to protected.

in src/wallet/external_signer_scriptpubkeyman.cpp:52 in 68d9fa0afe outdated

47@@ -48,3 +48,18 @@ ExternalSigner ExternalSignerScriptPubKeyMan::GetExternalSigner() {
48 #endif
49 
50 }
51+
52+bool ExternalSignerScriptPubKeyMan::DisplayAddress(const CScript scriptPubKey, const ExternalSigner &signer) const
53+{
54+#ifdef ENABLE_EXTERNAL_SIGNER

fjahr commented at 11:27 pm on January 2, 2021:

Not sure if we need so many of these. This function should never be called if external signer is not enabled, right? Is this a belts & suspenders approach or is there a reason why it is absolutely necessary?

Sjors commented at 3:31 pm on January 3, 2021:

I don’t think it compiles otherwise, because this calls an ExternalSigner function which needs boost::process

fjahr commented at 9:41 pm on January 3, 2021:

Oh, that makes sense and I didn’t think about that. Might be helpful to note that somewhere in a comment.

in src/wallet/externalsigner.cpp:54 in 68d9fa0afe outdated

48@@ -49,6 +49,11 @@ bool ExternalSigner::Enumerate(const std::string& command, std::vector<ExternalS
49     return true;
50 }
51 
52+UniValue ExternalSigner::displayAddress(const std::string& descriptor) const
53+{
54+    return RunCommandParseJSON(m_command + " --fingerprint \"" + m_fingerprint + "\"" + (m_mainnet ? "" : " --testnet ") + " displayaddress --desc \"" + descriptor + "\"");

fjahr commented at 11:31 pm on January 2, 2021:

Could add a helper function to avoid the need of repeating the common part of these commands.

in src/wallet/rpcsigner.cpp:73 in 68d9fa0afe outdated

71+        RPCExamples{""},
72+        [](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue {
73+            std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
74+            CWallet* const pwallet = wallet.get();
75+
76+            if (!wallet) return NullUniValue;

fjahr commented at 11:34 pm on January 2, 2021:

I think this goes before the line above

Sjors commented at 4:18 pm on January 22, 2021:

Oops

in configure.ac:1404 in 1fb4b8a60b outdated

1332@@ -1320,8 +1333,8 @@ AX_BOOST_SYSTEM
1333 AX_BOOST_FILESYSTEM
1334 AX_BOOST_THREAD
1335 
1336-dnl Opt-in to boost-process
1337-AS_IF([ test x$with_boost_process != x ], [ AX_BOOST_PROCESS ], [ ax_cv_boost_process=no ] )
1338+dnl Opt-in to boost-process, unless external signer support is requested

fjahr commented at 11:57 pm on January 2, 2021:

s/Opt-in/Opt-out/ ?

Sjors commented at 4:20 pm on January 22, 2021:

In this PR you have to use --with-boost-process in order to enable external signer support. So it’s opt-in. But in the GUI followup that’s flipped if you have GUI enabled.

Alternatively you can use --with-external-signer which implies --with-boost-process and fails if Boost::Process is missing.

in src/wallet/externalsigner.cpp:1 in be811a8d3c outdated

0@@ -0,0 +1,8 @@
1+// Copyright (c) 2018 The Bitcoin Core developers

fjahr commented at 0:39 am on January 3, 2021:

Copyright headers need to be updated, might as well get it over with now :)

in test/functional/wallet_signer.py:50 in 0f6ff74efc outdated

45+    def clear_mock_result(self, node):
46+        os.remove(os.path.join(node.cwd, "mock_result"))
47+
48+    def run_test(self):
49+        self.log.info('-signer=%s' % self.mock_signer_path())
50+        assert_equal(self.nodes[0].getbalance(), 1250)

fjahr commented at 0:53 am on January 3, 2021:

I am guessing this was just a placeholder to see that things are working and this can be removed when the actual tests are added?

Sjors commented at 4:24 pm on January 22, 2021:

I dropped the balance sanity check and demoted the signer path to debug.

in src/wallet/wallet.cpp:266 in a9cd63f8bc outdated

258@@ -259,6 +259,19 @@ std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, const std::strin
259         wallet_creation_flags |= WALLET_FLAG_BLANK_WALLET;
260     }
261 
262+    // Private keys must be disabled for an external signer wallet
263+    if ((wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) && !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
264+        error = Untranslated("Private keys must be disabled when using an external signer");
265+        status = DatabaseStatus::FAILED_CREATE;
266+        return nullptr;    }

fjahr commented at 0:55 am on January 3, 2021:

} was probably not formated right by accident

fjahr commented at 3:26 pm on January 3, 2021: member

Leaving a first set of questions and comments from my first look at the code. Will do some manual testing soon.

DrahtBot added the label Needs rebase on Jan 7, 2021

felipsoarez commented at 1:25 pm on January 9, 2021: none

utACK

laanwj commented at 3:46 pm on January 11, 2021: member

There was a merge conflict in configure.ac with the NAT-PMP introduction. Rebased version: https://github.com/laanwj/bitcoin/tree/tmp

in src/Makefile.am:269 in 8aa7f0fead outdated

249@@ -250,10 +250,13 @@ BITCOIN_CORE_H = \
250   wallet/crypter.h \
251   wallet/db.h \
252   wallet/dump.h \
253+  wallet/externalsigner.h \
254+  wallet/external_signer_scriptpubkeyman.h \

laanwj commented at 5:14 pm on January 11, 2021:

nit: I’d prefer these to be consistent in naming; either

0  wallet/externalsigner.h \
1  wallet/externalsigner_scriptpubkeyman.h \

or

0  wallet/external_signer.h \
1  wallet/external_signer_scriptpubkeyman.h \

I guess the former is most consistent with how things are done in other places. (same for the associated .cpp, of course).

Sjors commented at 4:31 pm on January 22, 2021:

I’ll go for the second option, because it’s more readable. I renamed it in a separate commit to avoid rebase hell.

laanwj requested review from achow101 on Jan 11, 2021

laanwj requested review from meshcollider on Jan 11, 2021

in src/wallet/wallet.cpp:4473 in 8aa7f0fead outdated

4470+    if (IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) {
4471+#ifdef ENABLE_EXTERNAL_SIGNER
4472+        auto spk_manager = std::unique_ptr<ScriptPubKeyMan>(new ExternalSignerScriptPubKeyMan(*this, desc));
4473+        m_spk_managers[id] = std::move(spk_manager);
4474+#else
4475+        throw std::runtime_error(std::string(__func__) + ": Wallets with external signers require Boost::System library.");

laanwj commented at 5:26 pm on January 11, 2021:

Wouldn’t it be better to mention --enable-external-signer configure flag in this error message instead of boost (which is just an implementation detail)?

in src/wallet/wallet.h:839 in 8aa7f0fead outdated

832@@ -829,6 +833,12 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
833 
834     std::vector<OutputGroup> GroupOutputs(const std::vector<COutput>& outputs, bool single_coin, const size_t max_ancestors) const;
835 
836+#ifdef ENABLE_EXTERNAL_SIGNER
837+    ExternalSigner GetExternalSigner() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
838+#endif
839+    /* Display address on an external signer. Returns false if external signer support is not compiled */

laanwj commented at 5:29 pm on January 11, 2021:

Please use a doxygen compatible comment

0/** Display address on an external signer. Returns false if external signer support is not compiled */

laanwj added this to the milestone 22.0 on Jan 11, 2021

in src/wallet/external_signer_scriptpubkeyman.cpp:36 in 8aa7f0fead outdated

29+
30+    m_storage.UnsetBlankWalletFlag(batch);
31+    return true;
32+}
33+
34+ExternalSigner ExternalSignerScriptPubKeyMan::GetExternalSigner() {

laanwj commented at 5:35 pm on January 11, 2021:

Do you really want to construct a new instance every time and return by value here? This function is called reasonably often (e.g. when signing, when displaying addresses). It seems that these could be cached, and in that case a pointer (e.g. std::shared_ptr) would make more sense. But not sure.

Sjors commented at 4:47 pm on January 22, 2021:

My guess would be that this is negligible compared to the slow USB communication and sluggish hardware wallets in general. I might leave that for someone else to improve.

in src/wallet/externalsigner.h:54 in 8aa7f0fead outdated

49+    static bool Enumerate(const std::string& command, std::vector<ExternalSigner>& signers, bool mainnet = true, bool ignore_errors = false);
50+
51+    //! Display address on the device. Calls `<command> displayaddress --desc <descriptor>`.
52+    //! @param[in] descriptor Descriptor specifying which address to display.
53+    //!            Must include a public key or xpub, as well as key origin.
54+    UniValue displayAddress(const std::string& descriptor) const;

laanwj commented at 6:13 pm on January 11, 2021:

Please start the method names with a capital letter here like in other classes.

in src/wallet/externalsigner.h:38 in 8aa7f0fead outdated

33+
34+    //! Master key fingerprint of the signer
35+    std::string m_fingerprint;
36+
37+    //! Bitcoin mainnet or testnet
38+    bool m_mainnet;

laanwj commented at 6:19 pm on January 11, 2021:

Is a bool enough here? E.g. what if someone wants to use external signers on signet, regtest? Or are those all “testnet” in this description?

Sjors commented at 11:22 am on January 12, 2021:

I should update the comment, but yes: signet, testnet and regtest use the same address format, which is the only thing that seems to matter when using hardware wallets (at the least the devices I’ve worked with). They don’t even have their own BIP44/49/81 coin types.

achow101 commented at 8:21 pm on February 8, 2021:

For at least testnet and regtest, the bech32 address format is different.

But also, HWI only has mainnet and testnet so maybe just a bool is fine?

Sjors commented at 1:14 pm on February 9, 2021:

I overlooked that regtest has a different prefix. Maybe an enum is a good idea… https://github.com/satoshilabs/slips/blob/master/slip-0173.md#registered-human-readable-parts

Sjors commented at 11:19 am on January 12, 2021: member

@laanwj @fjahr thanks for the reviews! I was waiting for another PR (https://github.com/bitcoin-core/gui/pull/171) to land which I expect to require a rebase, but I might do it sooner.

hebasto commented at 6:02 pm on January 21, 2021: member

@Sjors

@laanwj @fjahr thanks for the reviews! I was waiting for another PR (bitcoin-core/gui#171) to land which I expect to require a rebase, but I might do it sooner.

https://github.com/bitcoin-core/gui/pull/171 just merged :)

Sjors force-pushed on Jan 22, 2021

Sjors commented at 5:03 pm on January 22, 2021: member

Rebased and addressed most feedback.

DrahtBot removed the label Needs rebase on Jan 22, 2021

in src/wallet/rpcwallet.cpp:4561 in 77742c4fc3 outdated

4557@@ -4548,7 +4558,7 @@ static const CRPCCommand commands[] =
4558     { "wallet",             "backupwallet",                     &backupwallet,                  {"destination"} },
4559     { "wallet",             "bumpfee",                          &bumpfee,                       {"txid", "options"} },
4560     { "wallet",             "psbtbumpfee",                      &psbtbumpfee,                   {"txid", "options"} },
4561-    { "wallet",             "createwallet",                     &createwallet,                  {"wallet_name", "disable_private_keys", "blank", "passphrase", "avoid_reuse", "descriptors", "load_on_startup"} },
4562+    { "wallet",             "createwallet",                     &createwallet,                  {"wallet_name", "disable_private_keys", "blank", "passphrase", "avoid_reuse", "descriptors", "load_on_startup", "external_signer"} },

MarcoFalke commented at 6:44 pm on January 28, 2021:

can remove this diff, and then rebase

DrahtBot added the label Needs rebase on Jan 28, 2021

Sjors force-pushed on Jan 29, 2021

Sjors commented at 11:20 am on January 29, 2021: member

Rebased after #20012

DrahtBot removed the label Needs rebase on Jan 29, 2021

Sjors force-pushed on Jan 29, 2021

in build_msvc/bitcoin_config.h:57 in 235445a431 outdated

52@@ -53,6 +53,9 @@
53 /* define if the Boost::Process library is available */
54 #define HAVE_BOOST_PROCESS /**/
55 
56+/* define if external signer support is enabled (requires Boost::Process) */
57+#define ENABLE_EXTERNAL_SIGNER 1 /**/

achow101 commented at 7:39 pm on February 8, 2021:

In 235445a431c6146bb194761335761fa423469ac0 “msvc: define ENABLE_EXTERNAL_SIGNER”

Since this is supposed to be defaults, I don’t think you should have 1 there. It should be like HAVE_BOOST_PROCESS right above.

Sjors commented at 12:50 pm on February 9, 2021:

Just defining or setting it to 1 always confused me. As does MSVC. But it sounds reasonable… @sipsorcery?

achow101 commented at 4:54 pm on February 9, 2021:

I think it’s supposed to be either the default or a fixed setting indicating what msvc should be building. Either way, having HAVE_BOOST_PROCESS not defined above and still defining ENABLE_EXTERNAL_SIGNER is a conflict because we can’t have external signer support without boost process.

sipsorcery commented at 10:35 pm on February 10, 2021:

I think msvc is the same as the other compilers. If you want to set a pre-processor define to act like a boolean flag there’s no need to set a value on it. #define ENABLE_EXTERNAL_SIGNER is all you need if you’re only checking for its existence with #ifdef ENABLE_EXTERNAL_SIGNER.

Sjors commented at 2:51 pm on February 12, 2021:

Ok, I’ll use that.

in src/dummywallet.cpp:41 in 1425e78427 outdated

37@@ -38,6 +38,9 @@ void DummyWalletInit::AddWalletOptions(ArgsManager& argsman) const
38         "-paytxfee=<amt>",
39         "-rescan",
40         "-salvagewallet",
41+#ifdef ENABLE_EXTERNAL_SIGNER

achow101 commented at 7:47 pm on February 8, 2021:

In 1425e784279cee4a04bf186990dcb8669af0bd01 “wallet: add -signer argument for external signer command”

Having -signer in the hidden options does not require it to be guarded like this.

in src/wallet/externalsigner.cpp:1 in 1425e78427 outdated

0@@ -0,0 +1,8 @@
1+// Copyright (c) 2018-2021 The Bitcoin Core developers

achow101 commented at 7:50 pm on February 8, 2021:

In 1425e784279cee4a04bf186990dcb8669af0bd01 “wallet: add -signer argument for external signer command”

Should just be 2021 here and elsewhere?

Sjors commented at 12:59 pm on February 9, 2021:

Good question. It’s been under development and publicly visible since 2018 though.

in src/wallet/external_signer_scriptpubkeyman.h:24 in 54cfea82c8 outdated

18+      {}
19+
20+  /** Provide a descriptor at setup time
21+  * Returns false if already setup or setup fails, true if setup is successful
22+  */
23+  bool SetupDescriptor(std::unique_ptr<Descriptor>desc);

achow101 commented at 8:00 pm on February 8, 2021:

In 54cfea82c85bcbcef012922ad78dcb3ba3ae1c6e “wallet: add ExternalSignerScriptPubKeyMan”

Should be

0  bool SetupDescriptor(std::unique_ptr<Descriptor> desc);

in src/wallet/wallet.cpp:4495 in 54cfea82c8 outdated

4428@@ -4428,8 +4429,17 @@ void CWallet::ConnectScriptPubKeyManNotifiers()
4429 
4430 void CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc)
4431 {
4432-    auto spk_manager = std::unique_ptr<ScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, desc));
4433-    m_spk_managers[id] = std::move(spk_manager);
4434+    if (IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) {

achow101 commented at 8:05 pm on February 8, 2021:

In 54cfea82c85bcbcef012922ad78dcb3ba3ae1c6e “wallet: add ExternalSignerScriptPubKeyMan”

This is not necessarily going to work since it is possible we read (and thus load) a descriptor before we read and load the wallet flags.

We really should always be loading in the flags first (right after minversion) before reading the rest of the records in the db.

Sjors commented at 2:56 pm on February 9, 2021:

I made a separate PR for this: #21127

in test/functional/rpc_help.py:109 in 2352f3ec13 outdated

104@@ -105,10 +105,13 @@ def test_categories(self):
105         if self.is_wallet_compiled():
106             components.append('Wallet')
107 
108+        if self.is_external_signer_compiled():
109+            components.append('Signer')

achow101 commented at 8:17 pm on February 8, 2021:

In 2352f3ec13e1d819e140e061f3d7927a20387cbe “rpc: add external signer RPC files”

Move this above to avoid sorted below?

Sjors commented at 3:05 pm on February 9, 2021:

But then I also have to add a line components.append('Util')

in src/wallet/externalsigner.h:30 in 9e0d6e145c outdated

25@@ -25,10 +26,27 @@ class ExternalSigner
26 public:
27     //! @param[in] command      the command which handles interaction with the external signer
28     //! @param[in] fingerprint  master key fingerprint of the signer
29-    ExternalSigner(const std::string& command, const std::string& fingerprint);
30+    //! @param[in] mainnet      Bitcoin mainnet (as opposed to regtest, testnet and signet)
31+    ExternalSigner(const std::string& command, const std::string& fingerprint, bool mainnet, std::string name);

achow101 commented at 8:22 pm on February 8, 2021:

In 9e0d6e145ca150f8e992a92558de4d0e5cd94dab “rpc: signer: add enumeratesigners to list external signers”

Need docstring for name.

in src/wallet/rpcsigner.cpp:40 in 9e0d6e145c outdated

47+            if (!wallet) return NullUniValue;
48+
49+            const std::string command = gArgs.GetArg("-signer", DEFAULT_EXTERNAL_SIGNER);
50+            if (command == "") throw JSONRPCError(RPC_WALLET_ERROR, "Error: restart bitcoind with -signer=<cmd>");
51+            std::string chain = gArgs.GetChainName();
52+            const bool mainnet = chain == CBaseChainParams::MAIN;

achow101 commented at 8:35 pm on February 8, 2021:

In 9e0d6e145ca150f8e992a92558de4d0e5cd94dab “rpc: signer: add enumeratesigners to list external signers”

It’s probably better to use Params().IsTestChain rather than checking gArgs and doing a string comparison.

0            const bool mainnet = !Params().IsTestChain()

in test/functional/mocks/signer.py:26 in 2926553d26 outdated

21+parser = argparse.ArgumentParser(prog='./signer.py', description='External signer mock')
22+subparsers = parser.add_subparsers()
23+
24+if len(sys.argv) == 1:
25+  args = parser.parse_args(['-h'])
26+  exit()

achow101 commented at 8:43 pm on February 8, 2021:

In 2926553d26e2f15e7986a73f6ee678112ab8ab7f “test: add external signer test”

argparse already has a way to force a subparser command to be provided.

0subparsers = parser.add_subparsers(description='Commands', dest='command')
1subparsers.required = True

in src/wallet/wallet.cpp:3592 in 3fdaea28fc outdated

3587+ExternalSigner CWallet::GetExternalSigner() {
3588+    const std::string command = gArgs.GetArg("-signer", DEFAULT_EXTERNAL_SIGNER);
3589+    if (command == "") throw std::runtime_error(std::string(__func__) + ": restart bitcoind with -signer=<cmd>");
3590+
3591+    std::string chain = gArgs.GetChainName();
3592+    const bool mainnet = chain == CBaseChainParams::MAIN;

achow101 commented at 8:48 pm on February 8, 2021:

In 3fdaea28fc9befb77caa8c1fd1f5f61c577eabe6 “wallet: add GetExternalSigner()”

Use Params().IsTestChain rather than checking gArgs and doing a string comparison.

0    const bool mainnet = !Params().IsTestChain()

in src/wallet/externalsigner.cpp:54 in 6e0a04004c outdated

50@@ -51,7 +51,7 @@ bool ExternalSigner::Enumerate(const std::string& command, std::vector<ExternalS
51 
52 UniValue ExternalSigner::GetDescriptors(int account)
53 {
54-    return runCommandParseJSON(m_command + " --fingerprint \"" + m_fingerprint + "\"" + (m_mainnet ? "" : " --testnet ") + " getdescriptors --account " + strprintf("%d", account));
55+    return RunCommandParseJSON(m_command + " --fingerprint \"" + m_fingerprint + "\"" + (m_mainnet ? "" : " --testnet ") + " getdescriptors --account " + strprintf("%d", account));

achow101 commented at 8:50 pm on February 8, 2021:

In 6e0a04004c2ea6266b912fa520340188bf930967 “wallet: fetch keys from external signer upon creation”

This change should be squashed into 1c8e9ad70c858dc0cbaa2b6b3b92f67dc8b002d0

in src/wallet/external_signer_scriptpubkeyman.cpp:92 in 57b26f44b9 outdated

87+    const bool mainnet = chain == CBaseChainParams::MAIN;
88+    std::vector<ExternalSigner> signers;
89+    ExternalSigner::Enumerate(command, signers, mainnet);
90+    if (signers.empty()) return TransactionError::EXTERNAL_SIGNER_NOT_FOUND;
91+    // TODO: add fingerprint argument in case of multiple signers
92+    ExternalSigner signer = signers[0];

achow101 commented at 9:01 pm on February 8, 2021:

In 57b26f44b9109eb0c366adeb54bf5ebfc816b45e “rpc: send: support external signer”

Perhaps use GetExternalSigner() instead of duplicate code?

in test/functional/mocks/signer.py:61 in 57b26f44b9 outdated

56+    if args.fingerprint != "00000001":
57+        return sys.stdout.write(json.dumps({"error": "Unexpected fingerprint", "fingerprint": args.fingerprint}))
58+
59+    f = open(os.path.join(os.getcwd(), "mock_psbt"), "r")
60+    mock_psbt = f.read()
61+    f.close()

achow101 commented at 9:04 pm on February 8, 2021:

In 57b26f44b9109eb0c366adeb54bf5ebfc816b45e “rpc: send: support external signer”

Use with

0    with open(os.path.join(os.getcwd(), "mock_psbt"), "r") as f:
1         mock_psbt = f.read()

in test/functional/wallet_signer.py:203 in 57b26f44b9 outdated

198+
199+        assert(hww.testmempoolaccept([mock_tx])[0]["allowed"])
200+
201+        f = open(os.path.join(self.nodes[1].cwd, "mock_psbt"), "w")
202+        f.write(mock_psbt_signed["psbt"])
203+        f.close()

achow101 commented at 9:07 pm on February 8, 2021:

In 57b26f44b9109eb0c366adeb54bf5ebfc816b45e “rpc: send: support external signer”

Use with

0        with open(os.path.join(os.getcwd(), "mock_psbt"), "w") as f:
1             f.write(mock_psbt_signed["psbt"])

in test/functional/wallet_signer.py:43 in 2926553d26 outdated

38+        self.skip_if_no_external_signer()
39+
40+    def set_mock_result(self, node, res):
41+        f = open(os.path.join(node.cwd, "mock_result"), "w", encoding="utf8")
42+        f.write(res)
43+        f.close()

achow101 commented at 9:10 pm on February 8, 2021:

In 2926553d26e2f15e7986a73f6ee678112ab8ab7f “test: add external signer test”

Use with

0        with open(os.path.join(node.cwd, "mock_result"), "w", encoding="utf8") as f:
1            f.write(res)

in test/functional/wallet_signer.py:33 in 2926553d26 outdated

28+        self.num_nodes = 3
29+
30+        self.extra_args = [
31+            [],
32+            ['-signer=%s' % self.mock_signer_path()],
33+            ['-signer=%s' % "fake.py"],

achow101 commented at 9:11 pm on February 8, 2021:

In 2926553d26e2f15e7986a73f6ee678112ab8ab7f “test: add external signer test”

Use f-strings

0            [f"-signer={self.mock_signer_path()}"],
1            ["-signer=fake.py"],

Sjors commented at 1:12 pm on February 9, 2021:

Ah, we upgraded to Python 3.6 “recently” :-)

laanwj commented at 11:04 am on February 12, 2021:

It’s funny but also a bit shameful that this has taken so long that the drift of linters and python versions is starting to eat into it.

Sjors commented at 1:42 pm on February 12, 2021:

Oh I’ve been plowing through rebases while descriptor wallets were being built. This is nothing :-)

Sjors commented at 5:59 pm on February 9, 2021: member

Rebased and addressed @achow101’s feedback. It now calls HWI with --regtest (unsupported), --testnet or --signet instead of using --testnet for all non-mainnet networks. I might change that to --chain ... depending on what happens to https://github.com/bitcoin-core/HWI/pull/451, but shouldn’t be blocking here.

Sjors force-pushed on Feb 9, 2021

laanwj commented at 10:59 am on February 12, 2021: member

This has been open for a long time. Hardware wallet support seems quite important. It would be nice if it got some more review and/or ACKs so that we can move forward with it.

laanwj commented at 11:02 am on February 12, 2021: member

Linting errors:

0Python's open(...) seems to be used to open text files without explicitly
1specifying encoding="utf8":
2
3test/functional/mocks/signer.py:        with open(mock_result_path, "r") as f:
4test/functional/wallet_signer.py:        with open(os.path.join(node.cwd, "mock_result"), "w") as f:
5^---- failure generated from test/lint/lint-python-utf8-encoding.sh

DrahtBot added the label Needs rebase on Feb 12, 2021

Sjors force-pushed on Feb 12, 2021

Sjors commented at 3:36 pm on February 12, 2021: member

Rebased, appeased linter and switched to using the new HWI --chain argument. Wallet experts, please bless #21127 as well.

DrahtBot removed the label Needs rebase on Feb 12, 2021

gruve-p commented at 4:55 pm on February 12, 2021: contributor

utACK https://github.com/bitcoin/bitcoin/pull/16546/commits/e0ce81b4a5d8b8ae617de211747aac5fcad48570

fanquake changes_requested

fanquake commented at 3:07 am on February 13, 2021: member

I’ve only looked at the build system commit. I don’t understand the changes here. There should be a single configure option enable/disable-external-signer, which defaults to off. with-boost-process should be removed entirely. Boost process checking should only occur with enable-external-signer, and if it’s not available, configure should fail. We don’t need all the duplicate checking for if the wallet is enabled for setting defaults etc.

Sjors commented at 10:47 am on February 13, 2021: member

@fanquake boost process is used by RunCommandParseJSON (introduced in #15382). This can be used by any feature that involves calling an external command. Hence the separation. However, if you prefer I can still drop --with-boost-process. It just means that if we add another feature that uses this, that feature will require a separate configure flag. That’s probably fine.

During the development, when switching between branches all the time, I find --with-boost-process convenient, because it works on this branch as well as master. But that’s not a strong argument for keeping it.

laanwj referenced this in commit 43981ee2c8 on Feb 13, 2021

sidhujag referenced this in commit 1aa9dbea4b on Feb 13, 2021

Sjors force-pushed on Feb 14, 2021

fanquake commented at 7:43 am on February 15, 2021: member

It just means that if we add another feature that uses this, that feature will require a separate configure flag. That’s probably fine.

That’s exactly what should happen. Having a --with-boost-process configure flag just to be a catch-all for “turn on anything that uses boost process” is not something we want. Significant new features should have their own configure flags.

I find –with-boost-process convenient, because it works on this branch as well as master. But that’s not a strong argument for keeping it.

No it isn’t.

The Boost Process macro itself is mostly just confusing. Boost Process is header only, so having a macro that run a bunch of checks for a library to link against is pointless? I don’t really understand why it was written like that in the first place, and am planning on removing it.

fanquake referenced this in commit 1e4b1973a0 on Feb 15, 2021

fanquake referenced this in commit 1b960f0780 on Feb 15, 2021

fanquake referenced this in commit ffe3d65aaa on Feb 15, 2021

Sjors force-pushed on Feb 15, 2021

in src/wallet/wallet.cpp:4466 in 4106d7e45f outdated

4463+    if (IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) {
4464+#ifdef ENABLE_EXTERNAL_SIGNER
4465+        auto spk_manager = std::unique_ptr<ScriptPubKeyMan>(new ExternalSignerScriptPubKeyMan(*this, desc));
4466+        m_spk_managers[id] = std::move(spk_manager);
4467+#else
4468+        throw std::runtime_error(std::string(__func__) + ": Configure with --enable-external-signer to use this");

meshcollider commented at 11:52 pm on February 16, 2021:

What “this” is may not be clear to the user

fanquake referenced this in commit 7bf04e358a on Feb 17, 2021

in doc/external-signer.md:7 in af6c0c39db outdated

0@@ -0,0 +1,171 @@
1+# Support for signing transactions outside of Bitcoin Core
2+
3+Bitcoin Core can be launched with `-signer=<cmd>` where `<cmd>` is an external tool which can sign transactions and perform other functions. For example, it can be used to communicate with a hardware wallet.
4+
5+## Example usage
6+
7+The following example is based on the [HWI](https://github.com/bitcoin-core/HWI) tool. Although this tool is hosted under the Bitcoin Core Github organization and maintained by Bitcoin Core developers, it should be used with caution. It is considered experimental and has far less review than Bitcoin Core itself. Be particularly careful when running tools such as these on a computer with private keys on it.

meshcollider commented at 0:14 am on February 17, 2021:

micro-nit: GitHub

in doc/external-signer.md:107 in af6c0c39db outdated

102+
103+The command returns a psbt with any signatures.
104+
105+The `psbt` SHOULD include bip32 derivations. The command SHOULD fail if none of the bip32 derivations match a key owned by the device.
106+
107+The command SHOULD fail if the user cancels (return code?).

meshcollider commented at 0:24 am on February 17, 2021:

Is this “return code?” meant for the final doc

in src/wallet/external_signer.cpp:14 in b27ec70b1a outdated

 9+#include <util/system.h>
10+#include <wallet/external_signer.h>
11+
12+ExternalSigner::ExternalSigner(const std::string& command, const std::string& fingerprint, std::string chain, std::string name): m_command(command), m_fingerprint(fingerprint), m_chain(chain), m_name(name) {}
13+
14+const std::string ExternalSigner::NetworkArg() const {

meshcollider commented at 0:30 am on February 17, 2021:

style nit: brace

in src/wallet/wallet.cpp:3579 in b27ec70b1a outdated

3574@@ -3558,6 +3575,37 @@ void ReserveDestination::ReturnDestination()
3575     address = CNoDestination();
3576 }
3577 
3578+#ifdef ENABLE_EXTERNAL_SIGNER
3579+ExternalSigner CWallet::GetExternalSigner() {

meshcollider commented at 0:30 am on February 17, 2021:

style nit: brace

meshcollider commented at 3:08 am on February 17, 2021: contributor

Great PR Sjors, really really appreciate your upkeep and the clean,easy-to-review commits.

Review in progress…

cad9cb16b6e13cec4110e6727986be0f067e2cd0 configure: add –enable-external-signer
e91c439d4b1afae1a2d595e04e90e4f18e8a7298 msvc: define ENABLE_EXTERNAL_SIGNER
48625eca5bbf80a8aed15f185c475e91b6669d09 doc: include Doxygen comments for ENABLE_EXTERNAL_SIGNER
f7961b77f1bfc10e0b147c08192c39294a09c523 test: framework: add skip_if_no_external_signer
1b34c1d86f467c32a6139bbaeed169e6617911c2 wallet: add -signer argument for external signer command
790e192fc077c7490089ab9bf63c1fa3dbb19e26 test: add external signer test
24bfbdcf165361a9c3a6aa8ed452ba409df11c16 wallet: add external_signer flag
4106d7e45f7ab5a48a0b57d64b767306dfd51934 wallet: add ExternalSignerScriptPubKeyMan
1ac8ee857089894223597ab5bf350b117301dc77 rpc: add external signer RPC files
6d7d5fe4adb8a6f659de7faed1fde7f5c5c3bbf2 rpc: signer: add enumeratesigners to list external signers
a4851d7b64f5dc99c4a19ee6555d6629af689d3e rpc: add external_signer option to createwallet
87a328215c864965867eb1f8a0712abff88f3b89 test: external_signer wallet flag is immutable
c73376b9824c79aec5166fa0cb5c49f4ee1fb2bb wallet: add GetExternalSigner()
345d94e1c346d7f3cee42091e82ed008ae651887 wallet: fetch keys from external signer upon creation
2f47ee89ef46289e22cb91c8a9c26c561aa94a24 rpc: signerdisplayaddress
fb3559c9d167d7d1128ca1db5f604f5292c0bb30 rpc: send: support external signer
af6c0c39dbf7f7023aaf9da04f1c7c0ad2428059 doc: add external-signer.md
b27ec70b1a04f3ece972721d54c2e1bc1ce4862a move-only: add underscore to externalsigner.h

in src/wallet/wallet.cpp:4527 in 345d94e1c3 outdated

4538+
4539+        // TODO: add account parameter
4540+        int account = 0;
4541+        UniValue signer_res = signer.GetDescriptors(account);
4542+
4543+        if (!signer_res.isObject()) throw std::runtime_error(std::string(__func__) + ": Unexpect result");

meshcollider commented at 3:21 am on February 17, 2021:

nit: Unexpect -> Unexpected

in src/wallet/wallet.cpp:4530 in 345d94e1c3 outdated

4541+        UniValue signer_res = signer.GetDescriptors(account);
4542+
4543+        if (!signer_res.isObject()) throw std::runtime_error(std::string(__func__) + ": Unexpect result");
4544+        for (bool internal : {false, true}) {
4545+            const UniValue& descriptor_vals = find_value(signer_res, internal ? "internal" : "receive");
4546+            if (!descriptor_vals.isArray()) throw std::runtime_error(std::string(__func__) + ": Unexpect result");

meshcollider commented at 3:22 am on February 17, 2021:

same here

in src/wallet/external_signer_scriptpubkeyman.cpp:82 in fb3559c9d1 outdated

77+        complete &= PSBTInputSigned(input);
78+    }
79+    if (complete) return TransactionError::OK;
80+
81+    std::string strFailReason;
82+    if( !GetExternalSigner().SignTransaction(psbt, strFailReason)) {

meshcollider commented at 3:29 am on February 17, 2021:

style nit: spacing after if(

in src/wallet/externalsigner.cpp:87 in fb3559c9d1 outdated

82+{
83+    // Serialize the PSBT
84+    CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
85+    ssTx << psbtx;
86+
87+    // Check if signer fingerpint matches any input master key fingerprint

meshcollider commented at 3:31 am on February 17, 2021:

nit: fingerpint -> fingerprint

meshcollider commented at 3:33 am on February 17, 2021: contributor

Code review ACK modulo my inexperience with build system stuff + the rebase on fanquake’s PR

laanwj referenced this in commit 04336086d3 on Feb 17, 2021

Sjors commented at 3:25 pm on February 17, 2021: member

Rebased and addressed nits.

Sjors force-pushed on Feb 17, 2021

sidhujag referenced this in commit 5d5698c0ae on Feb 17, 2021

meshcollider commented at 8:12 pm on February 17, 2021: contributor

Will do some testing

achow101 commented at 8:25 pm on February 17, 2021: member

ACK 85da1ad0442b243e10e7f671e14812e70e74f624

in configure.ac:1406 in 85da1ad044 outdated

1405- [ AC_MSG_ERROR([Boost::Process is not available!])]
1406+ [ AC_MSG_RESULT(yes)
1407+ AC_DEFINE([HAVE_BOOST_PROCESS],,[define if Boost::Process is available])
1408+ AC_DEFINE([ENABLE_EXTERNAL_SIGNER],,[define if external signer support is enabled])
1409+ ],
1410+ [ AC_MSG_RESULT(no)]

laanwj commented at 9:32 am on February 18, 2021:

I think this is due to a merge conflict: please keep the error message here. Not finding boost::process when --enable-external-signer is fatal.

Sjors commented at 1:18 pm on February 18, 2021:

Oops, good catch, that indeed fell through the rebase cracks. Hard to test presence of boost::process seperate from the rest of boost on macOS, because they’re bundled. But on Windows this matters.

in src/wallet/external_signer.cpp:16 in 85da1ad044 outdated

11+
12+ExternalSigner::ExternalSigner(const std::string& command, const std::string& fingerprint, std::string chain, std::string name): m_command(command), m_fingerprint(fingerprint), m_chain(chain), m_name(name) {}
13+
14+const std::string ExternalSigner::NetworkArg() const
15+{
16+    if (m_chain == CBaseChainParams::MAIN) {

laanwj commented at 9:41 am on February 18, 2021:

I would prefer to introduce a CBaseChainParams.externalSignerChainName() instead of an elaborate if() here, for this to keep the chain definition data-driven in one place.

(not necessarily in this PR though, we can do this later)

Sjors commented at 1:41 pm on February 18, 2021:

Alternatively, also out of scope, we could turn CBaseChainParams into an enum, so it’s easier to use switch statements, and less likely to be forgotten if we add another network type.

laanwj commented at 5:48 pm on February 18, 2021:

That’d be another option, but I prefer to have all this information in one place. It was the point of having chainparams structures in the first place.

laanwj commented at 5:50 pm on February 18, 2021:

But good point that they are strings. They also match exactly here:

0const std::string CBaseChainParams::MAIN = "main";
1const std::string CBaseChainParams::TESTNET = "test";
2const std::string CBaseChainParams::SIGNET = "signet";
3const std::string CBaseChainParams::REGTEST = "regtest";

So wouldn’t " --chain " + m_chain achieve exactly the same?

Sjors commented at 6:32 pm on February 18, 2021:

Good point…

Sjors force-pushed on Feb 18, 2021

laanwj commented at 6:44 pm on February 18, 2021: member

Code review ACK 96f991a11a871502d1a39f684994c1e2664c3af7 Verified that the only changes since 85da1ad0442b243e10e7f671e14812e70e74f624 are the configure.ac change, and simpllyfing ExternalSigner::NetworkArg() (removing a include for strencodings.h along the way).

achow101 commented at 7:54 pm on February 18, 2021: member

re-ACK 96f991a11a871502d1a39f684994c1e2664c3af7

in src/wallet/external_signer_scriptpubkeyman.cpp:45 in 96f991a11a outdated

40+    ExternalSigner::Enumerate(command, signers, Params().NetworkIDString());
41+    if (signers.empty()) throw std::runtime_error(std::string(__func__) + ": No external signers found");
42+    // TODO: add fingerprint argument in case of multiple signers
43+    return signers[0];
44+#else
45+    throw std::runtime_error(std::string(__func__) + ": Wallets with external signers require Boost::System library.");

fanquake commented at 3:38 am on February 21, 2021:

Shouldn’t this be Boost::Process?

in src/wallet/wallet.cpp:4569 in 96f991a11a outdated

4586-            uint256 id = spk_manager->GetID();
4587-            m_spk_managers[id] = std::move(spk_manager);
4588-            AddActiveScriptPubKeyMan(id, t, internal);
4589         }
4590+#else
4591+        throw std::runtime_error(std::string(__func__) + ": Wallets with external signers require Boost::System library.");

fanquake commented at 3:38 am on February 21, 2021:

Also Boost::Process here?

in src/wallet/external_signer_scriptpubkeyman.cpp:37 in 96f991a11a outdated

32+    return true;
33+}
34+
35+ExternalSigner ExternalSignerScriptPubKeyMan::GetExternalSigner() {
36+#ifdef ENABLE_EXTERNAL_SIGNER
37+    const std::string command = gArgs.GetArg("-signer", ""); // DEFAULT_EXTERNAL_SIGNER);

fanquake commented at 3:43 am on February 21, 2021:

0    const std::string command = gArgs.GetArg("-signer", DEFAULT_EXTERNAL_SIGNER);

Or if we are going to use "", we should remove the commented code.

Sjors commented at 2:23 pm on February 21, 2021:

Not sure what was going on there, but I put DEFAULT_EXTERNAL_SIGNER back.

Sjors commented at 3:13 pm on February 21, 2021:

Now I remember, I didn’t want a circular inclusion. I just switched to "".

in src/wallet/external_signer_scriptpubkeyman.cpp:36 in 96f991a11a outdated

31+    m_storage.UnsetBlankWalletFlag(batch);
32+    return true;
33+}
34+
35+ExternalSigner ExternalSignerScriptPubKeyMan::GetExternalSigner() {
36+#ifdef ENABLE_EXTERNAL_SIGNER

fanquake commented at 7:02 am on February 21, 2021:

From what I can see, GetExternalSigner() is only called from code that is inside ENABLE_EXTERNAL_SIGNER #idfefs. So why can’t we wrap this whole function in an #ifdef, and drop the need for the throw()? I think the same goes for most of this file, and some other functions in this PR. It seems weird that when external signing is not enabled, we’d still be compiling functions that aren’t called from anywhere, and who’s purpose is to throw a runtime error to tell you about external signing and it’s need of Boost Process.

Sjors commented at 2:54 pm on February 21, 2021:

If turns out I can put the entire ExternalSignerScriptPubKeyMan inside #ifdef. The most important throw()s are already in wallet.cpp.

fanquake commented at 7:04 am on February 21, 2021: member

In 903f277f982ed390dc68114c9c1eb70b4a59bb19 is where we should be able to drop HAVE_BOOST_PROCESS entirely. I realise that boost process could potentially be used for other things in future, but currently, it’s not, and as this is implemented, there’s a bit of an awkward split between HAVE_BOOST_PROCESS & ENABLE_EXTERNAL_SIGNER for code which, as I understand it, is all essentially the same feature. So I think for now we should just replace the few usages of HAVE_BOOST_PROCESS with ENABLE_EXTERNAL_SIGNER.

configure: add --enable-external-signer

This option replaces --with-boost-process

This prepares external signer support to be disabled by default.
It adds a configure option to enable this feature and to check
if Boost::Process is present.

This also exposes ENABLE_EXTERNAL_SIGNER to the test suite via test/config.ini

87a97941f6

test: framework: add skip_if_no_external_signer f7eb7ecc67

Sjors commented at 4:31 pm on February 21, 2021: member

Rebased onto CI fix. @fanquake I replaced the remaining instances of HAVE_BOOST_PROCESS with ENABLE_EXTERNAL_SIGNER. In the process I squashed the MSVC and doc commit, since they’re now just a simple rename.

Sjors force-pushed on Feb 21, 2021

laanwj commented at 11:30 am on February 22, 2021: member

Please squash the move-only: add underscore to externalsigner.h commit as well. It’s introduced in this PR, so let’s introduce it with the eventual name directly.

Sjors commented at 6:41 pm on February 22, 2021: member

That may be a rather big rebase hell though, because the commits make incremental changes to these files so I’d have to divide this commit in 10 pieces, squash them in the right place and then deal with rebase conflicts. Unless there’s an intelligent Git incantation to rename a file across all commits?

ryanofsky commented at 9:31 pm on February 22, 2021: member

Unless there’s an intelligent Git incantation to rename a file across all commits?

Something like this will rename the one file. You can extend it to rename other files or do more replacements across commits:

0end=f1824e7af7a8d53f9be50a6c6354511190dcb774
1start=b9a262ea0a86f498959e81113522ce26299f1984
2
3git checkout $end
4git filter-branch --tree-filter '
5  git mv src/wallet/externalsigner.h src/wallet/external_signer.h
6  git grep -l externalsigner | xargs sed -i s:wallet/externalsigner.h:wallet/external_signer.h:
7' --force $start^..HEAD

laanwj commented at 7:41 am on February 23, 2021: member

Also git tends to be smart enough to take into account renamed files when rebasing. If you go to the first commit with git rebase -i, amend it for editing, git mv src/wallet/externalsigner.h src/wallet/external_signer.h, then git rebase --continue, that should not cause any merge conflicts.

Sjors force-pushed on Feb 23, 2021

Sjors commented at 11:14 am on February 23, 2021: member

@ryanofsky’s thanks! That worked (with some tweaks), and despite warnings from the git, it still compiles and history looks sane. @laanwj I have indeed seen Git behave sanely when it comes to renames, so maybe my worries were not necessary.

wallet: add -signer argument for external signer command

Create basic ExternalSigner class with contructor. A Signer(<cmd>)
is added to CWallet on load if -signer=<cmd> is set.

8cf543f96d

test: add external signer test

Includes a mock to mimick the HWI interace.

f3e6ce78fb

wallet: add external_signer flag 157ea7c614

wallet: add ExternalSignerScriptPubKeyMan 8ce7767071

rpc: add external signer RPC files 07b7c940a7

rpc: signer: add enumeratesigners to list external signers 2700f09c41

rpc: add external_signer option to createwallet 2655197e1c

test: external_signer wallet flag is immutable 259f52cc33

wallet: add GetExternalSigner() fc5da520f5

wallet: ExternalSigner: add GetDescriptors method 7ebc7c0215

rpc: signerdisplayaddress 245b4457cf

rpc: send: support external signer d4b0107d68

doc: add external-signer.md f75e0c1edd

Sjors force-pushed on Feb 23, 2021

laanwj commented at 4:50 pm on February 23, 2021: member

re-ACK f75e0c1edde39a91cc353b0102638e232def9476

laanwj merged this on Feb 23, 2021

laanwj closed this on Feb 23, 2021

in doc/external-signer.md:40 in f75e0c1edd

35+The master key fingerprint is used to identify a device.
36+
37+Create a wallet, this automatically imports the public keys:
38+
39+```sh
40+$ bitcoin-cli createwallet "hww" true true "" true true true

MarcoFalke commented at 5:10 pm on February 23, 2021:

nit: Would be nice to use named args

in src/wallet/rpcsigner.cpp:35 in f75e0c1edd

30+                }
31+            }
32+        },
33+        RPCExamples{""},
34+        [](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue {
35+            std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);

MarcoFalke commented at 5:25 pm on February 23, 2021:

why does this rpc require a wallet?

Sjors commented at 7:34 pm on February 23, 2021:

I might get rid of that requirement in a followup.

sidhujag referenced this in commit a51a9d87b4 on Feb 23, 2021

Sjors deleted the branch on Feb 23, 2021

MarcoFalke referenced this in commit b54a10e777 on Feb 24, 2021

laanwj removed this from the "Blockers" column in a project

hebasto commented at 5:10 pm on March 2, 2021: member

There is a buggy execution path in the configure script. Fixed in #21339.

fanquake referenced this in commit 4f223e93e9 on Mar 3, 2021

fanquake referenced this in commit 993ecafa5e on Mar 17, 2021

Sjors referenced this in commit 19b30f3b57 on Mar 18, 2021

Sjors referenced this in commit 5eaf0d9421 on Mar 18, 2021

Sjors referenced this in commit cafc7d210d on Mar 19, 2021

Sjors referenced this in commit 1e8e41cc8a on Mar 22, 2021

Sjors referenced this in commit ef2adb407d on Apr 1, 2021

Sjors referenced this in commit 2110096202 on Apr 2, 2021

fanquake moved this from the "PRs" to the "Done" column in a project

Sjors referenced this in commit 0c9fd4f11d on Apr 7, 2021

Sjors referenced this in commit 0e416bd995 on Apr 8, 2021

Sjors referenced this in commit b0db187e5b on Apr 8, 2021

fanquake deleted a comment on Apr 8, 2021

fanquake locked this on Apr 8, 2021

External signer support - Wallet Box edition #16546

Conflicts