Miscellaneous external signer changes #21666

1. 
   fanquake commented at 1:01 pm on April 13, 2021: member

   These are a few followups after #21467.
2. refactor: move all signer code inside ENABLE_EXTERNAL_SIGNER #ifdefs 54569cc6d6
3. refactor: add missing includes to external signer code f4652bf125
4. refactor: unify external wallet runtime errors

   Rather than 3 different messages that are confusing / leak
   implementation details, use a single message, that is similar to other
   wallet related messages. i.e:
   "Compiled without sqlite support (required for descriptor wallets)".

   8fdbb899b8
5. external_signer: remove ignore_errors from Enumerate()

   This is undocumented and unused.

   06a0673351
6. wallet: remove CWallet::GetExternalSigner() aaa4e5a45b
7. external_signer: use const where appropriate 9e0b199b97
8. external_signer: remove ExternalSignerException

   It's not clear why this need it's own exception class, as opposed to just
   throwing std::runtime_error().

   c8f469c6d5
9. fanquake requested review from Sjors on Apr 13, 2021
10. fanquake requested review from instagibbs on Apr 13, 2021
11. DrahtBot added the label RPC/REST/ZMQ on Apr 13, 2021
12. DrahtBot added the label Wallet on Apr 13, 2021
13. 
    Sjors commented at 7:37 pm on April 13, 2021: member

    tACK c8f469c6d50a8db6d92f0aed47a5d1cc82f30f7f

    Tested with a locally rebased https://github.com/bitcoin-core/gui/pull/4/files (with minimal changes).

    06a0673351282fff1673f3679a7cad9a7faaf987 was added so that the GUI doesn’t throw an error in the create wallet menu if HWI is configured and there’s a problem. But it looks like I’m not even using that. I’ll bring it back if I need it.

    How did you find the includes that were needed for add missing includes to external signer code?

    With regards to c8f469c it may be useful to catch HWI specific errors in the GUI in order to present more actionable advise, but we can revert if needed. For now the GUI just throws rather useless “Can’t list signers” / “Sign failed” / “Can’t display address” messages anyway.

14. 
    instagibbs commented at 10:48 pm on April 13, 2021: member

    utACK https://github.com/bitcoin/bitcoin/pull/21666/commits/c8f469c6d50a8db6d92f0aed47a5d1cc82f30f7f

    straight forward cleanups

15. 
    DrahtBot commented at 11:58 pm on April 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:

    • #21206 (refactor: Make CWalletTx sync state type-safe 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.

16. 
    fanquake commented at 1:36 am on April 14, 2021: member

    How did you find the includes that were needed for add missing includes to external signer code?

    Just by reading through the code and looking at std:: usage.

    I agree that other code can be (re-)added if/when it’s actually used.

17. fanquake merged this on Apr 14, 2021
18. fanquake closed this on Apr 14, 2021

    ---

19. fanquake deleted the branch on Apr 14, 2021
20. sidhujag referenced this in commit 0a5d8a5a86 on Apr 14, 2021
21. DrahtBot locked this on Aug 18, 2022

Contributors
fanquake Sjors instagibbs DrahtBot

Review Requested
Sjors instagibbs

Labels
Wallet RPC/REST/ZMQ