-
Notifications
You must be signed in to change notification settings - Fork 268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UI external signer support (e.g. hardware wallet) #4
Conversation
The original bitcoin/bitcoin#16549 (comment) has a Concept ACK from @fjahr. I'll split out the interface changes and PR them to bitcoin/bitcoin later. |
29e6a9d
to
3ad6941
Compare
…ders 0ecff9d Improve "detected inconsistent lock order" error message (Hennadii Stepanov) bbe9cf4 test: Improve "potential deadlock detected" exception message (Hennadii Stepanov) 3559934 Fix mistakenly swapped "previous" and "current" lock orders (Hennadii Stepanov) Pull request description: In master (8ef15e8) the "previous" and "current" lock orders are mistakenly swapped. This PR: - fixes printed lock orders - improves the `sync_tests` unit test - makes the "detected inconsistent lock order" error message pointing to the lock location rather `tfm::format()` location. Debugger output example with this PR (with modified code, of course): ``` 2020-06-22T15:46:56Z [msghand] POTENTIAL DEADLOCK DETECTED 2020-06-22T15:46:56Z [msghand] Previous lock order was: 2020-06-22T15:46:56Z [msghand] (2) 'cs_main' in net_processing.cpp:2545 (in thread 'msghand') 2020-06-22T15:46:56Z [msghand] (1) 'g_cs_orphans' in net_processing.cpp:1400 (in thread 'msghand') 2020-06-22T15:46:56Z [msghand] Current lock order is: 2020-06-22T15:46:56Z [msghand] (1) 'g_cs_orphans' in net_processing.cpp:2816 (in thread 'msghand') 2020-06-22T15:46:56Z [msghand] (2) 'cs_main' in net_processing.cpp:2816 (in thread 'msghand') Assertion failed: detected inconsistent lock order for 'cs_main' in net_processing.cpp:2816 (in thread 'msghand'), details in debug log. Process 131393 stopped * thread bitcoin-core#15, name = 'b-msghand', stop reason = signal SIGABRT frame #0: 0x00007ffff775c18b libc.so.6`__GI_raise(sig=2) at raise.c:51:1 (lldb) bt * thread bitcoin-core#15, name = 'b-msghand', stop reason = signal SIGABRT * frame #0: 0x00007ffff775c18b libc.so.6`__GI_raise(sig=2) at raise.c:51:1 frame bitcoin-core#1: 0x00007ffff773b859 libc.so.6`__GI_abort at abort.c:79:7 frame bitcoin-core#2: 0x0000555555e5b196 bitcoind`(anonymous namespace)::potential_deadlock_detected(mismatch=0x00007fff99ff6f30, s1=size=2, s2=size=2, lock_location=0x00007fff99ff7010) at sync.cpp:134:9 frame bitcoin-core#3: 0x0000555555e5a1b1 bitcoind`(anonymous namespace)::push_lock(c=0x0000555556379220, locklocation=0x00007fff99ff7010) at sync.cpp:158:13 frame bitcoin-core#4: 0x0000555555e59e8a bitcoind`EnterCritical(pszName="cs_main", pszFile="net_processing.cpp", nLine=2816, cs=0x0000555556379220, fTry=false) at sync.cpp:177:5 frame bitcoin-core#5: 0x00005555555b0500 bitcoind`UniqueLock<AnnotatedMixin<std::recursive_mutex>, std::unique_lock<std::recursive_mutex> >::Enter(this=0x00007fff99ff8c20, pszName="cs_main", pszFile="net_processing.cpp", nLine=2816) at sync.h:134:9 frame bitcoin-core#6: 0x00005555555b017f bitcoind`UniqueLock<AnnotatedMixin<std::recursive_mutex>, std::unique_lock<std::recursive_mutex> >::UniqueLock(this=0x00007fff99ff8c20, mutexIn=0x0000555556379220, pszName="cs_main", pszFile="net_processing.cpp", nLine=2816, fTry=false) at sync.h:160:13 frame bitcoin-core#7: 0x00005555556aa57e bitcoind`ProcessMessage(pfrom=0x00007fff90001180, msg_type=error: summary string parsing error, vRecv=0x00007fff9c005ac0, nTimeReceived=1592840815980751, chainparams=0x00005555564b7110, chainman=0x0000555556380880, mempool=0x0000555556380ae0, connman=0x000055555657aa20, banman=0x00005555565167b0, interruptMsgProc=0x00005555565cae90) at net_processing.cpp:2816:9 ``` ACKs for top commit: laanwj: ACK 0ecff9d vasild: ACK 0ecff9d Tree-SHA512: ff285de8dd3198b5b33c4bfbdadf9b1448189c96143b9696bc4f41c07e784c00851ec169cf3ed45cc325f3617ba6783620803234f57fcce28bf6bc3d6a7234fb
Shouldn't the external signer program be per-wallet? |
@luke-jr in practice there's only 1 program out there: HWI. It already knows what to do for different wallets based on their fingerprint. So, unless more such tools show up, I prefer to wait until we can store arbitrary settings in the wallet: bitcoin/bitcoin#13044 and then keep |
fd7c7e1
to
e9b4675
Compare
e9b4675
to
7df0674
Compare
@Bosch-0 oops, I accidentally lost a commit in my rebase fury. Try again. |
Awesome, that worked! Having some issues with HWI though. After installing and setting the path to the hwi.py file I keep getting an exception error when creating a wallet. How would we overcome having to install HWI separately? Would it have to be something like shipping HWI alongside the GUI. I'm a not too familiar with this process but having the path set by default behind the scenes would be ideal from a UX perspective. |
What's the error you're seeing? Also, can you try from the command-line: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested 1c4b456 with HWW 2.0.2-rc.1
on Linux Mint 20.1 (Qt 5.12.8).
UPDATE: it was my fault -- hww connection issues. Ignore this.
The "External signer" checkbox is disabled when trying Menu -> File -> Create Wallet...
It could be enabled by checking the "Encrypt Wallet" checkbox, then unchecking it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 1c4b456, tested on Linux Mint 20.1 (Qt 5.12.8) with HWW 2.0.2-rc.1
.
Tested:
- build with
--without-bdb
configure option - build with
--without-sqlite
configure option - creating a wallet
- verifying an address on the hww screen
- signing a tx
The following comments could be addressed in follow ups.
-
If no external signer is connected, it is possible to activate the "External signer" checkbox by checking the "Encrypt Wallet" checkbox, then unchecking it. Of course, the further attempt to create a wallet with an external signer will ends with the crash. This illness pattern works even if the client is compiled without SQLite support:
-
The "Confirm send coins" could still be opened while verifying transaction details on a signing device.
-
All new translatable strings could be annotated with translator comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// set to true, enable it when isEncryptWalletChecked is false. | ||
ui->disable_privkeys_checkbox->setEnabled(!checked); | ||
ui->external_signer_checkbox->setEnabled(!checked); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should take into account ENABLE_EXTERNAL_SIGNER
.
May I suggest adding a private slot update
or updateState
and move all logic there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a fix in bitcoin/bitcoin#21935. I agree this whole toggling code needs a refactor.
@@ -199,6 +199,7 @@ void OptionsDialog::setModel(OptionsModel *_model) | |||
connect(ui->prune, &QCheckBox::clicked, this, &OptionsDialog::togglePruneWarning); | |||
connect(ui->pruneSize, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &OptionsDialog::showRestartWarning); | |||
connect(ui->databaseCache, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &OptionsDialog::showRestartWarning); | |||
connect(ui->externalSignerPath, &QLineEdit::textChanged, [this]{ showRestartWarning(); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should disable externalSignerPath
if not defined ENABLE_EXTERNAL_SIGNER
?
@@ -89,6 +89,12 @@ void ReceiveRequestDialog::setInfo(const SendCoinsRecipient &_info) | |||
ui->wallet_tag->hide(); | |||
ui->wallet_content->hide(); | |||
} | |||
|
|||
ui->btnVerify->setVisible(this->model->wallet().hasExternalSigner()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, drop this->
.
#ifdef ENABLE_EXTERNAL_SIGNER | ||
std::vector<ExternalSigner> externalSigners() override | ||
{ | ||
std::vector<ExternalSigner> signers = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, drop = {}
?
ui->blank_wallet_checkbox->setChecked(false); | ||
ui->disable_privkeys_checkbox->setEnabled(false); | ||
ui->disable_privkeys_checkbox->setChecked(true); | ||
const std::string label = signers[0].m_name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could add comment "TODO show available signers in combobox, for now use 1st signer name"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@promag it is just the name of the wallet so I don't think it would be worth adding a combobox tbh. That might be confusing because it wouldn't actually change between signers, it would just change the name.
@@ -23,6 +27,10 @@ class CreateWalletDialog : public QDialog | |||
explicit CreateWalletDialog(QWidget* parent); | |||
virtual ~CreateWalletDialog(); | |||
|
|||
#ifdef ENABLE_EXTERNAL_SIGNER | |||
void setSigners(std::vector<ExternalSigner>& signers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit const &
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm adding that to: bitcoin/bitcoin#21935
@@ -170,6 +170,16 @@ class NodeImpl : public Node | |||
} | |||
return false; | |||
} | |||
#ifdef ENABLE_EXTERNAL_SIGNER | |||
std::vector<ExternalSigner> externalSigners() override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we let this throw runtime_error
? cc @ryanofsky
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these ifdefs are gone in bitcoin/bitcoin#21935
Code Review ACK 1c4b456 Mostly reviewed the code, did a little bit of testing. I noticed that when HWI is doing something, there is no indication to the user that it is. It would be nice if there was some dialog box that said something was happening, otherwise there can be a minute or two where Core doesn't indicate anything is happening, and nothing appears on the device. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-code-review ACK 1c4b456
Agree with achow101's comment about a 'working' message.
ui->encrypt_wallet_checkbox->setChecked(false); | ||
// The order matters, because connect() is called when toggling a checkbox: | ||
ui->blank_wallet_checkbox->setEnabled(false); | ||
ui->blank_wallet_checkbox->setChecked(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: to be consistent with gui: create wallet with external signer
, this should be true
(modulo comment about it being ambiguous anyway).
ui->blank_wallet_checkbox->setChecked(false); | ||
ui->disable_privkeys_checkbox->setEnabled(false); | ||
ui->disable_privkeys_checkbox->setChecked(true); | ||
const std::string label = signers[0].m_name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@promag it is just the name of the wallet so I don't think it would be worth adding a combobox tbh. That might be confusing because it wouldn't actually change between signers, it would just change the name.
@@ -313,14 +322,14 @@ bool SendCoinsDialog::PrepareSendText(QString& question_string, QString& informa | |||
formatted.append(recipientElement); | |||
} | |||
|
|||
if (model->wallet().privateKeysDisabled()) { | |||
if (model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about noPrivateKeyAccess()
or something - the condition is that there are keys either in the wallet itself or that the wallet can 'use' via the external signer. Not a big deal though.
…t, reduce #ifdef 2f5bdcb gui: misc external signer fixes and translation hints (Sjors Provoost) d672404 refactor: make ExternalSigner NetworkArg() and m_chain private (Sjors Provoost) 4455145 refactor: reduce #ifdef ENABLE_EXTERNAL_SIGNER usage (Sjors Provoost) 5be90c9 build: enable external signer by default (Sjors Provoost) 7d94530 refactor: clean up external_signer.h includes (Sjors Provoost) fc0eca3 fuzz: fix fuzz binary linking order (Sjors Provoost) Pull request description: This follows the introduction of GUI support in #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: #4 (comment) ACKs for top commit: achow101: ACK 2f5bdcb hebasto: re-ACK 2f5bdcb Tree-SHA512: 1b71c5a8bea2be077ee9fa33a01130c957a0cf90951d4b7b04d3d0ef826bb77e474c3963abddfef2e2c1ea99d9c72cd2302d1eb9b5fcb7ba0bd2a625f006aa05
post-merge ACK, wanted to document that I tested this out with a coldcard and the process was smooth. |
@Sjors I tried this with my trezor1, I unlocked it on the command line on a wallet with a bip39 password, this is what pops up when I click create wallet:
dunno if I'm doing something wrong |
Mmm, I'm not sure how the passphrase flow works. @achow101? I might be that we need to add support for that, e.g. if However in your case those are |
The passphrase should be cached by the device, so if you try again after running HWI manually, it should work. But we do need to implement a passphrase workflow for |
@achow101 I've never been able to get the trezor_1 passphrase cached with hwi, I've always had to do |
Oh right, Trezor 1 doesn't have that, only Trezor T. |
@achow101 fwiw it caches fine when using trezorctl, dunno what they are doing. |
Could you add release notes into https://github.com/bitcoin-core/bitcoin-devwiki/wiki/22.0-Release-Notes-draft#gui-changes? |
@hebasto done, also for the RPC. |
This is needed to turn globals into member variables. Otherwise, this will lead to issues: runtime error: reference binding to null pointer of type 'CBlockFileInfo' #0 in std::vector<CBlockFileInfo, std::allocator<CBlockFileInfo> >::operator[](unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_vector.h:1046:2 #1 in BlockManager::FlushBlockFile(bool, bool) src/node/blockstorage.cpp:540:47 #2 in CChainState::FlushStateToDisk(BlockValidationState&, FlushStateMode, int) src/validation.cpp:2262:28 #3 in CChainState::ResizeCoinsCaches(unsigned long, unsigned long) src/validation.cpp:4414:15 #4 in validation_chainstate_tests::validation_chainstate_resize_caches::test_method() src/test/validation_chainstate_tests.cpp:66:12
…ndbox fa45597 util: Add missing unlinkat to syscall sandbox (MarcoFalke) Pull request description: This will be needed for g++-12 (after libstdc++6 12-20220206). Steps to reproduce: ``` gdb --args ./src/bitcoind -sandbox=log-and-abort -regtest ./src/bitcoin-cli -regtest -named createwallet wallet_name=a descriptors=false ./src/bitcoin-cli -regtest stop ``` BT: ``` Thread 1 "b-shutoff" received signal SIGSYS, Bad system call. 0x00007ffff79564f7 in unlinkat () from /lib/x86_64-linux-gnu/libc.so.6 (gdb) bt #0 0x00007ffff79564f7 in unlinkat () from /lib/x86_64-linux-gnu/libc.so.6 #1 0x00007ffff7cc7335 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6 #2 0x00007ffff7cc94e3 in std::filesystem::remove_all(std::filesystem::__cxx11::path const&) () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6 #3 0x00005555559d4918 in wallet::BerkeleyEnvironment::Flush (this=0x7fffc4005160, fShutdown=<optimized out>) at /usr/include/c++/12/bits/fs_path.h:595 #4 0x000055555592c058 in wallet::StopWallets (context=...) at /usr/include/c++/12/bits/shared_ptr_base.h:1665 #5 0x00005555556617ca in Shutdown (node=...) at ./src/init.cpp:293 #6 0x000055555563ada6 in AppInit (argv=<optimized out>, argc=<optimized out>, node=...) at ./src/bitcoind.cpp:249 #7 main (argc=<optimized out>, argv=<optimized out>) at ./src/bitcoind.cpp:273 ACKs for top commit: laanwj: Code review ACK fa45597 Tree-SHA512: e80a38828f8656040954c9befa2d1c9d5170e204dc09c61031633349897f51ccd85cc5c99a089c4726d7f5237875cd9ed3fa8ef864cd6c1c8a2b8250b392d57f
fa7e147 test: Fix intermittent Tsan issue (MarcoFalke) Pull request description: Fix https://cirrus-ci.com/task/5176769937408000?logs=ci#L5161 ``` WARNING: ThreadSanitizer: data race (pid=22965) Write of size 8 at 0x7f74d5e21f50 by main thread: #0 std::__1::ios_base::precision(long) /usr/lib/llvm-13/bin/../include/c++/v1/ios:513:18 (test_bitcoin+0x1a8366) #1 boost::io::ios_base_all_saver::restore() /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/io/ios_state.hpp:341:17 (test_bitcoin+0x1a8366) #2 boost::unit_test::unit_test_log_t::operator<<(boost::unit_test::log::begin const&) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/unit_test_log.ipp:336:55 (test_bitcoin+0x1a8366) #3 boost::test_tools::tt_detail::report_assertion(boost::test_tools::assertion_result const&, boost::unit_test::lazy_ostream const&, boost::unit_test::basic_cstring<char const>, unsigned long, boost::test_tools::tt_detail::tool_level, boost::test_tools::tt_detail::check_type, unsigned long, ...) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/test_tools.ipp:359:19 (test_bitcoin+0x1b3b9b) #4 txindex_tests::txindex_initial_sync::test_method() src/test/txindex_tests.cpp:31:5 (test_bitcoin+0x78aebc) #5 txindex_tests::txindex_initial_sync_invoker() src/test/txindex_tests.cpp:16:1 (test_bitcoin+0x78a384) #6 boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:117:11 (test_bitcoin+0x2bf30d) #7 boost::function0<void>::operator()() const /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:763:14 (test_bitcoin+0x224027) #8 boost::detail::forward::operator()() /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:1368:32 (test_bitcoin+0x224027) #9 boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:137:18 (test_bitcoin+0x224027) #10 boost::function0<int>::operator()() const /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:763:14 (test_bitcoin+0x1ac66c) #11 int boost::detail::do_invoke<boost::shared_ptr<boost::detail::translator_holder_base>, boost::function<int ()> >(boost::shared_ptr<boost::detail::translator_holder_base> const&, boost::function<int ()> const&) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:290:30 (test_bitcoin+0x1ac66c) #12 boost::execution_monitor::catch_signals(boost::function<int ()> const&) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:879:16 (test_bitcoin+0x1ac66c) #13 boost::execution_monitor::execute(boost::function<int ()> const&) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:1277:16 (test_bitcoin+0x1ac980) #14 boost::execution_monitor::vexecute(boost::function<void ()> const&) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:1377:5 (test_bitcoin+0x1a7f9b) #15 boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned long) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/unit_test_monitor.ipp:49:9 (test_bitcoin+0x1a7f9b) #16 boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/framework.ipp:823:44 (test_bitcoin+0x1e0d5c) #17 boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/framework.ipp:792:58 (test_bitcoin+0x1e14a6) #18 boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/framework.ipp:792:58 (test_bitcoin+0x1e14a6) #19 boost::unit_test::framework::run(unsigned long, bool) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/framework.ipp:1696:29 (test_bitcoin+0x1a6bfb) #20 boost::unit_test::unit_test_main(boost::unit_test::test_suite* (*)(int, char**), int, char**) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/unit_test_main.ipp:248:9 (test_bitcoin+0x1c4ed6) #21 main /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/unit_test_main.ipp:304:12 (test_bitcoin+0x1c5506) Previous write of size 8 at 0x7f74d5e21f50 by thread T4: [failed to restore the stack] Location is global 'std::__1::cout' of size 160 at 0x7f74d5e21f30 (libc++.so.1+0x0000000cdf50) Thread T4 'b-txindex' (tid=22989, running) created by main thread at: #0 pthread_create <null> (test_bitcoin+0x1184cd) #1 std::__1::__libcpp_thread_create(unsigned long*, void* (*)(void*), void*) /usr/lib/llvm-13/bin/../include/c++/v1/__threading_support:514:10 (test_bitcoin+0xa23f1b) #2 std::__1::thread::thread<void (*)(char const*, std::__1::function<void ()>), char const*, BaseIndex::Start(CChainState&)::$_0, void>(void (*&&)(char const*, std::__1::function<void ()>), char const*&&, BaseIndex::Start(CChainState&)::$_0&&) /usr/lib/llvm-13/bin/../include/c++/v1/thread:307:16 (test_bitcoin+0xa23f1b) #3 BaseIndex::Start(CChainState&) src/index/base.cpp:363:21 (test_bitcoin+0xa23f1b) #4 txindex_tests::txindex_initial_sync::test_method() src/test/txindex_tests.cpp:31:5 (test_bitcoin+0x78adfa) #5 txindex_tests::txindex_initial_sync_invoker() src/test/txindex_tests.cpp:16:1 (test_bitcoin+0x78a384) #6 boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:117:11 (test_bitcoin+0x2bf30d) #7 boost::function0<void>::operator()() const /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:763:14 (test_bitcoin+0x224027) #8 boost::detail::forward::operator()() /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:1368:32 (test_bitcoin+0x224027) #9 boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:137:18 (test_bitcoin+0x224027) #10 boost::function0<int>::operator()() const /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:763:14 (test_bitcoin+0x1ac66c) #11 int boost::detail::do_invoke<boost::shared_ptr<boost::detail::translator_holder_base>, boost::function<int ()> >(boost::shared_ptr<boost::detail::translator_holder_base> const&, boost::function<int ()> const&) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:290:30 (test_bitcoin+0x1ac66c) #12 boost::execution_monitor::catch_signals(boost::function<int ()> const&) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:879:16 (test_bitcoin+0x1ac66c) #13 boost::execution_monitor::execute(boost::function<int ()> const&) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:1277:16 (test_bitcoin+0x1ac980) #14 boost::execution_monitor::vexecute(boost::function<void ()> const&) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:1377:5 (test_bitcoin+0x1a7f9b) #15 boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned long) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/unit_test_monitor.ipp:49:9 (test_bitcoin+0x1a7f9b) #16 boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/framework.ipp:823:44 (test_bitcoin+0x1e0d5c) #17 boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/framework.ipp:792:58 (test_bitcoin+0x1e14a6) #18 boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/framework.ipp:792:58 (test_bitcoin+0x1e14a6) #19 boost::unit_test::framework::run(unsigned long, bool) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/framework.ipp:1696:29 (test_bitcoin+0x1a6bfb) #20 boost::unit_test::unit_test_main(boost::unit_test::test_suite* (*)(int, char**), int, char**) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/unit_test_main.ipp:248:9 (test_bitcoin+0x1c4ed6) #21 main /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/impl/unit_test_main.ipp:304:12 (test_bitcoin+0x1c5506) SUMMARY: ThreadSanitizer: data race /usr/lib/llvm-13/bin/../include/c++/v1/ios:513:18 in std::__1::ios_base::precision(long) ================== Exit status: 2 ACKs for top commit: fanquake: CI ignored ACK fa7e147 Tree-SHA512: 5194e026410b96ad3c8addeecce0a55ee0271c3cfac9fa0715345b1a50d59925549cee0a3e415e5837ae6d2f214a7b622c73cfc7fdf41d5e55c24fb87fddb9d1
Big picture overview in this gist.
This PR adds GUI support for external signers, based on the since merged bitcoin/bitcoin#16546 (RPC).
The UX isn't amazing - especially the blocking calls - but it works.
First we adds a GUI setting for the signer script (e.g. path to HWI):
Then we add an external signer checkbox to the wallet creation dialog:
It's checked by default if HWI detects a device. It also grabs the name. It then creates a fresh wallet and imports the keys.
You can verify an address on the device (blocking...):
Sending, including coin selection, Just Works(tm) as long the device is present.
External signer support is enabled by default when the GUI is configured and Boost::Process is present.External signer support remains disabled by default, see bitcoin/bitcoin#21935.