wallet: Pass in transactions and messages for signing instead of exporting the private keys #18115

achow101 · 2020-02-11T05:23:22Z

Following #17261, the way to sign transactions, PSBTs, and messages was to use GetSigningProvider() and get a SigningProvider containing the private keys. However this may not be feasible for future ScriptPubKeyMans, such as for hardware wallets. Instead of exporting a SigningProvider containing private keys, we need to pass these things into the ScriptPubKeyMan (via CWallet) so that they can do whatever is needed internally to sign them. This is largely a refactor as the logic of processing transactions, PSBTs, and messages for is moved into LegacyScriptPubKeyMan and CWallet instead of being handled by the caller (e.g. signrawtransaction).

To help with this, I've refactored the 3(!) implementations of a SignTransaction() function into one generic one. This function will be called by signrawtransactionwithkey and LegacyScriptPubKeyMan::SignTransaction(). CWallet::CreateTransaction() is changed to call CWallet::SignTransaction() which in turn, calls LegacyScriptPubKeyMan::SignTransaction(). Other ScriptPubKeyMans may implement SignTransaction() differently.

FillPSBT() is moved to be a member function of CWallet and the psbtwallet.cpp/h files removed. It is further split so that CWallet handles filling the UTXOs while the ScriptPubKeyMan handles adding keys, derivation paths, scripts, and signatures. In the end LegacyScriptPubKeyMan::FillPSBT still calls SignPSBTInput, but the SigningProvider is internal to LegacyScriptPubKeyMan. Other ScriptPubKeyMans may do something different.

A new SignMessage() function is added to both CWallet and ScriptPubKeyMan. Instead of having the caller (i.e. signmessage or the sign message dialog) get the private key, hash the message, and sign, ScriptPubKeyMan will now handle that (CWallet passes through to the ScriptPubKeyMans as it does for many functions). This signing code is thus consolidated into LegacyScriptPubKeyMan::SignMessage(), though other ScriptPubKeyMans may implement it differently. Additionally, a SigningError enum is introduced for the different errors that we expect to see from SignMessage().

Lastly, GetSigningProvider() is renamed to GetPublicSigningProvider(). It will now only provide pubkeys, key origins, and scripts. LegacySigningProvider has it's GetKey and HaveKey functions changed to only return false. Future implementations should return HidingSigningProviders where private keys are hidden.

Other things like dumpprivkey and dumpwallet are not changed because they directly need and access the LegacyScriptPubKeyMan so are not relevant to future changes.

DrahtBot · 2020-02-11T05:47:27Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

Disallow automatic conversion between disparate hash types #17938 (Disallow automatic conversion between disparate hash types by Empact)
Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs #17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
wallet: reduce loading time by using unordered maps #16910 (wallet: reduce loading time by using unordered maps by achow101)
UI external signer support (e.g. hardware wallet) #16549 ([WIP] UI external signer support (e.g. hardware wallet) by Sjors)
External signer support - Wallet Box edition #16546 ([WIP] External signer support - Wallet Box edition by Sjors)
Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528 (Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101)
BIP-322: Generic signed message format #16440 (BIP-322: Generic signed message format by kallewoof)
wallet: Show fee in results for signrawtransaction* for segwit inputs #12911 (wallet: Show fee in results for signrawtransaction* for segwit inputs by kallewoof)

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.

Sjors · 2020-02-12T08:57:51Z

@kallewoof may find this interesting too in light of BIP-322 (Generic signed message format) #16440

vasild · 2020-02-12T09:34:26Z

src/qt/signverifymessagedialog.cpp

+        case SigningError::OK:
+            error = tr("No error");
+        case SigningError::PRIVATE_KEY_NOT_AVAILABLE:
+            error = tr("Private key for the entered address is not available.");


Missing break;?

Also these strings can be obtained fromSigningErrorString in error.cpp (the wording here seems better)

The strings from SigningErrorString won't be translated (we don't translate things for RPC), so that logic had to be duplicated here. Also, the strings are different to maintain compatibility.

Added the breaks in.

vasild · 2020-02-12T09:51:16Z

https://github.com/achow101/bitcoin/blob/e4714e0/src/rpc/misc.cpp#L340 still calls the old signing code key.SignCompact(); directly. Maybe that needs to be updated to also use the newly added function SignMessage()?

src/wallet/scriptpubkeyman.h

Sjors

Concept ACK. I roughly reviewed code for e4714e0 and left some comments.

0eb285f Refactor rawtransaction's SignTransaction into generic SignTransaction function would be easier to review (--color-moved=dimmed-zebra -w) if you renamed the variables in separate commit.

It may be better to combine 4595326 with be7991f so the signing code is moved, rather than just appear (though it's short enough to compare manually).

Lastly, GetSigningProvider() is renamed to GetPublicSigningProvider(). It will now only provide pubkeys, key origins, and scripts. LegacySigningProvider has it's GetKey and HaveKey functions changed to only return false. Future implementations should return HidingSigningProviders where private keys are hidden.

This bit is fairly confusing.

This PR is fairly large, and only deduplicates a subset of @vasild's #17577 (only what's needed to move forward with the wallet refactor). I didn't study this other PR in detail, but if it's possible to build on top of that, it would split the review burden and remove more duplication.

src/rpc/rawtransaction_util.h

Sjors · 2020-02-12T09:47:31Z

src/script/sign.cpp

+
+        // amount must be specified for valid segwit signature
+        if (amount == MAX_MONEY && !txin.scriptWitness.IsNull()) {
+            input_errors[i] = "Missing amount";


In 0eb285f this seems a bit brittle: input_errors[i] = "Missing amount"; (which is then matched by string to throw an exception later). But at least this error message is covered by rpc_rawtransaction.py

It is brittle, and I'm not sure why only that error specifically throws an exception. But keeping it for compatibility.

Sjors · 2020-02-12T09:52:03Z

src/script/sign.cpp

+        if (!VerifyScript(txin.scriptSig, spk, &txin.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, TransactionSignatureChecker(&tx_const, i, amount), &serror)) {
+            if (serror == SCRIPT_ERR_INVALID_STACK_OPERATION) {
+                // Unable to sign input and verification failed (possible attempt to partially sign).
+                input_errors[i] = "Unable to sign input, invalid stack size (possibly missing key)";


This is a good time to add tests for the three error strings used here (SCRIPT_ERR_INVALID_STACK_OPERATION, SCRIPT_ERR_SIG_NULLFAIL, else). Not sure how though.

src/wallet/wallet.cpp

Sjors · 2020-02-12T10:08:34Z

src/wallet/wallet.cpp

+    // Sign the tx with ScriptPubKeyMans
+    bool result = false;
+    for (const auto& spk_man_pair : m_spk_managers) {
+        result |= spk_man_pair.second->SignTransaction(tx, coins, sighash, input_errors);


The way input_errors are set, if there are multiple matching ScriptPubKeyMans* they would override each-others results. The last commit e4714e0 clears the error for any given input, but only if SPKman SignTransaction() returns true, which requires all input errors to be empty, so that seems useless. At minimum this is confusing.

* = not the case with legacy, and in practice shouldn't happen with descriptor wallets, but that's not enforced

The expectation is that all of the errors will be the same across all failed ScriptPubKeyMans because they don't have the private keys. The ScriptPubKeyMan which fully signs an input will clear the error for that input. This clearing is not dependent on input_errors.empty().

src/wallet/wallet.cpp

src/wallet/scriptpubkeyman.cpp

Sjors · 2020-02-12T10:31:33Z

src/qt/signverifymessagedialog.cpp

+        case SigningError::OK:
+            error = tr("No error");
+        case SigningError::PRIVATE_KEY_NOT_AVAILABLE:
+            error = tr("Private key for the entered address is not available.");


Also these strings can be obtained fromSigningErrorString in error.cpp (the wording here seems better)

achow101 · 2020-02-12T17:53:26Z

0eb285f Refactor rawtransaction's SignTransaction into generic SignTransaction function would be easier to review (--color-moved=dimmed-zebra -w) if you renamed the variables in separate commit.

I've undone the renames.

It may be better to combine 4595326 with be7991f so the signing code is moved, rather than just appear (though it's short enough to compare manually).

Squashed them together.

Lastly, GetSigningProvider() is renamed to GetPublicSigningProvider(). It will now only provide pubkeys, key origins, and scripts. LegacySigningProvider has it's GetKey and HaveKey functions changed to only return false. Future implementations should return HidingSigningProviders where private keys are hidden.

This bit is fairly confusing.

How so? I'm just saying that GetPublicSigningProvider() will just return a SigningProvider where GetKey() and HaveKey() will fail.

This PR is fairly large, and only deduplicates a subset of @vasild's #17577 (only what's needed to move forward with the wallet refactor). I didn't study this other PR in detail, but if it's possible to build on top of that, it would split the review burden and remove more duplication.

I'll take a look at that.

https://github.com/achow101/bitcoin/blob/e4714e0/src/rpc/misc.cpp#L340 still calls the old signing code key.SignCompact(); directly. Maybe that needs to be updated to also use the newly added function SignMessage()?

Done

kallewoof

Looked at code a bit, but not extensively.

The SignMessage for each scriptpubman is a step backwards considering BIP322 will unify these all into a single place. It would be great if you reviewed that PR (if you already have, and still feel this is the right approach, I'd love to know why).

I'll review/test this PR as well and comment more generally but wanted to put this out there now.

src/util/error.h

kallewoof · 2020-02-13T13:08:03Z

I am trying to merge BIP322 #16440 on top of this, and am running into an issue demonstrated by the below patch:

diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index bde8a6099..dde7819fa 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -559,6 +559,14 @@ static UniValue signmessage(const JSONRPCRequest& request)
     }

     const PKHash *pkhash = boost::get<PKHash>(&dest);
+
+    auto provider = pwallet->GetPublicSigningProvider(GetScriptForDestination(dest));
+    auto keyid = GetKeyForDestination(*provider, dest);
+    CKey secret;
+    if (!provider->GetKey(keyid, secret)) {
+        assert(0);
+    }
+
     if (!pkhash) {
         throw JSONRPCError(RPC_TYPE_ERROR, "Address does not refer to key");
     }

The above code hits the assert. I.e. despite getting a signing provider for the destination, it still fails to get the key. This code (i.e. getting signing provider and then getting privkey from that) in current master.

achow101 · 2020-02-13T15:40:38Z

The SignMessage for each scriptpubman is a step backwards considering BIP322 will unify these all into a single place. It would be great if you reviewed that PR (if you already have, and still feel this is the right approach, I'd love to know why).

You can still have the message signing code unified to a single place, and in fact, this PR does that (it consolidates the signmessage RPC and the Sign Message dialog signing implementations). Each ScriptPubKeyMan can still call that single unified function from within it's own SignMessage function. But each one having it's own SignMessage also allows us to implement it differently depending on the ScriptPubKeyMan. In particular, for hardware wallets, we can't use the unified SignMessage function because that would require producing a SigningProvider containing private keys, which is not possible for hardware wallets. In order to sign anything with hardware wallets, we need to pass in the thing to be signed to the device and let it handle the signing rather than signing it ourselves.

This change means that a future HardwareScriptPubKeyMan can do exactly that: the message is passed into HardwareScriptPubKeyMan::SignMessage which then passes it along to the hardware wallet. This way, nothing external to the wallet needs to know that there is a hardware wallet or how to handle it. That can all be done within the HardwareScriptPubKeyMan.

The above code hits the assert. I.e. despite getting a signing provider for the destination, it still fails to get the key. This code (i.e. getting signing provider and then getting privkey from that) in current master.

That's because you are still using the old code and way of signing messages. There is no need for signmessage to get a SigningProvider anymore. Instead you should be implementing either within the SignMessage function that is now in sign.cpp. You'll probably need to change the arguments, but you shouldn't need to go to the specific call sites like signmessage or the Sign Message dialog.

Specifically the reason this hits the assert is that the SigningProvider that is returned only returns public keys and scripts. GetKey and HaveKey have been changed to always fail. Notice that the function named GetSigningProvider was renamed to GetPublicSigningProvider.

achow101 · 2020-02-13T23:05:45Z

I've based this on top of #17577

kallewoof · 2020-02-14T04:03:25Z

That's because you are still using the old code and way of signing messages. There is no need for signmessage to get a SigningProvider anymore. Instead you should be implementing either within the SignMessage function that is now in sign.cpp. You'll probably need to change the arguments, but you shouldn't need to go to the specific call sites like signmessage or the Sign Message dialog.

Ahh. In that case PublicSigningProvider is a bad name for it, I think? It's not providing any signing at all, after all. I assume this is used to verify signatures? Maybe GetPublicKeyProvider.

meshcollider

Light code review / build / test run ACK 08b9b68

If Empact, instagibbs, or Sjors could post-rebase re-ACK I'll merge this

Sjors · 2020-03-09T10:38:55Z

Will do after @achow101 fixes the Travis build failure :-) https://travis-ci.org/bitcoin/bitcoin/jobs/659844515#L2418

Marking fillPSBT() as const in src/interfaces/wallet.{h,cpp} might help, and would at least be consistent.

…iptPubKeyMan::FillPSBT Instead of fetching a SigningProvider from ScriptPubKeyMan in order to fill and sign the keys and scripts for a PSBT, just pass that PSBT to a new FillPSBT function that does all that for us.

…allet and ScriptPubKeyMan Instead of getting a SigningProvider and then going to MessageSign, have ScriptPubKeyMan handle the message signing internally.

Not all ScriptPubKeyMans will be able to provide private keys, but pubkeys and scripts should be. So only provide public-only SigningProviders, i.e. ones that can help with Solving.

Make sure that there are no errors set for an input after it is signed. This is useful for when there are multiple ScriptPubKeyMans. Some may fail to sign, but one may be able to sign, and after it does, we don't want there to be any more errors there.

instagibbs · 2020-03-09T15:27:19Z

reACK d2774c0

achow101 · 2020-03-09T15:29:05Z

Will do after @achow101 fixes the Travis build failure :-) https://travis-ci.org/bitcoin/bitcoin/jobs/659844515#L2418

Marking fillPSBT() as const in src/interfaces/wallet.{h,cpp} might help, and would at least be consistent.

I did that, but I don't think it will help. I'm not sure what would cause that travis failure; it's failing on something that the other targets are compiling too. I think travis may have gotten confused?

Sjors · 2020-03-09T18:15:45Z

re-utACK d2774c0

meshcollider · 2020-03-09T19:56:16Z

re-utACK d2774c0

…gning instead of exporting the private keys d2774c0 Clear any input_errors for an input after it is signed (Andrew Chow) dc17488 Replace GetSigningProvider with GetSolvingProvider (Andrew Chow) 6a9c429 Move direct calls to MessageSign into new SignMessage functions in CWallet and ScriptPubKeyMan (Andrew Chow) 82a30fa Move key and script filling and signing from CWallet::FillPSBT to ScriptPubKeyMan::FillPSBT (Andrew Chow) 3d70dd9 Move FillPSBT to be a member of CWallet (Andrew Chow) a4af324 Use CWallet::SignTransaction in CreateTransaction and signrawtransactionwithwallet (Andrew Chow) f37de92 Implement CWallet::SignTransaction using ScriptPubKeyMan::SignTransaction (Andrew Chow) d999dd5 Add SignTransaction function to ScriptPubKeyMan and LegacyScriptPubKeyMan (Andrew Chow) 2c52b59 Refactor rawtransaction's SignTransaction into generic SignTransaction function (Andrew Chow) Pull request description: Following bitcoin#17261, the way to sign transactions, PSBTs, and messages was to use `GetSigningProvider()` and get a `SigningProvider` containing the private keys. However this may not be feasible for future `ScriptPubKeyMan`s, such as for hardware wallets. Instead of exporting a `SigningProvider` containing private keys, we need to pass these things into the `ScriptPubKeyMan` (via `CWallet`) so that they can do whatever is needed internally to sign them. This is largely a refactor as the logic of processing transactions, PSBTs, and messages for is moved into `LegacyScriptPubKeyMan` and `CWallet` instead of being handled by the caller (e.g. `signrawtransaction`). To help with this, I've refactored the 3(!) implementations of a `SignTransaction()` function into one generic one. This function will be called by `signrawtransactionwithkey` and `LegacyScriptPubKeyMan::SignTransaction()`. `CWallet::CreateTransaction()` is changed to call `CWallet::SignTransaction()` which in turn, calls `LegacyScriptPubKeyMan::SignTransaction()`. Other `ScriptPubKeyMan`s may implement `SignTransaction()` differently. `FillPSBT()` is moved to be a member function of `CWallet` and the `psbtwallet.cpp/h` files removed. It is further split so that `CWallet` handles filling the UTXOs while the `ScriptPubKeyMan` handles adding keys, derivation paths, scripts, and signatures. In the end `LegacyScriptPubKeyMan::FillPSBT` still calls `SignPSBTInput`, but the `SigningProvider` is internal to `LegacyScriptPubKeyMan`. Other `ScriptPubKeyMan`s may do something different. A new `SignMessage()` function is added to both `CWallet` and `ScriptPubKeyMan`. Instead of having the caller (i.e. `signmessage` or the sign message dialog) get the private key, hash the message, and sign, `ScriptPubKeyMan` will now handle that (`CWallet` passes through to the `ScriptPubKeyMan`s as it does for many functions). This signing code is thus consolidated into `LegacyScriptPubKeyMan::SignMessage()`, though other `ScriptPubKeyMan`s may implement it differently. Additionally, a `SigningError` enum is introduced for the different errors that we expect to see from `SignMessage()`. Lastly, `GetSigningProvider()` is renamed to `GetPublicSigningProvider()`. It will now only provide pubkeys, key origins, and scripts. `LegacySigningProvider` has it's `GetKey` and `HaveKey` functions changed to only return false. Future implementations should return `HidingSigningProvider`s where private keys are hidden. Other things like `dumpprivkey` and `dumpwallet` are not changed because they directly need and access the `LegacyScriptPubKeyMan` so are not relevant to future changes. ACKs for top commit: instagibbs: reACK bitcoin@d2774c0 Sjors: re-utACK d2774c0 meshcollider: re-utACK d2774c0 Tree-SHA512: 89c83e7e7e9315e283fae145a2264648a9d7f7ace8f3281cb3f44f0b013c988d67ba4fa9726e50c643c0ed921bdd269adaec984840d11acf4a681f3e8a582cc1

these were removed as part of bitcoin#18115 We should find a way to deal with it in the future

…n function Summary: Backport of Core [[bitcoin/bitcoin#18115 | PR18115]] part [1/9] : bitcoin/bitcoin@2c52b59 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: PiRK Differential Revision: https://reviews.bitcoinabc.org/D8094

…yMan Summary: Backport of Core [[bitcoin/bitcoin#18115 | PR18115]] part [2/9] : bitcoin/bitcoin@d999dd5 Depends on D8094 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: PiRK Differential Revision: https://reviews.bitcoinabc.org/D8095

…tion Summary: Backport of Core [[bitcoin/bitcoin#18115 | PR18115]] part [3/9] : bitcoin/bitcoin@f37de92 Depends on D8095 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: PiRK Differential Revision: https://reviews.bitcoinabc.org/D8096

…ionwithwallet Summary: Instead of duplicating signing code, just use the function we already have. Backport of Core [[bitcoin/bitcoin#18115 | PR18115]] part [4/9] : bitcoin/bitcoin@a4af324 Depends on D8098 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8100

Summary: Make sure that there are no errors set for an input after it is signed. This is useful for when there are multiple ScriptPubKeyMans. Some may fail to sign, but one may be able to sign, and after it does, we don't want there to be any more errors there. Backport of Core [[bitcoin/bitcoin#18115 | PR18115]] part [9/9] : bitcoin/bitcoin@d2774c0 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8103

Summary: Backport of Core [[bitcoin/bitcoin#18115 | PR18115]] part [5/9] : bitcoin/bitcoin@3d70dd9 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D8097

…iptPubKeyMan::FillPSBT Summary: Instead of fetching a SigningProvider from ScriptPubKeyMan in order to fill and sign the keys and scripts for a PSBT, just pass that PSBT to a new FillPSBT function that does all that for us. Backport of Core [[bitcoin/bitcoin#18115 | PR18115]] part [6/9] : bitcoin/bitcoin@82a30fa Depends on D8097 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8101

…allet and ScriptPubKeyMan Summary: Instead of getting a SigningProvider and then going to MessageSign, have ScriptPubKeyMan handle the message signing internally. Backport of Core [[bitcoin/bitcoin#18115 | PR18115]] part [7/9] : bitcoin/bitcoin@6a9c429 Depends on D8101 and D8098 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8102

Summary: Not all ScriptPubKeyMans will be able to provide private keys, but pubkeys and scripts should be. So only provide public-only SigningProviders, i.e. ones that can help with Solving. Backport of Core [[bitcoin/bitcoin#18115 | PR18115]] part [8/9] : bitcoin/bitcoin@dc17488 Depends on D8100 and D8102 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: PiRK Differential Revision: https://reviews.bitcoinabc.org/D8114

…gning instead of exporting the private keys d2774c0 Clear any input_errors for an input after it is signed (Andrew Chow) dc17488 Replace GetSigningProvider with GetSolvingProvider (Andrew Chow) 6a9c429 Move direct calls to MessageSign into new SignMessage functions in CWallet and ScriptPubKeyMan (Andrew Chow) 82a30fa Move key and script filling and signing from CWallet::FillPSBT to ScriptPubKeyMan::FillPSBT (Andrew Chow) 3d70dd9 Move FillPSBT to be a member of CWallet (Andrew Chow) a4af324 Use CWallet::SignTransaction in CreateTransaction and signrawtransactionwithwallet (Andrew Chow) f37de92 Implement CWallet::SignTransaction using ScriptPubKeyMan::SignTransaction (Andrew Chow) d999dd5 Add SignTransaction function to ScriptPubKeyMan and LegacyScriptPubKeyMan (Andrew Chow) 2c52b59 Refactor rawtransaction's SignTransaction into generic SignTransaction function (Andrew Chow) Pull request description: Following bitcoin#17261, the way to sign transactions, PSBTs, and messages was to use `GetSigningProvider()` and get a `SigningProvider` containing the private keys. However this may not be feasible for future `ScriptPubKeyMan`s, such as for hardware wallets. Instead of exporting a `SigningProvider` containing private keys, we need to pass these things into the `ScriptPubKeyMan` (via `CWallet`) so that they can do whatever is needed internally to sign them. This is largely a refactor as the logic of processing transactions, PSBTs, and messages for is moved into `LegacyScriptPubKeyMan` and `CWallet` instead of being handled by the caller (e.g. `signrawtransaction`). To help with this, I've refactored the 3(!) implementations of a `SignTransaction()` function into one generic one. This function will be called by `signrawtransactionwithkey` and `LegacyScriptPubKeyMan::SignTransaction()`. `CWallet::CreateTransaction()` is changed to call `CWallet::SignTransaction()` which in turn, calls `LegacyScriptPubKeyMan::SignTransaction()`. Other `ScriptPubKeyMan`s may implement `SignTransaction()` differently. `FillPSBT()` is moved to be a member function of `CWallet` and the `psbtwallet.cpp/h` files removed. It is further split so that `CWallet` handles filling the UTXOs while the `ScriptPubKeyMan` handles adding keys, derivation paths, scripts, and signatures. In the end `LegacyScriptPubKeyMan::FillPSBT` still calls `SignPSBTInput`, but the `SigningProvider` is internal to `LegacyScriptPubKeyMan`. Other `ScriptPubKeyMan`s may do something different. A new `SignMessage()` function is added to both `CWallet` and `ScriptPubKeyMan`. Instead of having the caller (i.e. `signmessage` or the sign message dialog) get the private key, hash the message, and sign, `ScriptPubKeyMan` will now handle that (`CWallet` passes through to the `ScriptPubKeyMan`s as it does for many functions). This signing code is thus consolidated into `LegacyScriptPubKeyMan::SignMessage()`, though other `ScriptPubKeyMan`s may implement it differently. Additionally, a `SigningError` enum is introduced for the different errors that we expect to see from `SignMessage()`. Lastly, `GetSigningProvider()` is renamed to `GetPublicSigningProvider()`. It will now only provide pubkeys, key origins, and scripts. `LegacySigningProvider` has it's `GetKey` and `HaveKey` functions changed to only return false. Future implementations should return `HidingSigningProvider`s where private keys are hidden. Other things like `dumpprivkey` and `dumpwallet` are not changed because they directly need and access the `LegacyScriptPubKeyMan` so are not relevant to future changes. ACKs for top commit: instagibbs: reACK bitcoin@d2774c0 Sjors: re-utACK d2774c0 meshcollider: re-utACK d2774c0 Tree-SHA512: 89c83e7e7e9315e283fae145a2264648a9d7f7ace8f3281cb3f44f0b013c988d67ba4fa9726e50c643c0ed921bdd269adaec984840d11acf4a681f3e8a582cc1

fanquake added the Wallet label Feb 11, 2020

achow101 force-pushed the sign-in-spkman branch from 1145c59 to 885fa89 Compare February 11, 2020 22:29

This was referenced Feb 11, 2020

Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528

Merged

BIP-322: Generic signed message format #16440

Closed

wallet: Show fee in results for signrawtransaction* for segwit inputs #12911

Closed

fanquake changed the title ~~[wallet] Pass in transactions and messages for signing instead of exporting the private keys~~ wallet: Pass in transactions and messages for signing instead of exporting the private keys Feb 12, 2020

vasild reviewed Feb 12, 2020

View reviewed changes

src/wallet/scriptpubkeyman.h Outdated Show resolved Hide resolved

Sjors reviewed Feb 12, 2020

View reviewed changes

achow101 force-pushed the sign-in-spkman branch 2 times, most recently from 46a5deb to 62a66ca Compare February 12, 2020 17:52

achow101 force-pushed the sign-in-spkman branch from 62a66ca to 01839f3 Compare February 12, 2020 18:56

kallewoof reviewed Feb 13, 2020

View reviewed changes

src/util/error.h Outdated Show resolved Hide resolved

achow101 force-pushed the sign-in-spkman branch from 01839f3 to bb7af89 Compare February 13, 2020 23:05

This was referenced Feb 14, 2020

refactor: Abstract boost::variant out #17953

Closed

rpc: Auto-format RPCResult #17809

Merged

achow101 force-pushed the sign-in-spkman branch from 1f9a806 to 08b9b68 Compare March 8, 2020 16:36

meshcollider reviewed Mar 9, 2020

View reviewed changes

achow101 added 5 commits March 9, 2020 11:16

Move FillPSBT to be a member of CWallet

3d70dd9

Move direct calls to MessageSign into new SignMessage functions in CW…

6a9c429

…allet and ScriptPubKeyMan Instead of getting a SigningProvider and then going to MessageSign, have ScriptPubKeyMan handle the message signing internally.

Replace GetSigningProvider with GetSolvingProvider

dc17488

Not all ScriptPubKeyMans will be able to provide private keys, but pubkeys and scripts should be. So only provide public-only SigningProviders, i.e. ones that can help with Solving.

achow101 force-pushed the sign-in-spkman branch from 08b9b68 to d2774c0 Compare March 9, 2020 15:18

meshcollider merged commit dcf2ccb into bitcoin:master Mar 9, 2020

ryanofsky mentioned this pull request Mar 9, 2020

interfaces: Describe and follow some code conventions #18278

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 28, 2020

fix #401

248ed9d

these were removed as part of bitcoin#18115 We should find a way to deal with it in the future

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

ryanofsky mentioned this pull request May 26, 2020

[WIP] Splitting up the GUI into another project maflcko/bitcoin-core#26

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

wallet: Pass in transactions and messages for signing instead of exporting the private keys #18115

wallet: Pass in transactions and messages for signing instead of exporting the private keys #18115

Uh oh!

Conversation

achow101 commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors commented Feb 12, 2020

Uh oh!

vasild Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasild commented Feb 12, 2020

Uh oh!

Uh oh!

Sjors left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kallewoof commented Feb 13, 2020

Uh oh!

achow101 commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Feb 13, 2020

Uh oh!

kallewoof commented Feb 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Mar 9, 2020

Uh oh!

achow101 commented Mar 9, 2020

Uh oh!

Sjors commented Mar 9, 2020

Uh oh!

meshcollider commented Mar 9, 2020

Uh oh!

Uh oh!

achow101 commented Feb 11, 2020 •

edited

Loading

DrahtBot commented Feb 11, 2020 •

edited

Loading

vasild Feb 12, 2020 •

edited

Loading

Sjors left a comment •

edited

Loading

achow101 Feb 12, 2020 •

edited

Loading

achow101 commented Feb 12, 2020 •

edited

Loading

achow101 commented Feb 13, 2020 •

edited

Loading

kallewoof commented Feb 14, 2020 •

edited

Loading

Sjors commented Mar 9, 2020 •

edited

Loading