Merge #6096: feat: split type of error in submitchainlock - return enum in CL verifying code

0133c9866d feat: add functional test for submitchainlock far ahead in future (Konstantin Akimov)
6004e06769 feat: return enum in RecoveredSig verifying code, apply for RPC submitchainlock (Konstantin Akimov)
130b6d1e96 refactor: replace static private member method to static method (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  Currently by result of `submitchainlock` impossible to distinct a situation when a signature is invalid and when a core is far behind and just doesn't know about signing quorum yet.

  This PR aims to fix this issue, as requested by shumkov for needs of platform:

  > mailformed signature and can’t verify signature due to unknown quorum is the same error?
  > possible to distingush ?

  ## What was done?
  Return enum in CL verifying code `chainlock_handler.VerifyChainLock`.
  The RPC `submitchainlock` now returns error with code=-1 and message `no quorum found. Current tip height: {N} hash: {HASH}`

  ## How Has This Been Tested?
  Functional test `feature_llmq_chainlocks.py` is updated

  ## Breaking Changes
  `submitchainlock` return one more error code - not really a breaking change though, because v21 hasn't released yet.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] 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

ACKs for top commit:
  UdjinM6:
    utACK 0133c9866d
  PastaPastaPasta:
    utACK 0133c9866d

Tree-SHA512: 794ba410efa57aaa66c47a67914deed97c1d060326e5d11a722c9233a8447f5e9215aa4a5ca401cb2199b8fc445144b2b2a692fc35494bf3296a74e9e115bda7

src/evo/cbtx.cpp

@@ -374,7 +374,7 @@ bool CheckCbTxBestChainlock(const CBlock& block, const CBlockIndex* pindex,
             return true;
         }
         uint256 curBlockCoinbaseCLBlockHash = pindex->GetAncestor(curBlockCoinbaseCLHeight)->GetBlockHash();
-        if (!chainlock_handler.VerifyChainLock(llmq::CChainLockSig(curBlockCoinbaseCLHeight, curBlockCoinbaseCLBlockHash, opt_cbTx->bestCLSignature))) {
+        if (chainlock_handler.VerifyChainLock(llmq::CChainLockSig(curBlockCoinbaseCLHeight, curBlockCoinbaseCLBlockHash, opt_cbTx->bestCLSignature)) != llmq::VerifyRecSigStatus::Valid) {
             return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-invalid-clsig");
         }
     } else if (opt_cbTx->bestCLHeightDiff != 0) {

src/llmq/chainlocks.cpp

@@ -19,6 +19,7 @@
 #include <txmempool.h>
 #include <util/thread.h>
 #include <util/time.h>
+#include <util/underlying.h>
 #include <validation.h>
 #include <validationinterface.h>
 
@@ -130,8 +131,8 @@ PeerMsgRet CChainLocksHandler::ProcessNewChainLock(const NodeId from, const llmq
         }
     }
 
-    if (!VerifyChainLock(clsig)) {
+    if (const auto ret = VerifyChainLock(clsig); ret != VerifyRecSigStatus::Valid) {
-        LogPrint(BCLog::CHAINLOCKS, "CChainLocksHandler::%s -- invalid CLSIG (%s), peer=%d\n", __func__, clsig.ToString(), from);
+        LogPrint(BCLog::CHAINLOCKS, "CChainLocksHandler::%s -- invalid CLSIG (%s), status=%d peer=%d\n", __func__, clsig.ToString(), ToUnderlying(ret), from);
         if (from != -1) {
             return tl::unexpected{10};
         }
@@ -551,10 +552,12 @@ bool CChainLocksHandler::HasChainLock(int nHeight, const uint256& blockHash) con
     return InternalHasChainLock(nHeight, blockHash);
 }
 
-bool CChainLocksHandler::VerifyChainLock(const CChainLockSig& clsig) const
+
+VerifyRecSigStatus CChainLocksHandler::VerifyChainLock(const CChainLockSig& clsig) const
 {
     const auto llmqType = Params().GetConsensus().llmqTypeChainLocks;
     const uint256 nRequestId = ::SerializeHash(std::make_pair(llmq::CLSIG_REQUESTID_PREFIX, clsig.getHeight()));
+
     return llmq::VerifyRecoveredSig(llmqType, m_chainstate.m_chain, qman, clsig.getHeight(), nRequestId, clsig.getBlockHash(), clsig.getSig());
 }
 

src/llmq/chainlocks.h

@@ -9,6 +9,7 @@
 
 #include <crypto/common.h>
 #include <llmq/signing.h>
+#include <llmq/quorums.h>
 #include <net.h>
 #include <net_types.h>
 #include <primitives/block.h>
@@ -114,7 +115,7 @@ public:
 
     bool HasChainLock(int nHeight, const uint256& blockHash) const EXCLUSIVE_LOCKS_REQUIRED(!cs);
     bool HasConflictingChainLock(int nHeight, const uint256& blockHash) const EXCLUSIVE_LOCKS_REQUIRED(!cs);
-    bool VerifyChainLock(const CChainLockSig& clsig) const;
+    VerifyRecSigStatus VerifyChainLock(const CChainLockSig& clsig) const;
 
     bool IsTxSafeForMining(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(!cs);
 

src/llmq/quorums.cpp

@@ -1169,7 +1169,7 @@ CQuorumCPtr SelectQuorumForSigning(const Consensus::LLMQParams& llmq_params, con
     }
 }
 
-bool VerifyRecoveredSig(Consensus::LLMQType llmqType, const CChain& active_chain, const CQuorumManager& qman,
+VerifyRecSigStatus VerifyRecoveredSig(Consensus::LLMQType llmqType, const CChain& active_chain, const CQuorumManager& qman,
                         int signedAtHeight, const uint256& id, const uint256& msgHash, const CBLSSignature& sig,
                         const int signOffset)
 {
@@ -1177,10 +1177,11 @@ bool VerifyRecoveredSig(Consensus::LLMQType llmqType, const CChain& active_chain
     assert(llmq_params_opt.has_value());
     auto quorum = SelectQuorumForSigning(llmq_params_opt.value(), active_chain, qman, id, signedAtHeight, signOffset);
     if (!quorum) {
-        return false;
+        return VerifyRecSigStatus::NoQuorum;
     }
 
     uint256 signHash = BuildSignHash(llmqType, quorum->qc->quorumHash, id, msgHash);
-    return sig.VerifyInsecure(quorum->qc->quorumPublicKey, signHash);
+    const bool ret = sig.VerifyInsecure(quorum->qc->quorumPublicKey, signHash);
+    return ret ? VerifyRecSigStatus::Valid : VerifyRecSigStatus::Invalid;
 }
 } // namespace llmq

src/llmq/quorums.h

@@ -36,6 +36,13 @@ using CDeterministicMNCPtr = std::shared_ptr<const CDeterministicMN>;
 
 namespace llmq
 {
+enum class VerifyRecSigStatus
+{
+    NoQuorum,
+    Invalid,
+    Valid,
+};
+
 class CDKGSessionManager;
 class CQuorumBlockProcessor;
 
@@ -298,9 +305,9 @@ CQuorumCPtr SelectQuorumForSigning(const Consensus::LLMQParams& llmq_params, con
                                    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
-bool VerifyRecoveredSig(Consensus::LLMQType llmqType, const CChain& active_chain, const CQuorumManager& qman,
+VerifyRecSigStatus VerifyRecoveredSig(Consensus::LLMQType llmqType, const CChain& active_chain, const CQuorumManager& qman,
-                        int signedAtHeight, const uint256& id, const uint256& msgHash, const CBLSSignature& sig,
+                                      int signedAtHeight, const uint256& id, const uint256& msgHash, const CBLSSignature& sig,
-                        int signOffset = SIGN_HEIGHT_OFFSET);
+                                      int signOffset = SIGN_HEIGHT_OFFSET);
 } // namespace llmq
 
 template<typename T> struct SaltedHasherImpl;

src/llmq/signing.cpp

@@ -578,6 +578,30 @@ PeerMsgRet CSigningManager::ProcessMessage(const CNode& pfrom, const std::string
     return {};
 }
 
+static bool PreVerifyRecoveredSig(const CQuorumManager& quorum_manager, const CRecoveredSig& recoveredSig, bool& retBan)
+{
+    retBan = false;
+
+    auto llmqType = recoveredSig.getLlmqType();
+    if (!Params().GetLLMQ(llmqType).has_value()) {
+        retBan = true;
+        return false;
+    }
+
+    CQuorumCPtr quorum = quorum_manager.GetQuorum(llmqType, recoveredSig.getQuorumHash());
+
+    if (!quorum) {
+        LogPrint(BCLog::LLMQ, "CSigningManager::%s -- quorum %s not found\n", __func__,
+                  recoveredSig.getQuorumHash().ToString());
+        return false;
+    }
+    if (!IsQuorumActive(llmqType, quorum_manager, quorum->qc->quorumHash)) {
+        return false;
+    }
+
+    return true;
+}
+
 PeerMsgRet CSigningManager::ProcessMessageRecoveredSig(const CNode& pfrom, const std::shared_ptr<const CRecoveredSig>& recoveredSig)
 {
     {
@@ -614,30 +638,6 @@ PeerMsgRet CSigningManager::ProcessMessageRecoveredSig(const CNode& pfrom, const
     return {};
 }
 
-bool CSigningManager::PreVerifyRecoveredSig(const CQuorumManager& quorum_manager, const CRecoveredSig& recoveredSig, bool& retBan)
-{
-    retBan = false;
-
-    auto llmqType = recoveredSig.getLlmqType();
-    if (!Params().GetLLMQ(llmqType).has_value()) {
-        retBan = true;
-        return false;
-    }
-
-    CQuorumCPtr quorum = quorum_manager.GetQuorum(llmqType, recoveredSig.getQuorumHash());
-
-    if (!quorum) {
-        LogPrint(BCLog::LLMQ, "CSigningManager::%s -- quorum %s not found\n", __func__,
-                  recoveredSig.getQuorumHash().ToString());
-        return false;
-    }
-    if (!IsQuorumActive(llmqType, quorum_manager, quorum->qc->quorumHash)) {
-        return false;
-    }
-
-    return true;
-}
-
 void CSigningManager::CollectPendingRecoveredSigsToVerify(
         size_t maxUniqueSessions,
         std::unordered_map<NodeId, std::list<std::shared_ptr<const CRecoveredSig>>>& retSigShares,

src/llmq/signing.h

@@ -201,7 +201,6 @@ public:
 
 private:
     PeerMsgRet ProcessMessageRecoveredSig(const CNode& pfrom, const std::shared_ptr<const CRecoveredSig>& recoveredSig);
-    static bool PreVerifyRecoveredSig(const CQuorumManager& quorum_manager, const CRecoveredSig& recoveredSig, bool& retBan);
 
     void CollectPendingRecoveredSigsToVerify(size_t maxUniqueSessions,
             std::unordered_map<NodeId, std::list<std::shared_ptr<const CRecoveredSig>>>& retSigShares,

src/rpc/quorums.cpp

@@ -500,6 +500,18 @@ static RPCHelpMan quorum_sign()
     };
 }
 
+static bool VerifyRecoveredSigLatestQuorums(const Consensus::LLMQParams& llmq_params, const CChain& active_chain, const llmq::CQuorumManager& qman,
+                                            int signHeight, const uint256& id, const uint256& msgHash, const CBLSSignature& sig)
+{
+    // First check against the current active set, if it fails check against the last active set
+    for (int signOffset : {0, llmq_params.dkgInterval}) {
+        if (llmq::VerifyRecoveredSig(llmq_params.type, active_chain, qman, signHeight, id, msgHash, sig, signOffset) == llmq::VerifyRecSigStatus::Valid) {
+            return true;
+        }
+    }
+    return false;
+}
+
 static RPCHelpMan quorum_verify()
 {
     return RPCHelpMan{"quorum verify",
@@ -545,10 +557,7 @@ static RPCHelpMan quorum_verify()
         if (!request.params[5].isNull()) {
             signHeight = ParseInt32V(request.params[5], "signHeight");
         }
-        // First check against the current active set, if it fails check against the last active set
+        return VerifyRecoveredSigLatestQuorums(*llmq_params_opt, chainman.ActiveChain(), *llmq_ctx.qman, signHeight, id, msgHash, sig);
-        int signOffset{llmq_params_opt->dkgInterval};
-        return llmq::VerifyRecoveredSig(llmqType, chainman.ActiveChain(), *llmq_ctx.qman, signHeight, id, msgHash, sig, 0) ||
-               llmq::VerifyRecoveredSig(llmqType, chainman.ActiveChain(), *llmq_ctx.qman, signHeight, id, msgHash, sig, signOffset);
     }
 
     uint256 quorumHash(ParseHashV(request.params[4], "quorumHash"));
@@ -958,7 +967,7 @@ static RPCHelpMan verifychainlock()
     }
 
     const LLMQContext& llmq_ctx = EnsureLLMQContext(node);
-    return llmq_ctx.clhandler->VerifyChainLock(llmq::CChainLockSig(nBlockHeight, nBlockHash, sig));
+    return llmq_ctx.clhandler->VerifyChainLock(llmq::CChainLockSig(nBlockHeight, nBlockHash, sig)) == llmq::VerifyRecSigStatus::Valid;
 },
     };
 }
@@ -1030,12 +1039,9 @@ static RPCHelpMan verifyislock()
     const LLMQContext& llmq_ctx = EnsureLLMQContext(node);
 
     auto llmqType = Params().GetConsensus().llmqTypeDIP0024InstantSend;
-    // First check against the current active set, if it fails check against the last active set
     const auto llmq_params_opt = Params().GetLLMQ(llmqType);
     CHECK_NONFATAL(llmq_params_opt.has_value());
-    const int signOffset{llmq_params_opt->dkgInterval};
+    return VerifyRecoveredSigLatestQuorums(*llmq_params_opt, chainman.ActiveChain(), *llmq_ctx.qman, signHeight, id, txid, sig);
-    return llmq::VerifyRecoveredSig(llmqType, chainman.ActiveChain(), *llmq_ctx.qman, signHeight, id, txid, sig, 0) ||
-           llmq::VerifyRecoveredSig(llmqType, chainman.ActiveChain(), *llmq_ctx.qman, signHeight, id, txid, sig, signOffset);
 },
     };
 }
@@ -1060,7 +1066,8 @@ static RPCHelpMan submitchainlock()
     if (nBlockHeight <= 0) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid block height");
     }
-    const LLMQContext& llmq_ctx = EnsureLLMQContext(EnsureAnyNodeContext(request.context));
+    const NodeContext& node = EnsureAnyNodeContext(request.context);
+    const LLMQContext& llmq_ctx = EnsureLLMQContext(node);
     const int32_t bestCLHeight = llmq_ctx.clhandler->GetBestChainLock().getHeight();
     if (nBlockHeight <= bestCLHeight) return bestCLHeight;
 
@@ -1070,8 +1077,15 @@ static RPCHelpMan submitchainlock()
     }
 
 
-    auto clsig = llmq::CChainLockSig(nBlockHeight, nBlockHash, sig);
+    const auto clsig{llmq::CChainLockSig(nBlockHeight, nBlockHash, sig)};
-    if (!llmq_ctx.clhandler->VerifyChainLock(clsig)) {
+    const llmq::VerifyRecSigStatus ret{llmq_ctx.clhandler->VerifyChainLock(clsig)};
+    if (ret == llmq::VerifyRecSigStatus::NoQuorum) {
+        LOCK(cs_main);
+        const ChainstateManager& chainman = EnsureChainman(node);
+        const CBlockIndex* pIndex{chainman.ActiveChain().Tip()};
+        throw JSONRPCError(RPC_MISC_ERROR, strprintf("No quorum found. Current tip height: %d hash: %s\n", pIndex->nHeight, pIndex->GetBlockHash().ToString()));
+    }
+    if (ret != llmq::VerifyRecSigStatus::Valid) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid signature");
     }
 

test/functional/feature_llmq_chainlocks.py

@@ -84,11 +84,14 @@ class LLMQChainLocksTest(DashTestFramework):
             block = self.nodes[0].getblock(self.nodes[0].getblockhash(h))
             assert block['chainlock']
 
-        # Update spork to SPORK_19_CHAINLOCKS_ENABLED and test its behaviour
+        self.log.info(f"Test submitchainlock for too high block")
+        assert_raises_rpc_error(-1, f"No quorum found. Current tip height: {self.nodes[1].getblockcount()}", self.nodes[1].submitchainlock, '0000000000000000000000000000000000000000000000000000000000000000', 'a5c69505b5744524c9ed6551d8a57dc520728ea013496f46baa8a73df96bfd3c86e474396d747a4af11aaef10b17dbe80498b6a2fe81938fe917a3fedf651361bfe5367c800d23d3125820e6ee5b42189f0043be94ce27e73ea13620c9ef6064', self.nodes[1].getblockcount() + 300)
+
+        self.log.info("Update spork to SPORK_19_CHAINLOCKS_ENABLED and test its behaviour")
         self.nodes[0].sporkupdate("SPORK_19_CHAINLOCKS_ENABLED", 1)
         self.wait_for_sporks_same()
 
-        # Generate new blocks and verify that they are not chainlocked
+        self.log.info("Generate new blocks and verify that they are not chainlocked")
         previous_block_hash = self.nodes[0].getbestblockhash()
         for _ in range(2):
             block_hash = self.nodes[0].generate(1)[0]