GetExternalSigner(): fail if multiple signers are found #25235
Conversation
|
Resolves #22636 |
Sjors
left a comment
Sjors
left a comment
There was a problem hiding this comment.
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:
- When creating a new wallet
- When displaying an address
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.
amadeuszpawlik
force-pushed
the
fail_with_multiple_ext_signers
branch
from
3035182 to
f5341c2
Compare
|
@Sjors thanks for the input, test added! |
amadeuszpawlik
force-pushed
the
fail_with_multiple_ext_signers
branch
from
f5341c2 to
8e121cb
Compare
|
Looks good at first glance, will review and test in more detail. |
|
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. |
|
Concept ACK. Agree with @Sjors about the multisig additions we would have to make |
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
Nice work, ACK 8e121cb43cd34239567c148e167c48d93097d1dc with one suggested simplification
amadeuszpawlik
force-pushed
the
fail_with_multiple_ext_signers
branch
from
8e121cb to
c4f2d06
Compare
|
c4f2d06ecc4c48b9d080be35e0ec03dfdf75ffe5 looks good, and works with bitcoind. However, the GUI crashes when creating a new wallet. One solution is to add a check in |
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.
amadeuszpawlik
force-pushed
the
fail_with_multiple_ext_signers
branch
from
c4f2d06 to
292b1a3
Compare
|
Thanks @Sjors, I didn't catch that one. |
|
tACK 292b1a3 |
|
ACK 292b1a3 |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
|
Concept ACK |
…e found 292b1a3 GetExternalSigner(): fail if multiple signers are found (amadeuszpawlik) Pull request description: 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. ACKs for top commit: Sjors: tACK 292b1a3 achow101: ACK 292b1a3 Tree-SHA512: e2a41d3eecc607d4f94e708614bed0f3545f7abba85f300c5a5f0d3d17d72c815259734accc5ca370953eacd290f27894ba2c18016f5e9584cd50fa1ec2fbb0b
9 participants
If there are multiple external signers,
GetExternalSigner()willjust 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.