Merge #6608: fix: cycleHash should represent a cycle starting block of the signing quorum

d150a65 docs: add release notes (UdjinM6)
85c6b58 feat: bump PROTOCOL_VERSION/MIN_MASTERNODE_PROTO_VERSION to 70237 (UdjinM6)
c20dfd8 fix: `cycleHash` should represent a cycle starting block of the signing quorum (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  Currently `cycleHash` is calculated based on the current chain height we are signing `isdlock` at. This however can result in `cycleHash` to be a block hash of a new cycle where DKG is still in progress which can be confusing to verifiers.

  ## What was done?
  Select a signing quorum first and then calculate `cycleHash` based on the quorum's base block index instead of using the current chain height.

  ## How Has This Been Tested?
  Running a testnet MN for some time. It's creating new `isdlock`s and processing `isdlock`s from other nodes with no issues.

  ## Breaking Changes
  None, `cycleHash` is simply going to be more accurate now.

  ## Checklist:
  - [ ] 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
  - [ ] I have assigned this pull request to a milestone

ACKs for top commit:
  PastaPastaPasta:
    utACK d150a65
  knst:
    utACK d150a65

Tree-SHA512: ff7e8a9f37aded9ac330185a34db0eef346c6a3f4a6e22f11a6081ff074a789d260fbdf9efc1d8590d4013ecd95a273e07126dfcb5a7f6d6ebebdb45a543f053

Author: PastaPastaPasta

doc/release-notes-6608.md

@@ -0,0 +1,4 @@
+P2P Changes
+-----------
+
+* `cycleHash` field in `isdlock` message will now represent a DKG cycle starting block of the signing quorum instead of a DKG cycle starting block corresponding to the current chain height. While this is fully backwards compatible with older versions of Dash Core, other implementations might not be expecting this, so the P2P protocol version was bumped to 70237.

src/llmq/instantsend.cpp

@@ -671,21 +671,26 @@ void CInstantSendManager::TrySignInstantSendLock(const CTransaction& tx)
         islock.inputs.emplace_back(in.prevout);
     }
 
-    {
-        const auto &llmq_params_opt = Params().GetLLMQ(llmqType);
-        assert(llmq_params_opt);
-        LOCK(cs_main);
-        const auto dkgInterval = llmq_params_opt->dkgInterval;
-        const auto quorumHeight = m_chainstate.m_chain.Height() - (m_chainstate.m_chain.Height() % dkgInterval);
-        islock.cycleHash = m_chainstate.m_chain[quorumHeight]->GetBlockHash();
-    }
-
     auto id = islock.GetRequestId();
 
     if (sigman.HasRecoveredSigForId(llmqType, id)) {
         return;
     }
 
+    const auto& llmq_params_opt = Params().GetLLMQ(llmqType);
+    assert(llmq_params_opt);
+    const auto quorum = SelectQuorumForSigning(llmq_params_opt.value(), m_chainstate.m_chain, qman, id);
+
+    if (!quorum) {
+        LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- failed to select quorum. islock id=%s, txid=%s\n",
+                 __func__, id.ToString(), tx.GetHash().ToString());
+        return;
+    }
+
+    const int cycle_height = quorum->m_quorum_base_block_index->nHeight -
+                             quorum->m_quorum_base_block_index->nHeight % llmq_params_opt->dkgInterval;
+    islock.cycleHash = quorum->m_quorum_base_block_index->GetAncestor(cycle_height)->GetBlockHash();
+
     {
         LOCK(cs_creating);
         auto e = creatingInstantSendLocks.emplace(id, std::move(islock));
@@ -695,7 +700,7 @@ void CInstantSendManager::TrySignInstantSendLock(const CTransaction& tx)
         txToCreatingInstantSendLocks.emplace(tx.GetHash(), &e.first->second);
     }
 
-    sigman.AsyncSignIfMember(llmqType, shareman, id, tx.GetHash());
+    sigman.AsyncSignIfMember(llmqType, shareman, id, tx.GetHash(), quorum->m_quorum_base_block_index->GetBlockHash());
 }
 
 void CInstantSendManager::HandleNewInstantSendLockRecoveredSig(const llmq::CRecoveredSig& recoveredSig)

src/version.h

@@ -11,7 +11,7 @@
  */
 
 
-static const int PROTOCOL_VERSION = 70236;
+static const int PROTOCOL_VERSION = 70237;
 
 //! initial proto version, to be increased after version/verack negotiation
 static const int INIT_PROTO_VERSION = 209;
@@ -20,7 +20,7 @@ static const int INIT_PROTO_VERSION = 209;
 static const int MIN_PEER_PROTO_VERSION = 70216;
 
 //! minimum proto version of masternode to accept in DKGs
-static const int MIN_MASTERNODE_PROTO_VERSION = 70235;
+static const int MIN_MASTERNODE_PROTO_VERSION = 70237;
 
 //! protocol version is included in MNAUTH starting with this version
 static const int MNAUTH_NODE_VER_VERSION = 70218;
@@ -64,6 +64,9 @@ static const int INCREASE_MAX_HEADERS2_VERSION = 70235;
 //! Behavior of QRINFO is changed in this protocol version
 static const int EFFICIENT_QRINFO_VERSION = 70236;
 
+//! cycleHash in isdlock message switched to using quorum's base block in this version
+static const int ISDLOCK_CYCLEHASH_UPDATE_VERSION = 70237;
+
 // Make sure that none of the values above collide with `ADDRV2_FORMAT`.
 
 #endif // BITCOIN_VERSION_H

test/functional/test_framework/p2p.py

@@ -99,8 +99,8 @@
 # The minimum P2P version that this test framework supports
 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 = 70236
+# Version 70237 switched cycleHash in isdlock message to using quorum's base block
+P2P_VERSION = 70237
 # 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