Merge #6094: feat: support descriptor wallets for RPC governance votemany, votealias

c72ec70 feat: implement governance RPCs votealias and votemany for descriptor wallets (Konstantin Akimov)
4908329 refactor: new method to generate a signing message in CGovernanceVote (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented

  RPCs `governance votemany` and `governance votealias` use 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)
  ```
  See dashpay/dash-issues#59 to track progress

  ## What was done?
  Use CWallet's interfaces instead LegacyScriptPubKeyMan

  ## How Has This Been Tested?
  Functional tests `feature_governance.py` and `feature_governance_cl.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:
  UdjinM6:
    utACK c72ec70
  PastaPastaPasta:
    utACK c72ec70

Tree-SHA512: 2c18f0d4acb1c4d57da81bf54f0d155682f558eeb7271df7e6fe75c126ef7f047562794a6730e3ca5351abc4e2daded06b874c2ab77f9c47b840c89d8a158c9f

Author: PastaPastaPasta

src/governance/vote.cpp

@@ -162,41 +162,6 @@ uint256 CGovernanceVote::GetSignatureHash() const
     return SerializeHash(*this);
 }
 
-bool CGovernanceVote::Sign(const CKey& key, const CKeyID& keyID)
-{
-    std::string strError;
-
-    // Harden Spork6 so that it is active on testnet and no other networks
-    if (Params().NetworkIDString() == CBaseChainParams::TESTNET) {
-        uint256 signatureHash = GetSignatureHash();
-
-        if (!CHashSigner::SignHash(signatureHash, key, vchSig)) {
-            LogPrintf("CGovernanceVote::Sign -- SignHash() failed\n");
-            return false;
-        }
-
-        if (!CHashSigner::VerifyHash(signatureHash, keyID, vchSig, strError)) {
-            LogPrintf("CGovernanceVote::Sign -- VerifyHash() failed, error: %s\n", strError);
-            return false;
-        }
-    } else {
-        std::string strMessage = masternodeOutpoint.ToStringShort() + "|" + nParentHash.ToString() + "|" +
-                                 ::ToString(nVoteSignal) + "|" + ::ToString(nVoteOutcome) + "|" + ::ToString(nTime);
-
-        if (!CMessageSigner::SignMessage(strMessage, vchSig, key)) {
-            LogPrintf("CGovernanceVote::Sign -- SignMessage() failed\n");
-            return false;
-        }
-
-        if (!CMessageSigner::VerifyMessage(keyID, vchSig, strMessage, strError)) {
-            LogPrintf("CGovernanceVote::Sign -- VerifyMessage() failed, error: %s\n", strError);
-            return false;
-        }
-    }
-
-    return true;
-}
-
 bool CGovernanceVote::CheckSignature(const CKeyID& keyID) const
 {
     std::string strError;
@@ -208,12 +173,7 @@ bool CGovernanceVote::CheckSignature(const CKeyID& keyID) const
             return false;
         }
     } else {
-        std::string strMessage = masternodeOutpoint.ToStringShort() + "|" + nParentHash.ToString() + "|" +
-                                 ::ToString(nVoteSignal) + "|" +
-                                 ::ToString(nVoteOutcome) + "|" +
-                                 ::ToString(nTime);
-
-        if (!CMessageSigner::VerifyMessage(keyID, vchSig, strMessage, strError)) {
+        if (!CMessageSigner::VerifyMessage(keyID, vchSig, GetSignatureString(), strError)) {
             LogPrint(BCLog::GOBJECT, "CGovernanceVote::IsValid -- VerifyMessage() failed, error: %s\n", strError);
             return false;
         }
@@ -275,6 +235,14 @@ bool CGovernanceVote::IsValid(const CDeterministicMNList& tip_mn_list, bool useV
     }
 }
 
+std::string CGovernanceVote::GetSignatureString() const
+{
+    return masternodeOutpoint.ToStringShort() + "|" + nParentHash.ToString() + "|" +
+                             ::ToString(nVoteSignal) + "|" +
+                             ::ToString(nVoteOutcome) + "|" +
+                             ::ToString(nTime);
+}
+
 bool operator==(const CGovernanceVote& vote1, const CGovernanceVote& vote2)
 {
     bool fResult = ((vote1.masternodeOutpoint == vote2.masternodeOutpoint) &&

src/governance/vote.h

@@ -105,6 +105,7 @@ class CGovernanceVote
     bool Sign(const CActiveMasternodeManager& mn_activeman);
     bool CheckSignature(const CBLSPublicKey& pubKey) const;
     bool IsValid(const CDeterministicMNList& tip_mn_list, bool useVotingKey) const;
+    std::string GetSignatureString() const;
     void Relay(PeerManager& peerman, const CMasternodeSync& mn_sync, const CDeterministicMNList& tip_mn_list) const;
 
     const COutPoint& GetMasternodeOutpoint() const { return masternodeOutpoint; }

src/rpc/governance.cpp

@@ -404,7 +404,37 @@ static RPCHelpMan gobject_submit()
     };
 }
 
-static UniValue VoteWithMasternodes(const JSONRPCRequest& request, const std::map<uint256, CKey>& keys,
+#ifdef ENABLE_WALLET
+static bool SignVote(const CWallet& wallet, const CKeyID& keyID, CGovernanceVote& vote)
+{
+    // Special implementation for testnet (Harden Spork6 that has not been deployed to other networks)
+    if (Params().NetworkIDString() == CBaseChainParams::TESTNET) {
+        std::vector<unsigned char> signature;
+        if (!wallet.SignSpecialTxPayload(vote.GetSignatureHash(), keyID, signature)) {
+            LogPrintf("SignVote -- SignHash() failed\n");
+            return false;
+        }
+        vote.SetSignature(signature);
+        return true;
+    } // end of testnet implementation
+
+    std::string strMessage{vote.GetSignatureString()};
+    std::string signature;
+    SigningResult err = wallet.SignMessage(strMessage, PKHash{keyID}, signature);
+    if (err != SigningResult::OK) {
+        LogPrintf("SignVote failed due to: %s\n", SigningResultString(err));
+        return false;
+    }
+    bool ret = true;
+    const auto decoded = DecodeBase64(signature, &ret);
+    assert(!ret); // it should not fail
+
+    vote.SetSignature(std::vector<unsigned char>(decoded.data(), decoded.data() + decoded.size()));
+    return true;
+}
+
+static UniValue VoteWithMasternodes(const JSONRPCRequest& request, const CWallet& wallet,
+                             const std::map<uint256, CKeyID>& votingKeys,
                              const uint256& hash, vote_signal_enum_t eVoteSignal,
                              vote_outcome_enum_t eVoteOutcome)
 {
@@ -425,9 +455,9 @@ static UniValue VoteWithMasternodes(const JSONRPCRequest& request, const std::ma
 
     UniValue resultsObj(UniValue::VOBJ);
 
-    for (const auto& p : keys) {
+    for (const auto& p : votingKeys) {
         const auto& proTxHash = p.first;
-        const auto& key = p.second;
+        const auto& keyID = p.second;
 
         UniValue statusObj(UniValue::VOBJ);
 
@@ -441,7 +471,8 @@ static UniValue VoteWithMasternodes(const JSONRPCRequest& request, const std::ma
         }
 
         CGovernanceVote vote(dmn->collateralOutpoint, hash, eVoteSignal, eVoteOutcome);
-        if (!vote.Sign(key, key.GetPubKey().GetID())) {
+
+        if (!SignVote(wallet, keyID, vote) || !vote.CheckSignature(keyID)) {
             nFailed++;
             statusObj.pushKV("result", "failed");
             statusObj.pushKV("errorMessage", "Failure to sign.");
@@ -471,7 +502,14 @@ static UniValue VoteWithMasternodes(const JSONRPCRequest& request, const std::ma
     return returnObj;
 }
 
-#ifdef ENABLE_WALLET
+static bool CheckWalletOwnsKey(const CWallet& wallet, const CKeyID& keyid)
+{
+    const CScript script{GetScriptForDestination(PKHash(keyid))};
+    LOCK(wallet.cs_wallet);
+
+    return wallet.IsMine(script) == isminetype::ISMINE_SPENDABLE;
+}
+
 static RPCHelpMan gobject_vote_many()
 {
     return RPCHelpMan{"gobject vote-many",
@@ -510,22 +548,17 @@ static RPCHelpMan gobject_vote_many()
 
     EnsureWalletIsUnlocked(wallet.get());
 
-    LegacyScriptPubKeyMan* spk_man = wallet->GetLegacyScriptPubKeyMan();
-    if (!spk_man) {
-        throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command");
-    }
-
-    std::map<uint256, CKey> votingKeys;
+    std::map<uint256, CKeyID> votingKeys;
 
     auto mnList = node.dmnman->GetListAtChainTip();
     mnList.ForEachMN(true, [&](auto& dmn) {
-        CKey votingKey;
-        if (spk_man->GetKey(dmn.pdmnState->keyIDVoting, votingKey)) {
-            votingKeys.emplace(dmn.proTxHash, votingKey);
+        const bool is_mine = CheckWalletOwnsKey(*wallet, dmn.pdmnState->keyIDVoting);
+        if (is_mine) {
+            votingKeys.emplace(dmn.proTxHash, dmn.pdmnState->keyIDVoting);
         }
     });
 
-    return VoteWithMasternodes(request, votingKeys, hash, eVoteSignal, eVoteOutcome);
+    return VoteWithMasternodes(request, *wallet, votingKeys, hash, eVoteSignal, eVoteOutcome);
 },
     };
 }
@@ -575,20 +608,16 @@ static RPCHelpMan gobject_vote_alias()
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid or unknown proTxHash");
     }
 
-    LegacyScriptPubKeyMan* spk_man = wallet->GetLegacyScriptPubKeyMan();
-    if (!spk_man) {
-        throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command");
-    }
 
-    CKey votingKey;
-    if (!spk_man->GetKey(dmn->pdmnState->keyIDVoting, votingKey)) {
+    const bool is_mine = CheckWalletOwnsKey(*wallet, dmn->pdmnState->keyIDVoting);
+    if (!is_mine) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Private key for voting address %s not known by wallet", EncodeDestination(PKHash(dmn->pdmnState->keyIDVoting))));
     }
 
-    std::map<uint256, CKey> votingKeys;
-    votingKeys.emplace(proTxHash, votingKey);
+    std::map<uint256, CKeyID> votingKeys;
+    votingKeys.emplace(proTxHash, dmn->pdmnState->keyIDVoting);
 
-    return VoteWithMasternodes(request, votingKeys, hash, eVoteSignal, eVoteOutcome);
+    return VoteWithMasternodes(request, *wallet, votingKeys, hash, eVoteSignal, eVoteOutcome);
 },
     };
 }

test/functional/test_runner.py

@@ -288,7 +288,9 @@
     'feature_new_quorum_type_activation.py',
     'feature_governance_objects.py',
     'feature_governance.py --legacy-wallet',
+    'feature_governance.py --descriptors',
     'feature_governance_cl.py --legacy-wallet',
+    'feature_governance_cl.py --descriptors',
     'rpc_uptime.py',
     'feature_discover.py',
     'wallet_resendwallettransactions.py --legacy-wallet',