backport: bitcoin#25222, #25648, #25651, #25772, #25905, #25951, #25971, #26103, #26250 (kernel 3) - #7525
backport: bitcoin#25222, #25648, #25651, #25772, #25905, #25951, #25971, #26103, #26250 (kernel 3)#7525knst wants to merge 10 commits into
Conversation
…gnerImpl members public, rm temporaries, simplify 4bedfd7 refactor: remove unneeded temporaries in node/interfaces, simplify code (Jon Atack) b27ba16 refactor: make all NodeImpl/ChainImpl/ExternalSignerImpl members public (Jon Atack) Pull request description: - Make all `NodeImpl`, `ChainImpl` and `ExternalSignerImpl` class members `public` (and document why), to be consistent in all the `*Impl` classes in `src/node/interfaces.cpp` and `src/wallet/interfaces.cpp` and to help future reviewers and contributors. - Remove unneeded temporaries in `NodeImpl` and `ChainImpl` methods in `src/node/interfaces.cpp` and simplify, to make the code easier to read and understand and to improve performance by avoiding unnecessary move operations. ACKs for top commit: ryanofsky: Code review ACK 4bedfd7. Changes since last review, applying suggested style & simplifiying first commit. Also avoiding another lock in second commit. Tree-SHA512: 112f7cad5e2838c94c5b79d61328f42fe75fdb97f401ab49eccf696fc2c6a8a0c0ee55ec974c0602acf7423f78bb82e90eb8a0cc531e1d3347f73b7c83685504
WalkthroughThe change centralizes mempool limits and relay-policy settings in Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ArgsManager
participant NodeInit
participant MemPool
participant Validation
ArgsManager->>NodeInit: provide mempool arguments
NodeInit->>MemPool: apply limits and policy settings
MemPool->>Validation: expose configured policy and limits
Validation->>MemPool: accept transactions using instance state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rpc/net.cpp`:
- Around line 720-724: Update the getnetworkinfo result schema to mark the
relayfee and incrementalfee entries optional, matching the conditional emission
in the node.mempool block. Keep the existing field values and conditional
serialization unchanged.
In `@src/test/fuzz/tx_pool.cpp`:
- Around line 120-128: The fuzzed mempool constraint updates currently modify
only ArgsManager after MakeMempool constructs tx_pool, so the live pool never
receives them. Remove the later SetMempoolConstraints calls in the affected fuzz
branches, or add a test-only mechanism that reapplies the updated options
directly to tx_pool while preserving the existing fuzzed limits and expiry
behavior.
In `@src/util/thread.h`:
- Around line 9-15: Update the header containing the TraceThread declaration to
include the standard <string_view> header directly, alongside the existing
includes, so its std::string_view parameter is self-contained and does not
depend on transitive includes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 29690af4-b7d1-4e38-b565-c32ed7b15233
📒 Files selected for processing (72)
ci/dash/lint-tidy.shsrc/Makefile.amsrc/Makefile.test_fuzz.includesrc/index/addressindex.cppsrc/index/addressindex.hsrc/index/base.cppsrc/index/base.hsrc/index/blockfilterindex.cppsrc/index/blockfilterindex.hsrc/index/coinstatsindex.cppsrc/index/coinstatsindex.hsrc/index/spentindex.cppsrc/index/spentindex.hsrc/index/timestampindex.cppsrc/index/timestampindex.hsrc/index/txindex.cppsrc/index/txindex.hsrc/init.cppsrc/kernel/mempool_limits.hsrc/kernel/mempool_options.hsrc/mempool_args.cppsrc/node/chainstate.cppsrc/node/interfaces.cppsrc/node/mempool_args.cppsrc/node/mempool_args.hsrc/node/miner.cppsrc/node/psbt.cppsrc/policy/policy.cppsrc/policy/policy.hsrc/policy/settings.cppsrc/policy/settings.hsrc/qt/guiutil.cppsrc/rpc/blockchain.cppsrc/rpc/fees.cppsrc/rpc/mempool.cppsrc/rpc/net.cppsrc/script/standard.cppsrc/script/standard.hsrc/test/coinstatsindex_tests.cppsrc/test/evo_assetlocks_tests.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/fuzz/integer.cppsrc/test/fuzz/key.cppsrc/test/fuzz/partially_downloaded_block.cppsrc/test/fuzz/policy_estimator.cppsrc/test/fuzz/script.cppsrc/test/fuzz/transaction.cppsrc/test/fuzz/tx_pool.cppsrc/test/fuzz/util.cppsrc/test/fuzz/util.hsrc/test/fuzz/util/mempool.cppsrc/test/fuzz/util/mempool.hsrc/test/fuzz/validation_load_mempool.cppsrc/test/mempool_tests.cppsrc/test/multisig_tests.cppsrc/test/script_p2sh_tests.cppsrc/test/transaction_tests.cppsrc/test/txpackage_tests.cppsrc/test/util/setup_common.cppsrc/test/util/txmempool.cppsrc/txmempool.cppsrc/txmempool.hsrc/util/system.cppsrc/util/system.hsrc/util/thread.cppsrc/util/thread.hsrc/validation.cppsrc/validation.hsrc/wallet/fees.cppsrc/wallet/interfaces.cppsrc/wallet/test/coinjoin_tests.cppsrc/wallet/test/wallet_tests.cpp
💤 Files with no reviewable changes (12)
- src/script/standard.cpp
- src/wallet/test/coinjoin_tests.cpp
- src/policy/settings.h
- src/test/fuzz/util.cpp
- src/index/blockfilterindex.h
- src/index/addressindex.h
- src/wallet/test/wallet_tests.cpp
- src/index/spentindex.h
- src/index/timestampindex.h
- src/index/txindex.h
- src/mempool_args.cpp
- src/test/fuzz/util.h
| if (node.mempool) { | ||
| // Those fields can be deprecated, to be replaced by the getmempoolinfo fields | ||
| obj.pushKV("relayfee", ValueFromAmount(node.mempool->m_min_relay_feerate.GetFeePerK())); | ||
| obj.pushKV("incrementalfee", ValueFromAmount(node.mempool->m_incremental_relay_feerate.GetFeePerK())); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Mark the conditional RPC fields as optional.
When node.mempool is null, this code omits relayfee and incrementalfee. The getnetworkinfo result schema still declares both fields as required. Mark both result entries as optional.
Proposed fix
- {RPCResult::Type::NUM, "relayfee", "minimum relay fee for transactions in " + CURRENCY_UNIT + "/kB"},
- {RPCResult::Type::NUM, "incrementalfee", "minimum fee increment for mempool limiting in " + CURRENCY_UNIT + "/kB"},
+ {RPCResult::Type::NUM, "relayfee", /*optional=*/true, "minimum relay fee for transactions in " + CURRENCY_UNIT + "/kB"},
+ {RPCResult::Type::NUM, "incrementalfee", /*optional=*/true, "minimum fee increment for mempool limiting in " + CURRENCY_UNIT + "/kB"},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/rpc/net.cpp` around lines 720 - 724, Update the getnetworkinfo result
schema to mark the relayfee and incrementalfee entries optional, matching the
conditional emission in the node.mempool block. Keep the existing field values
and conditional serialization unchanged.
| #include <string> | ||
|
|
||
| namespace util { | ||
| /** | ||
| * A wrapper for do-something-once thread functions. | ||
| */ | ||
| void TraceThread(const char* thread_name, std::function<void()> thread_func); | ||
| void TraceThread(std::string_view thread_name, std::function<void()> thread_func); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if ! rg -n '^\s*`#include`\s*<string_view>' src/util/thread.h; then
echo "src/util/thread.h is missing a direct <string_view> include"
exit 1
fiRepository: dashpay/dash
Length of output: 210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "File slice:"
sed -n '1,40p' src/util/thread.h | cat -n
echo
echo "Header include status:"
if grep -qE '^\s*`#include`\s*<string_view>' src/util/thread.h; then
echo "src/util/thread.h directly includes <string_view>"
else
echo "src/util/thread.h does not directly include <string_view>"
fi
echo
echo "All includes in src/util/thread.h:"
grep -nE '^\s*`#include`\s*<' src/util/thread.h || true
echo
echo "TraceThread declarations/usages:"
rg -n "TraceThread|std::string_view|thread_name" src/util/thread.h src -g '*.h' -g '*.cpp' | head -80Repository: dashpay/dash
Length of output: 10846
Include <string_view> directly.
src/util/thread.h declares std::string_view but does not include <string_view>. Add the standard header so callers do not rely on transitive includes.
Proposed fix
`#include` <string>
+#include <string_view>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <string> | |
| namespace util { | |
| /** | |
| * A wrapper for do-something-once thread functions. | |
| */ | |
| void TraceThread(const char* thread_name, std::function<void()> thread_func); | |
| void TraceThread(std::string_view thread_name, std::function<void()> thread_func); | |
| `#include` <string> | |
| `#include` <string_view> | |
| namespace util { | |
| /** | |
| * A wrapper for do-something-once thread functions. | |
| */ | |
| void TraceThread(std::string_view thread_name, std::function<void()> thread_func); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/util/thread.h` around lines 9 - 15, Update the header containing the
TraceThread declaration to include the standard <string_view> header directly,
alongside the existing includes, so its std::string_view parameter is
self-contained and does not depend on transitive includes.
BACKPORT NOTE: Moving the -acceptnonstdtxn check out of AppInitParameterInteraction and into ApplyArgsManOptions() means it now runs from AppInitMain, after StartLogging() has already opened the log file. The mainnet node that feature_config_args.py starts to trigger that error therefore leaves a debug.log behind, and a mainnet datadir has no chain subdirectory to name. A chain directory always sits directly under the datadir, so match exactly one level instead. Both nested cases disappear and the escape hatch is no longer needed. This restores the glob depth bitcoin#16973 changed while keeping what that PR actually fixed: pathlib plus as_posix(), so the regex still matches on Windows separators. ----- ddddd69 sort after scripted-diff (MacroFake) fac812c scripted-diff: Move mempool_args to src/node (MacroFake) 6666438 Remove ::g_max_datacarrier_bytes global (MacroFake) fad0b4f Pass datacarrier setting into IsStandard (MacroFake) fa2a6b8 Combine datacarrier globals into one (MacroFake) fa477d3 Remove ::GetVirtualTransactionSize() alias (MacroFake) fa2f6c1 Remove ::fIsBareMultisigStd global (MacroFake) fadc14e Remove ::dustRelayFee (MacroFake) fa8a7f0 Remove ::IsStandardTx(tx, reason) alias (MacroFake) fa7a911 test: Remove unused cs_main (MacroFake) fa9cba7 Remove ::incrementalRelayFee and ::minRelayTxFee globals (MacroFake) fa14860 Remove ::fRequireStandard global (MacroFake) fa468bd Return optional error from ApplyArgsManOptions (MacroFake) Pull request description: This change is good because: * It moves module-specific init-logic out of the bloated init.cpp * It removes a global from validation.cpp and places it into the data structure that needs it (mempool) ACKs for top commit: glozow: re ACK ddddd69 ryanofsky: Code review ACK ddddd69 ariard: Light Code Review ACK ddddd69 Tree-SHA512: 9de2ce601cfcaa4dfd7d1c92270568895ce8702ccdffb59829fbe9618eab0fd88d738afef33ed66988c66861115e0340e881056bfb71e2aed4af2440bd37eb1e
fad5bc4 test: Add missing static to IsStandardTx helper (MacroFake) Pull request description: Requested in bitcoin#25648 (comment) Also remove line break from the other two helpers. ACKs for top commit: glozow: utACK fad5bc4 aureleoules: ACK fad5bc4. theStack: ACK fad5bc4 Tree-SHA512: 771411e1fb5939a58491ecf719e1929ab0150b0faae2078ac72bd13117f1d4dcffdeed5027bfae53e4336af25a4f1db47d564abc06a5a2c9ec006a9f67bae104
…options struct 7bc33a8 refactor: Move ChainstateManager options into m_options struct (Ryan Ofsky) Pull request description: Move `ChainstateManager` options into `m_options` struct to simplify class initialization, organize class members, and to name external option variables differently than internal state variables. This change was originally in bitcoin#25862, but it was suggested to split off in bitcoin#25862 (comment) so it could be merged earlier and reduce conflicts with other PRs. ACKs for top commit: naumenkogs: ACK 7bc33a8 Tree-SHA512: 1c3c77be7db60222732221c087fd01cb802b84ac93333fccb38c8d16645f5f950c3362981021e7a3ae054f19fa7dd9e1cd15daaa101b61ca8853e42a1fd21474
…state() fa4c59d Move blockstorage option logging to LoadChainstate() (MacroFake) fa3358b Move validation option logging to LoadChainstate() (MacroFake) Pull request description: This would allow libbitcoinkernel users to see the options logged as well. Currently they would only be logged for bitcoind. Behavior change suggested in the refactoring pull bitcoin#25704 (comment) ACKs for top commit: ryanofsky: Code review ACK fa4c59d. Only change since last review is moving pruning logprints out of `AppInitParameterInteraction` as suggested jonatack: Review ACK fa4c59d Tree-SHA512: f27508ca06a78ef162f002d556cf830df374fe95fd4f10bf22c24b6b48276ce49f52f82ffedc43596c872ddcf08321ca03651495fd3abde16254cb8afab39d33
faa3d38 refactor: Pass reference to LookUpStats (MacroFake) Pull request description: I find it confusing to have an interface that accepts nullptr, but immediately crashes the program when someone does pass nullptr. Fix that. Also some include fixups. ACKs for top commit: aureleoules: ACK faa3d38 Tree-SHA512: f90b649e9991e137b83a9899258ee73605719c081a6b789ac27fe7fe73eb70fbb41d89479bcd536d5c3ad788a5795de8451bc1b94e5c9267dcf9636d9e4a1109
…ames 26cf9ea scripted-diff: rename pszThread to thread_name (stickies-v) 200d84d refactor: use std::string for index names (stickies-v) 97f5b20 refactor: use std::string for thread names (stickies-v) Pull request description: As a follow-up to bitcoin#25967 (comment), this PR changes the return type of [`BaseIndex::GetName()`](https://github.com/bitcoin/bitcoin/blob/fa5c224d444802dabec5841009e029b9754c92f1/src/index/base.h#L120) to `const std::string&` instead of `const char*`. The first commit is not essential for this change, but since the code is touched and index names are commonly used to specify thread names, I've made the same update there. No behaviour change, just refactoring to further phase out C-style strings. Note: `util::ThreadRename()` used to take an rvalue ref, but since it then passes this to `SetInternalName()` by value, I don't think there's any benefit to having both an rvalue and lvalue ref function so I just changed it into lvalue ref. Not 100% sure I'm missing something? ACKs for top commit: MarcoFalke: review ACK 26cf9ea only change is new scripted-diff 😀 hebasto: ACK 26cf9ea, I have reviewed the code and it looks OK. w0xlt: reACK bitcoin@26cf9ea Tree-SHA512: 44a03ebf2bb86ca1411a36222a575217cdba8ee3a3c985e74d74c934516f002b27336147fa22f59eda7dac21204a93951563317005d475da95b23c427014d77b
8a6b6df fuzz: pass max fee into ConsumeTxMemPoolEntry (fanquake) eb15569 fuzz: add util/mempool/h.cpp (fanquake) Pull request description: Moving the heavy (Boost) mempool code out of fuzz/util.h. Means that (for ex) a crypto_common fuzz unit doesn't need to care about seeing endless Boost headers. This results in a ~10% speedup (for me) when compiling the fuzz tests. Your results may vary. ACKs for top commit: MarcoFalke: review ACK 8a6b6df 🍮 Tree-SHA512: 27dc9d9581ac0b1b319cc0dc08fe5f8fbf9269386a5cb23f6fd5d8231bf015ed942ab4414d8001220541be0013756354578ddab1fec607c6fba04daf421bc870
33b12e5 docs: improve docs where MemPoolLimits is used (stickies-v) 6945853 test: use NoLimits() in MempoolIndexingTest (stickies-v) 3a86f24 refactor: mempool: use CTxMempool::Limits (stickies-v) b85af25 refactor: mempool: add MemPoolLimits::NoLimits() (stickies-v) Pull request description: Mempool currently considers 4 limits regarding ancestor and descendant count and size, which get passed around between functions quite a bit. This PR uses `CTxMemPool::Limits` introduced in bitcoin#25290 to simplify those signatures and callsites. The purpose of this PR is to improve readability and maintenance, without behaviour change. As noted in the first commit "refactor: mempool: change MemPoolLimits members to uint", we currently have an underflow issue where a user could pass a negative `-limitancestorsize`, which is eventually cast to an unsigned integer. This behaviour already exists. Because it's orthogonal and to minimize scope, I think this should be fixed in a separate PR. ACKs for top commit: hebasto: ACK 33b12e5, I have reviewed the code and it looks OK, I agree it can be merged. glozow: reACK 33b12e5 Tree-SHA512: 591c6dcee1894f1c3ca28b34a680eeadcf0d40cda92451b4a422c03087b27d682b5e30ba4367abd75a99b5ccb115b7884b0026958d3c7dddab030549db5a4056
|
🔍 Review in progress — actively reviewing now (commit 4bab3fe) |
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/node/mempool_args.cpp`:
- Around line 32-36: Validate parsed values for -limitancestorsize,
-limitdescendantcount, -limitdescendantsize, and -maxmempool before scaling or
assigning them to mempool_limits. Reject negative or boundary-saturated results
before multiplying size values by 1'000, and ensure a saturated negative
-maxmempool value is not accepted as a valid mempool size.
In `@test/functional/combine_logs.py`:
- Around line 83-88: Update the log-path discovery logic around the existing
node0 glob to detect both layouts: direct mainnet logs at node{i}/debug.log and
nested logs at node{i}/<chain>/debug.log. Preserve the single-match validation,
select the corresponding path pattern so mainnet logs are included instead of
falling back to regtest, and add regression coverage for both layouts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 93b899c1-b306-447e-892c-ecbe48f73e01
📒 Files selected for processing (72)
ci/dash/lint-tidy.shsrc/Makefile.amsrc/Makefile.test_fuzz.includesrc/index/addressindex.cppsrc/index/addressindex.hsrc/index/base.cppsrc/index/base.hsrc/index/blockfilterindex.cppsrc/index/blockfilterindex.hsrc/index/coinstatsindex.cppsrc/index/coinstatsindex.hsrc/index/spentindex.cppsrc/index/spentindex.hsrc/index/timestampindex.cppsrc/index/timestampindex.hsrc/index/txindex.cppsrc/index/txindex.hsrc/init.cppsrc/kernel/mempool_limits.hsrc/kernel/mempool_options.hsrc/mempool_args.cppsrc/node/chainstate.cppsrc/node/interfaces.cppsrc/node/mempool_args.cppsrc/node/mempool_args.hsrc/node/miner.cppsrc/node/psbt.cppsrc/policy/policy.cppsrc/policy/policy.hsrc/policy/settings.cppsrc/policy/settings.hsrc/qt/guiutil.cppsrc/rpc/blockchain.cppsrc/rpc/fees.cppsrc/rpc/mempool.cppsrc/rpc/net.cppsrc/script/standard.cppsrc/script/standard.hsrc/test/coinstatsindex_tests.cppsrc/test/evo_assetlocks_tests.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/fuzz/integer.cppsrc/test/fuzz/key.cppsrc/test/fuzz/partially_downloaded_block.cppsrc/test/fuzz/policy_estimator.cppsrc/test/fuzz/script.cppsrc/test/fuzz/transaction.cppsrc/test/fuzz/tx_pool.cppsrc/test/fuzz/util.cppsrc/test/fuzz/util.hsrc/test/fuzz/util/mempool.cppsrc/test/fuzz/util/mempool.hsrc/test/fuzz/validation_load_mempool.cppsrc/test/mempool_tests.cppsrc/test/multisig_tests.cppsrc/test/script_p2sh_tests.cppsrc/test/transaction_tests.cppsrc/test/txpackage_tests.cppsrc/test/util/setup_common.cppsrc/test/util/txmempool.cppsrc/txmempool.cppsrc/txmempool.hsrc/util/system.cppsrc/util/system.hsrc/util/thread.cppsrc/util/thread.hsrc/validation.cppsrc/validation.hsrc/wallet/fees.cppsrc/wallet/test/coinjoin_tests.cppsrc/wallet/test/wallet_tests.cpptest/functional/combine_logs.py
💤 Files with no reviewable changes (12)
- src/index/txindex.h
- src/test/fuzz/util.h
- src/index/addressindex.h
- src/wallet/test/wallet_tests.cpp
- src/index/timestampindex.h
- src/mempool_args.cpp
- src/index/spentindex.h
- src/script/standard.cpp
- src/index/blockfilterindex.h
- src/policy/settings.h
- src/wallet/test/coinjoin_tests.cpp
- src/test/fuzz/util.cpp
🚧 Files skipped from review as they are similar to previous changes (53)
- src/index/blockfilterindex.cpp
- src/wallet/fees.cpp
- src/rpc/net.cpp
- src/test/transaction_tests.cpp
- src/util/system.h
- src/qt/guiutil.cpp
- src/test/evo_deterministicmns_tests.cpp
- src/test/fuzz/validation_load_mempool.cpp
- src/Makefile.test_fuzz.include
- src/test/mempool_tests.cpp
- src/rpc/mempool.cpp
- ci/dash/lint-tidy.sh
- src/test/util/setup_common.cpp
- src/test/multisig_tests.cpp
- src/node/psbt.cpp
- src/util/system.cpp
- src/index/timestampindex.cpp
- src/index/spentindex.cpp
- src/index/coinstatsindex.h
- src/index/addressindex.cpp
- src/util/thread.cpp
- src/index/txindex.cpp
- src/test/txpackage_tests.cpp
- src/kernel/mempool_options.h
- src/node/chainstate.cpp
- src/policy/settings.cpp
- src/policy/policy.h
- src/kernel/mempool_limits.h
- src/rpc/blockchain.cpp
- src/util/thread.h
- src/node/miner.cpp
- src/test/fuzz/script.cpp
- src/test/fuzz/key.cpp
- src/Makefile.am
- src/test/util/txmempool.cpp
- src/policy/policy.cpp
- src/script/standard.h
- src/index/base.h
- src/rpc/fees.cpp
- src/test/fuzz/partially_downloaded_block.cpp
- src/test/fuzz/policy_estimator.cpp
- src/index/coinstatsindex.cpp
- src/test/fuzz/integer.cpp
- src/init.cpp
- src/node/mempool_args.h
- src/txmempool.h
- src/test/fuzz/util/mempool.h
- src/test/fuzz/transaction.cpp
- src/test/coinstatsindex_tests.cpp
- src/txmempool.cpp
- src/node/interfaces.cpp
- src/validation.cpp
- src/validation.h
| if (auto vkb = argsman.GetIntArg("-limitancestorsize")) mempool_limits.ancestor_size_vbytes = *vkb * 1'000; | ||
|
|
||
| mempool_limits.descendant_count = argsman.GetIntArg("-limitdescendantcount", mempool_limits.descendant_count); | ||
|
|
||
| if (auto vkb = argsman.GetIntArg("-limitdescendantsize")) mempool_limits.descendant_size_vbytes = *vkb * 1'000; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'GetIntArg|max_size_bytes|ancestor_size_vbytes|descendant_size_vbytes' \
src/util/system.h src/util/system.cpp \
src/kernel/mempool_limits.h src/kernel/mempool_options.hRepository: dashpay/dash
Length of output: 4488
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Application of affected mempool_args.cpp =="
file=$(fd -a 'mempool_args\.cpp$' . | head -n 1)
echo "$file"
if [ -n "$file" ]; then
wc -l "$file"
sed -n '1,90p' "$file" | cat -n
fi
echo
echo "== System parser references =="
sed -n '500,570p' src/util/system.cpp | cat -n
rg -n -C 4 'ParseInt|SettingToInt|GetSetting|SetArg' src/util/system.cpp src/common/ | head -n 120
echo
echo "== mempool options construction callers =="
rg -n -C 5 'ApplyArgsManOptions|mempool_opts|max_size_bytes|ancestor_size_vbytes|descendant_size_vbytes' src node test -g '*.cpp' -g '*.h' | head -n 200Repository: dashpay/dash
Length of output: 28118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== LocaleIndependentAtoi implementation =="
rg -n -C 8 'LocaleIndependentAtoi' src util include | head -n 200
echo
echo "== SettingsValue integer/string conversion definitions =="
rg -n -C 8 'class SettingsValue|struct SettingsValue|SettingValue|isNum|get_str|getInt|int64_t|ParseInt' src/util include -g '*.h' -g '*.cpp' | head -n 240
echo
echo "== CTxMemPool construction validation =="
sed -n '1820,1975p' src/init.cpp | cat -n
rg -n -C 6 'CTxMemPool::Options|CTxMemPool\(mempool_opts\)|m_max_size_bytes|Limits::NoLimits' src/txmempool.cpp src/txmempool.h | head -n 220Repository: dashpay/dash
Length of output: 11780
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== LocaleIndependentAtoi full implementation =="
sed -n '113,180p' src/util/strencodings.h | cat -n
echo
echo "== SettingsValue definition and parsing definitions =="
sed -n '1,140p' src/util/settings.cpp | cat -n
rg -n -C 5 'class SettingsValue|struct SettingsValue|boost::json::value|GetSetting|ParseSettings' src/util src/util -g '*.h' -g '*.cpp' | head -n 220
echo
echo "== MemPoolOptions construction / init validation =="
sed -n '1930,1950p' src/init.cpp | cat -n
echo
echo "== Behavioral probe of C++ int64 multiplication for representative negative/max values =="
python3 - <<'PY'
import ctypes
def wrap64(x):
return (x & ((1 << 64) - 1)) if x != 0 else 0
cases = [
(-1, 1_000_000),
(2_147_483_648, 1_000),
(9_223_372_036_854_775_808, 1_000),
(9_223_372_036_854_775_809, 1_000),
]
for a, b in cases:
res = (a * b) % (1 << 64)
print(f"{a} * {b} -> {res}")
PYRepository: dashpay/dash
Length of output: 22420
Validate scaled mempool limits before multiplication.
GetIntArg can return negative or clamped boundary values, so these conversions can still overflow or pass invalid mempool limits. Check each parsed limit before multiplying before assignment. Also apply the same check to -maxmempool and avoid treating the saturated negative result as a valid mempool size.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/node/mempool_args.cpp` around lines 32 - 36, Validate parsed values for
-limitancestorsize, -limitdescendantcount, -limitdescendantsize, and -maxmempool
before scaling or assigning them to mempool_limits. Reject negative or
boundary-saturated results before multiplying size values by 1'000, and ensure a
saturated negative -maxmempool value is not accepted as a valid mempool size.
| # Match exactly one level: a mainnet node logs into the datadir itself and has no | ||
| # chain name, and feature_config_args.py runs nodes from datadirs nested in node0's. | ||
| glob = pathlib.Path(tmp_dir).glob('node0/*/debug.log') | ||
| path = next(glob, None) | ||
| if path: | ||
| assert next(glob, None) is None or '/feature_config_args_' in tmp_dir # more than one debug.log, should never happen | ||
| assert next(glob, None) is None # more than one debug.log, should never happen |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle direct mainnet log paths.
When tmp_dir/node0/debug.log is used, glob('node0/*/debug.log') cannot match it. The code then falls back to regtest, and the later path construction searches for node{i}/regtest/debug.log. This omits mainnet node logs from the combined output. Detect the direct layout separately and use node{i}/debug.log for that mode. Add a regression test for both layouts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/functional/combine_logs.py` around lines 83 - 88, Update the log-path
discovery logic around the existing node0 glob to detect both layouts: direct
mainnet logs at node{i}/debug.log and nested logs at node{i}/<chain>/debug.log.
Preserve the single-match validation, select the corresponding path pattern so
mainnet logs are included instead of falling back to regtest, and add regression
coverage for both layouts.
Issue being fixed or feature implemented
Kernel / assumeutxo related backports
What was done?
See commits.
How Has This Been Tested?
Run unit & functional tets
Breaking Changes
N/A
Checklist: