From 117548660d2b28122ad50846453c2054b098726b Mon Sep 17 00:00:00 2001 From: pasta Date: Thu, 11 Jul 2024 11:37:25 -0500 Subject: [PATCH 1/7] Merge #6073: feat: add logging for RPC HTTP requests: command, user, http-code, time of running 1a691bd100434deeddaec820d80826e0f35d072f feat: add logging for RPC HTTP requests: command, user, http-code, time of running (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Currently there is no way to gather stats for http rpc in dash core. This PR aim to change it. ## What was done? Implemented some basic stats for each RPC request: - rpc command - flag "is external" - http status - time to serve query (rpc time, not http time) ## How Has This Been Tested? See new logs: ```log [httpworker.0] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=platform-user command=getbestblockhash is_external=false status=200 elapsed_time_ms=0 [httpworker.2] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=platform-user command=quorum is_external=false status=500 elapsed_time_ms=0 [httpworker.3] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=platform-user command= is_external=false status=401 elapsed_time_ms=0 [httpworker.3] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=platform-user command=getbestblockhash is_external=true status=200 elapsed_time_ms=28 [httpworker.0] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=operator command=getbestblockhash is_external=false status=200 elapsed_time_ms=0 ``` ## Breaking Changes N/A It doesn't change behavior of rpc server and http server. ## Checklist: - [x] I have performed a self-review of my own code - [x] 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 - [x] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK 1a691bd100434deeddaec820d80826e0f35d072f Tree-SHA512: b62fceedb9a901e87c23c4aa6e6dfa7226d44da84a081ea245b40e7ff887103302147cebe0f7ff90bf9c8d4fa9ecafbaa6f25f39d2008f62c4f2beeef2877b57 --- src/httprpc.cpp | 73 +++++++++++++++++++++++++++++++--------------- src/httpserver.cpp | 4 +-- 2 files changed, 52 insertions(+), 25 deletions(-) diff --git a/src/httprpc.cpp b/src/httprpc.cpp index a691e87c00..af02b53c55 100644 --- a/src/httprpc.cpp +++ b/src/httprpc.cpp @@ -72,7 +72,36 @@ static std::vector> g_rpcauth; static std::map> g_rpc_whitelist; static bool g_rpc_whitelist_default = false; -static void JSONErrorReply(HTTPRequest* req, const UniValue& objError, const UniValue& id) +extern std::vector g_external_usernames; +class RpcHttpRequest +{ +public: + HTTPRequest* m_req; + int64_t m_startTime; + int m_status{0}; + std::string user; + std::string command; + + RpcHttpRequest(HTTPRequest* req) : + m_req{req}, + m_startTime{GetTimeMicros()} + {} + + ~RpcHttpRequest() + { + const bool is_external = find(g_external_usernames.begin(), g_external_usernames.end(), user) != g_external_usernames.end(); + LogPrint(BCLog::BENCHMARK, "HTTP RPC request handled: user=%s command=%s external=%s status=%d elapsed_time_ms=%d\n", user, command, is_external, m_status, (GetTimeMicros() - m_startTime) / 1000); + } + + bool send_reply(int status, const std::string& response = "") + { + m_status = status; + m_req->WriteReply(status, response); + return m_status == HTTP_OK; + } +}; + +static bool JSONErrorReply(RpcHttpRequest& rpcRequest, const UniValue& objError, const UniValue& id) { // Send error reply from json-rpc error object int nStatus = HTTP_INTERNAL_SERVER_ERROR; @@ -88,8 +117,8 @@ static void JSONErrorReply(HTTPRequest* req, const UniValue& objError, const Uni std::string strReply = JSONRPCReply(NullUniValue, objError, id); - req->WriteHeader("Content-Type", "application/json"); - req->WriteReply(nStatus, strReply); + rpcRequest.m_req->WriteHeader("Content-Type", "application/json"); + return rpcRequest.send_reply(nStatus, strReply); } //This function checks username and password against -rpcauth @@ -146,24 +175,25 @@ static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUserna return multiUserAuthorized(strUserPass); } -static bool HTTPReq_JSONRPC(const CoreContext& context, HTTPRequest* req, bool external = false) +static bool HTTPReq_JSONRPC(const CoreContext& context, HTTPRequest* req) { + RpcHttpRequest rpcRequest(req); + // JSONRPC handles only POST if (req->GetRequestMethod() != HTTPRequest::POST) { - req->WriteReply(HTTP_BAD_METHOD, "JSONRPC server handles only POST requests"); - return false; + return rpcRequest.send_reply(HTTP_BAD_METHOD, "JSONRPC server handles only POST requests"); } // Check authorization std::pair authHeader = req->GetHeader("authorization"); if (!authHeader.first) { req->WriteHeader("WWW-Authenticate", WWW_AUTH_HEADER_DATA); - req->WriteReply(HTTP_UNAUTHORIZED); - return false; + return rpcRequest.send_reply(HTTP_UNAUTHORIZED); } JSONRPCRequest jreq(context); + jreq.peerAddr = req->GetPeer().ToString(); - if (!RPCAuthorized(authHeader.second, jreq.authUser)) { + if (!RPCAuthorized(authHeader.second, rpcRequest.user)) { LogPrintf("ThreadRPCServer incorrect password attempt from %s\n", jreq.peerAddr); /* Deter brute-forcing @@ -172,9 +202,9 @@ static bool HTTPReq_JSONRPC(const CoreContext& context, HTTPRequest* req, bool e UninterruptibleSleep(std::chrono::milliseconds{250}); req->WriteHeader("WWW-Authenticate", WWW_AUTH_HEADER_DATA); - req->WriteReply(HTTP_UNAUTHORIZED); - return false; + return rpcRequest.send_reply(HTTP_UNAUTHORIZED); } + jreq.authUser = rpcRequest.user; try { // Parse request @@ -189,16 +219,16 @@ static bool HTTPReq_JSONRPC(const CoreContext& context, HTTPRequest* req, bool e bool user_has_whitelist = g_rpc_whitelist.count(jreq.authUser); if (!user_has_whitelist && g_rpc_whitelist_default) { LogPrintf("RPC User %s not allowed to call any methods\n", jreq.authUser); - req->WriteReply(HTTP_FORBIDDEN); - return false; + return rpcRequest.send_reply(HTTP_FORBIDDEN); // singleton request } else if (valRequest.isObject()) { jreq.parse(valRequest); + rpcRequest.command = jreq.strMethod; + if (user_has_whitelist && !g_rpc_whitelist[jreq.authUser].count(jreq.strMethod)) { LogPrintf("RPC User %s not allowed to call method %s\n", jreq.authUser, jreq.strMethod); - req->WriteReply(HTTP_FORBIDDEN); - return false; + return rpcRequest.send_reply(HTTP_FORBIDDEN); } UniValue result = tableRPC.execute(jreq); @@ -217,8 +247,7 @@ static bool HTTPReq_JSONRPC(const CoreContext& context, HTTPRequest* req, bool e std::string strMethod = find_value(request, "method").get_str(); if (!g_rpc_whitelist[jreq.authUser].count(strMethod)) { LogPrintf("RPC User %s not allowed to call method %s\n", jreq.authUser, strMethod); - req->WriteReply(HTTP_FORBIDDEN); - return false; + return rpcRequest.send_reply(HTTP_FORBIDDEN); } } } @@ -229,15 +258,13 @@ static bool HTTPReq_JSONRPC(const CoreContext& context, HTTPRequest* req, bool e throw JSONRPCError(RPC_PARSE_ERROR, "Top-level object parse error"); req->WriteHeader("Content-Type", "application/json"); - req->WriteReply(HTTP_OK, strReply); + return rpcRequest.send_reply(HTTP_OK, strReply); } catch (const UniValue& objError) { - JSONErrorReply(req, objError, jreq.id); - return false; + return JSONErrorReply(rpcRequest, objError, jreq.id); } catch (const std::exception& e) { - JSONErrorReply(req, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id); - return false; + return JSONErrorReply(rpcRequest, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id); } - return true; + assert(false); } static bool InitRPCAuthentication() diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 9fbd0be71b..a7dc2cfaa7 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -154,8 +154,8 @@ static struct evhttp* eventHTTP = nullptr; static std::vector rpc_allow_subnets; //! Work queue for handling longer requests off the event loop thread static std::unique_ptr> g_work_queue{nullptr}; -//! List of 'external' RPC users -static std::vector g_external_usernames; +//! List of 'external' RPC users (global variable, used by httprpc) +std::vector g_external_usernames; //! Handlers for (sub)paths static std::vector pathHandlers; //! Bound listening sockets From c1c2c5569007f12100ddb1c79044ef7e5193924c Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 9 Jul 2024 09:36:46 -0500 Subject: [PATCH 2/7] Merge #6092: fix: mixing for partially unlocked descriptor wallets c94490839912e21965a01655fa24f837f037b4f6 refactor: simplify implementation of function CWallet::IsLocked (Konstantin Akimov) 685cf34cb9d4505c0ff2980e515bfa7dbc8d3e81 fix: unlock descriptor wallet for mixing-only (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented As [noticed by kwvg](https://github.com/dashpay/dash/pull/6090#pullrequestreview-2153639183), mixing doesn't work with descriptor wallets if do "unlock only for mixing". This PR is aiming to fix this issue. https://github.com/dashpay/dash-issues/issues/59 ## What was done? Removed default argument "bool mixing = false" from WalletStorage interface, Refactored a logic of CWallet::IsLocked to make it shorter, clearer and unified with bitcoin. ## How Has This Been Tested? A. Run Dash-Qt with descriptor wallet, run mixing, enter passphrase. The wallet is partially unlocked (for mixing only) - possible to see yellow lock in status. Mixing happens. B. Open "send transaction dialog", try to send a transaction: the app asks password to unlock wallet as expected. Though, I am not sure how exactly to test that **all** rpc are indeed locked when descriptor wallet is unlocked for mixing only. ## Breaking Changes N/A ## Checklist: - [x] 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 - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: LGTM, ~utACK~ light ACK c94490839912e21965a01655fa24f837f037b4f6 kwvg: ACK c94490839912e21965a01655fa24f837f037b4f6 PastaPastaPasta: utACK c94490839912e21965a01655fa24f837f037b4f6 Tree-SHA512: 236c895dd75042449282b051e90781ace1c941a3b2c34bb29ddadb6e62ba9c8d57c2a677ed98847630ff7fb6df4e14d2b59f3473d8f299ec104afeeb8103930c --- src/wallet/scriptpubkeyman.cpp | 8 ++++---- src/wallet/scriptpubkeyman.h | 2 +- src/wallet/wallet.cpp | 14 ++------------ 3 files changed, 7 insertions(+), 17 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index b3840905ea..678022e1f2 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -331,7 +331,7 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(WalletBatch &batch, const CScrip void LegacyScriptPubKeyMan::UpgradeKeyMetadata() { LOCK(cs_KeyStore); // mapKeyMetadata - if (m_storage.IsLocked() || m_storage.IsWalletFlagSet(WALLET_FLAG_KEY_ORIGIN_METADATA) || !IsHDEnabled()) { + if (m_storage.IsLocked(false) || m_storage.IsWalletFlagSet(WALLET_FLAG_KEY_ORIGIN_METADATA) || !IsHDEnabled()) { return; } @@ -1901,7 +1901,7 @@ void DescriptorScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, std::map DescriptorScriptPubKeyMan::GetKeys() const { AssertLockHeld(cs_desc_man); - if (m_storage.HasEncryptionKeys() && !m_storage.IsLocked()) { + if (m_storage.HasEncryptionKeys() && !m_storage.IsLocked(true)) { KeyMap keys; for (auto key_pair : m_map_crypted_keys) { const CPubKey& pubkey = key_pair.second.first; @@ -2014,7 +2014,7 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const } if (m_storage.HasEncryptionKeys()) { - if (m_storage.IsLocked()) { + if (m_storage.IsLocked(true)) { return false; } @@ -2423,7 +2423,7 @@ bool DescriptorScriptPubKeyMan::GetDescriptorString(std::string& out) const void DescriptorScriptPubKeyMan::UpgradeDescriptorCache() { LOCK(cs_desc_man); - if (m_storage.IsLocked() || m_storage.IsWalletFlagSet(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED)) { + if (m_storage.IsLocked(false) || m_storage.IsWalletFlagSet(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED)) { return; } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 7188ce9d1d..9c639b73eb 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -40,7 +40,7 @@ public: virtual void SetMinVersion(enum WalletFeature, WalletBatch* = nullptr) = 0; virtual const CKeyingMaterial& GetEncryptionKey() const = 0; virtual bool HasEncryptionKeys() const = 0; - virtual bool IsLocked(bool fForMixing = false) const = 0; + virtual bool IsLocked(bool fForMixing) const = 0; // for LegacyScriptPubKeyMan::TopUpInner needs: virtual void UpdateProgress(const std::string&, int) = 0; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4bd880888c..29d6104270 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -5459,21 +5459,11 @@ bool CWallet::IsLocked(bool fForMixing) const { if (!IsCrypted()) return false; - bool result; - { - LOCK(cs_wallet); - result = vMasterKey.empty(); - } - // fForMixing fOnlyMixingAllowed return - // --------------------------------------- - // true true result - // true false result - // false true true - // false false result if(!fForMixing && fOnlyMixingAllowed) return true; - return result; + LOCK(cs_wallet); + return vMasterKey.empty(); } bool CWallet::Lock(bool fAllowMixing) From cdf7a250125c3eeccc23554640ecb9c6066ecf2b Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 8 Jul 2024 12:55:07 -0500 Subject: [PATCH 3/7] Merge #6095: fix: createwallet to require 'load_on_startup' for descriptor wallets a42e9df06fa0d0dd08fc58450d69cbbc8f3d1bf8 fix: createwallet to require 'load_on_startup' for descriptor wallets (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented RPC `createwallet` has changed list of arguments: `createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup )` since https://github.com/dashpay/dash/pull/5965 `load_on_startup` used to be an argument 5 but now it has a number 6. Both arguments 5 and 6 are boolean and it can confuse an user: they may even not notice that something wrong when it meant to be "load on startup" but got "descriptor wallet" which is not expected. See also previous attempt to resolve issue: https://github.com/dashpay/dash/pull/6029 ## What was done? To prevent confusion if user is not aware about this breaking changes, the RPC createwallet throws an exception if user trying to create descriptor wallet but has not mentioned load_on_startup. This requirement can be removed when major amount of users updated to v21. ## How Has This Been Tested? Run unit/functional tests Tested with CLI: ``` $ createwallet "tmp-create" true true "" false true RPC createwallet would not accept creating descriptor wallets without specifying 'load_on_startup' flag. It is required explicitly in v21 due to breaking changes in createwallet RPC (code -8) $ createwallet "tmp-create" true true "" false true true { "name": "tmp-create", "warning": "Empty string given as passphrase, wallet will not be encrypted.\nWallet is an experimental descriptor wallet" } ``` ## Breaking Changes You can't more pass 'descriptor=NN' without https://github.com/dashpay/dash/pull/5965 which has not been 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 a42e9df06fa0d0dd08fc58450d69cbbc8f3d1bf8 PastaPastaPasta: utACK a42e9df06fa0d0dd08fc58450d69cbbc8f3d1bf8 thephez: utACK a42e9df06fa0d0dd08fc58450d69cbbc8f3d1bf8 Tree-SHA512: bf57fed40d04a32e756e4f8bfabbe39c0cbf87275546c92f4efc19523bc3c5dd3ddc5a884d67285971dc301a6c5808bef979db52c35645ca2db0810046fe1bdc --- doc/release-notes-16528.md | 2 ++ src/wallet/rpcwallet.cpp | 3 +++ test/functional/test_framework/test_node.py | 2 ++ 3 files changed, 7 insertions(+) diff --git a/doc/release-notes-16528.md b/doc/release-notes-16528.md index 84af219e90..bc67977f86 100644 --- a/doc/release-notes-16528.md +++ b/doc/release-notes-16528.md @@ -35,6 +35,7 @@ Descriptor Wallet should be created. Without those options being set, a Legacy Wallet will be created instead. + #### `IsMine` Semantics `IsMine` refers to the function used to determine whether a script belongs to the wallet. @@ -117,3 +118,4 @@ descriptors with private keys for now as explained earlier. ## RPC changes - `createwallet` has changed list of arguments: `createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup )` `load_on_startup` used to be an argument 5 but now has a number 6. + - `createwallet` requires specifying the `load_on_startup` flag when creating descriptor wallets due to breaking changes in v21. diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 2a7b557b31..f9b29f28e5 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3032,6 +3032,9 @@ static RPCHelpMan createwallet() #ifndef USE_SQLITE throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without sqlite support (required for descriptor wallets)"); #endif + if (request.params[6].isNull()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "The createwallet RPC requires specifying the 'load_on_startup' flag when creating descriptor wallets. Dash Core v21 introduced this requirement due to breaking changes in the createwallet RPC."); + } flags |= WALLET_FLAG_DESCRIPTORS; warnings.emplace_back(Untranslated("Wallet is an experimental descriptor wallet")); } diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index b1d6f70bfa..d92b5bc4a3 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -704,6 +704,8 @@ class RPCOverloadWrapper(): def createwallet(self, wallet_name, disable_private_keys=None, blank=None, passphrase='', avoid_reuse=None, descriptors=None, load_on_startup=None): if descriptors is None: descriptors = self.descriptors + if descriptors is not None and load_on_startup is None: + load_on_startup = False return self.__getattr__('createwallet')(wallet_name, disable_private_keys, blank, passphrase, avoid_reuse, descriptors, load_on_startup) def importprivkey(self, privkey, label=None, rescan=None): From 9998ffd92bbd37d436bd2ec130d14f18902fedc9 Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 9 Jul 2024 08:47:44 -0500 Subject: [PATCH 4/7] 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] From 7330982631412162161dfdd17e9a090baf4e0f92 Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 15 Jul 2024 11:43:38 -0500 Subject: [PATCH 5/7] Merge #6100: feat: make whitelist works with composite commands for platform needs 85abbb97b4cbc292d81a29e94f75ee20028e8186 chore: add release notes for composite command for whitelist (Konstantin Akimov) 78ad778bb0ef88c6d24505699b0dfdea9891aa83 feat: test composite commands in functional test for whitelist (Konstantin Akimov) a102a5978718f399aa71c22afad720f2f686eac6 feat: add support of composite commands in RPC'c whitelists (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented https://github.com/dashpay/dash-issues/issues/66 https://github.com/dashpay/dash-issues/issues/65 ## What was done? Our composite commands such as "quorum list" have been refactored to make them truly compatible with other features, such as whitelist, see https://github.com/dashpay/dash/pull/6052 https://github.com/dashpay/dash/pull/6051 https://github.com/dashpay/dash/pull/6055 and other related PRs This PR makes whitelist feature to be compatible with composite commands. Instead implementing additional users such "dapi" better to provide universal way which do not require new build for every new API that has been used by platform, let's simplify things. Platform at their side can use config such as this one (created based on shumkov's example): ``` rpc: { host: '127.0.0.1', port: 9998, users: [ { user: 'dashmate', password: 'rpcpassword', whitelist: null, lowPriority: false, }, { username: 'platform-dapi', password: 'rpcpassword', whitelist: [], lowPriority: true, }, { username: 'platform-drive-consensus', password: 'rpcpassword', whitelist: [getbestchainlock,getblockchaininfo,getrawtransaction,submitchainlock,verifychainlock,protx_listdiff,quorum_listextended,quorum_info,getassetunlockstatuses,sendrawtransaction,mnsync_status] lowPriority: false, }, { username: 'platform-drive-other', password: 'rpcpassword', whitelist: [getbestchainlock,getblockchaininfo,getrawtransaction,submitchainlock,verifychainlock,protx_listdiff,quorum_listextended,quorum_info,getassetunlockstatuses,sendrawtransaction,mnsync_status] ], lowPriority: true, }, ], allowIps: ['127.0.0.1', '172.16.0.0/12', '192.168.0.0/16'], }, ``` ## How Has This Been Tested? Updated functional tests, see commits ## Breaking Changes n/a ## 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: LGTM, utACK 85abbb97b4cbc292d81a29e94f75ee20028e8186 PastaPastaPasta: utACK 85abbb97b4cbc292d81a29e94f75ee20028e8186 Tree-SHA512: 88608179c347420269880c352cf9f3b46272f3fc62e8e7158042e53ad69dc460d5210a1f89e1e09081d090250c87fcececade88e2ddec09f73f1175836d7867b --- doc/release-notes-6100.md | 9 +++++++++ src/httprpc.cpp | 15 +++++++++++++-- test/functional/rpc_whitelist.py | 19 ++++++++++++++++--- 3 files changed, 38 insertions(+), 5 deletions(-) create mode 100644 doc/release-notes-6100.md diff --git a/doc/release-notes-6100.md b/doc/release-notes-6100.md new file mode 100644 index 0000000000..656e0b0aac --- /dev/null +++ b/doc/release-notes-6100.md @@ -0,0 +1,9 @@ +## Remote Procedure Call (RPC) Changes + +### Improved support of composite commands + +Dash Core's composite commands such as `quorum list` or `bls generate` now are compatible with a whitelist feature. + +For example, whitelist `getblockcount,quorumlist` will let to call commands `getblockcount`, `quorum list`, but not `quorum sign` + +Note, that adding simple `quorum` in whitelist will allow to use all kind of `quorum...` commands, such as `quorum`, `quorum list`, `quorum sign`, etc diff --git a/src/httprpc.cpp b/src/httprpc.cpp index af02b53c55..5a5f0f601c 100644 --- a/src/httprpc.cpp +++ b/src/httprpc.cpp @@ -101,6 +101,17 @@ public: } }; +static bool whitelisted(JSONRPCRequest jreq) +{ + if (g_rpc_whitelist[jreq.authUser].count(jreq.strMethod)) return true; + + // check for composite command after + if (!jreq.params.isArray() || jreq.params.empty()) return false; + if (!jreq.params[0].isStr()) return false; + + return g_rpc_whitelist[jreq.authUser].count(jreq.strMethod + jreq.params[0].get_str()); +} + static bool JSONErrorReply(RpcHttpRequest& rpcRequest, const UniValue& objError, const UniValue& id) { // Send error reply from json-rpc error object @@ -226,7 +237,7 @@ static bool HTTPReq_JSONRPC(const CoreContext& context, HTTPRequest* req) jreq.parse(valRequest); rpcRequest.command = jreq.strMethod; - if (user_has_whitelist && !g_rpc_whitelist[jreq.authUser].count(jreq.strMethod)) { + if (user_has_whitelist && !whitelisted(jreq)) { LogPrintf("RPC User %s not allowed to call method %s\n", jreq.authUser, jreq.strMethod); return rpcRequest.send_reply(HTTP_FORBIDDEN); } @@ -245,7 +256,7 @@ static bool HTTPReq_JSONRPC(const CoreContext& context, HTTPRequest* req) const UniValue& request = valRequest[reqIdx].get_obj(); // Parse method std::string strMethod = find_value(request, "method").get_str(); - if (!g_rpc_whitelist[jreq.authUser].count(strMethod)) { + if (!whitelisted(jreq)) { LogPrintf("RPC User %s not allowed to call method %s\n", jreq.authUser, strMethod); return rpcRequest.send_reply(HTTP_FORBIDDEN); } diff --git a/test/functional/rpc_whitelist.py b/test/functional/rpc_whitelist.py index 77aa79ec37..534d37ffbd 100755 --- a/test/functional/rpc_whitelist.py +++ b/test/functional/rpc_whitelist.py @@ -12,7 +12,9 @@ from test_framework.util import ( assert_equal, str_to_b64str ) +import json import http.client +import re import urllib.parse def rpccall(node, user, method): @@ -20,7 +22,17 @@ def rpccall(node, user, method): headers = {"Authorization": "Basic " + str_to_b64str('{}:{}'.format(user[0], user[3]))} conn = http.client.HTTPConnection(url.hostname, url.port) conn.connect() - conn.request('POST', '/', '{"method": "' + method + '"}', headers) + + # composite commands are presented without space in whitelist + # but space can't be ommitted when using CLI/http rpc + # for sack of test, substitute missing space for quorum composite command + params = [] + if re.match(r"^quorum[^ ]", method): + params = [method[6:]] + method = "quorum" + query = {"method" : method, "params" : params} + + conn.request('POST', '/', json.dumps(query), headers) resp = conn.getresponse() conn.close() return resp @@ -39,7 +51,8 @@ class RPCWhitelistTest(BitcoinTestFramework): # 3 => Password Plaintext self.users = [ ["user1", "50358aa884c841648e0700b073c32b2e$b73e95fff0748cc0b517859d2ca47d9bac1aa78231f3e48fa9222b612bd2083e", "getbestblockhash,getblockcount,", "12345"], - ["user2", "8650ba41296f62092377a38547f361de$4620db7ba063ef4e2f7249853e9f3c5c3592a9619a759e3e6f1c63f2e22f1d21", "getblockcount", "54321"] + ["user2", "8650ba41296f62092377a38547f361de$4620db7ba063ef4e2f7249853e9f3c5c3592a9619a759e3e6f1c63f2e22f1d21", "getblockcount", "54321"], + ["platform-user", "8650ba41296f62092377a38547f361de$4620db7ba063ef4e2f7249853e9f3c5c3592a9619a759e3e6f1c63f2e22f1d21", "getblockcount,quorumlist", "54321"], ] # For exceptions self.strange_users = [ @@ -55,7 +68,7 @@ class RPCWhitelistTest(BitcoinTestFramework): ["strangedude5", "d12c6e962d47a454f962eb41225e6ec8$2dd39635b155536d3c1a2e95d05feff87d5ba55f2d5ff975e6e997a836b717c9", ":getblockcount,getblockcount", "s7R4nG3R7H1nGZ"] ] # These commands shouldn't be allowed for any user to test failures - self.never_allowed = ["getnetworkinfo"] + self.never_allowed = ["getnetworkinfo", "quorum sign"] with open(os.path.join(get_datadir_path(self.options.tmpdir, 0), "dash.conf"), 'a', encoding='utf8') as f: f.write("\nrpcwhitelistdefault=0\n") for user in self.users: From a45e6df58b248ab7c496938b6501d6a696563cdb Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 8 Jul 2024 18:43:20 -0500 Subject: [PATCH 6/7] Merge #6104: fix: adjust incorrect parameter description that says there is a default that doesn't exist 60e36b786a04374c2404190cfffd5a713a225708 fix: adjust incorrect parameter description that says there is a default that doesn't exist (pasta) Pull request description: ## Issue being fixed or feature implemented This default doesn't actually exist in code. ## What was done? Remove default from text ## How Has This Been Tested? ## Breaking Changes None ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] 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 - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 60e36b786a04374c2404190cfffd5a713a225708 Tree-SHA512: 658eb2cf101d0450619461b7ffe6069de5c04c1fdcca216713f9374cc1e60413ec9b96c3a2931fb69a4c2f8277dd6677100270960a58197da3b92dffb1e9e327 --- src/chainparamsbase.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/chainparamsbase.cpp b/src/chainparamsbase.cpp index 0ed763e735..d18ba6da8d 100644 --- a/src/chainparamsbase.cpp +++ b/src/chainparamsbase.cpp @@ -25,7 +25,7 @@ void SetupChainParamsBaseOptions(ArgsManager& argsman) argsman.AddArg("-highsubsidyblocks=", "The number of blocks with a higher than normal subsidy to mine at the start of a chain. Block after that height will have fixed subsidy base. (default: 0, devnet-only)", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS); argsman.AddArg("-highsubsidyfactor=", "The factor to multiply the normal block subsidy by while in the highsubsidyblocks window of a chain (default: 1, devnet-only)", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS); argsman.AddArg("-llmqchainlocks=", "Override the default LLMQ type used for ChainLocks. Allows using ChainLocks with smaller LLMQs. (default: llmq_devnet, devnet-only)", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS); - argsman.AddArg("-llmqdevnetparams=:", "Override the default LLMQ size for the LLMQ_DEVNET quorum (default: 3:2, devnet-only)", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS); + argsman.AddArg("-llmqdevnetparams=:", "Override the default LLMQ size for the LLMQ_DEVNET quorum (devnet-only)", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS); argsman.AddArg("-llmqinstantsenddip0024=", "Override the default LLMQ type used for InstantSendDIP0024. (default: llmq_devnet_dip0024, devnet-only)", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS); argsman.AddArg("-llmqplatform=", "Override the default LLMQ type used for Platform. (default: llmq_devnet_platform, devnet-only)", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS); argsman.AddArg("-llmqmnhf=", "Override the default LLMQ type used for EHF. (default: llmq_devnet, devnet-only)", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS); From db828177bf315cad8efb0f8f65d31b81b190617d Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 15 Jul 2024 11:47:39 -0500 Subject: [PATCH 7/7] Merge #6106: feat: create new composite quorum-command platformsign 2db69d7b8115f075092563fe67c486fbd6a86836 chore: add release notes for "quorum platformsign" (Konstantin Akimov) 283c5f89a2d8cce91d85b5aeeca0e93fcb159430 feat: create new composite command "quorum platformsign" (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented It splits from #6100 With just whitelist it is impossible to limit the RPC `quorum sign` to use only one specific quorum type, this PR aim to provide ability for quorum signing for platform quorum only. ## What was done? Implemented a new composite command "quorum platformsign" This composite command let to limit quorum type for signing for case of whitelist. After that old way to limit platform commands can be deprecated - #6105 ## How Has This Been Tested? Updated a functional tests to use platform signing for Asset Unlocks feature. ## Breaking Changes N/A ## 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 2db69d7b8115f075092563fe67c486fbd6a86836 PastaPastaPasta: utACK 2db69d7b8115f075092563fe67c486fbd6a86836 Tree-SHA512: b0dff9934137c4faa85664058e1e77f85067cc8d931e6d76ee5b9e610164ac8b0609736d5f09475256cb78d65bf92466624d784f0b13d20136df7e75613662cb --- doc/release-notes-6106.md | 6 ++ src/rpc/quorums.cpp | 77 +++++++++++++------ test/functional/feature_asset_locks.py | 2 +- .../test_framework/test_framework.py | 7 +- 4 files changed, 67 insertions(+), 25 deletions(-) create mode 100644 doc/release-notes-6106.md diff --git a/doc/release-notes-6106.md b/doc/release-notes-6106.md new file mode 100644 index 0000000000..b161888a9b --- /dev/null +++ b/doc/release-notes-6106.md @@ -0,0 +1,6 @@ +## Remote Procedure Call (RPC) Changes + +### The new RPCs are: + +- `quorum signplatform` This RPC is added for Platform needs. This composite command let to limit quorum type for signing by platform. It is equivalent of `quorum sign `. + diff --git a/src/rpc/quorums.cpp b/src/rpc/quorums.cpp index 7b523541ba..ee6c990391 100644 --- a/src/rpc/quorums.cpp +++ b/src/rpc/quorums.cpp @@ -427,42 +427,27 @@ static RPCHelpMan quorum_memberof() }; } -static RPCHelpMan quorum_sign() -{ - return RPCHelpMan{"quorum sign", - "Threshold-sign a message\n", - { - {"llmqType", RPCArg::Type::NUM, RPCArg::Optional::NO, "LLMQ type."}, - {"id", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "Request id."}, - {"msgHash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "Message hash."}, - {"quorumHash", RPCArg::Type::STR_HEX, /* default */ "", "The quorum identifier."}, - {"submit", RPCArg::Type::BOOL, /* default */ "true", "Submits the signature share to the network if this is true. " - "Returns an object containing the signature share if this is false."}, - }, - RPCResults{}, - RPCExamples{""}, - [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue +static UniValue quorum_sign_helper(const JSONRPCRequest& request, Consensus::LLMQType llmqType) { const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); const LLMQContext& llmq_ctx = EnsureLLMQContext(node); - const Consensus::LLMQType llmqType{static_cast(ParseInt32V(request.params[0], "llmqType"))}; const auto llmq_params_opt = Params().GetLLMQ(llmqType); if (!llmq_params_opt.has_value()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid LLMQ type"); } - const uint256 id(ParseHashV(request.params[1], "id")); - const uint256 msgHash(ParseHashV(request.params[2], "msgHash")); + const uint256 id(ParseHashV(request.params[0], "id")); + const uint256 msgHash(ParseHashV(request.params[1], "msgHash")); uint256 quorumHash; - if (!request.params[3].isNull() && !request.params[3].get_str().empty()) { - quorumHash = ParseHashV(request.params[3], "quorumHash"); + if (!request.params[2].isNull() && !request.params[2].get_str().empty()) { + quorumHash = ParseHashV(request.params[2], "quorumHash"); } bool fSubmit{true}; - if (!request.params[4].isNull()) { - fSubmit = ParseBoolV(request.params[4], "submit"); + if (!request.params[3].isNull()) { + fSubmit = ParseBoolV(request.params[3], "submit"); } if (fSubmit) { return llmq_ctx.sigman->AsyncSignIfMember(llmqType, *llmq_ctx.shareman, id, msgHash, quorumHash); @@ -496,6 +481,53 @@ static RPCHelpMan quorum_sign() return obj; } +} + +static RPCHelpMan quorum_sign() +{ + return RPCHelpMan{"quorum sign", + "Threshold-sign a message\n", + { + {"llmqType", RPCArg::Type::NUM, RPCArg::Optional::NO, "LLMQ type."}, + {"id", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "Request id."}, + {"msgHash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "Message hash."}, + {"quorumHash", RPCArg::Type::STR_HEX, /* default */ "", "The quorum identifier."}, + {"submit", RPCArg::Type::BOOL, /* default */ "true", "Submits the signature share to the network if this is true. " + "Returns an object containing the signature share if this is false."}, + }, + RPCResults{}, + RPCExamples{""}, + [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue +{ + const Consensus::LLMQType llmqType{static_cast(ParseInt32V(request.params[0], "llmqType"))}; + + JSONRPCRequest new_request{request}; + new_request.params.setArray(); + for (unsigned int i = 1; i < request.params.size(); ++i) { + new_request.params.push_back(request.params[i]); + } + return quorum_sign_helper(new_request, llmqType); +}, + }; +} + +static RPCHelpMan quorum_platformsign() +{ + return RPCHelpMan{"quorum platformsign", + "Threshold-sign a message. It signs messages only for platform quorums\n", + { + {"id", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "Request id."}, + {"msgHash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "Message hash."}, + {"quorumHash", RPCArg::Type::STR_HEX, /* default */ "", "The quorum identifier."}, + {"submit", RPCArg::Type::BOOL, /* default */ "true", "Submits the signature share to the network if this is true. " + "Returns an object containing the signature share if this is false."}, + }, + RPCResults{}, + RPCExamples{""}, + [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue +{ + const Consensus::LLMQType llmqType{Params().GetConsensus().llmqTypePlatform}; + return quorum_sign_helper(request, llmqType); }, }; } @@ -1110,6 +1142,7 @@ static const CRPCCommand commands[] = { "evo", "quorum", "dkgstatus", &quorum_dkgstatus, {"detail_level"} }, { "evo", "quorum", "memberof", &quorum_memberof, {"proTxHash", "scanQuorumsCount"} }, { "evo", "quorum", "sign", &quorum_sign, {"llmqType", "id", "msgHash", "quorumHash", "submit"} }, + { "evo", "quorum", "platformsign", &quorum_platformsign, {"id", "msgHash", "quorumHash", "submit"} }, { "evo", "quorum", "verify", &quorum_verify, {"llmqType", "id", "msgHash", "signature", "quorumHash", "signHeight"} }, { "evo", "quorum", "hasrecsig", &quorum_hasrecsig, {"llmqType", "id", "msgHash"} }, { "evo", "quorum", "getrecsig", &quorum_getrecsig, {"llmqType", "id", "msgHash"} }, diff --git a/test/functional/feature_asset_locks.py b/test/functional/feature_asset_locks.py index 45a80fc152..c4a1b030be 100755 --- a/test/functional/feature_asset_locks.py +++ b/test/functional/feature_asset_locks.py @@ -119,7 +119,7 @@ class AssetLocksTest(DashTestFramework): unlock_tx.calc_sha256() msgHash = format(unlock_tx.sha256, '064x') - recsig = self.get_recovered_sig(request_id, msgHash, llmq_type=llmq_type_test) + recsig = self.get_recovered_sig(request_id, msgHash, llmq_type=llmq_type_test, use_platformsign=True) unlockTx_payload.quorumSig = bytearray.fromhex(recsig["sig"]) unlock_tx.vExtraPayload = unlockTx_payload.serialize() diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index f0a9f974db..f419f58d0c 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -2033,13 +2033,16 @@ class DashTestFramework(BitcoinTestFramework): return True wait_until_helper(check_recovered_sig, timeout=timeout, sleep=1) - def get_recovered_sig(self, rec_sig_id, rec_sig_msg_hash, llmq_type=100): + def get_recovered_sig(self, rec_sig_id, rec_sig_msg_hash, llmq_type=100, use_platformsign=False): # Note: recsigs aren't relayed to regular nodes by default, # make sure to pick a mn as a node to query for recsigs. try: quorumHash = self.mninfo[0].node.quorum("selectquorum", llmq_type, rec_sig_id)["quorumHash"] for mn in self.mninfo: - mn.node.quorum("sign", llmq_type, rec_sig_id, rec_sig_msg_hash, quorumHash) + if use_platformsign: + mn.node.quorum("platformsign", rec_sig_id, rec_sig_msg_hash, quorumHash) + else: + mn.node.quorum("sign", llmq_type, rec_sig_id, rec_sig_msg_hash, quorumHash) self.wait_for_recovered_sig(rec_sig_id, rec_sig_msg_hash, llmq_type, 10) return self.mninfo[0].node.quorum("getrecsig", llmq_type, rec_sig_id, rec_sig_msg_hash) except JSONRPCException as e: