Move external signer out of wallet module

Sjors commented at 2:23 pm on March 18, 2021: member

In addition, this PR enables external signer testing on CI.

This PR moves the ExternalSigner class and RPC methods out of the wallet module.

The enumeratesigners RPC can be used without a wallet since #21417. With additional modifications external signers could be used without a wallet in general, e.g. via signrawtransaction.

The signerdisplayaddress RPC is ranamed to walletdisplayaddress because it requires wallet context. A future displayaddress RPC call without wallet context could take a descriptor argument.

This commit fixes a rpc_help.py failure when configured with --disable-wallet.

DrahtBot added the label Build system on Mar 18, 2021

DrahtBot added the label Docs on Mar 18, 2021

DrahtBot added the label RPC/REST/ZMQ on Mar 18, 2021

DrahtBot added the label Wallet on Mar 18, 2021

Sjors force-pushed on Mar 18, 2021

DrahtBot commented at 11:05 pm on March 18, 2021: 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:

#21538 (fuzz: Add fuzzing syscall sandbox: detect use of unexpected syscalls when fuzzing (“syscall sanitizer”) by practicalswift)
#21378 (Convert taproot to flag day activation by ajtowns)
#21207 (MOVEONLY: CWallet transaction code out of wallet.cpp/.h by ryanofsky)
#21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)
#20744 ([POC] Use std::filesystem. Remove Boost Filesystem & System by fanquake)
#20586 (Fix Windows build with –enable-werror by hebasto)
#20487 (Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) by practicalswift)
#19602 (wallet: Migrate legacy wallets to descriptor wallets by achow101)
#19438 (Introduce deploymentstatus by ajtowns)

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.

Sjors force-pushed on Mar 19, 2021

fanquake requested review from achow101 on Mar 19, 2021

fanquake requested review from instagibbs on Mar 19, 2021

in src/rpc/external_signer.cpp:7 in cafc7d210d outdated

2@@ -3,13 +3,12 @@
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 
5 #include <chainparamsbase.h>
6+#include <external_signer.h>
7 #include <key_io.h>

fanquake commented at 7:49 am on March 22, 2021:

I don’t think you need key_io.h anymore?

fanquake commented at 7:52 am on March 22, 2021: member

Concept ACK. While you’re moving this around, could you also pull in a change similar to https://github.com/fanquake/bitcoin/commit/42228d716a263cb787cbe8125c855518127e9449. Note there are changes other than whitespace & formattting.

Sjors force-pushed on Mar 22, 2021

Sjors commented at 2:51 pm on March 22, 2021: member

@fanquake done

in src/wallet/rpcwallet.cpp:4531 in a1991f44fa outdated

4526@@ -4527,36 +4527,36 @@ static RPCHelpMan upgradewallet()
4527 #ifdef ENABLE_EXTERNAL_SIGNER
4528 static RPCHelpMan walletdisplayaddress()
4529 {
4530-    return RPCHelpMan{
4531-        "walletdisplayaddress",
4532-        "Display address on an external signer for verification.\n",
4533+    return RPCHelpMan{"walletdisplayaddress",
4534+        "\nDisplay address on an external signer for verification.\n",

MarcoFalke commented at 6:45 pm on March 22, 2021:

nit: I think the leading and trailing newlines should be the responsibility of RPCHelpMan. Also, they wouldn’t make sense if the help is exported as json.

in src/wallet/rpcwallet.cpp:4538 in a1991f44fa outdated

4540-        [](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue {
4541-            std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
4542-            if (!wallet) return NullUniValue;
4543-            CWallet* const pwallet = wallet.get();
4544+    [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
4545+{

MarcoFalke commented at 6:46 pm on March 22, 2021:

nit: For new code it would be good to use properly clang-formatted code. Any reason to make a change to move away from clang-formatted code?

fanquake commented at 5:26 am on March 23, 2021: member

@Sjors I discussed with @MarcoFalke, and given there isn’t currently a correct RPCHelpMan formatting style, can you drop the formatting/whitespace changes? We’ll just add the [&]s, the examples etc. Sorry for the bum steer.

Sjors force-pushed on Mar 26, 2021

Sjors commented at 9:28 am on March 26, 2021: member

@fanquake done (I think) @MarcoFalke wrote:

I think the leading and trailing newlines should be the responsibility of RPCHelpMan. Also, they wouldn’t make sense if the help is exported as json.

I left out the newline characters.

Sjors force-pushed on Apr 1, 2021

Sjors force-pushed on Apr 2, 2021

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

DrahtBot added the label Needs rebase on Apr 7, 2021

Sjors force-pushed on Apr 7, 2021

Sjors commented at 6:57 pm on April 7, 2021: member

After rebase the command walletdisplayaddress(address1) in test/function/wallet_signer.py line 99 fails with

0test_framework.authproxy.JSONRPCException: rpc/util.cpp:539 (HandleRequest)
1Internal bug detected: 'std::any_of(m_results.m_results.begin(), m_results.m_results.end(), [ret](const RPCResult& res) { return res.MatchesType(ret); })'

Not sure what I’m doing wrong here… @MarcoFalke?

DrahtBot removed the label Needs rebase on Apr 7, 2021

in src/wallet/rpcwallet.cpp:4537 in fdab2d042f outdated

4532+    return RPCHelpMan{"walletdisplayaddress",
4533+        "Display address on an external signer for verification.",
4534+        {
4535+            {"address",     RPCArg::Type::STR, RPCArg::Optional::NO, /* default_val */ "", "bitcoin address to display"},
4536+        },
4537+        RPCResult{RPCResult::Type::NONE,"",""},

MarcoFalke commented at 7:08 am on April 8, 2021:

This is wrong. It is an obj

DrahtBot added the label Needs rebase on Apr 8, 2021

Sjors force-pushed on Apr 8, 2021

DrahtBot removed the label Needs rebase on Apr 8, 2021

Move external signer out of wallet module

This commit moves the ExternalSigner class and RPC methods out of the wallet module.

The enumeratesigners RPC can be used without a wallet since #21417.
With additional modifications external signers could be used without a wallet in general, e.g. via signrawtransaction.

The signerdisplayaddress RPC is ranamed to walletdisplayaddress because it requires wallet context.
A future displayaddress RPC call without wallet context could take a descriptor argument.

This commit fixes a rpc_help.py failure when configured with --disable-wallet.

b54b2e7b1a

ci: use --enable-external-signer instead of --with-boost-process

An earlier version of #16546 used both --with-boost-process and --enable-external-signer, which was simplified to only use the latter. However I forgot to update CI, so the external signer tests were not run.

b0db187e5b

rpc: add help for enumeratesigners and walletdisplayaddress 88d4d5ff2f

Sjors force-pushed on Apr 8, 2021

fanquake deleted a comment on Apr 9, 2021

ryanofsky approved

ryanofsky commented at 6:18 pm on April 9, 2021: member

Code review ACK 88d4d5ff2f5c71a9a2f4c78c2b2e2fd00568cfee

RonSherfey changes_requested

RonSherfey commented at 3:17 pm on April 11, 2021: none

requested changes

fanquake approved

fanquake commented at 6:34 am on April 13, 2021: member

ACK 88d4d5ff2f5c71a9a2f4c78c2b2e2fd00568cfee

fanquake merged this on Apr 13, 2021

fanquake closed this on Apr 13, 2021

Sjors deleted the branch on Apr 13, 2021

sidhujag referenced this in commit ad75fd3f1b on Apr 13, 2021

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

fanquake referenced this in commit e7af2f35af on Apr 14, 2021

DrahtBot locked this on Aug 18, 2022

Move external signer out of wallet module #21467

Conflicts