From d7e30765cc493b4c0b31813213083124249dc98b Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 18 Dec 2024 12:51:11 -0600 Subject: [PATCH 01/19] refactor: drop unneccessary if guard --- src/llmq/signing_shares.cpp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/llmq/signing_shares.cpp b/src/llmq/signing_shares.cpp index 6bcc0e8996..d9345588f7 100644 --- a/src/llmq/signing_shares.cpp +++ b/src/llmq/signing_shares.cpp @@ -739,16 +739,14 @@ void CSigSharesManager::ProcessSigShare(PeerManager& peerman, const CSigShare& s // Update the time we've seen the last sigShare timeSeenForSessions[sigShare.GetSignHash()] = GetTime().count(); - if (!quorumNodes.empty()) { - // don't announce and wait for other nodes to request this share and directly send it to them - // there is no way the other nodes know about this share as this is the one created on this node - for (auto otherNodeId : quorumNodes) { - auto& nodeState = nodeStates[otherNodeId]; - auto& session = nodeState.GetOrCreateSessionFromShare(sigShare); - session.quorum = quorum; - session.requested.Set(sigShare.getQuorumMember(), true); - session.knows.Set(sigShare.getQuorumMember(), true); - } + // don't announce and wait for other nodes to request this share and directly send it to them + // there is no way the other nodes know about this share as this is the one created on this node + for (auto otherNodeId : quorumNodes) { + auto& nodeState = nodeStates[otherNodeId]; + auto& session = nodeState.GetOrCreateSessionFromShare(sigShare); + session.quorum = quorum; + session.requested.Set(sigShare.getQuorumMember(), true); + session.knows.Set(sigShare.getQuorumMember(), true); } size_t sigShareCount = sigShares.CountForSignHash(sigShare.GetSignHash()); From a04d178a3199f516cf35c1796f0ea7cfaec2a667 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 18 Dec 2024 12:31:37 -0600 Subject: [PATCH 02/19] perf: use unordered_set / unordered_map where order isn't relevant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Did a few quickbench to ensure there isn't some advantage to using the "worse" data structure at small sizes. set: https://quick-bench.com/q/ApPAGguzG2F9AnqM2GkgDAXCzX4 map: https://quick-bench.com/q/PXDdF0HVGpiSmC9Hh5FUSTp0uJU Using unordered_map did show a tiny insertion regression for ~expected sizes on masternodes, but it appears that only 1 access will compensate for that overhead, and each su, some perf results I have are here, show that this std::set usage is causing an imo unacceptably high cpu usage. ``` - 9.18% 0.00% d-mncon dashd [.] CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager&, CMasternodeMetaMan&, CMasternodeSync&)::{lambda()#▒ - CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager&, CMasternodeMetaMan&, CMasternodeSync&)::{lambda()#1}::operator()() const ▒ + 6.16% CConnman::IsMasternodeOrDisconnectRequested(CService const&) ▒ - 2.66% std::set, std::allocator >::count(CService const&) const ▒ - std::_Rb_tree, std::less, std::allocator >::find(CService const&) const ▒ - 2.02% std::_Rb_tree, std::less, std::allocator >::_M_lower_bound(std::_Rb_tree_node const▒ + 1.99% std::less::operator()(CService const&, CService const&) const ▒ + 0.62% std::less::operator()(CService const&, CService const&) const ``` --- src/llmq/dkgsession.h | 6 ++++-- src/llmq/quorums.cpp | 2 +- src/llmq/signing_shares.cpp | 2 +- src/llmq/utils.cpp | 29 ++++++++++++++++------------- src/llmq/utils.h | 13 ++++++++++--- src/net.cpp | 19 +++++++++---------- src/net.h | 12 ++++++------ 7 files changed, 47 insertions(+), 36 deletions(-) diff --git a/src/llmq/dkgsession.h b/src/llmq/dkgsession.h index 82a1263b92..b1897482fc 100644 --- a/src/llmq/dkgsession.h +++ b/src/llmq/dkgsession.h @@ -11,10 +11,12 @@ #include #include #include +#include #include #include #include +#include class CActiveMasternodeManager; class CInv; @@ -293,7 +295,7 @@ class CDKGSession private: std::vector> members; std::map membersMap; - std::set relayMembers; + std::unordered_set relayMembers; BLSVerificationVectorPtr vvecContribution; std::vector m_sk_contributions; @@ -382,7 +384,7 @@ class CDKGSession public: [[nodiscard]] CDKGMember* GetMember(const uint256& proTxHash) const; - [[nodiscard]] const std::set& RelayMembers() const { return relayMembers; } + [[nodiscard]] const std::unordered_set& RelayMembers() const { return relayMembers; } [[nodiscard]] const CBlockIndex* BlockIndex() const { return m_quorum_base_block_index; } [[nodiscard]] const uint256& ProTx() const { return myProTxHash; } [[nodiscard]] const Consensus::LLMQParams GetParams() const { return params; } diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index 67b2890b57..a27fa04494 100644 --- a/src/llmq/quorums.cpp +++ b/src/llmq/quorums.cpp @@ -370,7 +370,7 @@ void CQuorumManager::CheckQuorumConnections(CConnman& connman, const Consensus:: LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- llmqType[%d] h[%d] keeping mn quorum connections for quorum: [%d:%s]\n", __func__, ToUnderlying(llmqParams.type), pindexNew->nHeight, quorum->m_quorum_base_block_index->nHeight, quorum->m_quorum_base_block_index->GetBlockHash().ToString()); } } else if (watchOtherISQuorums && !quorum->IsMember(myProTxHash)) { - std::set connections; + std::unordered_set connections; const auto& cindexes = utils::CalcDeterministicWatchConnections(llmqParams.type, quorum->m_quorum_base_block_index, quorum->members.size(), 1); for (auto idx : cindexes) { connections.emplace(quorum->members[idx]->proTxHash); diff --git a/src/llmq/signing_shares.cpp b/src/llmq/signing_shares.cpp index 6bcc0e8996..fad33e63ee 100644 --- a/src/llmq/signing_shares.cpp +++ b/src/llmq/signing_shares.cpp @@ -717,7 +717,7 @@ void CSigSharesManager::ProcessSigShare(PeerManager& peerman, const CSigShare& s bool canTryRecovery = false; // prepare node set for direct-push in case this is our sig share - std::set quorumNodes; + std::unordered_set quorumNodes; if (!IsAllMembersConnectedEnabled(llmqType, m_sporkman) && sigShare.getQuorumMember() == quorum->GetMemberIndex(m_mn_activeman->GetProTxHash())) { quorumNodes = connman.GetMasternodeQuorumNodes(sigShare.getLlmqType(), sigShare.getQuorumHash()); } diff --git a/src/llmq/utils.cpp b/src/llmq/utils.cpp index 4f7d635958..a9bb1e3c81 100644 --- a/src/llmq/utils.cpp +++ b/src/llmq/utils.cpp @@ -653,12 +653,13 @@ uint256 DeterministicOutboundConnection(const uint256& proTxHash1, const uint256 return proTxHash2; } -std::set GetQuorumConnections(const Consensus::LLMQParams& llmqParams, CDeterministicMNManager& dmnman, const CSporkManager& sporkman, - gsl::not_null pQuorumBaseBlockIndex, const uint256& forMember, bool onlyOutbound) +std::unordered_set GetQuorumConnections( + const Consensus::LLMQParams& llmqParams, CDeterministicMNManager& dmnman, const CSporkManager& sporkman, + gsl::not_null pQuorumBaseBlockIndex, const uint256& forMember, bool onlyOutbound) { if (IsAllMembersConnectedEnabled(llmqParams.type, sporkman)) { auto mns = GetAllQuorumMembers(llmqParams.type, dmnman, pQuorumBaseBlockIndex); - std::set result; + std::unordered_set result; for (const auto& dmn : mns) { if (dmn->proTxHash == forMember) { @@ -677,23 +678,25 @@ std::set GetQuorumConnections(const Consensus::LLMQParams& llmqParams, return GetQuorumRelayMembers(llmqParams, dmnman, pQuorumBaseBlockIndex, forMember, onlyOutbound); } -std::set GetQuorumRelayMembers(const Consensus::LLMQParams& llmqParams, CDeterministicMNManager& dmnman, gsl::not_null pQuorumBaseBlockIndex, - const uint256& forMember, bool onlyOutbound) +std::unordered_set GetQuorumRelayMembers(const Consensus::LLMQParams& llmqParams, + CDeterministicMNManager& dmnman, + gsl::not_null pQuorumBaseBlockIndex, + const uint256& forMember, bool onlyOutbound) { auto mns = GetAllQuorumMembers(llmqParams.type, dmnman, pQuorumBaseBlockIndex); - std::set result; + std::unordered_set result; auto calcOutbound = [&](size_t i, const uint256& proTxHash) { + // Relay to nodes at indexes (i+2^k)%n, where + // k: 0..max(1, floor(log2(n-1))-1) + // n: size of the quorum/ring + std::unordered_set r{}; if (mns.size() == 1) { // No outbound connections are needed when there is one MN only. // Also note that trying to calculate results via the algorithm below // would result in an endless loop. - return std::set(); + return r; } - // Relay to nodes at indexes (i+2^k)%n, where - // k: 0..max(1, floor(log2(n-1))-1) - // n: size of the quorum/ring - std::set r; int gap = 1; int gap_max = (int)mns.size() - 1; int k = 0; @@ -775,8 +778,8 @@ bool EnsureQuorumConnections(const Consensus::LLMQParams& llmqParams, CConnman& LogPrint(BCLog::NET_NETCONN, "%s -- isMember=%d for quorum %s:\n", __func__, isMember, pQuorumBaseBlockIndex->GetBlockHash().ToString()); - std::set connections; - std::set relayMembers; + std::unordered_set connections; + std::unordered_set relayMembers; if (isMember) { connections = GetQuorumConnections(llmqParams, dmnman, sporkman, pQuorumBaseBlockIndex, myProTxHash, true); relayMembers = GetQuorumRelayMembers(llmqParams, dmnman, pQuorumBaseBlockIndex, myProTxHash, true); diff --git a/src/llmq/utils.h b/src/llmq/utils.h index 2b5dc3a88e..2795db58dc 100644 --- a/src/llmq/utils.h +++ b/src/llmq/utils.h @@ -5,13 +5,15 @@ #ifndef BITCOIN_LLMQ_UTILS_H #define BITCOIN_LLMQ_UTILS_H +#include #include +#include #include -#include #include #include #include +#include #include class CConnman; @@ -34,8 +36,13 @@ namespace utils std::vector GetAllQuorumMembers(Consensus::LLMQType llmqType, CDeterministicMNManager& dmnman, gsl::not_null pQuorumBaseBlockIndex, bool reset_cache = false); uint256 DeterministicOutboundConnection(const uint256& proTxHash1, const uint256& proTxHash2); -std::set GetQuorumConnections(const Consensus::LLMQParams& llmqParams, CDeterministicMNManager& dmnman, const CSporkManager& sporkman, gsl::not_null pQuorumBaseBlockIndex, const uint256& forMember, bool onlyOutbound); -std::set GetQuorumRelayMembers(const Consensus::LLMQParams& llmqParams, CDeterministicMNManager& dmnman, gsl::not_null pQuorumBaseBlockIndex, const uint256& forMember, bool onlyOutbound); +std::unordered_set GetQuorumConnections( + const Consensus::LLMQParams& llmqParams, CDeterministicMNManager& dmnman, const CSporkManager& sporkman, + gsl::not_null pQuorumBaseBlockIndex, const uint256& forMember, bool onlyOutbound); +std::unordered_set GetQuorumRelayMembers(const Consensus::LLMQParams& llmqParams, + CDeterministicMNManager& dmnman, + gsl::not_null pQuorumBaseBlockIndex, + const uint256& forMember, bool onlyOutbound); std::set CalcDeterministicWatchConnections(Consensus::LLMQType llmqType, gsl::not_null pQuorumBaseBlockIndex, size_t memberCount, size_t connectionCount); bool EnsureQuorumConnections(const Consensus::LLMQParams& llmqParams, CConnman& connman, CDeterministicMNManager& dmnman, const CSporkManager& sporkman, diff --git a/src/net.cpp b/src/net.cpp index 9bfb813178..5b07e5f9bf 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3718,12 +3718,11 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, if (!fNetworkActive || !m_masternode_thread_active || !mn_sync.IsBlockchainSynced()) continue; - std::set connectedNodes; - std::map connectedProRegTxHashes; + std::unordered_set connectedNodes; + std::unordered_map connectedProRegTxHashes; ForEachNode([&](const CNode* pnode) { - auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash(); connectedNodes.emplace(pnode->addr); - if (!verifiedProRegTxHash.IsNull()) { + if (auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash(); !verifiedProRegTxHash.IsNull()) { connectedProRegTxHashes.emplace(verifiedProRegTxHash, pnode->IsInboundConn()); } }); @@ -4617,7 +4616,7 @@ bool CConnman::AddPendingMasternode(const uint256& proTxHash) return true; } -void CConnman::SetMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash, const std::set& proTxHashes) +void CConnman::SetMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash, const std::unordered_set& proTxHashes) { LOCK(cs_vPendingMasternodes); auto it = masternodeQuorumNodes.emplace(std::make_pair(llmqType, quorumHash), proTxHashes); @@ -4626,7 +4625,7 @@ void CConnman::SetMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint } } -void CConnman::SetMasternodeQuorumRelayMembers(Consensus::LLMQType llmqType, const uint256& quorumHash, const std::set& proTxHashes) +void CConnman::SetMasternodeQuorumRelayMembers(Consensus::LLMQType llmqType, const uint256& quorumHash, const std::unordered_set& proTxHashes) { { LOCK(cs_vPendingMasternodes); @@ -4657,10 +4656,10 @@ bool CConnman::HasMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint return masternodeQuorumNodes.count(std::make_pair(llmqType, quorumHash)); } -std::set CConnman::GetMasternodeQuorums(Consensus::LLMQType llmqType) const +std::unordered_set CConnman::GetMasternodeQuorums(Consensus::LLMQType llmqType) const { LOCK(cs_vPendingMasternodes); - std::set result; + std::unordered_set result; for (const auto& p : masternodeQuorumNodes) { if (p.first.first != llmqType) { continue; @@ -4670,7 +4669,7 @@ std::set CConnman::GetMasternodeQuorums(Consensus::LLMQType llmqType) c return result; } -std::set CConnman::GetMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash) const +std::unordered_set CConnman::GetMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash) const { LOCK2(m_nodes_mutex, cs_vPendingMasternodes); auto it = masternodeQuorumNodes.find(std::make_pair(llmqType, quorumHash)); @@ -4679,7 +4678,7 @@ std::set CConnman::GetMasternodeQuorumNodes(Consensus::LLMQType llmqType } const auto& proRegTxHashes = it->second; - std::set nodes; + std::unordered_set nodes; for (const auto pnode : m_nodes) { if (pnode->fDisconnect) { continue; diff --git a/src/net.h b/src/net.h index 524f0a244b..4901507a6c 100644 --- a/src/net.h +++ b/src/net.h @@ -1501,12 +1501,12 @@ friend class CNode; EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex, !mutexMsgProc); bool AddPendingMasternode(const uint256& proTxHash); - void SetMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash, const std::set& proTxHashes); - void SetMasternodeQuorumRelayMembers(Consensus::LLMQType llmqType, const uint256& quorumHash, const std::set& proTxHashes); + void SetMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash, const std::unordered_set& proTxHashes); + void SetMasternodeQuorumRelayMembers(Consensus::LLMQType llmqType, const uint256& quorumHash, const std::unordered_set& proTxHashes); bool HasMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash) const; - std::set GetMasternodeQuorums(Consensus::LLMQType llmqType) const; + std::unordered_set GetMasternodeQuorums(Consensus::LLMQType llmqType) const; // also returns QWATCH nodes - std::set GetMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash) const; + std::unordered_set GetMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash) const; void RemoveMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash); bool IsMasternodeQuorumNode(const CNode* pnode, const CDeterministicMNList& tip_mn_list) const; bool IsMasternodeQuorumRelayMember(const uint256& protxHash); @@ -1815,8 +1815,8 @@ friend class CNode; std::vector vPendingMasternodes; mutable RecursiveMutex cs_vPendingMasternodes; - std::map, std::set> masternodeQuorumNodes GUARDED_BY(cs_vPendingMasternodes); - std::map, std::set> masternodeQuorumRelayMembers GUARDED_BY(cs_vPendingMasternodes); + std::map, std::unordered_set> masternodeQuorumNodes GUARDED_BY(cs_vPendingMasternodes); + std::map, std::unordered_set> masternodeQuorumRelayMembers GUARDED_BY(cs_vPendingMasternodes); std::set masternodePendingProbes GUARDED_BY(cs_vPendingMasternodes); mutable Mutex cs_mapSocketToNode; From d3c0059186b00f69868b2a74347ccb0674901667 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 20 Dec 2024 17:58:57 +0000 Subject: [PATCH 03/19] merge bitcoin#22689: deprecate top-level fee fields in getmempool RPCs Also includes formatting changes introduced by bitcoin#23694 --- doc/release-notes-6499.md | 11 +++ src/rpc/blockchain.cpp | 54 +++++++++------ test/functional/feature_asset_locks.py | 2 +- test/functional/mempool_packages.py | 26 +++---- test/functional/rpc_fundrawtransaction.py | 10 +-- ...pc_mempool_entry_fee_fields_deprecation.py | 67 +++++++++++++++++++ test/functional/test_runner.py | 1 + 7 files changed, 129 insertions(+), 42 deletions(-) create mode 100644 doc/release-notes-6499.md create mode 100755 test/functional/rpc_mempool_entry_fee_fields_deprecation.py diff --git a/doc/release-notes-6499.md b/doc/release-notes-6499.md new file mode 100644 index 0000000000..ee00c83b6e --- /dev/null +++ b/doc/release-notes-6499.md @@ -0,0 +1,11 @@ +Updated RPCs +------------ + +- The top-level fee fields `fee`, `modifiedfee`, `ancestorfees` and `descendantfees` + returned by RPCs `getmempoolentry`,`getrawmempool(verbose=true)`, + `getmempoolancestors(verbose=true)` and `getmempooldescendants(verbose=true)` + are deprecated and will be removed in the next major version (use + `-deprecated=fees` if needed in this version). The same fee fields can be accessed + through the `fees` object in the result. WARNING: deprecated + fields `ancestorfees` and `descendantfees` are denominated in sats, whereas all + fields in the `fees` object are denominated in DASH. diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 2eb41cee62..8f15edffa3 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -460,22 +460,29 @@ static RPCHelpMan getdifficulty() static std::vector MempoolEntryDescription() { return { RPCResult{RPCResult::Type::NUM, "vsize", "virtual transaction size. This can be different from actual serialized size for high-sigop transactions."}, - RPCResult{RPCResult::Type::STR_AMOUNT, "fee", /*optional=*/true, "transaction fee in " + CURRENCY_UNIT + " (DEPRECATED)"}, - RPCResult{RPCResult::Type::STR_AMOUNT, "modifiedfee", /*optional=*/true, "transaction fee with fee deltas used for mining priority (DEPRECATED)"}, + RPCResult{RPCResult::Type::STR_AMOUNT, "fee", /*optional=*/true, + "transaction fee, denominated in " + CURRENCY_UNIT + " (DEPRECATED, returned only if config option -deprecatedrpc=fees is passed)"}, + RPCResult{RPCResult::Type::STR_AMOUNT, "modifiedfee", /*optional=*/true, + "transaction fee with fee deltas used for mining priority, denominated in " + CURRENCY_UNIT + + " (DEPRECATED, returned only if config option -deprecatedrpc=fees is passed)"}, RPCResult{RPCResult::Type::NUM_TIME, "time", "local time transaction entered pool in " + UNIX_EPOCH_TIME}, RPCResult{RPCResult::Type::NUM, "height", "block height when transaction entered pool"}, RPCResult{RPCResult::Type::NUM, "descendantcount", "number of in-mempool descendant transactions (including this one)"}, RPCResult{RPCResult::Type::NUM, "descendantsize", "size of in-mempool descendants (including this one)"}, - RPCResult{RPCResult::Type::STR_AMOUNT, "descendantfees", /*optional=*/true, "modified fees (see above) of in-mempool descendants (including this one) (DEPRECATED)"}, + RPCResult{RPCResult::Type::STR_AMOUNT, "descendantfees", /*optional=*/true, + "transaction fees of in-mempool descendants (including this one) with fee deltas used for mining priority, denominated in " + + CURRENCY_ATOM + "s (DEPRECATED, returned only if config option -deprecatedrpc=fees is passed)"}, RPCResult{RPCResult::Type::NUM, "ancestorcount", "number of in-mempool ancestor transactions (including this one)"}, RPCResult{RPCResult::Type::NUM, "ancestorsize", "size of in-mempool ancestors (including this one)"}, - RPCResult{RPCResult::Type::STR_AMOUNT, "ancestorfees", /*optional=*/true, "modified fees (see above) of in-mempool ancestors (including this one) (DEPRECATED)"}, + RPCResult{RPCResult::Type::STR_AMOUNT, "ancestorfees", /*optional=*/true, + "transaction fees of in-mempool ancestors (including this one) with fee deltas used for mining priority, denominated in " + + CURRENCY_ATOM + "s (DEPRECATED, returned only if config option -deprecatedrpc=fees is passed)"}, RPCResult{RPCResult::Type::OBJ, "fees", "", { - RPCResult{RPCResult::Type::STR_AMOUNT, "base", "transaction fee in " + CURRENCY_UNIT}, - RPCResult{RPCResult::Type::STR_AMOUNT, "modified", "transaction fee with fee deltas used for mining priority in " + CURRENCY_UNIT}, - RPCResult{RPCResult::Type::STR_AMOUNT, "ancestor", "transaction fees of in-mempool ancestors (including this one) in " + CURRENCY_UNIT}, - RPCResult{RPCResult::Type::STR_AMOUNT, "descendant", "transaction fees of in-mempool descendants (including this one) in " + CURRENCY_UNIT}, + RPCResult{RPCResult::Type::STR_AMOUNT, "base", "transaction fee, denominated in " + CURRENCY_UNIT}, + RPCResult{RPCResult::Type::STR_AMOUNT, "modified", "transaction fee with fee deltas used for mining priority, denominated in " + CURRENCY_UNIT}, + RPCResult{RPCResult::Type::STR_AMOUNT, "ancestor", "transaction fees of in-mempool ancestors (including this one) with fee deltas used for mining priority, denominated in " + CURRENCY_UNIT}, + RPCResult{RPCResult::Type::STR_AMOUNT, "descendant", "transaction fees of in-mempool descendants (including this one) with fee deltas used for mining priority, denominated in " + CURRENCY_UNIT}, }}, RPCResult{RPCResult::Type::ARR, "depends", "unconfirmed transactions used as inputs for this transaction", {RPCResult{RPCResult::Type::STR_HEX, "transactionid", "parent transaction id"}}}, @@ -489,24 +496,33 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool { AssertLockHeld(pool.cs); - UniValue fees(UniValue::VOBJ); - fees.pushKV("base", ValueFromAmount(e.GetFee())); - fees.pushKV("modified", ValueFromAmount(e.GetModifiedFee())); - fees.pushKV("ancestor", ValueFromAmount(e.GetModFeesWithAncestors())); - fees.pushKV("descendant", ValueFromAmount(e.GetModFeesWithDescendants())); - info.pushKV("fees", fees); - info.pushKV("vsize", (int)e.GetTxSize()); - info.pushKV("fee", ValueFromAmount(e.GetFee())); - info.pushKV("modifiedfee", ValueFromAmount(e.GetModifiedFee())); + // TODO: top-level fee fields are deprecated. deprecated_fee_fields_enabled blocks should be removed in v24 + const bool deprecated_fee_fields_enabled{IsDeprecatedRPCEnabled("fees")}; + if (deprecated_fee_fields_enabled) { + info.pushKV("fee", ValueFromAmount(e.GetFee())); + info.pushKV("modifiedfee", ValueFromAmount(e.GetModifiedFee())); + } info.pushKV("time", count_seconds(e.GetTime())); info.pushKV("height", (int)e.GetHeight()); info.pushKV("descendantcount", e.GetCountWithDescendants()); info.pushKV("descendantsize", e.GetSizeWithDescendants()); - info.pushKV("descendantfees", e.GetModFeesWithDescendants()); + if (deprecated_fee_fields_enabled) { + info.pushKV("descendantfees", e.GetModFeesWithDescendants()); + } info.pushKV("ancestorcount", e.GetCountWithAncestors()); info.pushKV("ancestorsize", e.GetSizeWithAncestors()); - info.pushKV("ancestorfees", e.GetModFeesWithAncestors()); + if (deprecated_fee_fields_enabled) { + info.pushKV("ancestorfees", e.GetModFeesWithAncestors()); + } + + UniValue fees(UniValue::VOBJ); + fees.pushKV("base", ValueFromAmount(e.GetFee())); + fees.pushKV("modified", ValueFromAmount(e.GetModifiedFee())); + fees.pushKV("ancestor", ValueFromAmount(e.GetModFeesWithAncestors())); + fees.pushKV("descendant", ValueFromAmount(e.GetModFeesWithDescendants())); + info.pushKV("fees", fees); + const CTransaction& tx = e.GetTx(); std::set setDepends; for (const CTxIn& txin : tx.vin) diff --git a/test/functional/feature_asset_locks.py b/test/functional/feature_asset_locks.py index a69c217d57..994626d2fa 100755 --- a/test/functional/feature_asset_locks.py +++ b/test/functional/feature_asset_locks.py @@ -365,7 +365,7 @@ def test_asset_unlocks(self, node_wallet, node, pubkey): self.wait_for_sporks_same() txid = self.send_tx(asset_unlock_tx) - assert_equal(node.getmempoolentry(txid)['fee'], Decimal("0.0007")) + assert_equal(node.getmempoolentry(txid)['fees']['base'], Decimal("0.0007")) is_id = node_wallet.sendtoaddress(node_wallet.getnewaddress(), 1) for node in self.nodes: self.wait_for_instantlock(is_id, node) diff --git a/test/functional/mempool_packages.py b/test/functional/mempool_packages.py index 35416aaa22..f5da9e66e9 100755 --- a/test/functional/mempool_packages.py +++ b/test/functional/mempool_packages.py @@ -7,7 +7,6 @@ from decimal import Decimal from test_framework.blocktools import COINBASE_MATURITY -from test_framework.messages import COIN from test_framework.p2p import P2PTxInvStore from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( @@ -73,7 +72,7 @@ def run_test(self): ancestor_vsize = sum([mempool[tx]['vsize'] for tx in mempool]) ancestor_count = MAX_ANCESTORS - ancestor_fees = sum([mempool[tx]['fee'] for tx in mempool]) + ancestor_fees = sum([mempool[tx]['fees']['base'] for tx in mempool]) descendants = [] ancestors = list(chain) @@ -84,11 +83,8 @@ def run_test(self): # Check that the descendant calculations are correct assert_equal(entry['descendantcount'], descendant_count) - descendant_fees += entry['fee'] - assert_equal(entry['modifiedfee'], entry['fee']) - assert_equal(entry['fees']['base'], entry['fee']) - assert_equal(entry['fees']['modified'], entry['modifiedfee']) - assert_equal(entry['descendantfees'], descendant_fees * COIN) + descendant_fees += entry['fees']['base'] + assert_equal(entry['fees']['modified'], entry['fees']['base']) assert_equal(entry['fees']['descendant'], descendant_fees) descendant_vsize += entry['vsize'] assert_equal(entry['descendantsize'], descendant_vsize) @@ -96,10 +92,10 @@ def run_test(self): # Check that ancestor calculations are correct assert_equal(entry['ancestorcount'], ancestor_count) - assert_equal(entry['ancestorfees'], ancestor_fees * COIN) + assert_equal(entry['fees']['ancestor'], ancestor_fees) assert_equal(entry['ancestorsize'], ancestor_vsize) ancestor_vsize -= entry['vsize'] - ancestor_fees -= entry['fee'] + ancestor_fees -= entry['fees']['base'] ancestor_count -= 1 # Check that parent/child list is correct @@ -150,9 +146,8 @@ def run_test(self): ancestor_fees = 0 for x in chain: entry = self.nodes[0].getmempoolentry(x) - ancestor_fees += entry['fee'] + ancestor_fees += entry['fees']['base'] assert_equal(entry['fees']['ancestor'], ancestor_fees + Decimal('0.00001')) - assert_equal(entry['ancestorfees'], ancestor_fees * COIN + 1000) # Undo the prioritisetransaction for later tests self.nodes[0].prioritisetransaction(chain[0], -1000) @@ -164,9 +159,8 @@ def run_test(self): descendant_fees = 0 for x in reversed(chain): entry = self.nodes[0].getmempoolentry(x) - descendant_fees += entry['fee'] + descendant_fees += entry['fees']['base'] assert_equal(entry['fees']['descendant'], descendant_fees + Decimal('0.00001')) - assert_equal(entry['descendantfees'], descendant_fees * COIN + 1000) # Adding one more transaction on to the chain should fail. assert_raises_rpc_error(-26, "too-long-mempool-chain", chain_transaction, self.nodes[0], [txid], [vout], value, fee, 1) @@ -187,11 +181,9 @@ def run_test(self): descendant_fees = 0 for x in reversed(chain): entry = self.nodes[0].getmempoolentry(x) - descendant_fees += entry['fee'] + descendant_fees += entry['fees']['base'] if (x == chain[-1]): - assert_equal(entry['modifiedfee'], entry['fee'] + Decimal("0.00002")) - assert_equal(entry['fees']['modified'], entry['fee'] + Decimal("0.00002")) - assert_equal(entry['descendantfees'], descendant_fees * COIN + 2000) + assert_equal(entry['fees']['modified'], entry['fees']['base'] + Decimal("0.00002")) assert_equal(entry['fees']['descendant'], descendant_fees + Decimal("0.00002")) # Check that node1's mempool is as expected (-> custom ancestor limit) diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 6e14c9f44f..eeac900658 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -368,7 +368,7 @@ def test_fee_p2pkh(self): # Create same transaction over sendtoaddress. txId = self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 11) - signedFee = self.nodes[0].getmempoolentry(txId)['fee'] + signedFee = self.nodes[0].getmempoolentry(txId)['fees']['base'] # Compare fee. feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) @@ -391,7 +391,7 @@ def test_fee_p2pkh_multi_out(self): # Create same transaction over sendtoaddress. txId = self.nodes[0].sendmany("", outputs) - signedFee = self.nodes[0].getmempoolentry(txId)['fee'] + signedFee = self.nodes[0].getmempoolentry(txId)['fees']['base'] # Compare fee. feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) @@ -415,7 +415,7 @@ def test_fee_p2sh(self): # Create same transaction over sendtoaddress. txId = self.nodes[0].sendtoaddress(mSigObj, 11) - signedFee = self.nodes[0].getmempoolentry(txId)['fee'] + signedFee = self.nodes[0].getmempoolentry(txId)['fees']['base'] # Compare fee. feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) @@ -456,7 +456,7 @@ def test_fee_4of5(self): # Create same transaction over sendtoaddress. txId = self.nodes[0].sendtoaddress(mSigObj, 11) - signedFee = self.nodes[0].getmempoolentry(txId)['fee'] + signedFee = self.nodes[0].getmempoolentry(txId)['fees']['base'] # Compare fee. feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) @@ -584,7 +584,7 @@ def test_many_inputs_fee(self): # Create same transaction over sendtoaddress. txId = self.nodes[1].sendmany("", outputs) - signedFee = self.nodes[1].getmempoolentry(txId)['fee'] + signedFee = self.nodes[1].getmempoolentry(txId)['fees']['base'] # Compare fee. feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) diff --git a/test/functional/rpc_mempool_entry_fee_fields_deprecation.py b/test/functional/rpc_mempool_entry_fee_fields_deprecation.py new file mode 100755 index 0000000000..82761ff7c8 --- /dev/null +++ b/test/functional/rpc_mempool_entry_fee_fields_deprecation.py @@ -0,0 +1,67 @@ +#!/usr/bin/env python3 +# Copyright (c) 2021 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Test deprecation of fee fields from top level mempool entry object""" + +from test_framework.blocktools import COIN +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import assert_equal +from test_framework.wallet import MiniWallet + + +def assertions_helper(new_object, deprecated_object, deprecated_fields): + for field in deprecated_fields: + assert field in deprecated_object + assert field not in new_object + + +class MempoolFeeFieldsDeprecationTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 2 + self.extra_args = [[], ["-deprecatedrpc=fees"]] + + def run_test(self): + # we get spendable outputs from the premined chain starting + # at block 76. see BitcoinTestFramework._initialize_chain() for details + self.wallet = MiniWallet(self.nodes[0]) + self.wallet.rescan_utxos() + + # we create the tx on the first node and wait until it syncs to node_deprecated + # thus, any differences must be coming from getmempoolentry or getrawmempool + tx = self.wallet.send_self_transfer(from_node=self.nodes[0]) + self.nodes[1].sendrawtransaction(tx["hex"]) + + deprecated_fields = ["ancestorfees", "descendantfees", "modifiedfee", "fee"] + self.test_getmempoolentry(tx["txid"], deprecated_fields) + self.test_getrawmempool(tx["txid"], deprecated_fields) + self.test_deprecated_fields_match(tx["txid"]) + + def test_getmempoolentry(self, txid, deprecated_fields): + + self.log.info("Test getmempoolentry rpc") + entry = self.nodes[0].getmempoolentry(txid) + deprecated_entry = self.nodes[1].getmempoolentry(txid) + assertions_helper(entry, deprecated_entry, deprecated_fields) + + def test_getrawmempool(self, txid, deprecated_fields): + + self.log.info("Test getrawmempool rpc") + entry = self.nodes[0].getrawmempool(verbose=True)[txid] + deprecated_entry = self.nodes[1].getrawmempool(verbose=True)[txid] + assertions_helper(entry, deprecated_entry, deprecated_fields) + + def test_deprecated_fields_match(self, txid): + + self.log.info("Test deprecated fee fields match new fees object") + entry = self.nodes[0].getmempoolentry(txid) + deprecated_entry = self.nodes[1].getmempoolentry(txid) + + assert_equal(deprecated_entry["fee"], entry["fees"]["base"]) + assert_equal(deprecated_entry["modifiedfee"], entry["fees"]["modified"]) + assert_equal(deprecated_entry["descendantfees"], entry["fees"]["descendant"] * COIN) + assert_equal(deprecated_entry["ancestorfees"], entry["fees"]["ancestor"] * COIN) + + +if __name__ == "__main__": + MempoolFeeFieldsDeprecationTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index e1c2b7680a..51d115bdb6 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -379,6 +379,7 @@ 'rpc_getdescriptorinfo.py', 'rpc_addresses_deprecation.py', 'rpc_getpeerinfo_deprecation.py', + 'rpc_mempool_entry_fee_fields_deprecation.py', 'rpc_help.py', 'feature_dirsymlinks.py', 'feature_help.py', From 8392d2317fda466a32cc67de509d7da70a18dc98 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 20 Dec 2024 09:31:26 +0000 Subject: [PATCH 04/19] merge bitcoin#23381: refactoring for package submission changes involving RBF have been excluded as RBF isn't implemented in Dash Core excluded: - 8fa2936b34fda9c0bea963311fa80a04b4bf5867 - 3d3e4598b6e570b1f8248b1ee43ec59165a3ff5c --- src/Makefile.test.include | 1 + src/rpc/rawtransaction.cpp | 2 +- src/test/txpackage_tests.cpp | 121 ++++++++++++++++++++++++++++ src/test/txvalidation_tests.cpp | 95 ---------------------- src/validation.cpp | 134 +++++++++++++++++++++----------- src/validation.h | 10 ++- 6 files changed, 219 insertions(+), 144 deletions(-) create mode 100644 src/test/txpackage_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 7583a616b9..c859010acf 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -174,6 +174,7 @@ BITCOIN_TESTS =\ test/torcontrol_tests.cpp \ test/transaction_tests.cpp \ test/txindex_tests.cpp \ + test/txpackage_tests.cpp \ test/txreconciliation_tests.cpp \ test/txvalidation_tests.cpp \ test/txvalidationcache_tests.cpp \ diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 261296e952..1a1cb2ba9a 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1282,7 +1282,7 @@ static RPCHelpMan testmempoolaccept() if (tx_result.m_result_type == MempoolAcceptResult::ResultType::VALID) { const CAmount fee = tx_result.m_base_fees.value(); // Check that fee does not exceed maximum fee - const int64_t virtual_size = GetVirtualTransactionSize(*tx); + const int64_t virtual_size = tx_result.m_vsize.value(); const CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size); if (max_raw_tx_fee && fee > max_raw_tx_fee) { result_inner.pushKV("allowed", false); diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp new file mode 100644 index 0000000000..64a9f67242 --- /dev/null +++ b/src/test/txpackage_tests.cpp @@ -0,0 +1,121 @@ +// Copyright (c) 2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include +#include