backport: wait for a matching superblock trigger before voting NO on the others (#8131) for v24.0.x

depends/packages/miniupnpc.mk

@@ -1,6 +1,6 @@
 package=miniupnpc
 $(package)_version=2.2.7
-$(package)_download_path=https://miniupnp.tuxfamily.org/files/
+$(package)_download_path=http://miniupnp.free.fr/files/
 $(package)_file_name=$(package)-$($(package)_version).tar.gz
 $(package)_sha256_hash=b0c3a27056840fd0ec9328a5a9bac3dc5e0ec6d2e8733349cf577b0aa1e70ac1
 $(package)_patches=dont_leak_info.patch cmake_get_src_addr.patch fix_windows_snprintf.patch

src/core_write.cpp

@@ -289,11 +289,11 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry
     }
 
     if (tx.nType == TRANSACTION_PROVIDER_REGISTER) {
-        if (const auto opt_proTx = GetTxPayload<CProRegTx>(tx)) {
+        if (const auto opt_proTx = GetTxPayload<CProRegTx>(tx); opt_proTx && opt_proTx->netInfo) {
             entry.pushKV("proRegTx", opt_proTx->ToJson());
         }
     } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_SERVICE) {
-        if (const auto opt_proTx = GetTxPayload<CProUpServTx>(tx)) {
+        if (const auto opt_proTx = GetTxPayload<CProUpServTx>(tx); opt_proTx && opt_proTx->netInfo) {
             entry.pushKV("proUpServTx", opt_proTx->ToJson());
         }
     } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REGISTRAR) {

src/evo/specialtxman.cpp

@@ -1093,6 +1093,11 @@ bool CSpecialTxProcessor::UndoSpecialTxsInBlock(const Chainstate& chainstate, co
     return true;
 }
 
+void CSpecialTxProcessor::RemoveMineableCommitmentsForBlock(const CBlock& block, gsl::not_null<const CBlockIndex*> pindex)
+{
+    m_qblockman.RemoveMineableCommitmentsForBlock(block, pindex);
+}
+
 bool CSpecialTxProcessor::CheckCreditPoolDiffForBlock(const CBlock& block, const CBlockIndex* pindex, const CCbTx& cbTx,
                                                       CAmount blockSubsidy, BlockValidationState& state)
 {

src/evo/specialtxman.h

@@ -97,6 +97,8 @@ class CSpecialTxProcessor
         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
     bool UndoSpecialTxsInBlock(const Chainstate& chainstate, const CBlock& block, const CBlockIndex* pindex, MNListUpdates& updatesRet)
         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+    void RemoveMineableCommitmentsForBlock(const CBlock& block, gsl::not_null<const CBlockIndex*> pindex)
+        EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
 
     // the returned list will not contain the correct block hash (we can't know it yet as the coinbase TX is not updated yet)

src/governance/governance.cpp

@@ -23,6 +23,7 @@
 #include <timedata.h>
 #include <util/check.h>
 #include <util/time.h>
+#include <validation.h>
 #include <validationinterface.h>
 
 #include <univalue.h>
@@ -102,6 +103,7 @@ bool CGovernanceManager::LoadCache(bool load_cache)
     assert(m_db != nullptr);
     is_loaded = load_cache ? m_db->Load(*this) : m_db->Store(*this);
     if (is_loaded && load_cache) {
+        nCachedBlockHeight = WITH_LOCK(::cs_main, return m_chainman.ActiveChain().Height());
         CheckAndRemove();
         InitOnLoad();
     }
@@ -1190,10 +1192,14 @@ void CGovernanceManager::RemoveInvalidVotes()
 
     std::vector<COutPoint> changedKeyMNs;
     for (const auto& [internalId, pdmnState] : diff.updatedMNs) {
-        auto oldDmn = lastMNListForVotingKeys->GetMNByInternalId(internalId);
-        // BuildDiff will construct itself with MNs that we already have knowledge
-        // of, meaning that fetch operations should never fail.
-        assert(oldDmn);
+        auto newDmn = tip_mn_list.GetMNByInternalId(internalId);
+        if (!Assume(newDmn)) {
+            continue;
+        }
+        auto oldDmn = lastMNListForVotingKeys->GetMN(newDmn->proTxHash);
+        if (!Assume(oldDmn)) {
+            continue;
+        }
         if ((pdmnState.fields & CDeterministicMNStateDiff::Field_keyIDVoting) && pdmnState.state.keyIDVoting != oldDmn->pdmnState->keyIDVoting) {
             changedKeyMNs.emplace_back(oldDmn->collateralOutpoint);
         } else if ((pdmnState.fields & CDeterministicMNStateDiff::Field_pubKeyOperator) && pdmnState.state.pubKeyOperator != oldDmn->pdmnState->pubKeyOperator) {
@@ -1202,9 +1208,9 @@ void CGovernanceManager::RemoveInvalidVotes()
     }
     for (const auto& id : diff.removedMns) {
         auto oldDmn = lastMNListForVotingKeys->GetMNByInternalId(id);
-        // BuildDiff will construct itself with MNs that we already have knowledge
-        // of, meaning that fetch operations should never fail.
-        assert(oldDmn);
+        if (!Assume(oldDmn)) {
+            continue;
+        }
         changedKeyMNs.emplace_back(oldDmn->collateralOutpoint);
     }
 

src/governance/signing.cpp

@@ -21,6 +21,7 @@
 
 namespace {
 constexpr std::chrono::seconds GOVERNANCE_FUDGE_WINDOW{2h};
+constexpr int NO_VOTE_DEADLINE_BLOCKS{3};
 } // anonymous namespace
 
 GovernanceSigner::GovernanceSigner(CDeterministicMNManager& dmnman, CGovernanceManager& govman,
@@ -178,7 +179,8 @@ std::optional<const CGovernanceObject> GovernanceSigner::CreateGovernanceTrigger
     return std::make_optional<CGovernanceObject>(gov_sb);
 }
 
-void GovernanceSigner::VoteGovernanceTriggers(const std::optional<const CGovernanceObject>& trigger_opt, int nHeight)
+void GovernanceSigner::VoteGovernanceTriggers(const std::optional<const CGovernanceObject>& trigger_opt,
+                                              std::optional<int> superblock_height, int nHeight)
 {
     // only active masternodes can vote on triggers
     if (m_mn_activeman.GetProTxHash().IsNull()) return;
@@ -189,7 +191,7 @@ void GovernanceSigner::VoteGovernanceTriggers(const std::optional<const CGoverna
 
     if (trigger_opt.has_value()) {
         // We should never vote "yes" on another trigger or the same trigger twice
-        assert(!votedFundingYesTriggerHash.has_value());
+        assert(!votedFundingYesTrigger.has_value());
         // Vote YES-FUNDING for the trigger we like, unless we already did
         const uint256 gov_sb_hash = trigger_opt.value().GetHash();
         bool voted_already{false};
@@ -199,7 +201,7 @@ void GovernanceSigner::VoteGovernanceTriggers(const std::optional<const CGoverna
             voted_already = std::ranges::any_of(voteRecord.mapInstances, [&](const auto& voteInstancePair) {
                 if (voteInstancePair.first == VOTE_SIGNAL_FUNDING) {
                     if (voteInstancePair.second.eOutcome == VOTE_OUTCOME_YES) {
-                        votedFundingYesTriggerHash = gov_sb_hash;
+                        votedFundingYesTrigger = FundingTriggerVote{gov_sb_hash, superblock_height.value()};
                     }
                     LogPrint(BCLog::GOBJECT, /* Continued */
                              "%s -- Not voting YES-FUNDING for trigger:%s, we voted %s for it already\n", strFunc,
@@ -215,7 +217,7 @@ void GovernanceSigner::VoteGovernanceTriggers(const std::optional<const CGoverna
             if (VoteFundingTrigger(gov_sb_hash, VOTE_OUTCOME_YES)) {
                 LogPrint(BCLog::GOBJECT, "%s -- Voting YES-FUNDING for new trigger:%s success\n", __func__,
                          gov_sb_hash.ToString());
-                votedFundingYesTriggerHash = gov_sb_hash;
+                votedFundingYesTrigger = FundingTriggerVote{gov_sb_hash, superblock_height.value()};
             } else {
                 LogPrint(BCLog::GOBJECT, "%s -- Voting YES-FUNDING for new trigger:%s failed\n", __func__,
                          gov_sb_hash.ToString());
@@ -225,6 +227,13 @@ void GovernanceSigner::VoteGovernanceTriggers(const std::optional<const CGoverna
         }
     }
 
+    if (superblock_height.has_value() && !trigger_opt.has_value() &&
+        nHeight + NO_VOTE_DEADLINE_BLOCKS < superblock_height.value()) {
+        LogPrint(BCLog::GOBJECT, "%s -- Not voting NO-FUNDING at height:%d, waiting for a trigger for superblock:%d\n",
+                 __func__, nHeight, superblock_height.value());
+        return;
+    }
+
     // Vote NO-FUNDING for the rest of the active triggers
     const auto activeTriggers = m_superblocks.GetActiveTriggers();
     for (const auto& trigger : activeTriggers) {
@@ -248,7 +257,7 @@ void GovernanceSigner::VoteGovernanceTriggers(const std::optional<const CGoverna
                      __func__, trigger_hash.ToString());
             continue;
         }
-        if (trigger_hash == votedFundingYesTriggerHash) {
+        if (votedFundingYesTrigger.has_value() && trigger_hash == votedFundingYesTrigger->hash) {
             // Skip actual trigger
             LogPrint(BCLog::GOBJECT, "%s -- Not voting NO-FUNDING for trigger:%s, we voted yes for it already\n",
                      __func__, trigger_hash.ToString());
@@ -296,19 +305,23 @@ bool GovernanceSigner::VoteFundingTrigger(const uint256& nHash, const vote_outco
 
 bool GovernanceSigner::HasAlreadyVotedFundingTrigger() const
 {
-    return votedFundingYesTriggerHash.has_value();
+    return votedFundingYesTrigger.has_value();
 }
 
 void GovernanceSigner::ResetVotedFundingTrigger()
 {
-    votedFundingYesTriggerHash = std::nullopt;
+    votedFundingYesTrigger = std::nullopt;
 }
 
 void GovernanceSigner::UpdatedBlockTip(const CBlockIndex* pindex)
 {
+    if (votedFundingYesTrigger.has_value() && pindex->nHeight >= votedFundingYesTrigger->superblock_height) {
+        ResetVotedFundingTrigger();
+    }
     const auto sb_opt = CreateSuperblockCandidate(pindex->nHeight);
     const auto trigger_opt = CreateGovernanceTrigger(sb_opt);
-    VoteGovernanceTriggers(trigger_opt, pindex->nHeight);
+    VoteGovernanceTriggers(trigger_opt, sb_opt ? std::make_optional(sb_opt->GetBlockHeight()) : std::nullopt,
+                           pindex->nHeight);
     CSuperblock_sptr pSuperblock;
     if (m_superblocks.GetBestSuperblock(m_dmnman.GetListAtChainTip(), pSuperblock, pindex->nHeight)) {
         ResetVotedFundingTrigger();

src/governance/signing.h

@@ -36,7 +36,11 @@ class GovernanceSigner
     const CMasternodeSync& m_mn_sync;
 
 private:
-    std::optional<uint256> votedFundingYesTriggerHash{std::nullopt};
+    struct FundingTriggerVote {
+        uint256 hash;
+        int superblock_height{0};
+    };
+    std::optional<FundingTriggerVote> votedFundingYesTrigger{std::nullopt};
 
 public:
     GovernanceSigner() = delete;
@@ -54,7 +58,8 @@ class GovernanceSigner
     std::optional<const CGovernanceObject> CreateGovernanceTrigger(const std::optional<const CSuperblock>& sb_opt);
     std::optional<const CSuperblock> CreateSuperblockCandidate(int nHeight) const;
     void ResetVotedFundingTrigger();
-    void VoteGovernanceTriggers(const std::optional<const CGovernanceObject>& trigger_opt, int nHeight);
+    void VoteGovernanceTriggers(const std::optional<const CGovernanceObject>& trigger_opt,
+                                std::optional<int> superblock_height, int nHeight);
 };
 
 #endif // BITCOIN_GOVERNANCE_SIGNING_H

src/llmq/blockprocessor.cpp

@@ -9,6 +9,7 @@
 #include <llmq/commitment.h>
 #include <llmq/options.h>
 #include <llmq/utils.h>
+#include <util/check.h>
 #include <util/helpers.h>
 #include <util/std23.h>
 
@@ -254,8 +255,7 @@ bool CQuorumBlockProcessor::ProcessBlock(Chainstate& chainstate, const CBlock& b
 
     // ProcessCommitment accepts a commitment identical to the stored one, so
     // rotation's multiple commitments per type need an explicit per-block check.
-    // It runs before any commitment is processed, because ProcessCommitment
-    // updates in-memory state that a rejected block would not restore.
+    // It runs before the signature checks, so a duplicate is rejected cheaply.
     // Null commitments stay exempt: they were never subject to bad-qc-dup.
     std::set<std::pair<Consensus::LLMQType, uint256>> seen_quorums;
     for (const auto& [_, qc] : qcs) {
@@ -445,12 +445,7 @@ bool CQuorumBlockProcessor::ProcessCommitment(Chainstate& chainstate, int nHeigh
     // invalidation should not depend on that.
     DropQcHashesCache();
 
-    {
-        LOCK(minableCommitmentsCs);
-        mapMinedCommitmentBlockCache.erase(qc.llmqType, qc.quorumHash);
-        minableCommitmentsByQuorum.erase(cacheKey);
-        minableCommitments.erase(::SerializeHash(qc));
-    }
+    WITH_LOCK(minableCommitmentsCs, mapMinedCommitmentBlockCache.erase(qc.llmqType, qc.quorumHash));
 
     LogPrint(BCLog::LLMQ, "%s -- processed commitment from block. type=%d, quorumIndex=%d, quorumHash=%s\n", __func__,
              std23::to_underlying(qc.llmqType), qc.quorumIndex, quorumHash.ToString());
@@ -561,6 +556,23 @@ bool CQuorumBlockProcessor::UndoBlock(const Chainstate& chainstate, const CBlock
     return true;
 }
 
+void CQuorumBlockProcessor::RemoveMineableCommitmentsForBlock(const CBlock& block, gsl::not_null<const CBlockIndex*> pindex)
+{
+    AssertLockHeld(::cs_main);
+
+    std::multimap<Consensus::LLMQType, CFinalCommitment> qcs;
+    if (BlockValidationState dummy; !Assume(GetCommitmentsFromBlock(block, pindex, qcs, dummy))) {
+        return;
+    }
+
+    LOCK(minableCommitmentsCs);
+    for (const auto& [_, qc] : qcs) {
+        if (qc.IsNull()) continue;
+        minableCommitmentsByQuorum.erase(std::make_pair(qc.llmqType, qc.quorumHash));
+        minableCommitments.erase(::SerializeHash(qc));
+    }
+}
+
 bool CQuorumBlockProcessor::GetCommitmentsFromBlock(const CBlock& block, gsl::not_null<const CBlockIndex*> pindex, std::multimap<Consensus::LLMQType, CFinalCommitment>& ret, BlockValidationState& state)
 {
     AssertLockHeld(::cs_main);

src/llmq/blockprocessor.h

@@ -108,6 +108,9 @@ class CQuorumBlockProcessor
                       bool fJustCheck, bool fBLSChecks) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs, !m_qc_hashes_cache_mutex);
     bool UndoBlock(const Chainstate& chainstate, const CBlock& block, gsl::not_null<const CBlockIndex*> pindex)
         EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs, !m_qc_hashes_cache_mutex);
+    //! Drop the commitments mined by a block that has been fully connected from the mineable pool.
+    void RemoveMineableCommitmentsForBlock(const CBlock& block, gsl::not_null<const CBlockIndex*> pindex)
+        EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs);
 
     //! it returns hash of commitment if it should be relay, otherwise nullopt
     std::optional<CInv> AddMineableCommitment(const CFinalCommitment& fqc) EXCLUSIVE_LOCKS_REQUIRED(!minableCommitmentsCs);

src/llmq/dkgsessionhandler.cpp

@@ -52,7 +52,12 @@ void CDKGPendingMessages::PushPendingMessage(NodeId from, const uint256& sender_
     count++;
 
     seenMessages.emplace(hash);
-    pendingMessages.emplace_back(from, std::move(pm));
+    if (from == -1) {
+        // Our own message must not wait behind a backlog of peer messages before it is relayed
+        pendingMessages.emplace_front(from, std::move(pm));
+    } else {
+        pendingMessages.emplace_back(from, std::move(pm));
+    }
 }
 
 std::list<CDKGPendingMessages::BinaryMessage> CDKGPendingMessages::PopPendingMessages(size_t maxCount)

src/llmq/dkgsessionhandler.h

@@ -71,7 +71,8 @@ class CDKGPendingMessages
      * Enqueue a serialized DKG message under @p from with content hash @p hash.
      * @p sender_protx keys the per-proTx quota: the sender's MNAuth-verified
      * proTxHash for remote messages, or this node's own for messages it
-     * produced itself (@p from == -1). Drops the message silently on quota
+     * produced itself (@p from == -1), which go to the front of the queue.
+     * Drops the message silently on quota
      * overflow or duplicate hash; quota-dropped messages are not marked seen,
      * so another peer with budget can re-deliver them. Caller is responsible
      * for hashing the payload and (for real peers) routing the erase-request

src/llmq/dkgsessionmgr.cpp

@@ -130,11 +130,22 @@ void CDKGSessionManager::WriteVerifiedVvecContribution(Consensus::LLMQType llmqT
         s << CBLSPublicKeyVersionWrapper(pubkey, false);
     }
     db->Write(std::make_tuple(DB_VVEC, llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash), Span{s});
+
+    // Reading the vvec back at commit time costs a point decompression and subgroup check per coefficient
+    LOCK(contributionsCacheCs);
+    auto& entry = contributionsCache[{llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash}];
+    entry.entryTime = SteadyClock::now();
+    entry.vvec = vvec;
 }
 
 void CDKGSessionManager::WriteVerifiedSkContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSSecretKey& skContribution)
 {
     db->Write(std::make_tuple(DB_SKCONTRIB, llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash), skContribution);
+
+    LOCK(contributionsCacheCs);
+    if (auto it = contributionsCache.find({llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash}); it != contributionsCache.end()) {
+        it->second.skContribution = skContribution;
+    }
 }
 
 void CDKGSessionManager::WriteEncryptedContributions(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSIESMultiRecipientObjects<CBLSSecretKey>& contributions)
@@ -228,13 +239,10 @@ void CDKGSessionManager::CleanupCache() const
 {
     LOCK(contributionsCacheCs);
     const auto curTime = SteadyClock::now();
-    for (auto it = contributionsCache.begin(); it != contributionsCache.end(); ) {
-        if (curTime - it->second.entryTime > MAX_CONTRIBUTION_CACHE_TIME) {
-            it = contributionsCache.erase(it);
-        } else {
-            ++it;
-        }
-    }
+    // A vvec still referenced elsewhere (by its DKG session) costs nothing to keep and is expensive to reload
+    std::erase_if(contributionsCache, [&](const auto& entry) {
+        return entry.second.vvec.use_count() == 1 && curTime - entry.second.entryTime > MAX_CONTRIBUTION_CACHE_TIME;
+    });
 }
 
 void CDKGSessionManager::CleanupOldContributions() const

src/llmq/dkgsessionmgr.h

@@ -140,8 +140,10 @@ class CDKGSessionManager
     bool GetPrematureCommitment(const uint256& hash, CDKGPrematureCommitment& ret) const;
 
     // Contributions are written while in the DKG
-    void WriteVerifiedVvecContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const BLSVerificationVectorPtr& vvec);
-    void WriteVerifiedSkContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSSecretKey& skContribution);
+    void WriteVerifiedVvecContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const BLSVerificationVectorPtr& vvec)
+        EXCLUSIVE_LOCKS_REQUIRED(!contributionsCacheCs);
+    void WriteVerifiedSkContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSSecretKey& skContribution)
+        EXCLUSIVE_LOCKS_REQUIRED(!contributionsCacheCs);
     bool GetVerifiedContributions(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex,
                                   const std::vector<bool>& validMembers, std::vector<uint16_t>& memberIndexesRet,
                                   std::vector<BLSVerificationVectorPtr>& vvecsRet,

src/net.cpp

@@ -1771,6 +1771,7 @@ std::pair<size_t, bool> CConnman::SocketSendData(CNode& node) const
                     LogPrint(BCLog::NET, "socket send error for peer=%d: %s\n", node.GetId(), NetworkErrorString(nErr));
                     node.fDisconnect = true;
                 }
+                node.fCanSendData = false;
             }
             break;
         }