Move external signer out of wallet module #21467

Sjors · 2021-03-18T14:23:22Z

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 · 2021-03-18T23:05:39Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

fanquake

Concept ACK. While you're moving this around, could you also pull in a change similar to fanquake@42228d7. Note there are changes other than whitespace & formattting.

src/rpc/external_signer.cpp

Sjors · 2021-03-22T14:51:39Z

@fanquake done

maflcko · 2021-03-22T18:45:54Z

src/wallet/rpcwallet.cpp

-        "walletdisplayaddress",
-        "Display address on an external signer for verification.\n",
+    return RPCHelpMan{"walletdisplayaddress",
+        "\nDisplay address on an external signer for verification.\n",


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.

maflcko · 2021-03-22T18:46:50Z

src/wallet/rpcwallet.cpp

-            if (!wallet) return NullUniValue;
-            CWallet* const pwallet = wallet.get();
+    [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+{


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 · 2021-03-23T05:26:27Z

@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 · 2021-03-26T09:28:58Z

@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 · 2021-04-07T18:57:16Z

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

test_framework.authproxy.JSONRPCException: rpc/util.cpp:539 (HandleRequest)
Internal 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?

src/wallet/rpcwallet.cpp

This commit moves the ExternalSigner class and RPC methods out of the wallet module. The enumeratesigners RPC can be used without a wallet since bitcoin#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.

An earlier version of bitcoin#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.

ryanofsky

Code review ACK 88d4d5f

RonSherfey

requested changes

fanquake

ACK 88d4d5f

c8f469c external_signer: remove ExternalSignerException (fanquake) 9e0b199 external_signer: use const where appropriate (fanquake) aaa4e5a wallet: remove CWallet::GetExternalSigner() (fanquake) 06a0673 external_signer: remove ignore_errors from Enumerate() (fanquake) 8fdbb89 refactor: unify external wallet runtime errors (fanquake) f4652bf refactor: add missing includes to external signer code (fanquake) 54569cc refactor: move all signer code inside ENABLE_EXTERNAL_SIGNER #ifdefs (fanquake) Pull request description: These are a few followups after #21467. ACKs for top commit: Sjors: tACK c8f469c instagibbs: utACK c8f469c Tree-SHA512: 3d5ac5df81680075e71e0e4a7595c520d746c3e37f016cf168c1e10da15541ebb1595aecaf2c08575636e9ff77d499644cae53180232b7049cfae0b923106e4e

DrahtBot added Build system Docs RPC/REST/ZMQ Wallet labels Mar 18, 2021

Sjors force-pushed the 2021/03/signer_no_wallet branch from 19b30f3 to 5eaf0d9 Compare March 18, 2021 17:03

Sjors force-pushed the 2021/03/signer_no_wallet branch from 5eaf0d9 to cafc7d2 Compare March 19, 2021 08:34

fanquake requested review from achow101 and instagibbs March 19, 2021 08:53

This was referenced Mar 22, 2021

MOVEONLY: CWallet transaction code out of wallet.cpp/.h #21207

Merged

refactor: Make CWalletTx sync state type-safe #21206

Merged

fanquake reviewed Mar 22, 2021

View reviewed changes

src/rpc/external_signer.cpp Outdated Show resolved Hide resolved

Sjors force-pushed the 2021/03/signer_no_wallet branch from cafc7d2 to a1991f4 Compare March 22, 2021 14:51

maflcko reviewed Mar 22, 2021

View reviewed changes

Sjors force-pushed the 2021/03/signer_no_wallet branch from a1991f4 to 1f6b367 Compare March 26, 2021 09:28

DrahtBot mentioned this pull request Mar 28, 2021

fuzz: Add fuzzing syscall sandbox: detect use of unexpected syscalls when fuzzing ("syscall sanitizer") #21538

Closed

Sjors mentioned this pull request Mar 28, 2021

UI external signer support (e.g. hardware wallet) bitcoin-core/gui#4

Merged

Sjors force-pushed the 2021/03/signer_no_wallet branch 2 times, most recently from a7b9a42 to 8b0c553 Compare April 2, 2021 17:00

Sjors mentioned this pull request Apr 2, 2021

rpc, gui: bumpfee signer support #21576

Merged

ryanofsky mentioned this pull request Apr 2, 2021

Drop JSONRPCRequest constructors after #21366 #21574

Merged

This was referenced Apr 3, 2021

Fix wrong wallet RPC context set after #21366 #21572

Merged

refactor: remove ::vpwallets and related global variables #19101

Merged

DrahtBot added the Needs rebase label Apr 7, 2021

Sjors force-pushed the 2021/03/signer_no_wallet branch from 8b0c553 to fdab2d0 Compare April 7, 2021 18:54

DrahtBot removed the Needs rebase label Apr 7, 2021

maflcko reviewed Apr 8, 2021

View reviewed changes

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Apr 8, 2021

Sjors force-pushed the 2021/03/signer_no_wallet branch from fdab2d0 to 80ed396 Compare April 8, 2021 08:15

DrahtBot removed the Needs rebase label Apr 8, 2021

Sjors added 3 commits April 8, 2021 17:56

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

b0db187

An earlier version of bitcoin#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.

rpc: add help for enumeratesigners and walletdisplayaddress

88d4d5f

Sjors force-pushed the 2021/03/signer_no_wallet branch from 80ed396 to 88d4d5f Compare April 8, 2021 15:56

bitcoin deleted a comment from xzavjierr Apr 9, 2021

ryanofsky approved these changes Apr 9, 2021

View reviewed changes

RonSherfey suggested changes Apr 11, 2021

View reviewed changes

fanquake approved these changes Apr 13, 2021

View reviewed changes

fanquake merged commit f0b4572 into bitcoin:master Apr 13, 2021

Sjors deleted the 2021/03/signer_no_wallet branch April 13, 2021 11:16

fanquake mentioned this pull request Apr 13, 2021

Miscellaneous external signer changes #21666

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 13, 2021

Merge bitcoin#21467: Move external signer out of wallet module

ad75fd3

Sjors mentioned this pull request Jun 15, 2021

Enable external signer support by default, reduce #ifdef #21935

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Move external signer out of wallet module #21467

Move external signer out of wallet module #21467

Uh oh!

Sjors commented Mar 18, 2021

Uh oh!

DrahtBot commented Mar 18, 2021 •

edited

Loading

Uh oh!

fanquake left a comment

Uh oh!

Uh oh!

Sjors commented Mar 22, 2021

Uh oh!

maflcko Mar 22, 2021

Uh oh!

maflcko Mar 22, 2021

Uh oh!

fanquake commented Mar 23, 2021

Uh oh!

Sjors commented Mar 26, 2021

Uh oh!

Sjors commented Apr 7, 2021

Uh oh!

Uh oh!

ryanofsky left a comment

Uh oh!

RonSherfey left a comment

Uh oh!

fanquake left a comment

Uh oh!

Uh oh!

Move external signer out of wallet module #21467

Move external signer out of wallet module #21467

Uh oh!

Conversation

Sjors commented Mar 18, 2021

Uh oh!

DrahtBot commented Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Mar 22, 2021

Uh oh!

maflcko Mar 22, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 22, 2021

Choose a reason for hiding this comment

Uh oh!

fanquake commented Mar 23, 2021

Uh oh!

Sjors commented Mar 26, 2021

Uh oh!

Sjors commented Apr 7, 2021

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

RonSherfey left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Mar 18, 2021 •

edited

Loading