Enable external signer support by default, reduce #ifdef

Sjors commented at 7:53 pm on May 12, 2021: member

This follows the introduction of GUI support in https://github.com/bitcoin-core/gui/pull/4

I don’t think we should expect GUI users to self compile. This also enables external signer support by default for RPC users.

In addition this PR reduces the number of #ifdef ENABLE_EXTERNAL_SIGNER, which also fixes #21919. When compiled with --disable-external-signer such wallets can’t be created in RPC or GUI, but they can be loaded. Attempting any action that calls HWI will trigger an error.

Side-note: this PR may or may not (currently) break CI for the GUI repository, as explained here: https://github.com/bitcoin-core/gui/pull/4#issuecomment-769859001

DrahtBot added the label Build system on May 12, 2021

DrahtBot added the label GUI on May 12, 2021

DrahtBot added the label interfaces on May 12, 2021

DrahtBot added the label Wallet on May 12, 2021

in src/interfaces/node.h:114 in e5f3bda6c0 outdated

110@@ -110,6 +111,11 @@ class Node
111     //! Disconnect node by id.
112     virtual bool disconnectById(NodeId id) = 0;
113 
114+#ifdef ENABLE_EXTERNAL_SIGNER

ryanofsky commented at 8:54 pm on May 12, 2021:

Would be nice to drop #ifdef ENABLE_EXTERNAL_SIGNER in the interface here and just return an empty vector in the implementation or throw a not-implemented exception. Cross-process interfaces are trying not to be conditional to avoid build complexity and fragile incompatibilities between builds.

Also maybe in general in this PR, preprocessor is a little more widely used that it needs to be. Preprocessor is great when you need to write platform specific fragments or cut out a significant external or internal dependency like boost or the wallet. But if you just want to act conditionally, normal code is a good way to go. I’d try to avoid conditional compilation except for the parts of the code directly depending on boost.

Sjors commented at 1:35 pm on May 13, 2021:

I added a commit to reduce #ifdef ENABLE_EXTERNAL_SIGNER usage, including in the interface.

Rspigler commented at 9:32 pm on May 12, 2021: contributor

Some thoughts:

Arguments in favor: Many people use HWWs. This will lead to an increase in Core full node use. GUI users don’t self-compile (and shouldn’t be expected to) ~~Still opt-in for bitcoind~~ Advanced users can still compile GUI without

Arguments against: Requires boost::process Many security concerns were raised in this PR (https://github.com/bitcoin/bitcoin/pull/15382) - I can’t tell if they were addressed fully enough to change the default to enabled (@practicalswift, @jonasschnelli, @laanwj, @MarcoFalke)

DrahtBot commented at 2:56 am on May 13, 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:

#22100 (refactor: Clean up new wallet spend, receive files added #21207 by ryanofsky)
#21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)
#20744 ([POC] Use std::filesystem. Remove Boost Filesystem & System by fanquake)
#19162 (ci: tsan gui by MarcoFalke)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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 renamed this:
~~Enable external signer support for GUI builds~~
Enable external signer support for GUI builds, reduce #ifdef
on May 13, 2021

Sjors force-pushed on May 13, 2021

Sjors commented at 4:35 pm on May 13, 2021: member

The Win64 machine is failing to build the fuzz executable: undefined reference to ExternalSigner::GetDescriptors(int)’. It's configured with –disable-external-signer`, but that shouldn’t matter…

Sjors force-pushed on May 25, 2021

meshcollider referenced this in commit 68a89d7a46 on Jun 9, 2021

Sjors force-pushed on Jun 9, 2021

Sjors commented at 6:03 pm on June 9, 2021: member

Rebased after bitcoin-core/gui#4 was merged.

meshcollider added this to the milestone 22.0 on Jun 9, 2021

fanquake commented at 1:38 am on June 10, 2021: member

Windows build failure:

0/usr/bin/x86_64-w64-mingw32-ld: libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o):wallet.cpp:(.text+0x1011f): undefined reference to `ExternalSigner::GetDescriptors(int)'
1/usr/bin/x86_64-w64-mingw32-ld: libbitcoin_wallet.a(libbitcoin_wallet_a-external_signer_scriptpubkeyman.o):external_signer_scriptpubkeyman.cpp:(.text+0x422): undefined reference to `ExternalSigner::DisplayAddress(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const'
2/usr/bin/x86_64-w64-mingw32-ld: libbitcoin_wallet.a(libbitcoin_wallet_a-external_signer_scriptpubkeyman.o):external_signer_scriptpubkeyman.cpp:(.text+0xf68): undefined reference to `ExternalSigner::Enumerate(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<ExternalSigner, std::allocator<ExternalSigner> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
3/usr/bin/x86_64-w64-mingw32-ld: libbitcoin_wallet.a(libbitcoin_wallet_a-external_signer_scriptpubkeyman.o):external_signer_scriptpubkeyman.cpp:(.text+0x1496): undefined reference to `ExternalSigner::SignTransaction(PartiallySignedTransaction&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)'
4collect2: error: ld returned 1 exit status

Sjors commented at 8:00 am on June 10, 2021: member

@fanquake indeed. I was hoping a rebase would magically make it go away… #21935 (comment)

I also haven’t checked how this interacts with #22173.

in src/wallet/external_signer_scriptpubkeyman.h:36 in 6db75d69dc outdated

30@@ -32,5 +31,3 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan
31   TransactionError FillPSBT(PartiallySignedTransaction& psbt, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr) const override;
32 };
33 #endif
34-
35-#endif // BITCOIN_WALLET_EXTERNAL_SIGNER_SCRIPTPUBKEYMAN_H

meshcollider commented at 8:53 pm on June 10, 2021:

I think you should keep the comment for style

Sjors commented at 4:41 pm on June 13, 2021:

I’m surprised the linter didn’t pick that up, fixed.

achow101 commented at 9:14 pm on June 10, 2021: member

I also haven’t checked how this interacts with #22173.

#22173 can be reverted following the changes in this PR.

achow101 commented at 10:29 pm on June 10, 2021: member

The Windows linker error is because it is building without external signer enabled. I am able to replicate the same error on linux by using --disable-external-signer. I haven’t been able to figure out why there’s a linker error.

hebasto commented at 1:42 am on June 11, 2021: member

In the “build: enable external signer by default” commit all --enable-external-signer could be removed from CI tasks.

hebasto commented at 3:02 am on June 11, 2021: member

The link error could be fixed with the following patch:

 0--- a/src/Makefile.am
 1+++ b/src/Makefile.am
 2@@ -399,6 +399,7 @@ endif
 3 libbitcoin_wallet_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(SQLITE_CFLAGS)
 4 libbitcoin_wallet_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
 5 libbitcoin_wallet_a_SOURCES = \
 6+  external_signer.cpp \
 7   wallet/coincontrol.cpp \
 8   wallet/context.cpp \
 9   wallet/crypter.cpp \
10@@ -539,7 +540,6 @@ libbitcoin_common_a_SOURCES = \
11   compressor.cpp \
12   core_read.cpp \
13   core_write.cpp \
14-  external_signer.cpp \
15   init/common.cpp \
16   key.cpp \
17   key_io.cpp \

Although, not sure if it is a proper fix.

hebasto commented at 4:30 pm on June 11, 2021: member

Here is another possible fixup for the link error:

 0--- a/src/Makefile.test.include
 1+++ b/src/Makefile.test.include
 2@@ -160,7 +160,7 @@ BITCOIN_TESTS += \
 3   wallet/test/scriptpubkeyman_tests.cpp
 4 
 5 FUZZ_SUITE_LD_COMMON +=\
 6- $(LIBBITCOIN_WALLET) \
 7+ $(LIBBITCOIN_WALLET) $(LIBBITCOIN_COMMON) \
 8  $(SQLITE_LIBS) \
 9  $(BDB_LIBS)
10

The choice between two suggested fixups depends on the decision which library the external_signer belongs to – libbitcoin_wallet or libbitcoin_common.

Sjors force-pushed on Jun 13, 2021

Sjors commented at 5:22 pm on June 13, 2021: member

@hebasto I tried your second approach. The ExternalSigner class is independent of the wallet, e.g. enumerate can be used and later on we can support signing stuff without a wallet (similar to signrawtransaction)

But that change didn’t help (LIBBITCOIN_COMMON is already part of FUZZ_SUITE_LD_COMMON).

I’m able to reproduce on Linux as well, like @achow101. Using something like:

0 ./configure --without-gui --disable-external-signer --disable-bench

It’s as if the fuzzer binary forgets to include something from LIBBITCOIN_COMMON. Paging @brakmic and @MarcoFalke too. @achow101 I reverted your commit

Also rebased, because who doesn’t like that?

In the “build: enable external signer by default” commit all --enable-external-signer could be removed from CI tasks.

Done

I’m marking this ready for review despite the above build issue.

Sjors force-pushed on Jun 13, 2021

Sjors marked this as ready for review on Jun 13, 2021

hebasto commented at 5:54 pm on June 13, 2021: member

@Sjors

@hebasto I tried your second approach. The ExternalSigner class is independent of the wallet, e.g. enumerate can be used and later on we can support signing stuff without a wallet (similar to signrawtransaction)

But that change didn’t help (LIBBITCOIN_COMMON is already part of FUZZ_SUITE_LD_COMMON).

It is not about being a part, but about ordering.

I’m able to reproduce on Linux as well, like @achow101. Using something like:
0 ./configure --without-gui --disable-external-signer --disable-bench

Works fine for me with #21935 (comment)

Sjors commented at 7:25 pm on June 13, 2021: member

Odd, it indeed does work. Maybe I forgot to clean something the last time.

Sjors force-pushed on Jun 13, 2021

hebasto approved

hebasto commented at 2:36 am on June 14, 2021: member

ACK 718fdce93c83f971d5fc70fe53c74d8a2165381a, tested on Linux Mint 20.1 (Qt 5.12.8) with --enable-external-signer and --disable-external-signer configure options. #21919 is fixed.

In commit “wallet: add missing external_signer.h include” (fe337a84652f3fd3a261d01587b0a2f032944ca1) including of the <external_signer.h> header could be improved:

add to node/interfaces.cpp, qt/walletcontroller.cpp
drop in wallet/scriptpubkeyman.cpp, wallet/wallet.h

In commit “build: enable external signer by default” (f24268f5fbf37de8a3716bd862f86dd7b831e80d) the --enable-external-signer configure option could be dropped in the 00_setup_env_native_multiprocess.sh as well.

Not related to this PR, but is there any reasons why NetworkArg and m_chain are not private in the ExternalSigner class?

meshcollider commented at 5:47 am on June 14, 2021: contributor

utACK 718fdce93c83f971d5fc70fe53c74d8a2165381a

Sjors commented at 11:01 am on June 14, 2021: member

Thanks. Will address the above comments if this needs rebase / touching.

Not related to this PR, but is there any reasons why NetworkArg and m_chain are not private in the ExternalSigner class?

Possibly because in earlier iterations I would use them directly.

promag commented at 1:02 pm on June 14, 2021: member

Tested ACK 718fdce93c83f971d5fc70fe53c74d8a2165381a.

nit, since walletdisplayaddress is not included when external signer is disabled then it could also hide verify button with:

 0diff --git a/src/qt/receiverequestdialog.cpp b/src/qt/receiverequestdialog.cpp
 1index abe7de8f89..015b6c0b8d 100644
 2--- a/src/qt/receiverequestdialog.cpp
 3+++ b/src/qt/receiverequestdialog.cpp
 4@@ -11,6 +11,9 @@
 5 #include <qt/qrimagewidget.h>
 6 #include <qt/walletmodel.h>
 7
 8+#include <interfaces/node.h>
 9+#include <interfaces/wallet.h>
10+
11 #include <QDialog>
12 #include <QString>
13
14@@ -90,7 +93,7 @@ void ReceiveRequestDialog::setInfo(const SendCoinsRecipient &_info)
15         ui->wallet_content->hide();
16     }
17
18-    ui->btnVerify->setVisible(this->model->wallet().hasExternalSigner());
19+    ui->btnVerify->setVisible(!model->node().externalSigners().empty() && this->model->wallet().hasExternalSigner());
20
21     connect(ui->btnVerify, &QPushButton::clicked, [this] {
22         model->displayAddress(info.address.toStdString());

achow101 commented at 7:38 pm on June 14, 2021: member

ACK 718fdce93c83f971d5fc70fe53c74d8a2165381a

in src/Makefile.test.include:167 in 718fdce93c outdated

160@@ -161,6 +161,7 @@ BITCOIN_TESTS += \
161 
162 FUZZ_SUITE_LD_COMMON +=\
163  $(LIBBITCOIN_WALLET) \
164+ $(LIBBITCOIN_COMMON) \

fanquake commented at 2:54 am on June 15, 2021:

yay for new circular dependencies 🙄. If this is going to be done, then this change should be in it’s own commit, with an explanation of the issue, and why this is the fix. Not thrown in with f24268f5fbf37de8a3716bd862f86dd7b831e80d. Or, ideally, this code is fixed so that this isn’t necessary.

Sjors commented at 2:14 pm on June 15, 2021:

All I can do here is split it into a separate commit and explain that I have no idea why it works. The error messages that lead to this were utterly useless. I have no idea what code is broken, so no idea what needs repairing. I also never worked on fuzzer code, which doesn’t help.

ryanofsky commented at 2:36 pm on June 15, 2021:

All I can do here is split it into a separate commit and explain that I have no idea why it works. The error messages that lead to this were utterly useless. I have no idea what code is broken, so no idea what needs repairing. I also never worked on fuzzer code, which doesn’t help.

This seems fine but I wonder what is confusing about the error messages. On the surface, the error messages seem to just be saying libbitcoin_wallet.a can’t find a few external_signer.cpp symbols. Since the symbols are in libbitcoin_common.a, it makes sense that linking libbitcoin_common.a after libbitcoin_wallet.a would fix the error.

I’m also a little confused by fanquake’s comment https://github.com/bitcoin/bitcoin/pull/21935/files#r651410470 about adding a new circular dependency. I don’t doubt that this PR adds one, but I’m not sure I see what it is. Maybe the comment is just referring to updating the Makefile to reflect circular dependencies in the code, though.

achow101 commented at 8:11 pm on June 15, 2021:

It looks like the linker error is simply due to the linking order. Everywhere else we have LIBBITCOIN_WALLET, it is always before LIBBITCOIN_COMMON. But if you go up to where FUZZ_SUITE_LD_COMMON is first set, you see that we will end up having LIBBITCOIN_COMMON set before LIBBITCOIN_WALLET which means that the linker will have problems linking things common things that the wallet uses. Because the order is correct for the other targets, we only see a linker error for test/fuzz/fuzz.

I would consider the following diff to be more correct than currently implemented:

 0diff --git a/src/Makefile.test.include b/src/Makefile.test.include
 1index 157d8ee2e1..fc2fd80166 100644
 2--- a/src/Makefile.test.include
 3+++ b/src/Makefile.test.include
 4@@ -35,11 +35,12 @@ BITCOIN_TEST_SUITE = \
 5   $(TEST_UTIL_H)
 6 
 7 FUZZ_SUITE_LD_COMMON = \
 8+ $(LIBTEST_UTIL) \
 9+ $(LIBTEST_FUZZ) \
10  $(LIBBITCOIN_SERVER) \
11+ $(LIBBITCOIN_WALLET) \
12  $(LIBBITCOIN_COMMON) \
13  $(LIBBITCOIN_UTIL) \
14- $(LIBTEST_UTIL) \
15- $(LIBTEST_FUZZ) \
16  $(LIBBITCOIN_CONSENSUS) \
17  $(LIBBITCOIN_CRYPTO) \
18  $(LIBBITCOIN_CLI) \
19@@ -159,12 +160,7 @@ BITCOIN_TESTS += \
20   wallet/test/ismine_tests.cpp \
21   wallet/test/scriptpubkeyman_tests.cpp
22 
23-# This adds libbitcoin again to fix linking external_signer_scriptpubkeyman.cpp
24-# when configured with --disable-external-signer. It is unclear why this is
25-# needed and may indicate a bug elsewhere.
26 FUZZ_SUITE_LD_COMMON +=\
27- $(LIBBITCOIN_WALLET) \
28- $(LIBBITCOIN_COMMON) \
29  $(SQLITE_LIBS) \
30  $(BDB_LIBS)
31

In this diff, LIBTEST_UTIL and LIBTEST_FUZZ are moved to the top because they include LIBBITCOIN_SERVER and LIBBITCOIN_COMMON. LIBBITCOIN_SERVER always needs to be the first item in the linker order since it has the most dependencies.

I think, in general, the makefiles for making the fuzz and test binaries should be revisited so that the linking order is made consistent with the rest of the code and to avoid other linker order issues that may crop up in the future.

Also 5c06af49efba407f062fdbf124b6d571fedd73fa does not build because of the linker error. This commit that fixes the linker order should go first so that all commits build.

fanquake changes_requested

fanquake commented at 3:44 am on June 15, 2021: member

Can you clean this up. The (re)-review burden here is basically non-existent, so I don’t understand why obvious issues wouldn’t be addressed. I’m also not sure what having to rebase (something I don’t think should be an issue for contributors) has to do with fixing anything.

Thanks. Will address the above comments if this needs rebase / touching.

This does need touching, because as pointed out by that comment, the changes here are incomplete. If the bar for “retouching” isn’t fixing a commit so that it’s actually complete / correct, then what is it?

Besides the current review comments, some other things to consider:

(depends build) Boost Process seems to use Boost ASIO, which uses std::allocator<void>, which was deprecated in C++17. This leads to a nice stack of warnings, at least on macOS:

 0bitcoin/depends/x86_64-apple-darwin20.5.0/include/boost/asio/associated_allocator.hpp:78:49: warning: 'allocator<void>' is deprecated [-Wdeprecated-declarations]
 1template <typename T, typename Allocator = std::allocator<void> >
 2                                                ^
 3/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/memory:730:28: note: 'allocator<void>' has been explicitly marked deprecated here
 4class _LIBCPP_TEMPLATE_VIS _LIBCPP_DEPRECATED_IN_CXX17 allocator<void>
 5                           ^
 6/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/__config:1058:39: note: expanded from macro '_LIBCPP_DEPRECATED_IN_CXX17'
 7#  define _LIBCPP_DEPRECATED_IN_CXX17 _LIBCPP_DEPRECATED
 8                                      ^
 9/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/__config:1035:48: note: expanded from macro '_LIBCPP_DEPRECATED'
10#    define _LIBCPP_DEPRECATED __attribute__ ((deprecated))

I’m also seeing unused-result warnings (from Boost) when building on my Ubuntu machine:

 0In file included from /home/ubuntu/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/process/detail/execute_impl.hpp:24,
 1                 from /home/ubuntu/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/process/child.hpp:22,
 2                 from /home/ubuntu/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/process/async_system.hpp:22,
 3                 from /home/ubuntu/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/process.hpp:24,
 4                 from util/system.cpp:9:
 5/home/ubuntu/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/process/detail/posix/executor.hpp: In member function ‘void boost::process::detail::posix::executor<Sequence>::write_error(const std::error_code&, const char*) [with Sequence = boost::fusion::joint_view<boost::fusion::tuple<boost::process::detail::posix::exe_cmd_init<char> >, boost::fusion::filter_view<const boost::fusion::tuple<const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, boost::process::detail::posix::pipe_out<1, -1>&, boost::process::detail::posix::pipe_out<2, -1>&, boost::process::detail::posix::pipe_in&>, boost::process::detail::is_initializer<mpl_::arg<-1> > > >]’:
 6/home/ubuntu/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/process/detail/posix/executor.hpp:156:16: warning: ignoring return value of ‘ssize_t write(int, const void*, size_t)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
 7  156 |         ::write(_pipe_sink, &len, sizeof(int));
 8      |         ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 9/home/ubuntu/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/process/detail/posix/executor.hpp:159:16: warning: ignoring return value of ‘ssize_t write(int, const void*, size_t)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
10  159 |         ::write(_pipe_sink, &len, sizeof(int));
11      |         ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
12/home/ubuntu/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/process/detail/posix/executor.hpp:160:16: warning: ignoring return value of ‘ssize_t write(int, const void*, size_t)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
13  160 |         ::write(_pipe_sink, msg, len);
14      |         ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
15  AR       libbitcoin_util.a

Also, it looks like a bunch of review comments from #15382 were either just never implemented / followed up on. Any reason for that?

Sjors commented at 2:21 pm on June 15, 2021: member

Also, it looks like a bunch of review comments from #15382 were either just never implemented / followed up on. Any reason for that?

I attempted to address several in #20614, but that was a dead end. Other things were done in master. Is there anything specific you’re missing?

I’ll look into the above issues meanwhile. Some of that seems to have been missed in bitcoin-core/gui#4 , but now is a good time to fix things.

ryanofsky commented at 2:45 pm on June 15, 2021: member

re: #21935#issue-643455659

This adds 3 commits on top of bitcoin-core/gui#4 and reverts #22173.

Does it say anywhere what motivation is for reverting #22173? It looks like it is no longer an error to load a wallet using an external signer when there is no external signer support compiled in. It seems like a good change, but maybe release notes or some description of the behavior change if there is one would be good. Is this the part of the PR that fixes #21919?

I don’t think we should expect GUI users to self compile.

For manual bitcoind builds this feature is still opt-in.

Is this still true? It seems like external signer default is just changed from no to yes for the whole build. Is the option applied differently to different binaries somehow?

Side-note: this PR may or may not (currently) break CI for the GUI repository, as explained here: bitcoin-core/gui#4 (comment)

Is this still true?

in src/node/interfaces.cpp:189 in 718fdce93c outdated

178         const std::string command = gArgs.GetArg("-signer", "");
179         if (command == "") return signers;
180         ExternalSigner::Enumerate(command, signers, Params().NetworkIDString());
181         return signers;
182+#else
183+        return {};

ryanofsky commented at 2:55 pm on June 15, 2021:

In commit “build: enable external signer by default” (f24268f5fbf37de8a3716bd862f86dd7b831e80d)

Could be nice to have a comment here explaining the return value. It seems unexpected that this would return an empty list instead of std::optional<std::vector> or maybe throwing an exception, because if this just returning an empty list the GUI doesn’t seem to have a way to distinguish between not finding any devices, and not looking for any devices.

Sjors force-pushed on Jun 15, 2021

Sjors commented at 3:00 pm on June 15, 2021: member

Rebased and addressed some of the above comments. Will continue shortly with the std::allocator issue and the display address button.

ryanofsky approved

ryanofsky commented at 3:01 pm on June 15, 2021: member

Code review ACK 718fdce93c83f971d5fc70fe53c74d8a2165381a. Great to see this feature enabled by default, and to be able to flexibly load wallets without the signer. Nice to remove a bunch of ifdef’s in the code, too!

Agree with fanquake moving up the dependency tweak to a different commit would be nice too, because we will probably revisit that at some point, and a separate commit would make the origin less mysterious.

ryanofsky approved

ryanofsky commented at 3:32 pm on June 15, 2021: member

Code review ACK d0ce4f2c6098761b3076d64ea65e37f1950cf085. Changes since last review: rebase, splitting commit fixing fuzz link error, adding commit cleaning up externalsigner class, and some tweaked includes

Sjors commented at 4:08 pm on June 15, 2021: member

@promag I’m not sure what change you’re proposing in #21935#pullrequestreview-682882043, but note that model->node().externalSigners() can be slow, since it calls hwi.py enumerate and the result is currently not cached.

@ryanofsky I’ll add a clarification to the revert commit. Basically as of 9599db6 loading an external signer wallet will just work, until you actually try to use the external signer. The commit describes the behaviour change, but I’ll check if I can split it.

On second thought, I’m going to unrevert #22173. It’s needlessly confusing to allow loading them.

Sjors renamed this:
~~Enable external signer support for GUI builds, reduce #ifdef~~
Enable external signer support by default, reduce #ifdef
on Jun 15, 2021

Sjors force-pushed on Jun 15, 2021

Sjors commented at 4:59 pm on June 15, 2021: member

For manual bitcoind builds this feature is still opt-in.

Is this still true? It seems like external signer default is just changed from no to yes for the whole build. Is the option applied differently to different binaries somehow?

You’re right, in an earlier incarnation of https://github.com/bitcoin-core/gui/pull/4 configure.ac would turn external signer support on by default if the GUI was compiled. That was lost, either in a rebase or because it was too complicated.

I updated the title reflect this change. Alternatively I can try to recover how I did it before.

Regarding the impact of this PR on the GUI repo, I don’t know, it depends on if @jonasschnelli changed the Win64 CI machine to match (or actually use) 00_setup_env_win64.sh. See https://github.com/bitcoin-core/gui/pull/4#issuecomment-769859001 (but it’s a trivial fix in any case). @fanquake regarding std::allocator<void> deprecation and its use in Boost::Process, that’s quite frustrating. Do we know if newer versions of Boost fix this?

Update: various Boost release notes mention improvements to std::allocator usage, but none specific to Boost ASIO. I’ll do some trial and error to see if a depends version bump fixes this (in a separate PR).

Update 2: nope, this an upstream headache, see #22254

Sjors force-pushed on Jun 15, 2021

promag commented at 5:29 pm on June 15, 2021: member

@promag I’m not sure what change you’re proposing in #21935 (review), but note that model->node().externalSigners() can be slow, since it calls hwi.py enumerate and the result is currently not cached.

Right! Anyway, not a problem now that #22173 is not reverted.

Sjors commented at 5:38 pm on June 15, 2021: member

I added one commit that addresses a bunch of small issues raised in the GUI PR, mostly adding translation hints, and disabling some fields when support is not compiled.

Rspigler commented at 11:42 pm on June 15, 2021: contributor

Was this ever fixed?

Sjors commented at 8:27 am on June 16, 2021: member

@Rspigler I attempted to in #20614, but got stuck. If someone can review that PR and provide some feedback, then I can rebase it and try again.

I applied @achow101’s fuzz linker fix and moved that commit up.

fuzz: fix fuzz binary linking order

We encountered a linking error when attempting to include external_signer_scriptpubkeyman.cpp when configured with --disable-external-signer.

Everywhere else we have LIBBITCOIN_WALLET, it is always before LIBBITCOIN_COMMON. But if you go up to where FUZZ_SUITE_LD_COMMON is first set, you see that we will end up having LIBBITCOIN_COMMON set before LIBBITCOIN_WALLET which means that the linker will have problems linking things common things that the wallet uses. Because the order is correct for the other targets, we only see a linker error for test/fuzz/fuzz.

In this diff, LIBTEST_UTIL and LIBTEST_FUZZ are moved to the top because they include LIBBITCOIN_SERVER and LIBBITCOIN_COMMON. LIBBITCOIN_SERVER always needs to be the first item in the linker order since it has the most dependencies.

The makefiles for making the fuzz and test binaries should be revisited so that the linking order is made consistent with the rest of the code and to avoid other linker order issues that may crop up in the future.

Co-Authored-By: Andrew Chow <achow101-github@achow101.com>

fc0eca31b3

Sjors force-pushed on Jun 16, 2021

refactor: clean up external_signer.h includes

Co-Authored-By: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>

7d9453041b

build: enable external signer by default 5be90c907e

refactor: reduce #ifdef ENABLE_EXTERNAL_SIGNER usage

In particular this make the node interface independent on whether external signer support is compiled.

4455145e26

refactor: make ExternalSigner NetworkArg() and m_chain private d672404466

gui: misc external signer fixes and translation hints 2f5bdcbc31

Sjors force-pushed on Jun 16, 2021

achow101 commented at 4:46 pm on June 16, 2021: member

ACK 2f5bdcbc31a2eeb7c11226a9e51c56f02ac807dd

hebasto approved

hebasto commented at 4:30 am on June 17, 2021: member

re-ACK 2f5bdcbc31a2eeb7c11226a9e51c56f02ac807dd

fanquake commented at 4:46 am on June 17, 2021: member

Thanks @ryanofsky, @achow101, @hebasto, @promag, @Rspigler and everyone else for your input here.

fanquake merged this on Jun 17, 2021

fanquake closed this on Jun 17, 2021

Sjors deleted the branch on Jun 17, 2021

sidhujag referenced this in commit 135344e6b3 on Jun 18, 2021

Rspigler commented at 4:05 am on June 21, 2021: contributor

Is there still a CI issue?

jonatack commented at 1:19 pm on June 21, 2021: member

Is there still a CI issue?

Not sure it’s related but the https://bitcoinbuilds.org Win64 job has been failing for a few days now with configure: error: Boost::Process is required for external signer support, but not available!

Sjors commented at 1:29 pm on June 21, 2021: member

@Rspigler @jonatack very likely related; the GUI build settings are hardcoded rather than using the config in ci: https://github.com/bitcoin-core/gui/pull/4#issuecomment-769859001

Rspigler commented at 9:34 pm on June 21, 2021: contributor

I don’t know how I feel about this being merged when there is still an outstanding CI issue, it introduces a new circular dependency, and there are some serious upstream boost::process issues and unresolved followups

Sjors commented at 9:35 am on June 22, 2021: member

@Rspigler the GUI CI is misconfigured, which is unrelated to this PR (and trivial to fix by whoever has access to it). It’s caused problems with other commits that touch CI too, see e.g. https://github.com/bitcoin-core/gui/issues/327

The PR no longer introduces a circular dependency (see fc0eca31b33f87882e2aa329a3746d4e08af1985).

It makes sense to emphasise that this is still experimental (descriptor wallets already are, which this builds on). But the Boost::Process code is unused if you don’t configure -signer=, so I don’t think that should block enabling the code. If you point to a malicious Python script, bad things can happen anyway, because that script runs as the same user. So the worst case scenario here is probably just a crash when you try to use a hardware wallet.

gwillen referenced this in commit 1a0a945459 on Jun 1, 2022

DrahtBot locked this on Aug 18, 2022

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

Conflicts