llmq|rpc|test: Fix previous active set checks in RPC commands (#4048)

* llmq: Introduce signOffset in CSigningManager::VerifyRecoveredSig

* rpc|test: Fix `signHeight + dkgInterval` check in verifyislock

* rpc|test: Fix `signHeight + dkgInterval` check in `quorum verify`

* test: Test `signHeight + dkgInterval` checks

In `quorum verify` and `verifyislock` RPC commands

src/llmq/quorums_signing.cpp

@@ -930,9 +930,9 @@ CQuorumCPtr CSigningManager::SelectQuorumForSigning(Consensus::LLMQType llmqType
     return quorums[scores.front().second];
 }
 
-bool CSigningManager::VerifyRecoveredSig(Consensus::LLMQType llmqType, int signedAtHeight, const uint256& id, const uint256& msgHash, const CBLSSignature& sig)
+bool CSigningManager::VerifyRecoveredSig(Consensus::LLMQType llmqType, int signedAtHeight, const uint256& id, const uint256& msgHash, const CBLSSignature& sig, const int signOffset)
 {
-    auto quorum = SelectQuorumForSigning(llmqType, id, signedAtHeight);
+    auto quorum = SelectQuorumForSigning(llmqType, id, signedAtHeight, signOffset);
     if (!quorum) {
         return false;
     }

src/llmq/quorums_signing.h

@@ -186,7 +186,7 @@ public:
     static CQuorumCPtr SelectQuorumForSigning(Consensus::LLMQType llmqType, const uint256& selectionHash, int signHeight = -1 /*chain tip*/, int signOffset = SIGN_HEIGHT_OFFSET);
 
     // Verifies a recovered sig that was signed while the chain tip was at signedAtTip
-    static bool VerifyRecoveredSig(Consensus::LLMQType llmqType, int signedAtHeight, const uint256& id, const uint256& msgHash, const CBLSSignature& sig);
+    static bool VerifyRecoveredSig(Consensus::LLMQType llmqType, int signedAtHeight, const uint256& id, const uint256& msgHash, const CBLSSignature& sig, int signOffset = SIGN_HEIGHT_OFFSET);
 };
 
 extern CSigningManager* quorumSigningManager;

src/rpc/rpcquorums.cpp

@@ -432,30 +432,26 @@ UniValue quorum_sigs_cmd(const JSONRPCRequest& request)
             throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid signature format");
         }
 
-        llmq::CQuorumCPtr quorum{nullptr};
         if (request.params[5].isNull() || (request.params[5].get_str().empty() && !request.params[6].isNull())) {
             int signHeight{-1};
             if (!request.params[6].isNull()) {
                 signHeight = ParseInt32V(request.params[6], "signHeight");
             }
-            // First check against the current active set
-            quorum = llmq::quorumSigningManager->SelectQuorumForSigning(llmqType, id, signHeight, 0);
-            if (!quorum) {
-                // Then check against the previous active set in case it changed recently
-                int signOffset = Params().GetConsensus().llmqs.at(llmqType).dkgInterval;
-                quorum = llmq::quorumSigningManager->SelectQuorumForSigning(llmqType, id, signHeight, signOffset);
-            }
+            // First check against the current active set, if it fails check against the last active set
+            int signOffset{Params().GetConsensus().llmqs.at(llmqType).dkgInterval};
+            return llmq::quorumSigningManager->VerifyRecoveredSig(llmqType, signHeight, id, msgHash, sig, 0) ||
+                   llmq::quorumSigningManager->VerifyRecoveredSig(llmqType, signHeight, id, msgHash, sig, signOffset);
         } else {
             uint256 quorumHash = ParseHashV(request.params[5], "quorumHash");
-            quorum = llmq::quorumManager->GetQuorum(llmqType, quorumHash);
-        }
+            llmq::CQuorumCPtr quorum = llmq::quorumManager->GetQuorum(llmqType, quorumHash);
 
-        if (!quorum) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "quorum not found");
-        }
+            if (!quorum) {
+                throw JSONRPCError(RPC_INVALID_PARAMETER, "quorum not found");
+            }
 
-        uint256 signHash = llmq::CLLMQUtils::BuildSignHash(llmqType, quorum->qc.quorumHash, id, msgHash);
-        return sig.VerifyInsecure(quorum->qc.quorumPublicKey, signHash);
+            uint256 signHash = llmq::CLLMQUtils::BuildSignHash(llmqType, quorum->qc.quorumHash, id, msgHash);
+            return sig.VerifyInsecure(quorum->qc.quorumPublicKey, signHash);
+        }
     } else if (cmd == "hasrecsig") {
         return llmq::quorumSigningManager->HasRecoveredSig(llmqType, id, msgHash);
     } else if (cmd == "getrecsig") {
@@ -710,21 +706,12 @@ UniValue verifyislock(const JSONRPCRequest& request)
         signHeight = pindexMined->nHeight;
     }
 
-    // First check against the current active set
     auto llmqType = Params().GetConsensus().llmqTypeInstantSend;
-    auto quorum = llmq::quorumSigningManager->SelectQuorumForSigning(llmqType, id, signHeight, 0);
-    if (!quorum) {
-        // Then check against the previous active set in case it changed recently
-        int signOffset = Params().GetConsensus().llmqs.at(llmqType).dkgInterval;
-        quorum = llmq::quorumSigningManager->SelectQuorumForSigning(llmqType, id, signHeight, signOffset);
-        if (!quorum) {
-            // None of the above
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "quorum not found");
-        }
-    }
 
-    uint256 signHash = llmq::CLLMQUtils::BuildSignHash(llmqType, quorum->qc.quorumHash, id, txid);
-    return sig.VerifyInsecure(quorum->qc.quorumPublicKey, signHash);
+    // First check against the current active set, if it fails check against the last active set
+    int signOffset{Params().GetConsensus().llmqs.at(llmqType).dkgInterval};
+    return llmq::quorumSigningManager->VerifyRecoveredSig(llmqType, signHeight, id, txid, sig, 0) ||
+           llmq::quorumSigningManager->VerifyRecoveredSig(llmqType, signHeight, id, txid, sig, signOffset);
 }
 
 static const CRPCCommand commands[] =

test/functional/feature_llmq_signing.py

@@ -119,18 +119,39 @@ class LLMQSigningTest(DashTestFramework):
         assert(node.quorum("verify", 100, id, msgHash, recsig["sig"]))
         assert(node.quorum("verify", 100, id, msgHash, recsig["sig"], "", height))
         assert(not node.quorum("verify", 100, id, msgHashConflict, recsig["sig"]))
-        assert_raises_rpc_error(-8, "quorum not found", node.quorum, "verify", 100, id, msgHash, recsig["sig"], "", height_bad)
+        assert not node.quorum("verify", 100, id, msgHash, recsig["sig"], "", height_bad)
         # Use specifc quorum
         assert(node.quorum("verify", 100, id, msgHash, recsig["sig"], recsig["quorumHash"]))
         assert(not node.quorum("verify", 100, id, msgHashConflict, recsig["sig"], recsig["quorumHash"]))
         assert_raises_rpc_error(-8, "quorum not found", node.quorum, "verify", 100, id, msgHash, recsig["sig"], hash_bad)
 
-        recsig_time = self.mocktime
-
         # Mine one more quorum, so that we have 2 active ones, nothing should change
         self.mine_quorum()
         assert_sigs_nochange(True, False, True, 3)
 
+        # Create a recovered sig for the oldest quorum i.e. the active quorum which will be moved
+        # out of the active set when a new quorum appears
+        request_id = 2
+        oldest_quorum_hash = node.quorum("list")["llmq_test"][-1]
+        # Search for a request id which selects the last active quorum
+        while True:
+            selected_hash = node.quorum('selectquorum', 100, uint256_to_string(request_id))["quorumHash"]
+            if selected_hash == oldest_quorum_hash:
+                break
+            else:
+                request_id += 1
+        # Produce the recovered signature
+        id = uint256_to_string(request_id)
+        for mn in self.mninfo:
+            mn.node.quorum("sign", 100, id, msgHash)
+        # And mine a quorum to move the quorum which signed out of the active set
+        self.mine_quorum()
+        # Verify the recovered sig. This triggers the "signHeight + dkgInterval" verification
+        recsig = node.quorum("getrecsig", 100, id, msgHash)
+        assert node.quorum("verify", 100, id, msgHash, recsig["sig"], "", node.getblockcount())
+
+        recsig_time = self.mocktime
+
         # Mine 2 more quorums, so that the one used for the the recovered sig should become inactive, nothing should change
         self.mine_quorum()
         self.mine_quorum()
@@ -152,7 +173,7 @@ class LLMQSigningTest(DashTestFramework):
         wait_for_sigs(True, False, True, 15)
 
         if self.options.spork21:
-            id = "0000000000000000000000000000000000000000000000000000000000000002"
+            id = uint256_to_string(request_id + 1)
 
             # Isolate the node that is responsible for the recovery of a signature and assert that recovery fails
             q = self.nodes[0].quorum('selectquorum', 100, id)

test/functional/rpc_verifyislock.py

@@ -5,7 +5,7 @@
 
 from test_framework.messages import CTransaction, FromHex, hash256, ser_compact_size, ser_string
 from test_framework.test_framework import DashTestFramework
-from test_framework.util import assert_raises_rpc_error, wait_until
+from test_framework.util import bytes_to_hex_str, satoshi_round, wait_until
 
 '''
 rpc_verifyislock.py
@@ -20,6 +20,14 @@ class RPCVerifyISLockTest(DashTestFramework):
         self.set_dash_test_params(6, 5, [["-whitelist=127.0.0.1"], [], [], [], [], []], fast_dip3_enforcement=True)
         self.set_dash_llmq_test_params(5, 3)
 
+    def get_request_id(self, tx_hex):
+        tx = FromHex(CTransaction(), tx_hex)
+
+        request_id_buf = ser_string(b"islock") + ser_compact_size(len(tx.vin))
+        for txin in tx.vin:
+            request_id_buf += txin.prevout.serialize()
+        return hash256(request_id_buf)[::-1].hex()
+
     def run_test(self):
 
         node = self.nodes[0]
@@ -31,26 +39,55 @@ class RPCVerifyISLockTest(DashTestFramework):
         txid = node.sendtoaddress(node.getnewaddress(), 1)
         self.wait_for_instantlock(txid, node)
 
-        tx = FromHex(CTransaction(), node.getrawtransaction(txid))
-
-        request_id_buf = ser_string(b"islock") + ser_compact_size(len(tx.vin))
-        for txin in tx.vin:
-            request_id_buf += txin.prevout.serialize()
-        request_id = hash256(request_id_buf)[::-1].hex()
-
+        request_id = self.get_request_id(self.nodes[0].getrawtransaction(txid))
         wait_until(lambda: node.quorum("hasrecsig", 100, request_id, txid))
 
         rec_sig = node.quorum("getrecsig", 100, request_id, txid)['sig']
         assert(node.verifyislock(request_id, txid, rec_sig))
         # Not mined, should use maxHeight
-        assert_raises_rpc_error(-8, "quorum not found", node.verifyislock, request_id, txid, rec_sig, 1)
+        assert not node.verifyislock(request_id, txid, rec_sig, 1)
         node.generate(1)
         assert(txid not in node.getrawmempool())
         # Mined but at higher height, should use maxHeight
-        assert_raises_rpc_error(-8, "quorum not found", node.verifyislock, request_id, txid, rec_sig, 1)
+        assert not node.verifyislock(request_id, txid, rec_sig, 1)
         # Mined, should ignore higher maxHeight
         assert(node.verifyislock(request_id, txid, rec_sig, node.getblockcount() + 100))
 
+        # Mine one more quorum to have a full active set
+        self.mine_quorum()
+        # Create an ISLOCK for the oldest quorum i.e. the active quorum which will be moved
+        # out of the active set when a new quorum appears
+        selected_hash = None
+        request_id = None
+        oldest_quorum_hash = node.quorum("list")["llmq_test"][-1]
+        utxos = node.listunspent()
+        fee = 0.001
+        amount = 1
+        # Try all available utxo's until we have one resulting in a request id which selects the
+        # last active quorum
+        for utxo in utxos:
+            in_amount = float(utxo['amount'])
+            if in_amount < amount + fee:
+                continue
+            outputs = dict()
+            outputs[node.getnewaddress()] = satoshi_round(amount)
+            change = in_amount - amount - fee
+            if change > 0:
+                outputs[node.getnewaddress()] = satoshi_round(change)
+            rawtx = node.createrawtransaction([utxo], outputs)
+            rawtx = node.signrawtransactionwithwallet(rawtx)["hex"]
+            request_id = self.get_request_id(rawtx)
+            selected_hash = node.quorum('selectquorum', 100, request_id)["quorumHash"]
+            if selected_hash == oldest_quorum_hash:
+                break
+        assert selected_hash == oldest_quorum_hash
+        # Create the ISLOCK, then mine a quorum to move the signing quorum out of the active set
+        islock = self.create_islock(rawtx)
+        self.mine_quorum()
+        # Send the tx and verify the ISLOCK. This triggers the "signHeight + dkgInterval" verification
+        rawtx_txid = node.sendrawtransaction(rawtx)
+        assert node.verifyislock(request_id, rawtx_txid, bytes_to_hex_str(islock.sig), node.getblockcount())
+
 
 if __name__ == '__main__':
     RPCVerifyISLockTest().main()