backport: v24.0.x backports for v24.0.0-rc.5 (v23.1.9 fixes, #7947)

src/active/dkgsession.cpp

@@ -92,6 +92,14 @@ std::optional<CDKGContribution> ActiveDKGSession::SendContributions()
             skContrib.MakeNewKey();
         }
 
+        if (i != myIdx && ShouldSimulateError(DKGError::type::CONTRIBUTION_INVALID)) {
+            // Deliver a share that decrypts to an all-zero (structurally invalid)
+            // CBLSSecretKey on the recipient. Used to exercise the DKG intake's
+            // handling of a peer-supplied invalid contribution share.
+            logger.Batch("sending invalid (zero) share to %s", m->dmn->proTxHash.ToString());
+            skContrib = CBLSSecretKey{};
+        }
+
         if (!qc.contributions->Encrypt(i, m->dmn->pdmnState->pubKeyOperator.Get(), skContrib, PROTOCOL_VERSION)) {
             logger.Batch("failed to encrypt contribution for %s", m->dmn->proTxHash.ToString());
             return std::nullopt;

src/bls/bls_worker.cpp

@@ -5,6 +5,7 @@
 #include <bls/bls_worker.h>
 #include <hash.h>
 
+#include <logging.h>
 #include <util/system.h>
 
 #include <memory>
@@ -69,11 +70,6 @@ void CBLSWorker::Stop()
     workerPool.stop(true);
 }
 
-void CBLSWorker::PushJob(std::function<void()> job)
-{
-    workerPool.push([job = std::move(job)](int) { job(); });
-}
-
 #ifndef BUILD_BITCOIN_INTERNAL
 bool CBLSWorker::GenerateContributions(int quorumThreshold, Span<CBLSId> ids, BLSVerificationVectorPtr& vvecRet, std::vector<CBLSSecretKey>& skSharesRet)
 {
@@ -304,8 +300,14 @@ struct Aggregator : public std::enable_shared_from_this<Aggregator<T>> {
     template <typename TP>
     static T SyncAggregate(Span<TP> vec, size_t start, size_t count)
     {
+        // Defence in depth: AggregateInsecure() asserts that every operand is valid, so
+        // peer-influenced data (e.g. a decrypted contribution share) must never reach it
+        // while invalid. Short-circuit to an invalid aggregate instead; callers already
+        // treat an invalid result as an aggregation failure rather than aborting.
         T result = *vec[start];
+        if (!result.IsValid()) return {};
         for (size_t j = 1; j < count; j++) {
+            if (!vec[start + j]->IsValid()) return {};
             result.AggregateInsecure(*vec[start + j]);
         }
         return result;
@@ -686,16 +688,45 @@ CBLSPublicKey CBLSWorker::BuildPubKeyShare(const BLSVerificationVectorPtr& vvec,
 void CBLSWorker::AsyncVerifyContributionShares(const CBLSId& forId, Span<BLSVerificationVectorPtr> vvecs, Span<CBLSSecretKey> skShares,
                                                bool parallel, bool aggregated, std::function<void(const std::vector<bool>&)> doneCallback)
 {
+    if (!forId.IsValid()) {
+        std::vector<bool> result;
+        result.assign(vvecs.size(), false);
+        doneCallback(result);
+        return;
+    }
+
     // Empty input never schedules any work, so doneCallback would never fire and the
     // future-based wrappers would throw broken_promise. Mirror the empty-input
     // short-circuit in AsyncBuildQuorumVerificationVector / AsyncAggregateHelper.
     if (vvecs.empty()) {
         doneCallback(std::vector<bool>{});
         return;
     }
-    if (!forId.IsValid() || !VerifyVerificationVectors(vvecs)) {
+
+    if (!VerifyVerificationVectors(vvecs)) {
+        // A batch-level verification-vector problem must not fail the whole batch. The
+        // only thing the batched VerifyVerificationVectors adds over the per-member
+        // VerifyVerificationVector check already run in PreVerifyMessage is detecting a
+        // coefficient point shared between two members' vvecs. Failing the whole batch on
+        // that lets one member, by placing a coefficient that collides with another
+        // member's, force every other honest member in the same verification batch to be
+        // marked invalid and complained about - a denial of service against their
+        // participation in the DKG. Instead, verify each contribution on its own: a member
+        // fails only if its own share is inconsistent with its own vvec, so a member that
+        // placed a colliding coefficient is caught on its own while the honest members it
+        // collided with verify normally.
+        LogPrint(BCLog::LLMQ_DKG, "CBLSWorker::%s -- batch verification-vector check failed; verifying %d contributions individually\n",
+                  __func__, vvecs.size());
+        std::vector<std::future<bool>> futures;
+        futures.reserve(vvecs.size());
+        for (size_t i = 0; i < vvecs.size(); i++) {
+            futures.emplace_back(AsyncVerifyContributionShare(forId, vvecs[i], skShares[i]));
+        }
         std::vector<bool> result;
-        result.assign(vvecs.size(), false);
+        result.reserve(vvecs.size());
+        for (auto& f : futures) {
+            result.push_back(f.get());
+        }
         doneCallback(result);
         return;
     }

src/bls/bls_worker.h

@@ -56,7 +56,6 @@ class CBLSWorker
 
     void Start(int16_t worker_count);
     void Stop();
-    void PushJob(std::function<void()> job);
 
 #ifndef BUILD_BITCOIN_INTERNAL
     bool GenerateContributions(int threshold, Span<CBLSId> ids, BLSVerificationVectorPtr& vvecRet, std::vector<CBLSSecretKey>& skSharesRet);

src/chain.h

@@ -41,8 +41,8 @@ class CBlockFileInfo
 {
 public:
     unsigned int nBlocks{};      //!< number of blocks stored in file
-    unsigned int nSize{};        //!< number of used bytes of block file
-    unsigned int nUndoSize{};    //!< number of used bytes in the undo file
+    uint64_t nSize{};        //!< number of used bytes of block file
+    uint64_t nUndoSize{};    //!< number of used bytes in the undo file
     unsigned int nHeightFirst{}; //!< lowest height of block in file
     unsigned int nHeightLast{};  //!< highest height of block in file
     uint64_t nTimeFirst{};       //!< earliest time of block in file
@@ -158,10 +158,10 @@ class CBlockIndex
     int nFile GUARDED_BY(::cs_main){0};
 
     //! Byte offset within blk?????.dat where this block's data is stored
-    unsigned int nDataPos GUARDED_BY(::cs_main){0};
+    uint64_t nDataPos GUARDED_BY(::cs_main){0};
 
     //! Byte offset within rev?????.dat where this block's undo data is stored
-    unsigned int nUndoPos GUARDED_BY(::cs_main){0};
+    uint64_t nUndoPos GUARDED_BY(::cs_main){0};
 
     //! (memory only) Total amount of work (expected number of hashes) in the chain up to and including this block
     arith_uint256 nChainWork{};

src/flatfile.cpp

@@ -45,7 +45,7 @@ FILE* FlatFileSeq::Open(const FlatFilePos& pos, bool read_only)
         LogPrintf("Unable to open file %s\n", fs::PathToString(path));
         return nullptr;
     }
-    if (pos.nPos && fseek(file, pos.nPos, SEEK_SET)) {
+    if (pos.nPos && FileSeek64(file, pos.nPos, SEEK_SET)) {
         LogPrintf("Unable to seek to position %u of %s\n", pos.nPos, fs::PathToString(path));
         fclose(file);
         return nullptr;
@@ -57,11 +57,11 @@ size_t FlatFileSeq::Allocate(const FlatFilePos& pos, size_t add_size, bool& out_
 {
     out_of_space = false;
 
-    unsigned int n_old_chunks = (pos.nPos + m_chunk_size - 1) / m_chunk_size;
-    unsigned int n_new_chunks = (pos.nPos + add_size + m_chunk_size - 1) / m_chunk_size;
+    uint64_t n_old_chunks = (pos.nPos + m_chunk_size - 1) / m_chunk_size;
+    uint64_t n_new_chunks = (pos.nPos + add_size + m_chunk_size - 1) / m_chunk_size;
     if (n_new_chunks > n_old_chunks) {
-        size_t old_size = pos.nPos;
-        size_t new_size = n_new_chunks * m_chunk_size;
+        uint64_t old_size = pos.nPos;
+        uint64_t new_size = n_new_chunks * m_chunk_size;
         size_t inc_size = new_size - old_size;
 
         if (CheckDiskSpace(m_dir, inc_size)) {

src/flatfile.h

@@ -14,7 +14,7 @@
 struct FlatFilePos
 {
     int nFile{-1};
-    unsigned int nPos{0};
+    uint64_t nPos{0};
 
     SERIALIZE_METHODS(FlatFilePos, obj)
     {
@@ -23,7 +23,7 @@ struct FlatFilePos
 
     FlatFilePos() {}
 
-    FlatFilePos(int nFileIn, unsigned int nPosIn) :
+    FlatFilePos(int nFileIn, uint64_t nPosIn) :
         nFile(nFileIn),
         nPos(nPosIn)
     {}

src/governance/governance.cpp

@@ -19,6 +19,7 @@
 #include <net.h>
 #include <node/interface_ui.h>
 #include <protocol.h>
+#include <random.h>
 #include <timedata.h>
 #include <util/check.h>
 #include <util/time.h>
@@ -388,6 +389,14 @@ void CGovernanceManager::AddGovernanceObjectForTesting(const CGovernanceObject&
     mapObjects.emplace(govobj.GetHash(), std::make_shared<CGovernanceObject>(govobj));
 }
 
+void CGovernanceManager::AddOrphanVoteForTesting(const CGovernanceVote& vote)
+{
+    AssertLockNotHeld(cs_store);
+    LOCK(cs_store);
+    cmmapOrphanVotes.Insert(vote.GetParentHash(),
+                            governance::OrphanVote{vote, Now<NodeSeconds>() + GOVERNANCE_ORPHAN_EXPIRATION_TIME});
+}
+
 void CGovernanceManager::CheckAndRemove()
 {
     AssertLockNotHeld(cs_store);
@@ -1159,6 +1168,12 @@ std::vector<uint256> CGovernanceManager::GetOrphanVoteObjectHashes()
         }
     }
 
+    // A random subset, so that parent hashes ground to sort first cannot pin which ones are requested
+    if (vecHashesFiltered.size() > MAX_ORPHAN_OBJECT_REQUESTS) {
+        Shuffle(vecHashesFiltered.begin(), vecHashesFiltered.end(), FastRandomContext());
+        vecHashesFiltered.resize(MAX_ORPHAN_OBJECT_REQUESTS);
+    }
+
     return vecHashesFiltered;
 }
 

src/governance/governance.h

@@ -332,6 +332,8 @@ class CGovernanceManager : public GovernanceStore
      *  running collateral or chain validation. */
     void AddGovernanceObjectForTesting(const CGovernanceObject& govobj)
         EXCLUSIVE_LOCKS_REQUIRED(!cs_store);
+    /** Test-only helper: caches a vote as an orphan of its parent without validating it. */
+    void AddOrphanVoteForTesting(const CGovernanceVote& vote) EXCLUSIVE_LOCKS_REQUIRED(!cs_store);
 
     // Thread-safe accessors
     bool HaveObjectForHash(const uint256& nHash) const
@@ -358,7 +360,12 @@ class CGovernanceManager : public GovernanceStore
     // Used by NetGovernance
     std::vector<CInv> FetchRelayInventory() EXCLUSIVE_LOCKS_REQUIRED(!cs_relay);
     void CheckAndRemove() EXCLUSIVE_LOCKS_REQUIRED(!cs_store);
-    /** Get hashes of governance objects for which we have orphan votes. Also cleans up expired orphans. */
+    /** Upper bound on the orphan parent hashes GetOrphanVoteObjectHashes() returns per call. Each one is
+     *  re-requested from every peer, and one masternode key can sign votes for any number of invented
+     *  parent hashes. */
+    static constexpr size_t MAX_ORPHAN_OBJECT_REQUESTS{100};
+    /** Get hashes of governance objects for which we have orphan votes, at most
+     *  MAX_ORPHAN_OBJECT_REQUESTS of them chosen at random. Also cleans up expired orphans. */
     [[nodiscard]] std::vector<uint256> GetOrphanVoteObjectHashes() EXCLUSIVE_LOCKS_REQUIRED(!cs_store);
     std::pair<std::vector<uint256>, std::vector<uint256>> FetchGovernanceObjectVotes(
         size_t peers_per_hash_max, int64_t now, std::map<uint256, std::map<CService, int64_t>>& map_asked_recently) const

src/index/blockfilterindex.cpp

@@ -492,7 +492,7 @@ bool BlockFilterIndex::LookupFilterRange(int start_height, const CBlockIndex* st
         if (!file || entry.pos.nFile != file_num) {
             file = std::make_unique<AutoFile>(m_filter_fileseq->Open(entry.pos, true));
             file_num = entry.pos.nFile;
-        } else if (fseek(file->Get(), entry.pos.nPos, SEEK_SET) != 0) {
+        } else if (FileSeek64(file->Get(), entry.pos.nPos, SEEK_SET) != 0) {
             LogError("%s: unable to seek to position %u in filter file %d\n",
                      __func__, entry.pos.nPos, entry.pos.nFile);
             return false;

src/index/txindex.cpp

@@ -6,6 +6,7 @@
 
 #include <index/disktxpos.h>
 #include <node/blockstorage.h>
+#include <util/fs_helpers.h>
 #include <util/system.h>
 #include <validation.h>
 
@@ -87,7 +88,7 @@ bool TxIndex::FindTx(const uint256& tx_hash, uint256& block_hash, CTransactionRe
     CBlockHeader header;
     try {
         file >> header;
-        if (fseek(file.Get(), postx.nTxOffset, SEEK_CUR)) {
+        if (FileSeek64(file.Get(), postx.nTxOffset, SEEK_CUR)) {
             LogError("%s: fseek(...) failed\n", __func__);
             return false;
         }

src/interfaces/chain.h

@@ -94,7 +94,7 @@ struct BlockInfo {
     const uint256* prev_hash = nullptr;
     int height = -1;
     int file_number = -1;
-    unsigned data_pos = 0;
+    uint64_t data_pos = 0;
     const CBlock* data = nullptr;
     const CBlockUndo* undo_data = nullptr;
 

src/llmq/context.cpp

@@ -28,6 +28,5 @@ LLMQContext::LLMQContext(CDeterministicMNManager& dmnman, CEvoDB& evo_db, Chains
 
 LLMQContext::~LLMQContext()
 {
-    qman->InterruptWarming();
     bls_worker->Stop();
 }

src/llmq/dkgsession.cpp

@@ -238,6 +238,13 @@ std::optional<CInv> CDKGSession::ReceiveMessage(const CDKGContribution& qc)
     if (!MaybeDecrypt(*qc.contributions, *myIdx, skContribution, PROTOCOL_VERSION)) {
         logger.Batch("contribution from %s could not be decrypted", member->dmn->proTxHash.ToString());
         complain = true;
+    } else if (!skContribution.IsValid()) {
+        // A decryptable but structurally-invalid (e.g. all-zero) share must never be
+        // retained: it later reaches CBLSSecretKey::AggregateInsecure() in the batched
+        // contribution verification, whose assert(IsValid()) would abort an honest node.
+        // Treat it as a bad contribution and complain.
+        logger.Batch("contribution from %s decrypted to an invalid share", member->dmn->proTxHash.ToString());
+        complain = true;
     } else if (member->idx != myIdx && ShouldSimulateError(DKGError::type::COMPLAIN_LIE)) {
         logger.Batch("lying/complaining for %s", member->dmn->proTxHash.ToString());
         complain = true;

src/llmq/dkgsession.h

@@ -69,6 +69,7 @@ class DKGError {
         CONTRIBUTION_LIE,
         JUSTIFY_OMIT,
         JUSTIFY_LIE,
+        CONTRIBUTION_INVALID,
         _COUNT
     };
     static constexpr DKGError::type from_string(std::string_view in) {
@@ -79,6 +80,7 @@ class DKGError {
         if (in == "contribution-lie") return CONTRIBUTION_LIE;
         if (in == "justify-lie") return JUSTIFY_LIE;
         if (in == "justify-omit") return JUSTIFY_OMIT;
+        if (in == "contribution-invalid") return CONTRIBUTION_INVALID;
         return _COUNT;
     }
 };

src/llmq/net_quorum.cpp

@@ -336,8 +336,11 @@ bool NetQuorum::ProcessContribQDATA(const CNode& pfrom, CDataStream& vRecv,
     std::vector<CBLSSecretKey> vecSecretKeys;
     vecSecretKeys.resize(vecEncrypted.size());
     for (const auto i : util::irange(vecEncrypted.size())) {
-        if (!m_nodeman->Decrypt(vecEncrypted[i], memberIdx, vecSecretKeys[i], PROTOCOL_VERSION)) {
-            m_peer_manager->PeerMisbehaving(pfrom.GetId(), 10, "failed to decrypt");
+        if (!m_nodeman->Decrypt(vecEncrypted[i], memberIdx, vecSecretKeys[i], PROTOCOL_VERSION) ||
+            !vecSecretKeys[i].IsValid()) {
+            // Reject an undecryptable or structurally-invalid (e.g. all-zero) share before
+            // it reaches AggregateSecretKeys()/AggregateInsecure(), which asserts validity.
+            m_peer_manager->PeerMisbehaving(pfrom.GetId(), 10, "invalid contribution share");
             return false;
         }
     }

src/llmq/net_signing.cpp

@@ -401,7 +401,7 @@ void NetSigning::ProcessPendingSigShares(
 {
     // It's ok to perform insecure batched verification here as we verify against the quorum public key shares,
     // which are not craftable by individual entities, making the rogue public key attack impossible
-    CBLSBatchVerifier<NodeId, SigShareKey> batchVerifier(false, true);
+    SigShareBatchVerifier batchVerifier(false, true);
 
     cxxtimer::Timer prepareTimer(true);
     size_t verifyCount = 0;
@@ -432,7 +432,7 @@ void NetSigning::ProcessPendingSigShares(
                 assert(false);
             }
 
-            batchVerifier.PushMessage(nodeId, sigShare.GetKey(), sigShare.GetSignHash(), sig, pubKeyShare);
+            PushSigShareToBatchVerifier(batchVerifier, nodeId, sigShare, sig, pubKeyShare);
             verifyCount++;
         }
     }