From 9998ffd92bbd37d436bd2ec130d14f18902fedc9 Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 9 Jul 2024 08:47:44 -0500 Subject: [PATCH] Merge #6096: feat: split type of error in submitchainlock - return enum in CL verifying code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 0133c9866dafa63c1eeda07c76243192f1cc9393 feat: add functional test for submitchainlock far ahead in future (Konstantin Akimov) 6004e067693edc1118ae5748d67eedf5ed914dd0 feat: return enum in RecoveredSig verifying code, apply for RPC submitchainlock (Konstantin Akimov) 130b6d1e969976fe0a08f409d4706fab0de39876 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 0133c9866dafa63c1eeda07c76243192f1cc9393 PastaPastaPasta: utACK 0133c9866dafa63c1eeda07c76243192f1cc9393 Tree-SHA512: 794ba410efa57aaa66c47a67914deed97c1d060326e5d11a722c9233a8447f5e9215aa4a5ca401cb2199b8fc445144b2b2a692fc35494bf3296a74e9e115bda7 --- src/evo/cbtx.cpp | 2 +- src/llmq/chainlocks.cpp | 9 ++-- src/llmq/chainlocks.h | 3 +- src/llmq/quorums.cpp | 7 ++-- src/llmq/quorums.h | 13 ++++-- src/llmq/signing.cpp | 48 +++++++++++----------- src/llmq/signing.h | 1 - src/rpc/quorums.cpp | 38 +++++++++++------ test/functional/feature_llmq_chainlocks.py | 7 +++- 9 files changed, 78 insertions(+), 50 deletions(-) diff --git a/src/evo/cbtx.cpp b/src/evo/cbtx.cpp index 6f4c171b21..29a9999965 100644 --- a/src/evo/cbtx.cpp +++ b/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) { diff --git a/src/llmq/chainlocks.cpp b/src/llmq/chainlocks.cpp index eaea589290..39130b5a07 100644 --- a/src/llmq/chainlocks.cpp +++ b/src/llmq/chainlocks.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -130,8 +131,8 @@ PeerMsgRet CChainLocksHandler::ProcessNewChainLock(const NodeId from, const llmq } } - if (!VerifyChainLock(clsig)) { - LogPrint(BCLog::CHAINLOCKS, "CChainLocksHandler::%s -- invalid CLSIG (%s), peer=%d\n", __func__, clsig.ToString(), from); + if (const auto ret = VerifyChainLock(clsig); ret != VerifyRecSigStatus::Valid) { + 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()); } diff --git a/src/llmq/chainlocks.h b/src/llmq/chainlocks.h index a21cc342e7..f229f5b608 100644 --- a/src/llmq/chainlocks.h +++ b/src/llmq/chainlocks.h @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -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); diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index d2607f74fe..3f729aa1e4 100644 --- a/src/llmq/quorums.cpp +++ b/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 diff --git a/src/llmq/quorums.h b/src/llmq/quorums.h index dcf0bcf906..91b5eceb55 100644 --- a/src/llmq/quorums.h +++ b/src/llmq/quorums.h @@ -36,6 +36,13 @@ using CDeterministicMNCPtr = std::shared_ptr; 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, - int signedAtHeight, const uint256& id, const uint256& msgHash, const CBLSSignature& sig, - int signOffset = SIGN_HEIGHT_OFFSET); +VerifyRecSigStatus VerifyRecoveredSig(Consensus::LLMQType llmqType, const CChain& active_chain, const CQuorumManager& qman, + int signedAtHeight, const uint256& id, const uint256& msgHash, const CBLSSignature& sig, + int signOffset = SIGN_HEIGHT_OFFSET); } // namespace llmq template struct SaltedHasherImpl; diff --git a/src/llmq/signing.cpp b/src/llmq/signing.cpp index ff00bcd650..e9cc76e350 100644 --- a/src/llmq/signing.cpp +++ b/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& 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>>& retSigShares, diff --git a/src/llmq/signing.h b/src/llmq/signing.h index b76bd20508..16d24ef5c0 100644 --- a/src/llmq/signing.h +++ b/src/llmq/signing.h @@ -201,7 +201,6 @@ public: private: PeerMsgRet ProcessMessageRecoveredSig(const CNode& pfrom, const std::shared_ptr& recoveredSig); - static bool PreVerifyRecoveredSig(const CQuorumManager& quorum_manager, const CRecoveredSig& recoveredSig, bool& retBan); void CollectPendingRecoveredSigsToVerify(size_t maxUniqueSessions, std::unordered_map>>& retSigShares, diff --git a/src/rpc/quorums.cpp b/src/rpc/quorums.cpp index a45cfa808f..7b523541ba 100644 --- a/src/rpc/quorums.cpp +++ b/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 - 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); + return VerifyRecoveredSigLatestQuorums(*llmq_params_opt, chainman.ActiveChain(), *llmq_ctx.qman, signHeight, id, msgHash, sig); } 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 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); + return VerifyRecoveredSigLatestQuorums(*llmq_params_opt, chainman.ActiveChain(), *llmq_ctx.qman, signHeight, id, txid, sig); }, }; } @@ -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); - if (!llmq_ctx.clhandler->VerifyChainLock(clsig)) { + const auto clsig{llmq::CChainLockSig(nBlockHeight, nBlockHash, sig)}; + 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"); } diff --git a/test/functional/feature_llmq_chainlocks.py b/test/functional/feature_llmq_chainlocks.py index a74920e062..4e1618b582 100755 --- a/test/functional/feature_llmq_chainlocks.py +++ b/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]