If there are multiple external signers, GetExternalSigner() will
just pick the first one in the list. If the user has two or more
hardware wallets connected at the same time, he might not notice this.
This PR adds a check and fails with suitable message, forcing the user to disconnect all but one external signer, so that there is no ambiguity as to which external signer was used.
44@@ -45,7 +45,8 @@ ExternalSigner ExternalSignerScriptPubKeyMan::GetExternalSigner() {
45 std::vector<ExternalSigner> signers;
46 ExternalSigner::Enumerate(command, signers, Params().NetworkIDString());
47 if (signers.empty()) throw std::runtime_error(std::string(__func__) + ": No external signers found");
48- // TODO: add fingerprint argument in case of multiple signers
49+ // TODO: add fingerprint argument instead of failing in case of multiple signers.
50+ if (signers.size() > 1) throw std::runtime_error(std::string(__func__) + ": More than one external signer found");
More than one external signer found. Please connect only one at a time.
Concept ACK
Maybe add test/functional/mocks/multi_signers.py to cover the error message. It would only need the enumerate stub. This can later be expanded as we improve multisig support.
There are two scenarios where GetExternalSigner is called:
For the first scenario ideally we want the user to pick one, but this would involve adding a fingerprint argument to the RPC call and expanding the GUI to select a device.
For the second scenario ideally we want to display the address on both devices, if it’s a multisig wallet. This would involve changing DisplayAddress to perform a two stage operation: first call enumerate, and then call GetExternalSigner for each fingerprint it recognises.
Until then though, I think it’s fine to just tell the user to only connect one external signer at a time.
The only caveat I can think of is that a user might have one device permanently connected to their machine, e.g. a signer for their lightning node, that they don’t want to use as a Bitcoin Core wallet. But that use case is handled poorly with or without this PR.
ACK 8e121cb43cd34239567c148e167c48d93097d1dc
It would really be preferable to allow the user to choose which signer rather than forcing one at a time, but this is fine for now.
77@@ -78,7 +78,7 @@ def run_test(self):
78 self.clear_mock_result(self.nodes[1])
79
80 result = self.nodes[1].enumeratesigners()
81- assert_equal(len(result['signers']), 2)
82+ assert_equal(len(result['signers']), 1)
83 assert_equal(result['signers'][0]["fingerprint"], "00000001")
84 assert_equal(result['signers'][0]["name"], "trezor_t")
This whole section can now be one line.
0- result = self.nodes[1].enumeratesigners()
1- assert_equal(len(result['signers']), 1)
2- assert_equal(result['signers'][0]["fingerprint"], "00000001")
3- assert_equal(result['signers'][0]["name"], "trezor_t")
4+ assert_equal(self.nodes[1].enumeratesigners(), {'signers': [{'fingerprint': '00000001', 'name': 'trezor_t'}]})
0@@ -0,0 +1,30 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2022 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+import sys
7+import argparse
8+import json
c4f2d06ecc4c48b9d080be35e0ec03dfdf75ffe5 looks good, and works with bitcoind.
However, the GUI crashes when creating a new wallet. One solution is to add a check in CreateWalletActivity::create() in walletcontroller.cpp right before it calls setSigners().
If there are multiple external signers, `GetExternalSigner()` will
just pick the first one in the list. If the user has two or more
hardware wallets connected at the same time, he might not notice this.
This PR adds a check and fails with suitable message.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
