feat(wallet): hardware signing #6019
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request has conflicts, please rebase. |
f75e0c1 doc: add external-signer.md (Sjors Provoost) d4b0107 rpc: send: support external signer (Sjors Provoost) 245b445 rpc: signerdisplayaddress (Sjors Provoost) 7ebc7c0 wallet: ExternalSigner: add GetDescriptors method (Sjors Provoost) fc5da52 wallet: add GetExternalSigner() (Sjors Provoost) 259f52c test: external_signer wallet flag is immutable (Sjors Provoost) 2655197 rpc: add external_signer option to createwallet (Sjors Provoost) 2700f09 rpc: signer: add enumeratesigners to list external signers (Sjors Provoost) 07b7c94 rpc: add external signer RPC files (Sjors Provoost) 8ce7767 wallet: add ExternalSignerScriptPubKeyMan (Sjors Provoost) 157ea7c wallet: add external_signer flag (Sjors Provoost) f3e6ce7 test: add external signer test (Sjors Provoost) 8cf543f wallet: add -signer argument for external signer command (Sjors Provoost) f7eb7ec test: framework: add skip_if_no_external_signer (Sjors Provoost) 87a9794 configure: add --enable-external-signer (Sjors Provoost) Pull request description: Big picture overview in [this gist](https://gist.github.com/Sjors/29d06728c685e6182828c1ce9b74483d). This PR lets `bitcoind` call an arbitrary command `-signer=<cmd>`, e.g. a hardware wallet driver, where it can fetch public keys, ask to display an address, and sign a transaction (using PSBT under the hood). It's design to work with https://github.com/bitcoin-core/HWI, which supports multiple hardware wallets. Any command with the same arguments and return values will work. It simplifies the manual procedure described [here](https://github.com/bitcoin-core/HWI/blob/master/docs/bitcoin-core-usage.md). Usage is documented in [doc/external-signer.md]( https://github.com/Sjors/bitcoin/blob/2019/08/hww-box2/doc/external-signer.md), which also describes what protocol a different signer binary should conform to. Use `--enable-external-signer` to opt in, requires Boost::Process: ``` Options used to compile and link: with wallet = yes with gui / qt = no external signer = yes ``` It adds the following RPC methods: * `enumeratesigners`: asks <cmd> for a list of signers (e.g. devices) and their master key fingerprint * `signerdisplayaddress <address>`: asks <cmd> to display an address It enhances the following RPC methods: * `createwallet`: takes an additional `external_signer` argument and fetches keys from device * `send`: automatically sends transaction to device and waits Usage TL&DR: * clone HWI repo somewhere and launch `bitcoind -signer=../HWI/hwi.py` * check if you can see your hardware device: `bitcoin-cli enumeratesigners` * create wallet and auto import keys `bitcoin-cli createwallet "hww" true true "" true true true` * display address on device: `bitcoin-cli signerdisplayaddress ...` * to spend, use `send` RPC and approve transaction on device Prerequisites: - [x] bitcoin#21127 load wallet flags before everything else - [x] bitcoin#21182 remove mostly pointless BOOST_PROCESS macro Potentially useful followups: - GUI support: bitcoin-core/gui#4 - bumpfee support - (automatically) verify (a subset of) keys on the device after import, through message signing ACKs for top commit: laanwj: re-ACK f75e0c1 Tree-SHA512: 7db8afd54762295c1424c3f01d8c587ec256a72f34bd5256e04b21832dabd5dc212be8ab975ae3b67de75259fd569a561491945750492f417111dc7b6641e77f
…ion and typos 8b08d0f build, doc: Fix configure script output indentation and typos (Hennadii Stepanov) Pull request description: This PR is follow up of bitcoin#16546, that breaks the `configure` script output indentation for gui/qt/qr lines: ``` Options used to compile and link: external signer = no multiprocess = no with libs = yes with wallet = yes with sqlite = yes with bdb = yes with gui / qt = yes with qr = yes with zmq = yes with test = yes ... ``` With this PR: ``` Options used to compile and link: external signer = no multiprocess = no with libs = yes with wallet = yes with sqlite = yes with bdb = yes with gui / qt = yes with qr = yes with zmq = yes with test = yes ... ``` Also typos are fixed. ACKs for top commit: Sjors: utACK 8b08d0f vasild: ACK 8b08d0f Tree-SHA512: 46dfcfb754192dbcc080348781327d1714e2f9a696f3ed9252746b426e3afc628d84adb197ba3b8080eacaa6053ccac07e670998930ae92cef8ed713dd457c10
…ER]) unconditional a412813 build: Make AM_CONDITIONAL([ENABLE_EXTERNAL_SIGNER]) unconditional (Hennadii Stepanov) 9fef209 build, refactor: Fix indentation for if..then..fi (Hennadii Stepanov) Pull request description: bitcoin#16546 introduced a regression in the `configure`: ``` $ ./autogen.sh $ ./configure --disable-wallet --without-utils --without-daemon --without-gui --disable-tests --disable-bench ... checking whether to build test_bitcoin... no checking whether to reduce exports... no checking that generated files are newer than configure... done configure: error: conditional "ENABLE_EXTERNAL_SIGNER" was never defined. Usually this means the macro was only invoked conditionally. ``` This PR fixes this bug, and refactors indentation to make easier to spot similar bugs in the future. ACKs for top commit: Sjors: utACK a412813 fanquake: ACK a412813 - this fixes the bug described, and improves readability. Tree-SHA512: 4469dcc006690f38f93c3cdf8d15b76f5fc8ea76e87a1b5db5ee891dc9851f6ec539f2a6fd02a361aa76baa4f4b2b9fe8289137f5d9734ee5984f265cb131ef5
57ff5a4 doc: specify minimum HWI version (Sjors Provoost) 03308b2 rpc: don't require wallet for enumeratesigners (Sjors Provoost) Pull request description: HWI just released 2.0. See https://github.com/bitcoin-core/HWI/releases/tag/2.0.0 As of bitcoin#16546 we already rely on features that are in 2.0 and not in the previous 1.* releases: * `--chain` param This shouldn't be a problem, because HWI 2.0 has been released before we release v22. Misc improvements: * document that HWI 2.0 is required * drop wallet requirement for `enumeratesigners` ACKs for top commit: achow101: Code Review ACK 57ff5a4 Tree-SHA512: 3fb6ba20894e52a116f89525a5f5a1f61d363ccd904e1cffd0e6d095640fc6d2edf0388cd6ae20f83bbc31e5f458255ec090b6e823798d426eba3e45b4336bf9
88d4d5f rpc: add help for enumeratesigners and walletdisplayaddress (Sjors Provoost) b0db187 ci: use --enable-external-signer instead of --with-boost-process (Sjors Provoost) b54b2e7 Move external signer out of wallet module (Sjors Provoost) Pull request description: In addition, this PR enables external signer testing on CI. This PR moves the ExternalSigner class and RPC methods out of the wallet module. The `enumeratesigners` RPC can be used without a wallet since bitcoin#21417. With additional modifications external signers could be used without a wallet in general, e.g. via `signrawtransaction`. The `signerdisplayaddress` RPC is ranamed to `walletdisplayaddress` because it requires wallet context. A future `displayaddress` RPC call without wallet context could take a descriptor argument. This commit fixes a `rpc_help.py` failure when configured with `--disable-wallet`. ACKs for top commit: ryanofsky: Code review ACK 88d4d5f fanquake: ACK 88d4d5f Tree-SHA512: 3242a24e22313aed97eee32a520bfcb1c17495ba32a2b8e06a5e151e2611320e2da5ef35b572d84623af0a49a210d2f9377a2531250868d1a0ccf3e144352a97
c8f469c external_signer: remove ExternalSignerException (fanquake) 9e0b199 external_signer: use const where appropriate (fanquake) aaa4e5a wallet: remove CWallet::GetExternalSigner() (fanquake) 06a0673 external_signer: remove ignore_errors from Enumerate() (fanquake) 8fdbb89 refactor: unify external wallet runtime errors (fanquake) f4652bf refactor: add missing includes to external signer code (fanquake) 54569cc refactor: move all signer code inside ENABLE_EXTERNAL_SIGNER #ifdefs (fanquake) Pull request description: These are a few followups after bitcoin#21467. ACKs for top commit: Sjors: tACK c8f469c instagibbs: utACK bitcoin@c8f469c Tree-SHA512: 3d5ac5df81680075e71e0e4a7595c520d746c3e37f016cf168c1e10da15541ebb1595aecaf2c08575636e9ff77d499644cae53180232b7049cfae0b923106e4e
…allet) 1c4b456 gui: send using external signer (Sjors Provoost) 24815c6 gui: wallet creation detects external signer (Sjors Provoost) 3f845ea node: add externalSigners to interface (Sjors Provoost) 62ac119 gui: display address on external signer (Sjors Provoost) 450cb40 wallet: add displayAddress to interface (Sjors Provoost) eef8d64 gui: create wallet with external signer (Sjors Provoost) 6cdbc83 gui: add external signer path to options dialog (Sjors Provoost) Pull request description: Big picture overview in [this gist](https://gist.github.com/Sjors/29d06728c685e6182828c1ce9b74483d). This PR adds GUI support for external signers, based on the since merged 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): <img width="625" alt="Schermafbeelding 2019-08-05 om 19 32 59" src="https://user-images.githubusercontent.com/10217/62483415-e1ff1680-b7b7-11e9-97ca-8d2ce54ca1cb.png"> Then we add an external signer checkbox to the wallet creation dialog: <img width="374" alt="Schermafbeelding 2019-11-07 om 19 17 23" src="https://user-images.githubusercontent.com/10217/68416387-b57ee000-0194-11ea-9730-127d60273008.png"> 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...): <img width="673" alt="Schermafbeelding 2019-08-05 om 19 29 22" src="https://user-images.githubusercontent.com/10217/62483560-43bf8080-b7b8-11e9-9902-8a036116dc4b.png"> 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#21935. ACKs for top commit: achow101: Code Review ACK 1c4b456 hebasto: ACK 1c4b456, tested on Linux Mint 20.1 (Qt 5.12.8) with HWW `2.0.2-rc.1`. promag: Tested ACK 1c4b456 but rebased with e033ca1, with HWI 2.0.2, with Nano S and Nano X. meshcollider: re-code-review ACK 1c4b456 Tree-SHA512: 3503113c5c69d40adb6ce364d8e7cae23ce82d032a00474ba9aeb6202eb70f496ef4a6bf2e623e5171e524ad31ade7941a4e0e89539c64518aaec74f4562d86b
…n unsupported e60cd26 Do not load external signers wallets when unsupported (Andrew Chow) Pull request description: When external signer support is not compiled, do not load external signer wallets. Alternative to bitcoin#22168. ACKs for top commit: promag: Tested ACK e60cd26. meshcollider: Code review ACK e60cd26 Tree-SHA512: aed2d0038f448c2f89c6b48f412b106e63c9ed20e748e69aae21fb58c33fc7e4fa73375a52372c73788669eb2b968a8da6b022c65658fa4484f5bbcf205b1b15
…e #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 bitcoin-core/gui#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 bitcoin#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: bitcoin-core/gui#4 (comment) ACKs for top commit: achow101: ACK 2f5bdcb hebasto: re-ACK 2f5bdcb Tree-SHA512: 1b71c5a8bea2be077ee9fa33a01130c957a0cf90951d4b7b04d3d0ef826bb77e474c3963abddfef2e2c1ea99d9c72cd2302d1eb9b5fcb7ba0bd2a625f006aa05
…ocess 67669ab build: Fix Boost Process compatibility with mingw-w64 compiler (Hennadii Stepanov) Pull request description: On master (9c3751a) the cross build for Win64 is broken if configured with `--enable-external-signer`: ``` ... CXX crypto/libbitcoin_crypto_base_a-chacha_poly_aead.o In file included from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handles.hpp:11, from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/used_handles.hpp:17, from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/async_in.hpp:20, from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/async.hpp:49, from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process.hpp:23, from util/system.cpp:9: /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:208:51: error: expected ‘)’ before ‘*’ token 208 | typedef ::boost::winapi::NTSTATUS_ (__kernel_entry *nt_system_query_information_p )( | ~ ^~ | ) /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:223:51: error: expected ‘)’ before ‘*’ token 223 | typedef ::boost::winapi::NTSTATUS_ (__kernel_entry *nt_query_object_p )( | ~ ^~ | ) /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp: In function ‘boost::winapi::NTSTATUS_ boost::process::detail::windows::workaround::nt_system_query_information(boost::process::detail::windows::workaround::SYSTEM_INFORMATION_CLASS_, void*, boost::winapi::ULONG_, boost::winapi::PULONG_)’: /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:239:12: error: ‘nt_system_query_information_p’ does not name a type; did you mean ‘nt_system_query_information’? 239 | static nt_system_query_information_p f = reinterpret_cast<nt_system_query_information_p>(::boost::winapi::get_proc_address(h, "NtQuerySystemInformation")); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | nt_system_query_information In file included from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handles.hpp:11, from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/used_handles.hpp:17, from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/async_in.hpp:20, from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/async.hpp:49, from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process.hpp:23, from util/system.cpp:9: /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:241:14: error: ‘f’ was not declared in this scope 241 | return (*f)(SystemInformationClass, SystemInformation, SystemInformationLength, ReturnLength); | ^ /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp: In function ‘boost::winapi::BOOL_ boost::process::detail::windows::workaround::nt_query_object(boost::winapi::HANDLE_, boost::process::detail::windows::workaround::OBJECT_INFORMATION_CLASS_, void*, boost::winapi::ULONG_, boost::winapi::PULONG_)’: /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:253:12: error: ‘nt_query_object_p’ does not name a type; did you mean ‘nt_query_object’? 253 | static nt_query_object_p f = reinterpret_cast<nt_query_object_p>(::boost::winapi::get_proc_address(h, "NtQueryObject")); | ^~~~~~~~~~~~~~~~~ | nt_query_object /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:255:14: error: ‘f’ was not declared in this scope 255 | return (*f)(Handle, ObjectInformationClass, ObjectInformation, ObjectInformationLength, ReturnLength); | ^ make[2]: *** [Makefile:9906: util/libbitcoin_util_a-system.o] Error 1 make[2]: *** Waiting for unfinished jobs.... CXX crypto/libbitcoin_crypto_base_a-chacha20.o make[2]: Leaving directory '/home/hebasto/GitHub/bitcoin/src' make[1]: *** [Makefile:16141: all-recursive] Error 1 make[1]: Leaving directory '/home/hebasto/GitHub/bitcoin/src' make: *** [Makefile:820: all-recursive] Error 1 ``` The upstream bug: boostorg/process#96 Also see: https://stackoverflow.com/a/59338759 bitcoin#22348 (comment): > [This commit](boostorg/process@7fc41b2), containing the `__kernel_entry` [SAL annotations](https://docs.microsoft.com/en-us/cpp/code-quality/using-sal-annotations-to-reduce-c-cpp-code-defects?view=msvc-160) was included in Boost Process as part of the `1.71.0` release, which broke support for compiling with mingw-w64 because it doesn't define the `__kernel_entry` SAL annotation (but it does define some others, i.e see [`sal.h`](https://github.com/mirror/mingw-w64/blob/master/mingw-w64-headers/include/sal.h)). > > A [commit was made](boostorg/process@d7a721e) to remove the annotations, however, it hasn't made it into either of the two Boost releases that have happened since (1.75.0 & 1.76.0). Meaning that this is currently needed for all versions of Boost process from 1.71.0 onwards. ACKs for top commit: fanquake: ACK 67669ab - thanks for updating this. Tree-SHA512: 5931ca1fb77ce38c042cf5a7556add024ea2386c208bf26c792a8ca4a771d97fac9802c32fa8aa2e3de1ad35f3362d8c066f0a83ee675859d226c602fd0bcf93
…enBSD build guide e65d1d4 doc: recommend `--disable-external-signer` in OpenBSD build guide (Sebastian Falbesoner) Pull request description: Building the master branch with the default build settings (i.e. with external signer support enabled) leads to the following errors on my OpenBSD 6.9 machine: ``` In file included from util/system.cpp:9: In file included from /usr/local/include/boost/process.hpp:25: In file included from /usr/local/include/boost/process/group.hpp:32: /usr/local/include/boost/process/detail/posix/wait_group.hpp:38:17: error: no member named 'waitid' in the global namespace ret = ::waitid(P_PGID, p.grp, &status, WEXITED | WNOHANG); ~~^ /usr/local/include/boost/process/detail/posix/wait_group.hpp:38:24: error: use of undeclared identifier 'P_PGID' ret = ::waitid(P_PGID, p.grp, &status, WEXITED | WNOHANG); ^ /usr/local/include/boost/process/detail/posix/wait_group.hpp:38:48: error: use of undeclared identifier 'WEXITED' ret = ::waitid(P_PGID, p.grp, &status, WEXITED | WNOHANG); ^ /usr/local/include/boost/process/detail/posix/wait_group.hpp:144:17: error: no member named 'waitid' in the global namespace ret = ::waitid(P_PGID, p.grp, &siginfo, WEXITED | WSTOPPED | WNOHANG); ~~^ /usr/local/include/boost/process/detail/posix/wait_group.hpp:144:24: error: use of undeclared identifier 'P_PGID' ret = ::waitid(P_PGID, p.grp, &siginfo, WEXITED | WSTOPPED | WNOHANG); ^ /usr/local/include/boost/process/detail/posix/wait_group.hpp:144:49: error: use of undeclared identifier 'WEXITED' ret = ::waitid(P_PGID, p.grp, &siginfo, WEXITED | WSTOPPED | WNOHANG); ^ /usr/local/include/boost/process/detail/posix/wait_group.hpp:144:59: error: use of undeclared identifier 'WSTOPPED' ret = ::waitid(P_PGID, p.grp, &siginfo, WEXITED | WSTOPPED | WNOHANG); ^ 7 errors generated. ``` This PR recommends passing `--disable-external-signer` in the OpenBSD build guide ([as suggested by laanwj](bitcoin#22294 (comment))). The same commit also bumps the OpenBSD version mentioned in the header to 6.9 -- I recently used this document to setup a Bitcoin Core build on 6.9 and the description and all mentioned versions were still valid (before external signer support was enabled by default). Would be nice if another OpenBSD user could confirm the build error. ACKs for top commit: laanwj: ACK e65d1d4 Tree-SHA512: c3ae7eca29cf42b4b52024477e1c3fb7242bbf9d809bc95f8fa08b2f9bf4bcfd4f22457d58569a208ac1d8e5fe41b270addd13d85a5bba0521a0f9e325288448
…abled without signers a9b9ca8 gui: ensure external signer option remains disabled without signers (Andrew Chow) Pull request description: When no external signers are available, the option to enable external signers should always be disabled. However the encrypt wallet checkbox can erroneously re-enable the external signer checkbox. To avoid this, CreateWalletDialog now stores whether signers were available during setSigners so that future calls to external_signer_checkbox->setEnabled can account for whether signers are available. Fixes dashpay#395 ACKs for top commit: hebasto: ACK a9b9ca8, tested on Linux Mint 20.2 (Qt 5.12.8). Sjors: tACK a9b9ca8 jarolrod: ACK a9b9ca8 Tree-SHA512: 98951bcadc23fce99a66ea2d367c44360989e888c253845a767e1f7085c594562d0f099de4130f4a078c5072aa7806294097d976ee6407291f3d3c5a4a608b44
…gic (stop on first match) d047ed7 external_signer: improve fingerprint matching logic (stop on first match) (Sebastian Falbesoner) Pull request description: The fingerprint matching logic in `ExternalSigner::SignTransaction` currently always iterates all inputs of a PSBT, even after a match has already been found. I guess the reason for that is not that it was not thought of, but rather the fact that breaking out of a nested loop is simply not possible (at least not without adding ugly constructs like gotos or extra state variables). This PR fixes this by using `std::any_of` from C++'s standard library, see http://www.cplusplus.com/reference/algorithm/any_of/ ACKs for top commit: lsilva01: Code Review ACK bitcoin@d047ed7 Sjors: utACK d047ed7 Zero-1729: crACK d047ed7 mjdietzx: Code review ACK d047ed7 hebasto: ACK d047ed7, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 447e7c0c6a5b5549a2c09d52e55ba4146302c1a06e4d96de11f6945d09f98c89129cba221202dff7e0718e01a83dd173b9f19b1f02b6be228978f3f6e35d8096
a032fa3 multiprocess: add interfaces::ExternalSigner class (Russell Yanofsky) Pull request description: Add `interfaces::ExternalSigner` class to let signer objects be passed between processes and let signer code run in the original process where the object was created. --- This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). ACKs for top commit: laanwj: Concept and code review ACK a032fa3 hebasto: re-ACK a032fa3 Tree-SHA512: 99a729fb3a64d010e142cc778a9f1f358e58345b77faaf2664de7d2277715d59df3352326e8f0f2a6628038670eaa4556310a549079fb28af6d2eeb05aea1460
5493e92 Check descriptors returned by external signers (sstone) Pull request description: Check that descriptors returned by external signers have been parsed properly when creating a new wallet. See bitcoin#23627 for context. The problem is that parsing an invalid descriptor will return `null` which is not checked for in `CWallet::SetupDescriptorScriptPubKeyMans()`. I'm not completely sure what the best fix is since there several strategies for dealing with errors in the current codebase but the proposed fix is very simple and consistent with other validation checks in `CWallet::SetupDescriptorScriptPubKeyMans()`. ACKs for top commit: jamesob: Code review ACK bitcoin@5493e92 achow101: ACK 5493e92 Tree-SHA512: 63259f4aa519405a86c554b6813efdb741314bdaa18bf005b70ea8bb92a27abc6e2b65f7c584641dc257fc78a6499f42b51b5310c243e611c4663430dccf3d04
b75f4c8 RPC: Return external_signer in getwalletinfo (Kristaps Kaupe) Pull request description: Add `external_signer` to the result object of `getwalletinfo` RPC which indicates whether `WALLET_FLAG_EXTERNAL_SIGNER` flag is set for the wallet. ACKs for top commit: S3RK: utACK b75f4c8 achow101: ACK b75f4c8 prayank23: utACK bitcoin@b75f4c8 brunoerg: utACK b75f4c8 Tree-SHA512: 066ccb97541fd4dc3d9728834645db714a3c8c93ccf29142811af4d79cfb9440a97bbb6c845434a909bc6e1775ef3737fcbb368c1f0582bc63973f6deb17a45f
…or message 7f3a6a9 wallet: Add external-signer-support specific error message (Hennadii Stepanov) Pull request description: On master (5f44c5c) an attempt to load an external signer wallet using Bitcoin Core compiled without external signer support fails with the following log messages: ``` 2022-02-20T19:01:11Z [qt-walletctrl] Using SQLite Version 3.31.1 2022-02-20T19:01:11Z [qt-walletctrl] Using wallet /home/hebasto/.bitcoin/testnet3/wallets/coldcard-0220 2022-02-20T19:01:11Z [qt-walletctrl] init message: Loading wallet… 2022-02-20T19:01:11Z [qt-walletctrl] [coldcard-0220] Error: External signer wallet being loaded without external signer support compiled 2022-02-20T19:01:11Z [qt-walletctrl] [coldcard-0220] Releasing wallet ``` While log messages are good, a message in the GUI window is completely misleading:  This PR fixes this issue:  ACKs for top commit: achow101: ACK 7f3a6a9 kristapsk: ACK 7f3a6a9 brunoerg: crACK 7f3a6a9 Tree-SHA512: a4842751c0ca8a37ccc3ea00503678f6b712a7f53d6cbdc07ce02dcb85ca8a94890d1c2da20307be043faa347747abeba29185c88ba12edd5253bfca56531585
…rnalSigner::SignTransaction 2a22f03 parsing external signer master fingerprint string as bytes instead of caring for lower/upper case in ExternalSigner::SignTransaction (avirgovi) Pull request description: Some external signers scripts may provide master fingerprint in uppercase format. In that case core will fail with `Signer fingerprint 00000000 does not match any of the inputs` as it only works with lowercase format. Even if the fingerprints match, yet one is lowercase the other uppercase. ExternalSigner::SignTransaction is the only place where it is needed IMO, as changing it in other places may break the communication with the external signer (i.e. enumerating with lowercase may not find the device). ACKs for top commit: achow101: ACK 2a22f03 theStack: Code-review ACK 2a22f03 Sjors: utACK 2a22f03 Tree-SHA512: f3d84b83fb0b5e06c405eaf9bf20a2fa864bf4172fd4de113b80b9b9a525a76f2f8cf63031b480358b3a7666023a2aef131fb89ff50448c66df3ed541da10f99
…gner` configure option 8df063e build: Fix help string for `--enable-external-signer` configure option (Hennadii Stepanov) Pull request description: This PR is a follow up of bitcoin#24065 and fixes the help string according to the actual default value https://github.com/bitcoin/bitcoin/blob/816ca01650f4cc66a61ac2f9b0f8b74cd9cd0cf8/configure.ac#L324-L327 ACKs for top commit: kristapsk: cr utACK 8df063e jarolrod: ACK 8df063e Tree-SHA512: ad3f457a53c9238ddd8ded9efd1224e564e6cb9da8b7ff7733a11e32a7daad5c0f6c6223509218f44944a874470cb0d2447897662eaf4e78c763b30785717c50
…alizing a99ed89 psbt: sign without finalizing (Andrew Chow) Pull request description: It can be useful to sign an input with `walletprocesspsbt` but not finalize that input if it is complete. This PR adds another option to `walletprocesspsbt` to be able to do that. We will still finalize by default. This does not materially change the PSBT workflow since `finalizepsbt` needs to be called in order to extract the tx for broadcast. ACKs for top commit: meshcollider: utACK a99ed89 Sjors: utACK a99ed89 Tree-SHA512: c88e5d3222109c5f4e763b1b9d97ce4655f68f2985a4509caab2d4e7f5bac5047328fd69696e82a330f5c5a333e0312568ae293515689b77a4747ca2f17caca6
|
This pull request has conflicts, please rebase. |
…ationDialog BACKPORT NOTE: Formatting in src/qt/optionsmodel.h has been changed in bitcoin#3267. These changes has not been backported on time; done with this backport 742918c qt: hide Create Unsigned button behind an expert mode option (Andrew Chow) 5c3b800 qt: Add Create Unsigned button to SendConfirmationDialog (Andrew Chow) Pull request description: Instead of having different buttons or changing button behavior for making a PSBT, just have SendConfirmationDialog return whether the user wants a PSBT or a broadcasted transaction. Since this dialog is used by both the bumpFeeAction and the SendCoinsDialog, changes to both to support the different behavior is needed. They will check the return value of the SendConfirmationDialog for whether a PSBT needs to be created instead of checking whether private keys are disabled. Strings used in this dialog are being slightly modified to work with both private keys enabled and disabled wallets. Moved from bitcoin#18789 ACKs for top commit: jarolrod: ACK 742918c ryanofsky: Code review ACK 742918c. Just suggested changes since last review. Looks great! hebasto: ACK 742918c, tested on Linux Mint 20.2 (Qt 5.12.8). Tree-SHA512: dd29f4364c7b4f15befe8fe63257b26187918786b005e0f8336183270b1a162680b93f6ced60f0285c6e607c084cc0d24950fc68a8f9c056521ede614041be66
|
This pull request has conflicts, please rebase. |
❌ Backport Verification - CATASTROPHIC FAILUREOriginal Bitcoin commit: Critical violations detected:
This PR violates fundamental backporting principles:
Recommendation: Create focused, smaller PRs that backport specific Bitcoin commits for hardware wallet functionality, following the established backporting process with:
This PR has been automatically closed due to catastrophic validation failures. |
|
Automatically closed due to catastrophic validation failures. Please see the detailed analysis above and create focused PRs with proper Bitcoin commit references and proportional scope. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
IT DEPENDS ON DASHIFY OF HWI REPO which is not done at the moment
Issue being fixed or feature implemented
What was done?
Mostly, bitcoin backports
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran
to see how your change affects other areas of the code, etc.
Breaking Changes
Please describe any breaking changes your code introduces
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.