Merge dashpay#6622: fix: efficient build mnlistdiffs in rotation info

03c15a3 fix: actually bump protocol version (pasta)
606719a nit: s/use_leagcy_construction/use_legacy_construction/ (pasta)
7432879 fmt: apply clang-format suggestions (pasta)
bdfd597 feat: only use efficient qrinfo construction for new shared protocol version (pasta)
71112dc refactor: more clang-format (UdjinM6)
ff16e68 refactor: clang-format (UdjinM6)
1f16cf4 fix: sort indexes in GetLastBaseBlockHash (UdjinM6)
810ecd8 fix: don't use `baseBlockIndexes.back()` for the tip (UdjinM6)
ec78465 fix: move BuildSimplifiedMNListDiff for block h (UdjinM6)
2a12ff6 bump guix (Konstantin Akimov)
e8bbfd2 fix(perf): build mnlistdiffs in rotation info using dynamically the highest known base block (Odysseas Gabrielides)

Pull request description:

  ## Issue being fixed or feature implemented
  See dashpay#6587 for history

  ## Breaking Changes
  This should remain backwards compatible

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] 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 _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 03c15a3

Tree-SHA512: d378b519fe614f047b031509b0ec764160ae8674f7417ef571ce7e6fd98d6c94966e6c94681e7bb9c66ac334ce3640492948aae6bb45737dfdf480f6f58b4472

Author: PastaPastaPasta

src/llmq/snapshot.cpp

@@ -87,7 +87,7 @@ UniValue CQuorumRotationInfo::ToJson() const
 bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotManager& qsnapman,
                              const ChainstateManager& chainman, const CQuorumManager& qman,
                              const CQuorumBlockProcessor& qblockman, const CGetQuorumRotationInfo& request,
-                             CQuorumRotationInfo& response, std::string& errorRet)
+                             bool use_legacy_construction, CQuorumRotationInfo& response, std::string& errorRet)
 {
     AssertLockHeld(cs_main);
 
@@ -112,19 +112,23 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
             }
             baseBlockIndexes.push_back(blockIndex);
         }
-        std::sort(baseBlockIndexes.begin(), baseBlockIndexes.end(), [](const CBlockIndex* a, const CBlockIndex* b) {
-            return a->nHeight < b->nHeight;
-        });
+        if (use_legacy_construction) {
+            std::sort(baseBlockIndexes.begin(), baseBlockIndexes.end(),
+                      [](const CBlockIndex* a, const CBlockIndex* b) { return a->nHeight < b->nHeight; });
+        }
     }
 
     const CBlockIndex* tipBlockIndex = chainman.ActiveChain().Tip();
     if (!tipBlockIndex) {
         errorRet = strprintf("tip block not found");
         return false;
     }
-    //Build MN list Diff always with highest baseblock
-    if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, baseBlockIndexes.back()->GetBlockHash(), tipBlockIndex->GetBlockHash(), response.mnListDiffTip, errorRet)) {
-        return false;
+    if (use_legacy_construction) {
+        // Build MN list Diff always with highest baseblock
+        if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, baseBlockIndexes.back()->GetBlockHash(),
+                                       tipBlockIndex->GetBlockHash(), response.mnListDiffTip, errorRet)) {
+            return false;
+        }
     }
 
     const CBlockIndex* blockIndex = chainman.m_blockman.LookupBlockIndex(request.blockRequestHash);
@@ -149,15 +153,19 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
         return false;
     }
 
-    const CBlockIndex* pWorkBlockIndex = hBlockIndex->GetAncestor(hBlockIndex->nHeight - workDiff);
-    if (!pWorkBlockIndex) {
+    const CBlockIndex* pWorkBlockHIndex = hBlockIndex->GetAncestor(hBlockIndex->nHeight - workDiff);
+    if (!pWorkBlockHIndex) {
         errorRet = strprintf("Can not find work block H");
         return false;
     }
 
-    //Build MN list Diff always with highest baseblock
-    if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockIndex), pWorkBlockIndex->GetBlockHash(), response.mnListDiffH, errorRet)) {
-        return false;
+    if (use_legacy_construction) {
+        // Build MN list Diff always with highest baseblock
+        if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
+                                       GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHIndex, use_legacy_construction),
+                                       pWorkBlockHIndex->GetBlockHash(), response.mnListDiffH, errorRet)) {
+            return false;
+        }
     }
 
     const CBlockIndex* pBlockHMinusCIndex = tipBlockIndex->GetAncestor(hBlockIndex->nHeight - cycleLength);
@@ -202,8 +210,12 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
     const CBlockIndex* pWorkBlockHMinus4CIndex = pBlockHMinus4CIndex->GetAncestor(pBlockHMinus4CIndex->nHeight - workDiff);
     //Checked later if extraShare is on
 
-    if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinusCIndex), pWorkBlockHMinusCIndex->GetBlockHash(), response.mnListDiffAtHMinusC, errorRet)) {
-        return false;
+    if (use_legacy_construction) {
+        if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
+                                       GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinusCIndex, use_legacy_construction),
+                                       pWorkBlockHMinusCIndex->GetBlockHash(), response.mnListDiffAtHMinusC, errorRet)) {
+            return false;
+        }
     }
 
     auto snapshotHMinusC = qsnapman.GetSnapshotForBlock(llmqType, pBlockHMinusCIndex);
@@ -214,8 +226,13 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
         response.quorumSnapshotAtHMinusC = std::move(snapshotHMinusC.value());
     }
 
-    if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus2CIndex), pWorkBlockHMinus2CIndex->GetBlockHash(), response.mnListDiffAtHMinus2C, errorRet)) {
-        return false;
+    if (use_legacy_construction) {
+        if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
+                                       GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus2CIndex,
+                                                            use_legacy_construction),
+                                       pWorkBlockHMinus2CIndex->GetBlockHash(), response.mnListDiffAtHMinus2C, errorRet)) {
+            return false;
+        }
     }
 
     auto snapshotHMinus2C = qsnapman.GetSnapshotForBlock(llmqType, pBlockHMinus2CIndex);
@@ -226,8 +243,13 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
         response.quorumSnapshotAtHMinus2C = std::move(snapshotHMinus2C.value());
     }
 
-    if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus3CIndex), pWorkBlockHMinus3CIndex->GetBlockHash(), response.mnListDiffAtHMinus3C, errorRet)) {
-        return false;
+    if (use_legacy_construction) {
+        if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
+                                       GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus3CIndex,
+                                                            use_legacy_construction),
+                                       pWorkBlockHMinus3CIndex->GetBlockHash(), response.mnListDiffAtHMinus3C, errorRet)) {
+            return false;
+        }
     }
 
     auto snapshotHMinus3C = qsnapman.GetSnapshotForBlock(llmqType, pBlockHMinus3CIndex);
@@ -255,10 +277,15 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
         }
 
         CSimplifiedMNListDiff mn4c;
-        if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus4CIndex), pWorkBlockHMinus4CIndex->GetBlockHash(), mn4c, errorRet)) {
+        if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
+                                       GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus4CIndex,
+                                                            use_legacy_construction),
+                                       pWorkBlockHMinus4CIndex->GetBlockHash(), mn4c, errorRet)) {
             return false;
         }
-
+        if (!use_legacy_construction) {
+            baseBlockIndexes.push_back(pWorkBlockHMinus4CIndex);
+        }
         response.mnListDiffAtHMinus4C = std::move(mn4c);
     } else {
         response.extraShare = false;
@@ -311,19 +338,65 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
         }
 
         CSimplifiedMNListDiff mnhneeded;
-        if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, GetLastBaseBlockHash(baseBlockIndexes, pNeededWorkBlockIndex), pNeededWorkBlockIndex->GetBlockHash(), mnhneeded, errorRet)) {
+        if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
+                                       GetLastBaseBlockHash(baseBlockIndexes, pNeededWorkBlockIndex, use_legacy_construction),
+                                       pNeededWorkBlockIndex->GetBlockHash(), mnhneeded, errorRet)) {
             return false;
         }
-
+        if (!use_legacy_construction) {
+            baseBlockIndexes.push_back(pNeededWorkBlockIndex);
+        }
         response.mnListDiffList.push_back(mnhneeded);
     }
 
+    if (!use_legacy_construction) {
+        if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
+                                       GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus3CIndex,
+                                                            use_legacy_construction),
+                                       pWorkBlockHMinus3CIndex->GetBlockHash(), response.mnListDiffAtHMinus3C, errorRet)) {
+            return false;
+        }
+        baseBlockIndexes.push_back(pWorkBlockHMinus3CIndex);
+
+        if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
+                                       GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus2CIndex,
+                                                            use_legacy_construction),
+                                       pWorkBlockHMinus2CIndex->GetBlockHash(), response.mnListDiffAtHMinus2C, errorRet)) {
+            return false;
+        }
+        baseBlockIndexes.push_back(pWorkBlockHMinus2CIndex);
+
+        if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
+                                       GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinusCIndex, use_legacy_construction),
+                                       pWorkBlockHMinusCIndex->GetBlockHash(), response.mnListDiffAtHMinusC, errorRet)) {
+            return false;
+        }
+        baseBlockIndexes.push_back(pWorkBlockHMinusCIndex);
+
+        if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
+                                       GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHIndex, use_legacy_construction),
+                                       pWorkBlockHIndex->GetBlockHash(), response.mnListDiffH, errorRet)) {
+            return false;
+        }
+        baseBlockIndexes.push_back(pWorkBlockHIndex);
+
+        if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
+                                       GetLastBaseBlockHash(baseBlockIndexes, tipBlockIndex, use_legacy_construction),
+                                       tipBlockIndex->GetBlockHash(), response.mnListDiffTip, errorRet)) {
+            return false;
+        }
+    }
     return true;
 }
 
-uint256 GetLastBaseBlockHash(Span<const CBlockIndex*> baseBlockIndexes, const CBlockIndex* blockIndex)
+uint256 GetLastBaseBlockHash(Span<const CBlockIndex*> baseBlockIndexes, const CBlockIndex* blockIndex,
+                             bool use_legacy_construction)
 {
     uint256 hash;
+    if (!use_legacy_construction) {
+        std::sort(baseBlockIndexes.begin(), baseBlockIndexes.end(),
+                  [](const CBlockIndex* a, const CBlockIndex* b) { return a->nHeight < b->nHeight; });
+    }
     for (const auto baseBlock : baseBlockIndexes) {
         if (baseBlock->nHeight >= blockIndex->nHeight)
             break;

src/llmq/snapshot.h

@@ -210,8 +210,10 @@ class CQuorumRotationInfo
 bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotManager& qsnapman,
                              const ChainstateManager& chainman, const CQuorumManager& qman,
                              const CQuorumBlockProcessor& qblockman, const CGetQuorumRotationInfo& request,
-                             CQuorumRotationInfo& response, std::string& errorRet) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
-uint256 GetLastBaseBlockHash(Span<const CBlockIndex*> baseBlockIndexes, const CBlockIndex* blockIndex);
+                             bool use_legacy_construction, CQuorumRotationInfo& response, std::string& errorRet)
+    EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+uint256 GetLastBaseBlockHash(Span<const CBlockIndex*> baseBlockIndexes, const CBlockIndex* blockIndex,
+                             bool use_legacy_construction);
 
 class CQuorumSnapshotManager
 {

src/net_processing.cpp

@@ -5203,7 +5203,8 @@ void PeerManagerImpl::ProcessMessage(
 
         llmq::CQuorumRotationInfo quorumRotationInfoRet;
         std::string strError;
-        if (BuildQuorumRotationInfo(*m_dmnman, *m_llmq_ctx->qsnapman, m_chainman, *m_llmq_ctx->qman, *m_llmq_ctx->quorum_block_processor, cmd, quorumRotationInfoRet, strError)) {
+        bool use_legacy_construction = pfrom.GetCommonVersion() < EFFICIENT_QRINFO_VERSION;;
+        if (BuildQuorumRotationInfo(*m_dmnman, *m_llmq_ctx->qsnapman, m_chainman, *m_llmq_ctx->qman, *m_llmq_ctx->quorum_block_processor, cmd, use_legacy_construction, quorumRotationInfoRet, strError)) {
             m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QUORUMROTATIONINFO, quorumRotationInfoRet));
         } else {
             strError = strprintf("getquorumrotationinfo failed for size(baseBlockHashes)=%d, blockRequestHash=%s. error=%s", cmd.baseBlockHashes.size(), cmd.blockRequestHash.ToString(), strError);

src/rpc/quorums.cpp

@@ -860,7 +860,7 @@ static RPCHelpMan quorum_rotationinfo()
     LOCK(cs_main);
 
     if (!BuildQuorumRotationInfo(*CHECK_NONFATAL(node.dmnman), *llmq_ctx.qsnapman, chainman, *llmq_ctx.qman,
-                                 *llmq_ctx.quorum_block_processor, cmd, quorumRotationInfoRet, strError)) {
+                                 *llmq_ctx.quorum_block_processor, cmd, false, quorumRotationInfoRet, strError)) {
         throw JSONRPCError(RPC_INVALID_REQUEST, strError);
     }
 

src/version.h

@@ -11,7 +11,7 @@
  */
 
 
-static const int PROTOCOL_VERSION = 70235;
+static const int PROTOCOL_VERSION = 70236;
 
 //! initial proto version, to be increased after version/verack negotiation
 static const int INIT_PROTO_VERSION = 209;
@@ -61,6 +61,9 @@ static const int DSQ_INV_VERSION = 70234;
 //! Maximum header count for HEADRES2 message was increased from 2000 to 8000 in this version
 static const int INCREASE_MAX_HEADERS2_VERSION = 70235;
 
+//! Behavior of QRINFO is changed in this protocol version
+static const int EFFICIENT_QRINFO_VERSION = 70236;
+
 // Make sure that none of the values above collide with `ADDRV2_FORMAT`.
 
 #endif // BITCOIN_VERSION_H

test/functional/test_framework/p2p.py

@@ -100,7 +100,7 @@
 MIN_P2P_VERSION_SUPPORTED = 60001
 # The P2P version that this test framework implements and sends in its `version` message
 # Version 70235 increased max header count for HEADERS2 message from 2000 to 8000
-P2P_VERSION = 70235
+P2P_VERSION = 70236
 # The services that this test framework offers in its `version` message
 P2P_SERVICES = NODE_NETWORK | NODE_HEADERS_COMPRESSED
 # The P2P user agent string that this test framework sends in its `version` message