Merge #6066: feat: support descriptor wallets for RPC protx updatereg…

…istar

c9a600e fix: linkage error - message signer better to be common code rather than libconsensus (Konstantin Akimov)
8299b3b feat: protxregistar implementation for descriptor wallets (Konstantin Akimov)
6f45432 refactor: removed unused SignSpecialTxPayloadByString (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented

  RPC `protx updateregistar` uses forcely LegacyScriptPubKeyMan instead using CWallet's interface.
  It causes a failures such as
  ```
  test_framework.authproxy.JSONRPCException: This type of wallet does not support this command (-4)
  ```

  ## What was done?
  New method `SignSpecialTxPayloadByHash` is implemented in interface instead exporting raw private key for some address.

  See dashpay/dash-issues#59 to track progress

  ## How Has This Been Tested?
  Functional test `feature_dip3_deterministicmns.py` to run by both ways - legacy and descriptor wallets.

  Run unit and functional tests.

  Extra test done locally:
  ```diff
  --- a/test/functional/test_framework/test_framework.py
  +++ b/test/functional/test_framework/test_framework.py
  @@ -242,10 +242,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):

           if self.options.descriptors is None:
               # Prefer BDB unless it isn't available
  -            if self.is_bdb_compiled():
  -                self.options.descriptors = False
  -            elif self.is_sqlite_compiled():
  +            if self.is_sqlite_compiled():
                   self.options.descriptors = True
  +            elif self.is_bdb_compiled():
  +                self.options.descriptors = False
  ```

  to flip flag descriptor wallets/legacy wallets for all functional tests.

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  PastaPastaPasta:
    utACK c9a600e

Tree-SHA512: 00aea1cd9db3537b9a9dcdee096d47ea48337edeea3f52ad54aea91781678b8641ac2dd86b67f61f87e3912945bcb5361a42a3279b6c08bb8d9f096bed8fe842

src/Makefile.am

@@ -480,7 +480,6 @@ libbitcoin_server_a_SOURCES = \
   masternode/payments.cpp \
   masternode/sync.cpp \
   masternode/utils.cpp \
-  messagesigner.cpp \
   miner.cpp \
   net.cpp \
   netfulfilledman.cpp \
@@ -770,6 +769,7 @@ libbitcoin_util_a_SOURCES = \
   fs.cpp \
   interfaces/handler.cpp \
   logging.cpp \
+  messagesigner.cpp \
   random.cpp \
   randomenv.cpp \
   rpc/request.cpp \

src/rpc/evo.cpp

@@ -294,25 +294,13 @@ static void UpdateSpecialTxInputsHash(const CMutableTransaction& tx, SpecialTxPa
 }
 
 template<typename SpecialTxPayload>
-static void SignSpecialTxPayloadByHash(const CMutableTransaction& tx, SpecialTxPayload& payload, const CKey& key)
+static void SignSpecialTxPayloadByHash(const CMutableTransaction& tx, SpecialTxPayload& payload, const CKeyID& keyID, const CWallet& wallet)
 {
     UpdateSpecialTxInputsHash(tx, payload);
     payload.vchSig.clear();
 
-    uint256 hash = ::SerializeHash(payload);
-    if (!CHashSigner::SignHash(hash, key, payload.vchSig)) {
-        throw JSONRPCError(RPC_INTERNAL_ERROR, "failed to sign special tx");
-    }
-}
-
-template<typename SpecialTxPayload>
-static void SignSpecialTxPayloadByString(const CMutableTransaction& tx, SpecialTxPayload& payload, const CKey& key)
-{
-    UpdateSpecialTxInputsHash(tx, payload);
-    payload.vchSig.clear();
-
-    std::string m = payload.MakeSignString();
-    if (!CMessageSigner::SignMessage(m, payload.vchSig, key)) {
+    const uint256 hash = ::SerializeHash(payload);
+    if (!wallet.SignSpecialTxPayload(hash, keyID, payload.vchSig)) {
         throw JSONRPCError(RPC_INTERNAL_ERROR, "failed to sign special tx");
     }
 }
@@ -1111,14 +1099,12 @@ static UniValue protx_update_registrar_wrapper(const JSONRPCRequest& request, CC
         ptx.scriptPayout = GetScriptForDestination(payoutDest);
     }
 
-    LegacyScriptPubKeyMan* spk_man = wallet->GetLegacyScriptPubKeyMan();
-    if (!spk_man) {
-        throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command");
-    }
-
-    CKey keyOwner;
-    if (!spk_man->GetKey(dmn->pdmnState->keyIDOwner, keyOwner)) {
-        throw std::runtime_error(strprintf("Private key for owner address %s not found in your wallet", EncodeDestination(PKHash(dmn->pdmnState->keyIDOwner))));
+    {
+        const auto pkhash{PKHash(dmn->pdmnState->keyIDOwner)};
+        LOCK(wallet->cs_wallet);
+        if (wallet->IsMine(GetScriptForDestination(pkhash)) != isminetype::ISMINE_SPENDABLE) {
+            throw std::runtime_error(strprintf("Private key for owner address %s not found in your wallet", EncodeDestination(pkhash)));
+        }
     }
 
     CMutableTransaction tx;
@@ -1136,7 +1122,7 @@ static UniValue protx_update_registrar_wrapper(const JSONRPCRequest& request, CC
     }
 
     FundSpecialTx(wallet.get(), tx, ptx, feeSourceDest);
-    SignSpecialTxPayloadByHash(tx, ptx, keyOwner);
+    SignSpecialTxPayloadByHash(tx, ptx, dmn->pdmnState->keyIDOwner, *wallet);
     SetTxPayload(tx, ptx);
 
     return SignAndSendSpecialTx(request, chain_helper, chainman, tx);

src/wallet/scriptpubkeyman.cpp

@@ -5,6 +5,7 @@
 #include <key_io.h>
 #include <chainparams.h>
 #include <logging.h>
+#include <messagesigner.h>
 #include <script/descriptor.h>
 #include <script/sign.h>
 #include <shutdown.h>
@@ -726,6 +727,16 @@ SigningResult LegacyScriptPubKeyMan::SignMessage(const std::string& message, con
     return SigningResult::SIGNING_FAILED;
 }
 
+bool LegacyScriptPubKeyMan::SignSpecialTxPayload(const uint256& hash, const CKeyID& keyid, std::vector<unsigned char>& vchSig) const
+{
+    CKey key;
+    if (!GetKey(keyid, key)) {
+        return false;
+    }
+
+    return CHashSigner::SignHash(hash, key, vchSig);
+}
+
 TransactionError LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbtx, int sighash_type, bool sign, bool bip32derivs, int* n_signed) const
 {
     if (n_signed) {
@@ -2196,6 +2207,21 @@ SigningResult DescriptorScriptPubKeyMan::SignMessage(const std::string& message,
     return SigningResult::OK;
 }
 
+bool DescriptorScriptPubKeyMan::SignSpecialTxPayload(const uint256& hash, const CKeyID& keyid, std::vector<unsigned char>& vchSig) const
+{
+    std::unique_ptr<FlatSigningProvider> keys = GetSigningProvider(GetScriptForDestination(PKHash(keyid)), true);
+    if (!keys) {
+        return false;
+    }
+
+    CKey key;
+    if (!keys->GetKey(keyid, key)) {
+        return false;
+    }
+
+    return CHashSigner::SignHash(hash, key, vchSig);
+}
+
 TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbtx, int sighash_type, bool sign, bool bip32derivs, int* n_signed) const
 {
     if (n_signed) {

src/wallet/scriptpubkeyman.h

@@ -211,6 +211,7 @@ class ScriptPubKeyMan
     virtual bool SignTransaction(CMutableTransaction& tx, const std::map<COutPoint, Coin>& coins, int sighash, std::map<int, std::string>& input_errors) const { return false; }
     /** Sign a message with the given script */
     virtual SigningResult SignMessage(const std::string& message, const PKHash& pkhash, std::string& str_sig) const { return SigningResult::SIGNING_FAILED; };
+    virtual bool SignSpecialTxPayload(const uint256& hash, const CKeyID& keyid, std::vector<unsigned char>& vchSig) const { return false; }
     /** Adds script and derivation path information to a PSBT, and optionally signs it. */
     virtual TransactionError FillPSBT(PartiallySignedTransaction& psbt, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr) const { return TransactionError::INVALID_PSBT; }
 
@@ -358,6 +359,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
 
     bool SignTransaction(CMutableTransaction& tx, const std::map<COutPoint, Coin>& coins, int sighash, std::map<int, std::string>& input_errors) const override;
     SigningResult SignMessage(const std::string& message, const PKHash& pkhash, std::string& str_sig) const override;
+    bool SignSpecialTxPayload(const uint256& hash, const CKeyID& keyid, std::vector<unsigned char>& vchSig) const override;
     TransactionError FillPSBT(PartiallySignedTransaction& psbt, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr) const override;
 
     uint256 GetID() const override;
@@ -588,6 +590,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
 
     bool SignTransaction(CMutableTransaction& tx, const std::map<COutPoint, Coin>& coins, int sighash, std::map<int, std::string>& input_errors) const override;
     SigningResult SignMessage(const std::string& message, const PKHash& pkhash, std::string& str_sig) const override;
+    bool SignSpecialTxPayload(const uint256& hash, const CKeyID& keyid, std::vector<unsigned char>& vchSig) const override;
     TransactionError FillPSBT(PartiallySignedTransaction& psbt, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr) const override;
 
     uint256 GetID() const override;

src/wallet/wallet.cpp

@@ -3070,6 +3070,19 @@ SigningResult CWallet::SignMessage(const std::string& message, const PKHash& pkh
     return SigningResult::PRIVATE_KEY_NOT_AVAILABLE;
 }
 
+bool CWallet::SignSpecialTxPayload(const uint256& hash, const CKeyID& keyid, std::vector<unsigned char>& vchSig) const
+{
+    SignatureData sigdata;
+    CScript script_pub_key = GetScriptForDestination(PKHash(keyid));
+    for (const auto& spk_man_pair : m_spk_managers) {
+        if (spk_man_pair.second->CanProvide(script_pub_key, sigdata)) {
+            LOCK(cs_wallet);  // DescriptorScriptPubKeyMan calls IsLocked which can lock cs_wallet in a deadlocking order
+            return spk_man_pair.second->SignSpecialTxPayload(hash, keyid, vchSig);
+        }
+    }
+    return false;
+}
+
 bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl coinControl)
 {
     std::vector<CRecipient> vecSend;

src/wallet/wallet.h

@@ -1107,6 +1107,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     /** Sign the tx given the input coins and sighash. */
     bool SignTransaction(CMutableTransaction& tx, const std::map<COutPoint, Coin>& coins, int sighash, std::map<int, std::string>& input_errors) const;
     SigningResult SignMessage(const std::string& message, const PKHash& pkhash, std::string& str_sig) const;
+    /** Sign the payload of special transaction.
+      * Because wallet is not aware about special transactions entity,
+      * but it should work for any its type, we pass there directly a hash of payload.
+      */
+    bool SignSpecialTxPayload(const uint256& hash, const CKeyID& keyid, std::vector<unsigned char>& vchSig) const;
 
     /**
      * Fills out a PSBT with information from the wallet. Fills in UTXOs if we have

test/functional/test_runner.py

@@ -93,6 +93,7 @@
     # Scripts that are run by default.
     # Longest test should go first, to favor running tests in parallel
     'feature_dip3_deterministicmns.py --legacy-wallet', # NOTE: needs dash_hash to pass
+    'feature_dip3_deterministicmns.py --descriptors', # NOTE: needs dash_hash to pass
     'feature_llmq_data_recovery.py',
     'wallet_hd.py --legacy-wallet',
     'wallet_hd.py --descriptors',