backport: merge bitcoin#22689, #23381, #22674, #23804, #24310, #24537, #24582, #24404, #25013, #25029, #25254, #25388 (mempool backports) #6499
Conversation
WalkthroughThis pull request introduces significant changes to the Dash Core codebase, focusing on transaction package management, mempool operations, and RPC command restructuring. The modifications include the addition of new documentation about transaction relay policies, implementation of package acceptance rules for transactions, and comprehensive updates to RPC commands related to mempool interactions. Key areas of change include the introduction of new methods for handling transaction packages, refactoring of mempool acceptance logic, and the deprecation of certain fee-related fields in RPC responses. The changes enhance the robustness of transaction validation, provide more detailed fee information, and improve the overall transaction processing capabilities of the Dash Core implementation. The pull request also includes extensive test coverage for the new functionality, with new test cases specifically designed to validate the package transaction validation, mempool entry fee field changes, and RPC command behaviors. These modifications represent a significant evolution in the transaction processing and mempool management strategies of the Dash Core software. Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| RPCHelpMan getmempoolancestors() | ||
| { | ||
| return RPCHelpMan{"getmempoolancestors", | ||
| "\nIf txid is in the mempool, returns all in-mempool ancestors.\n", | ||
| { | ||
| {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id (must be in mempool)"}, | ||
| {"verbose", RPCArg::Type::BOOL, RPCArg::Default{false}, "True for a json object, false for array of transaction ids"}, | ||
| }, | ||
| { | ||
| RPCResult{"for verbose = false", | ||
| RPCResult::Type::ARR, "", "", | ||
| {{RPCResult::Type::STR_HEX, "", "The transaction id of an in-mempool ancestor transaction"}}}, | ||
| RPCResult{"for verbose = true", | ||
| RPCResult::Type::OBJ_DYN, "", "", | ||
| { | ||
| {RPCResult::Type::OBJ, "transactionid", "", MempoolEntryDescription()}, | ||
| }}, | ||
| }, | ||
| RPCExamples{ | ||
| HelpExampleCli("getmempoolancestors", "\"mytxid\"") | ||
| + HelpExampleRpc("getmempoolancestors", "\"mytxid\"") | ||
| }, | ||
| [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue | ||
| { | ||
| bool fVerbose = false; | ||
| if (!request.params[1].isNull()) | ||
| fVerbose = request.params[1].get_bool(); | ||
|
|
||
| uint256 hash(ParseHashV(request.params[0], "parameter 1")); | ||
|
|
||
| const NodeContext& node = EnsureAnyNodeContext(request.context); | ||
|
|
||
| const CTxMemPool& mempool = EnsureMemPool(node); | ||
| LOCK(mempool.cs); | ||
|
|
||
| CTxMemPool::txiter it = mempool.mapTx.find(hash); | ||
| if (it == mempool.mapTx.end()) { | ||
| throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction not in mempool"); | ||
| } | ||
|
|
||
| CTxMemPool::setEntries setAncestors; | ||
| uint64_t noLimit = std::numeric_limits<uint64_t>::max(); | ||
| std::string dummy; | ||
| mempool.CalculateMemPoolAncestors(*it, setAncestors, noLimit, noLimit, noLimit, noLimit, dummy, false); | ||
|
|
||
| if (!fVerbose) { | ||
| UniValue o(UniValue::VARR); | ||
| for (CTxMemPool::txiter ancestorIt : setAncestors) { | ||
| o.push_back(ancestorIt->GetTx().GetHash().ToString()); | ||
| } | ||
| return o; | ||
| } else { | ||
| UniValue o(UniValue::VOBJ); | ||
| const LLMQContext& llmq_ctx = EnsureLLMQContext(node); | ||
| for (CTxMemPool::txiter ancestorIt : setAncestors) { | ||
| const CTxMemPoolEntry &e = *ancestorIt; | ||
| const uint256& _hash = e.GetTx().GetHash(); | ||
| UniValue info(UniValue::VOBJ); | ||
| entryToJSON(mempool, info, e, llmq_ctx.isman); | ||
| o.pushKV(_hash.ToString(), info); | ||
| } | ||
| return o; | ||
| } | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check for ancestor-limit edge cases.
When calling CalculateMemPoolAncestors, ensure the method handles near-max-ancestor scenarios gracefully. A thorough test verifying behavior around boundary conditions is advisable.
| RPCHelpMan savemempool() | ||
| { | ||
| return RPCHelpMan{"savemempool", | ||
| "\nDumps the mempool to disk. It will fail until the previous dump is fully loaded.\n", | ||
| {}, | ||
| RPCResult{ | ||
| RPCResult::Type::OBJ, "", "", | ||
| { | ||
| {RPCResult::Type::STR, "filename", "the directory and file where the mempool was saved"}, | ||
| }}, | ||
| RPCExamples{ | ||
| HelpExampleCli("savemempool", "") | ||
| + HelpExampleRpc("savemempool", "") | ||
| }, | ||
| [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue | ||
| { | ||
| const ArgsManager& args{EnsureAnyArgsman(request.context)}; | ||
| const CTxMemPool& mempool = EnsureAnyMemPool(request.context); | ||
|
|
||
| if (!mempool.IsLoaded()) { | ||
| throw JSONRPCError(RPC_MISC_ERROR, "The mempool was not loaded yet"); | ||
| } | ||
|
|
||
| if (!DumpMempool(mempool)) { | ||
| throw JSONRPCError(RPC_MISC_ERROR, "Unable to dump mempool to disk"); | ||
| } | ||
|
|
||
| UniValue ret(UniValue::VOBJ); | ||
| ret.pushKV("filename", fs::path((args.GetDataDirNet() / "mempool.dat")).u8string()); | ||
|
|
||
| return ret; | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle race condition on shutdown.
savemempool writes the mempool to disk. If the node is shutting down or reloading, carefully ensure that the file is not corrupted. Possibly handle partial writes more robustly.
| /** A fee rate smaller than this is considered zero fee (for relaying, mining and transaction creation) */ | ||
| extern CFeeRate minRelayTxFee; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Declare minRelayTxFee as an external variable
Introducing minRelayTxFee helps unify zero-fee behaviors across relaying, mining, and transaction creation. Confirm it remains in sync with other fee rate changes (e.g., dustRelayFee, incrementalRelayFee) and that any future configuration changes apply consistently.
Additional Information
bitcoin#23694 (backported in dash#6256) was backported before bitcoin#22689 and therefore, the formatting changes in the former were not incorporated in the backport. This has been resolved by accommodating them in the latter.
The fields deprecated in bitcoin#22689 were originally deprecated in bitcoin#12240 but not gated behind runtime arguments, the backport does so.
This pull request includes multiple backports that include code specific to RBF (BIP 125) and SegWit, which have been removed due to Dash Core not supporting them, comments and documentation (like
doc/policy/packages.md) have been amended to reflect this accordingly.When backporting bitcoin#22674, a new constructor was introduced for submitting
MEMPOOL_ENTRYResultType, upstream this can be done trivially as it uses function overloading (source) but as Dash Core doesn't acceptreplaced_txns, there is no opportunity to differentiate arguments.ResultTypeas an argument but always passingMEMPOOL_ENTRYas the argument (source)Unit tests like
package_witness_swap_tests(introduced in bitcoin#23804) have been omitted due to functionality tested not being implemented in Dash Core. Subsequent backports that include modifications to these tests have mirrored similar omissions.Breaking Changes
The top-level fee fields
fee,modifiedfee,ancestorfeesanddescendantfeesreturned by RPCsgetmempoolentry,getrawmempool(verbose=true),getmempoolancestors(verbose=true)andgetmempooldescendants(verbose=true)are deprecated and will be removed in the next major version (use-deprecated=feesif needed in this version). The same fee fields can be accessed through thefeesobject in the result.WARNING: deprecated fields
ancestorfeesanddescendantfeesare denominated in sats, whereas all fields in thefeesobject are denominated in DASH.Checklist