Re-verify invalid IS sigs when the active quorum set rotated

src/llmq/quorums_instantsend.cpp

@@ -715,8 +715,6 @@ bool CInstantSendManager::PreVerifyInstantSendLock(NodeId nodeId, const llmq::CI
 
 bool CInstantSendManager::ProcessPendingInstantSendLocks()
 {
- auto llmqType = Params().GetConsensus().llmqForInstantSend;
-
  decltype(pendingInstantSendLocks) pend;
 
  {
@@ -738,6 +736,44 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks()
  tipHeight = chainActive.Height();
  }
 
+ auto llmqType = Params().GetConsensus().llmqForInstantSend;
+
+ // Every time a new quorum enters the active set, an older one is removed. This means that between two blocks, the
+ // active set can be different, leading to different selection of the signing quorum. When we detect such rotation
+ // of the active set, we must re-check invalid sigs against the previous active set and only ban nodes when this also
+ // fails.
+ auto quorums1 = quorumSigningManager->GetActiveQuorumSet(llmqType, tipHeight);
+ auto quorums2 = quorumSigningManager->GetActiveQuorumSet(llmqType, tipHeight - 1);
+ bool quorumsRotated = quorums1 != quorums2;
+
+ if (quorumsRotated) {
+ // first check against the current active set and don't ban
+ auto badISLocks = ProcessPendingInstantSendLocks(tipHeight, pend, false);
+ if (!badISLocks.empty()) {
+ LogPrintf("CInstantSendManager::%s -- detected LLMQ active set rotation, redoing verification on old active set\n", __func__);
+
+ // filter out valid IS locks from "pend"
+ for (auto it = pend.begin(); it != pend.end(); ) {
+ if (!badISLocks.count(it->first)) {
+ it = pend.erase(it);
+ } else {
+ ++it;
+ }
+ }
+ // now check against the previous active set and perform banning if this fails
+ ProcessPendingInstantSendLocks(tipHeight - 1, pend, true);
+ }
+ } else {
+ ProcessPendingInstantSendLocks(tipHeight, pend, true);
+ }
+
+ return true;
+}
+
+std::unordered_set<uint256> CInstantSendManager::ProcessPendingInstantSendLocks(int signHeight, const std::unordered_map<uint256, std::pair<NodeId, CInstantSendLock>>& pend, bool ban)
+{
+ auto llmqType = Params().GetConsensus().llmqForInstantSend;
+
  CBLSBatchVerifier<NodeId, uint256> batchVerifier(false, true, 8);
  std::unordered_map<uint256, std::pair<CQuorumCPtr, CRecoveredSig>> recSigs;
 
@@ -762,10 +798,10 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks()
  continue;
  }
 
- auto quorum = quorumSigningManager->SelectQuorumForSigning(llmqType, tipHeight, id);
+ auto quorum = quorumSigningManager->SelectQuorumForSigning(llmqType, signHeight, id);
  if (!quorum) {
  // should not happen, but if one fails to select, all others will also fail to select
- return false;
+ return {};
  }
  uint256 signHash = CLLMQUtils::BuildSignHash(llmqType, quorum->qc.quorumHash, id, islock.txid);
  batchVerifier.PushMessage(nodeId, hash, signHash, islock.sig.Get(), quorum->qc.quorumPublicKey);
@@ -788,7 +824,9 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks()
 
  batchVerifier.Verify();
 
- if (!batchVerifier.badSources.empty()) {
+ std::unordered_set<uint256> badISLocks;
+
+ if (ban && !batchVerifier.badSources.empty()) {
  LOCK(cs_main);
  for (auto& nodeId : batchVerifier.badSources) {
  // Let's not be too harsh, as the peer might simply be unlucky and might have sent us an old lock which
@@ -804,6 +842,7 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks()
  if (batchVerifier.badMessages.count(hash)) {
  LogPrintf("CInstantSendManager::%s -- txid=%s, islock=%s: invalid sig in islock, peer=%d\n", __func__,
  islock.txid.ToString(), hash.ToString(), nodeId);
+ badISLocks.emplace(hash);
  continue;
  }
 
@@ -824,7 +863,7 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks()
  }
  }
 
- return true;
+ return badISLocks;
 }
 
 void CInstantSendManager::ProcessInstantSendLock(NodeId from, const uint256& hash, const CInstantSendLock& islock)

src/llmq/quorums_instantsend.h

@@ -138,6 +138,7 @@ class CInstantSendManager : public CRecoveredSigsListener
  void ProcessMessageInstantSendLock(CNode* pfrom, const CInstantSendLock& islock, CConnman& connman);
  bool PreVerifyInstantSendLock(NodeId nodeId, const CInstantSendLock& islock, bool& retBan);
  bool ProcessPendingInstantSendLocks();
+ std::unordered_set<uint256> ProcessPendingInstantSendLocks(int signHeight, const std::unordered_map<uint256, std::pair<NodeId, CInstantSendLock>>& pend, bool ban);
  void ProcessInstantSendLock(NodeId from, const uint256& hash, const CInstantSendLock& islock);
  void UpdateWalletTransaction(const CTransactionRef& tx, const CInstantSendLock& islock);
 

src/llmq/quorums_signing.cpp

@@ -815,7 +815,7 @@ bool CSigningManager::GetVoteForId(Consensus::LLMQType llmqType, const uint256&
  return db.GetVoteForId(llmqType, id, msgHashRet);
 }
 
-CQuorumCPtr CSigningManager::SelectQuorumForSigning(Consensus::LLMQType llmqType, int signHeight, const uint256& selectionHash)
+std::vector<CQuorumCPtr> CSigningManager::GetActiveQuorumSet(Consensus::LLMQType llmqType, int signHeight)
 {
  auto& llmqParams = Params().GetConsensus().llmqs.at(llmqType);
  size_t poolSize = (size_t)llmqParams.signingActiveQuorumCount;
@@ -825,12 +825,17 @@ CQuorumCPtr CSigningManager::SelectQuorumForSigning(Consensus::LLMQType llmqType
  LOCK(cs_main);
  int startBlockHeight = signHeight - SIGN_HEIGHT_OFFSET;
  if (startBlockHeight > chainActive.Height()) {
- return nullptr;
+ return {};
  }
  pindexStart = chainActive[startBlockHeight];
  }
 
- auto quorums = quorumManager->ScanQuorums(llmqType, pindexStart, poolSize);
+ return quorumManager->ScanQuorums(llmqType, pindexStart, poolSize);
+}
+
+CQuorumCPtr CSigningManager::SelectQuorumForSigning(Consensus::LLMQType llmqType, int signHeight, const uint256& selectionHash)
+{
+ auto quorums = GetActiveQuorumSet(llmqType, signHeight);
  if (quorums.empty()) {
  return nullptr;
  }

src/llmq/quorums_signing.h

@@ -171,6 +171,7 @@ class CSigningManager
  bool HasVotedOnId(Consensus::LLMQType llmqType, const uint256& id);
  bool GetVoteForId(Consensus::LLMQType llmqType, const uint256& id, uint256& msgHashRet);
 
+ std::vector<CQuorumCPtr> GetActiveQuorumSet(Consensus::LLMQType llmqType, int signHeight);
  CQuorumCPtr SelectQuorumForSigning(Consensus::LLMQType llmqType, int signHeight, const uint256& selectionHash);
 
  // Verifies a recovered sig that was signed while the chain tip was at signedAtTip