fix: memoize governance vote signature checks to bound govsync cost

src/Makefile.test.include

@@ -123,6 +123,7 @@ BITCOIN_TESTS =\
   test/governance_inv_tests.cpp \
   test/governance_superblock_tests.cpp \
   test/governance_validators_tests.cpp \
+  test/governance_vote_sync_tests.cpp \
   test/governance_vote_wire_tests.cpp \
   test/coinjoin_inouts_tests.cpp \
   test/coinjoin_dstxmanager_tests.cpp \

src/governance/governance.cpp

@@ -656,17 +656,20 @@ std::vector<CInv> CGovernanceManager::GetSyncableVoteInvs(const uint256& nProp,
 
     const auto& govobj = *Assert(it->second);
     LOCK(govobj.cs);
-    const auto& fileVotes = govobj.GetVoteFile();
-    for (const auto& vote : fileVotes.GetVotes()) {
+    // Visit the stored votes in place: CheckSignature memoises its verdict on
+    // the vote instance, and a GetVotes() copy would discard that memo, so every
+    // walk would pay a fresh ECDSA recovery or BLS pairing per vote.
+    invs.reserve(govobj.GetVoteFile().GetVoteCount());
+    govobj.GetVoteFile().ForEachVote([&](const CGovernanceVote& vote) {
         uint256 nVoteHash = vote.GetHash();
 
         bool onlyVotingKeyAllowed = govobj.GetObjectType() == GovernanceObject::PROPOSAL && vote.GetSignal() == VOTE_SIGNAL_FUNDING;
 
         if (filter.contains(nVoteHash) || !vote.IsValid(tip_mn_list, onlyVotingKeyAllowed)) {
-            continue;
+            return;
         }
         invs.emplace_back(MSG_GOVERNANCE_OBJECT_VOTE, nVoteHash);
-    }
+    });
 
     return invs;
 }

src/governance/vote.cpp

@@ -19,6 +19,42 @@
 static_assert(CGovernanceVote::COMPACT_SIG_SIZE == CPubKey::COMPACT_SIGNATURE_SIZE);
 static_assert(CGovernanceVote::BLS_SIG_SIZE == CBLSSignature::SerSize);
 
+namespace {
+//! Hash the inputs that determine the verification result. Dropping any of them
+//! breaks the memo; see CGovernanceVote::SignatureMemo.
+uint256 SignatureCacheKey(const CKeyID& key, const uint256& sigHash, const std::vector<unsigned char>& vchSig)
+{
+    HashWriter ss{};
+    ss << key << sigHash << vchSig;
+    return ss.GetHash();
+}
+
+//! The BLS verdict is always computed with VerifyInsecure(..., /*specificLegacyScheme=*/false),
+//! so the key must be fingerprinted under that same scheme. Serializing via operator<< would
+//! instead use the mutable bls_legacy_scheme global, and a legacy encoding of P can equal the
+//! basic encoding of -P. Across an activation or a reorg that flips the global, a verdict cached
+//! for one key could then be served for the other.
+uint256 SignatureCacheKey(const CBLSPublicKey& key, const uint256& sigHash,
+                          const std::vector<unsigned char>& vchSig)
+{
+    HashWriter ss{};
+    const auto key_bytes = key.ToBytes(/*specificLegacyScheme=*/false);
+    ss.write(MakeByteSpan(key_bytes));
+    ss << sigHash << vchSig;
+    return ss.GetHash();
+}
+} // namespace
+
+std::optional<bool> CGovernanceVote::GetMemoisedVerdict(const CKeyID& keyID) const
+{
+    return m_sig_memo.Lookup(SignatureCacheKey(keyID, GetSignatureHash(), vchSig));
+}
+
+std::optional<bool> CGovernanceVote::GetMemoisedVerdict(const CBLSPublicKey& pubKey) const
+{
+    return m_sig_memo.Lookup(SignatureCacheKey(pubKey, GetSignatureHash(), vchSig));
+}
+
 std::string CGovernanceVoting::ConvertOutcomeToString(vote_outcome_enum_t nOutcome)
 {
     static const std::map<vote_outcome_enum_t, std::string> mapOutcomeString = {
@@ -127,33 +163,49 @@ uint256 CGovernanceVote::GetHash() const
 
 bool CGovernanceVote::CheckSignature(const CKeyID& keyID) const
 {
+    const uint256 sigHash{GetSignatureHash()};
+    const uint256 fingerprint{SignatureCacheKey(keyID, sigHash, vchSig)};
+    if (const auto memoised{m_sig_memo.Lookup(fingerprint)}) {
+        return *memoised;
+    }
+
     std::string strError;
+    bool valid{false};
 
     // Harden Spork6 so that it is active on testnet and no other networks
     if (Params().NetworkIDString() == CBaseChainParams::TESTNET) {
-        if (!CHashSigner::VerifyHash(GetSignatureHash(), keyID, vchSig, strError)) {
+        valid = CHashSigner::VerifyHash(sigHash, keyID, vchSig, strError);
+        if (!valid) {
             LogPrint(BCLog::GOBJECT, "CGovernanceVote::IsValid -- VerifyHash() failed, error: %s\n", strError);
-            return false;
         }
     } else {
-        if (!CMessageSigner::VerifyMessage(keyID, vchSig, GetSignatureString(), strError)) {
+        valid = CMessageSigner::VerifyMessage(keyID, vchSig, GetSignatureString(), strError);
+        if (!valid) {
             LogPrint(BCLog::GOBJECT, "CGovernanceVote::IsValid -- VerifyMessage() failed, error: %s\n", strError);
-            return false;
         }
     }
 
-    return true;
+    m_sig_memo.Store(fingerprint, valid);
+    return valid;
 }
 
 bool CGovernanceVote::CheckSignature(const CBLSPublicKey& pubKey) const
 {
+    const uint256 sigHash{GetSignatureHash()};
+    const uint256 fingerprint{SignatureCacheKey(pubKey, sigHash, vchSig)};
+    if (const auto memoised{m_sig_memo.Lookup(fingerprint)}) {
+        return *memoised;
+    }
+
     CBLSSignature sig;
     sig.SetBytes(vchSig, false);
-    if (!sig.VerifyInsecure(pubKey, GetSignatureHash(), false)) {
+    const bool valid{sig.VerifyInsecure(pubKey, sigHash, false)};
+    if (!valid) {
         LogPrintf("CGovernanceVote::CheckSignature -- VerifyInsecure() failed\n");
-        return false;
     }
-    return true;
+
+    m_sig_memo.Store(fingerprint, valid);
+    return valid;
 }
 
 bool CGovernanceVote::IsValid(const CDeterministicMNList& tip_mn_list, bool useVotingKey) const

src/governance/vote.h

@@ -12,6 +12,8 @@
 #include <util/string.h>
 #include <util/time.h>
 
+#include <optional>
+
 class CBLSPublicKey;
 class CDeterministicMNList;
 class CKeyID;
@@ -81,6 +83,44 @@ class CGovernanceVote
     const uint256 hash{0};
     void UpdateHash() const;
 
+    /**
+     * Result of the last CheckSignature call, so that re-verifying an unchanged
+     * vote costs a hash instead of an ECDSA recovery or a BLS pairing.
+     *
+     * The fingerprint covers the key, the signature hash and the signature
+     * bytes, so a hit means this exact verification already ran. Answering from
+     * a bare "already checked" flag would be wrong two ways: the same vote gets
+     * checked against different keys (voting vs operator, and operator keys
+     * rotate), and a vote can be modified after verification, which would
+     * otherwise keep its stale verdict.
+     */
+    class SignatureMemo
+    {
+        std::optional<uint256> m_fingerprint;
+        bool m_valid{false};
+
+    public:
+        std::optional<bool> Lookup(const uint256& fingerprint) const
+        {
+            if (m_fingerprint != fingerprint) return std::nullopt;
+            return m_valid;
+        }
+
+        void Store(const uint256& fingerprint, bool valid)
+        {
+            m_fingerprint = fingerprint;
+            m_valid = valid;
+        }
+    };
+
+    /**
+     * Memory only, and not internally synchronised. Shared votes live in
+     * CGovernanceObjectVoteFile, reachable only through
+     * CGovernanceObject::GetVoteFile() under the object's cs; every other vote
+     * has a single owner.
+     */
+    mutable SignatureMemo m_sig_memo;
+
 public:
     CGovernanceVote() = default;
     CGovernanceVote(const COutPoint& outpointMasternodeIn, const uint256& nParentHashIn, vote_signal_enum_t eVoteSignalIn, vote_outcome_enum_t eVoteOutcomeIn);
@@ -109,6 +149,11 @@ class CGovernanceVote
     bool CheckSignature(const CKeyID& keyID) const;
     bool CheckSignature(const CBLSPublicKey& pubKey) const;
     bool IsValid(const CDeterministicMNList& tip_mn_list, bool useVotingKey) const;
+
+    /** The memoised verdict for this key, or nullopt if the next CheckSignature
+     *  with it would have to run the cryptography. */
+    std::optional<bool> GetMemoisedVerdict(const CKeyID& keyID) const;
+    std::optional<bool> GetMemoisedVerdict(const CBLSPublicKey& pubKey) const;
     std::string GetSignatureString() const
     {
         return masternodeOutpoint.ToStringShort() + "|" + nParentHash.ToString() + "|" +

src/governance/votedb.h

@@ -66,6 +66,16 @@ class CGovernanceObjectVoteFile
 
     std::vector<CGovernanceVote> GetVotes() const;
 
+    /** Visit the stored votes in place, so callers warm and reuse the per-vote
+     *  signature memo instead of discarding it with a GetVotes() copy. Takes a
+     *  callback rather than returning the list: the caller's lock on the owning
+     *  object's cs cannot be expressed on a returned reference. */
+    template <typename Fn>
+    void ForEachVote(Fn&& fn) const
+    {
+        for (const auto& vote : listVotes) fn(vote);
+    }
+
     void RemoveVotesFromMasternode(const COutPoint& outpointMasternode);
     std::set<uint256> RemoveInvalidVotes(const CDeterministicMNList& tip_mn_list, const COutPoint& outpointMasternode, bool fProposal);