wallet: relax external_signer flag constraints #33112
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33112. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
https://cirrus-ci.com/task/6572167942373376?logs=ci#L1621: [06:14:38.327] /ci_container_base/src/wallet/rpc/wallet.cpp: In function ‘wallet::createwallet()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>’:
[06:14:38.327] /ci_container_base/src/wallet/rpc/wallet.cpp:420:45: error: ‘*(unsigned char*)((char*)&disable_private_keys + offsetof(std::optional<bool>,std::optional<bool>::<unnamed>.std::_Optional_base<bool, true, true>::<unnamed>))’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
[06:14:38.327] 420 | if (disable_private_keys.has_value() && *disable_private_keys) {
[06:14:38.327] | ^~~~~~~~~~~~~~~~~~~~~
[06:14:38.327] cc1plus: all warnings being treated as errors
[06:14:38.328] gmake[2]: *** [src/wallet/CMakeFiles/bitcoin_wallet.dir/build.make:370: src/wallet/CMakeFiles/bitcoin_wallet.dir/rpc/wallet.cpp.o] Error 1 |
src/wallet/rpc/wallet.cpp
Outdated
| #else | ||
| throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without external signing support (required for external signing)"); | ||
| #endif | ||
| } | ||
|
|
||
| if (disable_private_keys.has_value() && *disable_private_keys) { |
There was a problem hiding this comment.
not sure if this fixes the maybe-uninitialized false-positive from gcc, but you can try if (disable_private_keys.has_value() && disable_private_keys.value()) {, or add -Wno-error=maybe-uninitialized to the ci task config.
dee72e7 to
3909b0f
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
3909b0f to
5763b82
Compare
|
Added 7beb338 wallet: avoid createTransaction() with signer to prevent breaking GUI signing for private key enabled external signer wallets (even when they don't have other keys). |
5763b82 to
a973403
Compare
test/functional/wallet_signer.py
Outdated
| self.nodes[1].createwallet(wallet_name='not_hww', disable_private_keys=True, external_signer=False) | ||
| self.nodes[1].createwallet(wallet_name='not_hww', external_signer=False) |
There was a problem hiding this comment.
In 08f7813 "wallet: make watch-only optional for external signer"
Do you intend to add a test for an external signer wallet with private keys enabled in a later PR?
There was a problem hiding this comment.
I think that'll become more relevant, and probably easier to test, after MuSig2 support lands.
- use bitcoin rpc instead of bitcoin-cli - use named "external_signer" argument - mention GUI setting
There's no need to treat external signer wallets different in this regard. When the user sets the 'blank' flag, don't generate or import keys. For multisig setups that involve an external signer, it may be useful to start from a blank wallet and manually import descriptors.
External signer enabled wallets should always use the process PSBT flow. Avoid going through CreateTransaction. This has no effect until the next commit when WALLET_FLAG_EXTERNAL_SIGNER no longer implies WALLET_FLAG_DISABLE_PRIVATE_KEYS. Without this change signing with the GUI would break for external signers with private keys enabled.
Before this change the external_signer flag required the wallet to be watch-only. This precludes multisig setups in which we hold a hot key. Remove this as a requirement, but disable private keys by default. This leaves the typical (and only documented) use case of a single external signer unaffected.
With the removal of legacy wallets and the relaxing of restrictions in the previous commit, it's no longer a problem to toggle this flag.
a973403 to
0397853
Compare
|
Rebased and addressed nits. |
The
external_signerindicates that an external signer device may be called via HWI or equivalent application.When it was initially introduced some additional constraints were placed on wallets with this flag: it had to be a descriptor wallet and watch-only. Also the flag could not be added or removed later.
The constraints aren't a problem for the main supported and documented use case of connecting a single hardware wallet and using it just like a normal single sig Bitcoin Core wallet.
But they get in the way of more advanced constructions, which are becoming increasingly practical especially once MuSig2 support lands with #29675. See Sjors#91 and bitcoin-core/HWI#794 for a collection of changes on top of that which facilitate MuSig2 with a hardware wallet. And see #24861.
This pull requests drops the following constraints:
disable_private_keysis no longer mandatory (but still default)external_signerflag is now mutableAdditionally it does the following:
blankoption forcreatewalletconsistent with regular wallets by not importing keys from the connected signercreateTransactionfor external signers (otherwise the GUI breaks)external-signer.mdmainly to use named arguments and point out a GUI settingThis should no noticeable effect on the default single sig use case described above.