backport: merge bitcoin#23345, #23508, #24187, #24528, #24579, #24410, #15936, #25748, #25863, #24051, #25315, #26624, #26894, #25412, partial bitcoin#24595 (auxiliary backports: part 28) #6901
Conversation
|
kwvg
force-pushed
the
auxports28
branch
2 times, most recently
from
271e320 to
bfcf808
Compare
|
This pull request has conflicts, please rebase. |
kwvg
changed the title
WalkthroughThe patch moves coinstats types/APIs from node:: to kernel::, introduces kernel::ComputeUTXOStats returning std::optional, and changes CoinStatsIndex::LookUpStats to return std::optional. ChainstateManager now requires const CChainParams& (stores m_chainparams) and many validation/block-processing APIs were refactored to take ChainstateManager instead of Params. New getdeploymentinfo RPC and /rest/deploymentinfo REST endpoints were added. Settings persistence APIs and helpers (ArgsManager, util::settings) were extended. FlatSigningProvider::Merge became an in-place member; many call sites and tests were updated to match signature and API changes. Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/functional/rpc_blockchain.py (1)
192-251: Fix flake8 E121: continuation indent in assert_equal blockIndent the dict to align with the opening parenthesis to satisfy E121 and improve readability.
Apply this minimal reformat:
- def check_signalling_deploymentinfo_result(self, gdi_result, height, blockhash, status_next): + def check_signalling_deploymentinfo_result(self, gdi_result, height, blockhash, status_next): assert height >= 144 and height <= 287 - - assert_equal(gdi_result, { - "hash": blockhash, - "height": height, - "deployments": { + assert_equal( + gdi_result, + { + "hash": blockhash, + "height": height, + "deployments": { 'bip34': {'type': 'buried', 'active': True, 'height': 2}, 'bip66': {'type': 'buried', 'active': True, 'height': 3}, 'bip65': {'type': 'buried', 'active': True, 'height': 4}, 'csv': {'type': 'buried', 'active': True, 'height': 5}, 'bip147': {'type': 'buried', 'active': True, 'height': 6}, 'dip0001': { 'type': 'buried', 'active': True, 'height': 10}, 'dip0003': { 'type': 'buried', 'active': False, 'height': 411}, 'dip0008': { 'type': 'buried', 'active': True, 'height': 12}, 'dip0020': { 'type': 'buried', 'active': True, 'height': 1}, 'dip0024': { 'type': 'buried', 'active': True, 'height': 13}, 'realloc': { 'type': 'buried', 'active': True, 'height': 14}, 'v19': { 'type': 'buried', 'active': True, 'height': 15}, 'v20': { 'type': 'buried', 'active': False, 'height': 412}, 'mn_rr': { 'type': 'buried', 'active': False, 'height': 413}, 'withdrawals': { 'type': 'buried', 'active': False, 'height': 600}, 'v24': { 'type': 'bip9', 'bip9': { 'start_time': 0, 'timeout': 9223372036854775807, # "v24" does not have a timeout so is set to the max int64 value 'min_activation_height': 0, - 'since': 0, - 'status': 'defined', - 'status_next': 'defined', - 'ehf': True, + 'since': 0, + 'status': 'defined', + 'status_next': 'defined', + 'ehf': True, }, - 'active': False + 'active': False }, 'testdummy': { 'type': 'bip9', 'bip9': { 'bit': 28, 'start_time': 0, 'timeout': 9223372036854775807, # testdummy does not have a timeout so is set to the max int64 value - 'min_activation_height': 0, + 'min_activation_height': 0, 'since': 144, - 'status': 'started', - 'status_next': status_next, + 'status': 'started', + 'status_next': status_next, 'statistics': { 'period': 144, 'threshold': 108, - 'elapsed': height - 143, - 'count': height - 143, + 'elapsed': height - 143, + 'count': height - 143, 'possible': True, }, 'ehf': False, - 'signalling': '#'*(height-143), + 'signalling': '#'*(height-143), }, - 'active': False + 'active': False } - } - }) + }, + )src/init.cpp (1)
834-846: Do not hold cs_main while computing full UTXO stats (severe contention risk).WITH_LOCK wraps the entire GetUTXOStats call, which can scan the whole UTXO DB when the coinstats index is unavailable. This holds cs_main for a long time, stalling validation, p2p, and RPC paths.
Apply this change to lock only for grabbing the view pointer, then compute without cs_main:
- const auto maybe_stats = WITH_LOCK(::cs_main, return GetUTXOStats(&chainman.ActiveChainstate().CoinsDB(), chainman.m_blockman, /*hash_type=*/CoinStatsHashType::NONE, node.rpc_interruption_point, chainman.ActiveChain().Tip(), /*index_requested=*/true)); + CCoinsViewDB* coins_db = nullptr; + { + LOCK(::cs_main); + coins_db = &chainman.ActiveChainstate().CoinsDB(); + } + // Let GetUTXOStats/ComputeUTXOStats resolve the best block internally; pass nullptr for pindex. + const auto maybe_stats = GetUTXOStats( + coins_db, chainman.m_blockman, + /*hash_type=*/CoinStatsHashType::NONE, + node.rpc_interruption_point, + /*pindex=*/nullptr, + /*index_requested=*/true);Optional: if you need an explicit height for gauges, snapshot it under cs_main separately and only for reading scalar fields.
src/kernel/coinstats.cpp (1)
56-66: Serialization bug in ApplyHash(CHashWriter&): ternary precedence is wrong.The expression writes 1 or 0 depending on (height2 + coinbase) rather than (height2 + (coinbase ? 1 : 0)). This corrupts the legacy serialized hash commitment.
Fix the precedence:
- ss << VARINT(it->second.nHeight * 2 + it->second.fCoinBase ? 1u : 0u); + ss << VARINT(it->second.nHeight * 2 + (it->second.fCoinBase ? 1u : 0u));This matches upstream intent and preserves UTXO snapshot/assumeutxo commitments.
src/validation.h (1)
101-110: Updated MIN_DISK_SPACE_FOR_BLOCK_FILES — OK; minor doc nitValue raised to 945 MiB fits Dash’s larger block/undo assumptions. Doc string has “2B MiB” (typo). Consider “2 MiB”.
-// At 2B MiB per block, 288 blocks = 576 MiB. +// At 2 MiB per block, 288 blocks = 576 MiB.
🧹 Nitpick comments (25)
test/functional/wallet_crosschain.py (1)
68-69: LGTM! Teardown logic correctly handles descriptor and pruning state.The conditional stderr handling properly accounts for which nodes are running with pruning enabled based on the descriptor test path.
The condition could be slightly simplified for readability:
- self.nodes[idx].stop_node(expected_stderr=EXPECTED_STDERR_NO_GOV_PRUNE if not self.options.descriptors or (self.options.descriptors and idx == 1) else "") + self.nodes[idx].stop_node(expected_stderr=EXPECTED_STDERR_NO_GOV_PRUNE if (not self.options.descriptors or idx == 1) else "")src/script/signingprovider.h (1)
9-9: Merge API with LIFETIMEBOUND is solid; verified no leftover free function calls.The new
FlatSigningProvider& Merge(FlatSigningProvider&&)member function is properly implemented. Verification confirms no calls to the old freeMerge(a, b)function remain in the codebase.Optional suggestion: Consider adding a
const FlatSigningProvider&overload for ergonomics when merging lvalues without requiring an explicit temporary at the call site.src/util/settings.cpp (1)
180-182: Non‑persistent sources correctly skipped. Add tests for CLI/forced cases.Skipping COMMAND_LINE and FORCED when ignore_nonpersistent=true matches the intent. Please add a unit test covering both CLI and forced overrides to lock this behavior in.
I can supply a test diff in settings_tests exercising ignore_nonpersistent=true for both sources.
src/test/settings_tests.cpp (1)
109-116: Exercise ignore_nonpersistent=true in tests.Current tests pass false; add one that asserts CLI/forced are ignored when true.
Apply this minimal test addition:
@@ BOOST_FIXTURE_TEST_SUITE(settings_tests, BasicTestingSetup) @@ static void CheckValues(const util::Settings& settings, const std::string& single_val, const std::string& list_val) { - util::SettingsValue single_value = GetSetting(settings, "section", "name", false, false, false); + util::SettingsValue single_value = GetSetting(settings, "section", "name", false, false, false); @@ }; +BOOST_AUTO_TEST_CASE(NonPersistentIgnored) +{ + util::Settings settings; + // Non-persistent sources + settings.forced_settings["name"] = "forced"; + settings.command_line_options["name"].push_back("cli"); + // Persistent sources + settings.ro_config["section"]["name"].push_back("cfg"); + settings.rw_settings["name"] = "rw"; + // With ignore_nonpersistent=true we should prefer persistent sources + auto v = GetSetting(settings, "section", "name", + /*ignore_default_section_config=*/false, + /*ignore_nonpersistent=*/true, + /*get_chain_name=*/false); + BOOST_CHECK_EQUAL(v.write(), R"("rw")"); +} + BOOST_AUTO_TEST_CASE(Simple)src/interfaces/node.h (1)
197-214: Doc nit: s/bitcoin.conf/dash.conf/.Adjust comment to reflect Dash config filename.
Apply:
- //! Return setting value from <datadir>/settings.json or bitcoin.conf. + //! Return setting value from <datadir>/settings.json or dash.conf.src/node/interfaces.cpp (3)
447-456: Consider forced overrides in ignore check (or clarify doc).isSettingIgnored() flags CLI overrides only. If forced_settings should also shadow settings.json, include them here or explicitly state CLI‑only in the interface docs.
Example:
gArgs.LockSettings([&](util::Settings& settings) { - if (auto* options = util::FindKey(settings.command_line_options, name)) { - ignored = !options->empty(); - } + if (const auto* options = util::FindKey(settings.command_line_options, name)) { + ignored |= !options->empty(); + } + if (const auto* forced = util::FindKey(settings.forced_settings, name)) { + ignored |= !forced->isNull(); + } });
457-468: updateRwSetting: consider surfacing write failure.Method returns void; if WriteSettingsFile() fails, caller can’t react. Optional: return bool or log on failure.
479-486: Race condition confirmed: Between backup and clear, other writers can mutate settings.The race window is real. LockSettings acquires cs_args only during its callback, and WriteSettingsFile also acquires cs_args internally. In
resetSettings(), the firstWriteSettingsFile(backup=true)releasescs_argsbeforeLockSettings([&](util::Settings& settings) { settings.rw_settings.clear(); })acquires it again, allowing concurrent writes to mutate the settings file between backup and clear.This differs from the pattern in
updateRwSetting(), which wraps both the modification and write within a singleLockSettingscall, preventing the gap.Risk is low since
resetSettings()is unlikely called concurrently with other setting updates, and the backup becomes stale but causes no data corruption. However, to eliminate the race entirely and match the safer pattern used inupdateRwSetting(), consider adding a higher-levelResetSettings()helper onArgsManagerthat performs backup, clear, and write within a singlecs_argsacquisition.src/util/system.cpp (5)
562-575: Backup path toggle looks correct; confirm intent (writes to .bak only).
backupappends “.bak” and affects the single computed path. Together with the temp suffix this yieldssettings.json.bak.tmp→settings.json.bak. If the intent was to write both primary and backup in one operation (“alongside” per PR text), callers must invoke writes twice or a wrapper should handle both. Recommend clarifying in the header comment and adding a convenience wrapper if dual-write is desired.
611-629: Write-to-backup behavior is explicit; consider optional wrapper for dual-write.
WriteSettingsFile(..., /*backup=*/true)persists only the backup file. If you want automatic “write primary then backup”, consider a small wrapper to call this twice or to copy the just-written primary to*.bak. Otherwise, document that callers must opt-in and call it explicitly.
649-653:SettingToStringmirrors historical coercion.Numeric via
getValStr, bools to "0"/"1", elseget_str(). Consider a brief doc comment noting these rules for future readers.
660-664:SettingToIntis consistent; minor doc nit.Semantics match prior code. Optionally document overflow/invalid string behavior (Atoi→0).
671-675:SettingToBoolbehavior matchesInterpretBool.All good. Optional: add a note that non-numeric strings (e.g., "true"/"false") map to false per legacy rules.
src/util/system.h (3)
177-180: Helper prototypes added; consider namespacing or brief docs.Exposing
SettingTo*globally is fine; optionally place in a namespace or add short comments describing coercion rules.
483-487:WriteSettingsFile(..., bool backup)doc could be crisper.Current wording (“or backup settings file”) matches behavior; consider stating that
backup=truewrites only the backup file.
488-493:GetPersistentSettingAPI addition LGTM.Docstring clearly states exclusion of nonpersistent values. Consider
[[nodiscard]]to encourage use of the result.doc/REST-interface.md (1)
88-96: Fix heading style (MD003) and consider noting both accepted paths.
- markdownlint flags “#### Deployment info”. Switch to setext style or align with the file’s convention to silence MD003.
- The handler accepts both “/rest/deploymentinfo” and “/rest/deploymentinfo/”. Optionally document both to avoid ambiguity.
Apply one of:
-#### Deployment info +Deployment info +---------------or keep ATX everywhere and adjust markdownlint config/rule for this doc.
src/test/util/setup_common.cpp (1)
482-483: Check ProcessNewBlock result to surface failures in tests.The return value is ignored. Assert or throw on failure to make test failures explicit.
Apply:
- Assert(m_node.chainman)->ProcessNewBlock(shared_pblock, true, nullptr); + const bool accepted = Assert(m_node.chainman)->ProcessNewBlock(shared_pblock, /*force_processing=*/true, /*new_block=*/nullptr); + Assert(accepted);src/rpc/mining.cpp (1)
134-139: Minor: avoid copying CChainParams.Prefer a reference to
Params()to skip an unnecessary copy.Apply:
- CChainParams chainparams(Params()); + const CChainParams& chainparams = Params();src/init.cpp (1)
834-846: Avoid heavy fallback when coinstatsindex is off (reduce periodic load).PeriodicStats will iterate the entire UTXO set if the index is disabled. Consider skipping UTXO gauges (or sampling) unless g_coin_stats_index is available, to avoid I/O hotspots during runtime stats collection.
src/validation.cpp (5)
353-353: GetBlockScriptFlags now depends on ChainstateManager — add a preconditionThe refactor is correct. Add an explicit precondition check to avoid accidental nullptr deref when called early (e.g., before tip exists).
Apply this diff:
static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const ChainstateManager& chainman) { + assert(pindex != nullptr); unsigned int flags = SCRIPT_VERIFY_NONE;Please confirm no call sites can pass nullptr. See follow-up script below to scan for usage contexts.
Also applies to: 2156-2185
1000-1005: Defensive guard if tip is not yet set in mempool pathRare, but during early startup Tip() can be nullptr. Guard to a safe default to prevent UB.
- unsigned int currentBlockScriptVerifyFlags{GetBlockScriptFlags(m_active_chainstate.m_chain.Tip(), m_active_chainstate.m_chainman)}; + const CBlockIndex* tip = m_active_chainstate.m_chain.Tip(); + const unsigned int currentBlockScriptVerifyFlags = + tip ? GetBlockScriptFlags(tip, m_active_chainstate.m_chainman) + : STANDARD_SCRIPT_VERIFY_FLAGS;
3991-4011: ContextualCheckBlockHeader refactor is sound; tweak log
- Using blockman for checkpoints and chainman for consensus/time/PoW is the right split.
- Minor: the “time-too-new” debug string still hardcodes “+ 2 * 60 * 60”. Prefer reflecting MAX_FUTURE_BLOCK_TIME to avoid drift.
- return state.Invalid(BlockValidationResult::BLOCK_TIME_FUTURE, "time-too-new", strprintf("block timestamp too far in the future %d %d", block.GetBlockTime(), nAdjustedTime + 2 * 60 * 60)); + return state.Invalid(BlockValidationResult::BLOCK_TIME_FUTURE, "time-too-new", + strprintf("block timestamp too far in the future %d %d", + block.GetBlockTime(), nAdjustedTime + MAX_FUTURE_BLOCK_TIME));Also applies to: 4019-4020, 4035-4038
4251-4274: Header processing progress log: clamp to avoid >100% or negativesblocks_left can be negative if clocks are skewed; clamp and guard divide-by-zero.
- const int64_t blocks_left{(GetTime() - last_accepted.GetBlockTime()) / GetConsensus().nPowTargetSpacing}; - const double progress{100.0 * last_accepted.nHeight / (last_accepted.nHeight + blocks_left)}; + const int64_t spacing = std::max<int64_t>(1, GetConsensus().nPowTargetSpacing); + const int64_t delta = GetTime() - last_accepted.GetBlockTime(); + const int64_t blocks_left = std::max<int64_t>(0, delta / spacing); + const double denom = std::max<int64_t>(1, last_accepted.nHeight + blocks_left); + const double progress = std::clamp(100.0 * last_accepted.nHeight / denom, 0.0, 100.0);
5764-5773: Snapshot validation via kernel ComputeUTXOStats — nice
- Using kernel::ComputeUTXOStats with HASH_SERIALIZED and comparing against ExpectedAssumeutxo is correct.
- Optional: avoid file-scope usings to reduce namespace pollution; call with fully-qualified names.
-using kernel::CCoinsStats; -using kernel::CoinStatsHashType; -using kernel::ComputeUTXOStats; ... -const std::optional<CCoinsStats> maybe_stats = ComputeUTXOStats(CoinStatsHashType::HASH_SERIALIZED, snapshot_coinsdb, m_blockman, breakpoint_fnc); +const std::optional<kernel::CCoinsStats> maybe_stats = + kernel::ComputeUTXOStats(kernel::CoinStatsHashType::HASH_SERIALIZED, + snapshot_coinsdb, m_blockman, breakpoint_fnc);Also applies to: 5879-5880, 5887-5889
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
configure.ac(1 hunks)doc/REST-interface.md(1 hunks)doc/release-notes-6901.md(1 hunks)src/Makefile.am(3 hunks)src/index/coinstatsindex.cpp(2 hunks)src/index/coinstatsindex.h(2 hunks)src/init.cpp(4 hunks)src/interfaces/node.h(2 hunks)src/kernel/coinstats.cpp(5 hunks)src/kernel/coinstats.h(3 hunks)src/net_processing.cpp(4 hunks)src/node/interfaces.cpp(1 hunks)src/rest.cpp(2 hunks)src/rpc/blockchain.cpp(15 hunks)src/rpc/blockchain.h(3 hunks)src/rpc/mining.cpp(3 hunks)src/rpc/util.h(0 hunks)src/script/descriptor.cpp(1 hunks)src/script/signingprovider.cpp(1 hunks)src/script/signingprovider.h(2 hunks)src/test/blockfilter_index_tests.cpp(4 hunks)src/test/coinstatsindex_tests.cpp(3 hunks)src/test/descriptor_tests.cpp(5 hunks)src/test/evo_deterministicmns_tests.cpp(9 hunks)src/test/fuzz/coins_view.cpp(0 hunks)src/test/fuzz/rpc.cpp(1 hunks)src/test/fuzz/utxo_snapshot.cpp(1 hunks)src/test/fuzz/versionbits.cpp(4 hunks)src/test/miner_tests.cpp(1 hunks)src/test/settings_tests.cpp(3 hunks)src/test/util/mining.cpp(1 hunks)src/test/util/setup_common.cpp(3 hunks)src/test/validation_block_tests.cpp(4 hunks)src/test/validation_chainstate_tests.cpp(1 hunks)src/util/settings.cpp(2 hunks)src/util/settings.h(1 hunks)src/util/system.cpp(6 hunks)src/util/system.h(2 hunks)src/validation.cpp(28 hunks)src/validation.h(8 hunks)src/versionbits.cpp(3 hunks)src/versionbits.h(2 hunks)src/wallet/rpc/spend.cpp(1 hunks)src/wallet/scriptpubkeyman.cpp(3 hunks)test/functional/feature_asset_locks.py(1 hunks)test/functional/feature_cltv.py(1 hunks)test/functional/feature_dersig.py(1 hunks)test/functional/feature_governance.py(5 hunks)test/functional/feature_mnehf.py(2 hunks)test/functional/feature_new_quorum_type_activation.py(1 hunks)test/functional/interface_rest.py(1 hunks)test/functional/p2p_dos_header_tree.py(4 hunks)test/functional/rpc_blockchain.py(4 hunks)test/functional/test_framework/blocktools.py(1 hunks)test/functional/test_framework/util.py(1 hunks)test/functional/wallet_crosschain.py(3 hunks)test/functional/wallet_importdescriptors.py(1 hunks)test/functional/wallet_signrawtransactionwithwallet.py(2 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
💤 Files with no reviewable changes (2)
- src/rpc/util.h
- src/test/fuzz/coins_view.cpp
🧰 Additional context used
📓 Path-based instructions (4)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/interface_rest.pytest/functional/feature_governance.pytest/functional/p2p_dos_header_tree.pytest/functional/feature_cltv.pytest/functional/feature_asset_locks.pytest/functional/rpc_blockchain.pytest/functional/feature_new_quorum_type_activation.pytest/functional/wallet_importdescriptors.pytest/functional/wallet_signrawtransactionwithwallet.pytest/functional/test_framework/blocktools.pytest/functional/feature_dersig.pytest/functional/wallet_crosschain.pytest/functional/feature_mnehf.pytest/functional/test_framework/util.py
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/script/descriptor.cppsrc/test/fuzz/utxo_snapshot.cppsrc/test/util/mining.cppsrc/test/evo_deterministicmns_tests.cppsrc/index/coinstatsindex.hsrc/versionbits.hsrc/test/miner_tests.cppsrc/wallet/scriptpubkeyman.cppsrc/test/fuzz/versionbits.cppsrc/script/signingprovider.hsrc/script/signingprovider.cppsrc/util/system.hsrc/versionbits.cppsrc/util/settings.cppsrc/interfaces/node.hsrc/rpc/blockchain.hsrc/rpc/mining.cppsrc/wallet/rpc/spend.cppsrc/util/settings.hsrc/test/validation_block_tests.cppsrc/rest.cppsrc/test/fuzz/rpc.cppsrc/validation.hsrc/init.cppsrc/index/coinstatsindex.cppsrc/test/blockfilter_index_tests.cppsrc/test/descriptor_tests.cppsrc/kernel/coinstats.hsrc/test/coinstatsindex_tests.cppsrc/test/validation_chainstate_tests.cppsrc/validation.cppsrc/net_processing.cppsrc/util/system.cppsrc/node/interfaces.cppsrc/test/util/setup_common.cppsrc/kernel/coinstats.cppsrc/test/settings_tests.cppsrc/rpc/blockchain.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/fuzz/utxo_snapshot.cppsrc/test/util/mining.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/miner_tests.cppsrc/test/fuzz/versionbits.cppsrc/test/validation_block_tests.cppsrc/test/fuzz/rpc.cppsrc/test/blockfilter_index_tests.cppsrc/test/descriptor_tests.cppsrc/test/coinstatsindex_tests.cppsrc/test/validation_chainstate_tests.cppsrc/test/util/setup_common.cppsrc/test/settings_tests.cpp
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/REST-interface.mddoc/release-notes-6901.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
🧬 Code graph analysis (38)
test/functional/interface_rest.py (2)
src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)test/functional/test_framework/util.py (1)
assert_equal(69-74)
test/functional/feature_governance.py (1)
src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
src/test/util/mining.cpp (2)
src/node/interfaces.cpp (9)
Assert(92-92)Assert(93-93)Assert(117-117)Assert(259-259)Assert(282-282)Assert(404-404)Assert(653-656)Assert(865-865)Assert(1171-1174)src/node/miner.h (1)
node(40-77)
test/functional/feature_cltv.py (2)
test/functional/test_framework/util.py (1)
assert_equal(69-74)src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
test/functional/feature_asset_locks.py (2)
test/functional/test_framework/util.py (1)
assert_equal(69-74)src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
src/index/coinstatsindex.h (2)
src/kernel/coinstats.cpp (1)
CCoinsStats(21-23)src/index/coinstatsindex.cpp (2)
LookUpStats(319-344)LookUpStats(319-319)
src/versionbits.h (2)
src/versionbits.cpp (18)
GetStateStatisticsFor(134-180)GetStateStatisticsFor(134-134)pindex(258-261)pindex(258-258)params(227-227)params(227-227)params(243-243)params(243-243)params(244-244)params(244-244)params(245-245)params(245-245)params(246-256)params(246-246)params(265-265)params(265-265)Statistics(276-280)Statistics(276-276)src/test/fuzz/versionbits.cpp (12)
pindex(45-45)pindex(45-45)pindex(47-47)pindex(47-47)pindex(55-55)pindex(55-55)pindex(63-63)pindex(63-63)params(46-46)params(46-46)params(48-48)params(48-48)
test/functional/rpc_blockchain.py (2)
test/functional/test_framework/util.py (1)
assert_equal(69-74)src/rpc/blockchain.cpp (4)
getblockchaininfo(1501-1588)getblockchaininfo(1501-1501)getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
src/test/miner_tests.cpp (1)
src/node/interfaces.cpp (11)
Assert(92-92)Assert(93-93)Assert(117-117)Assert(259-259)Assert(282-282)Assert(404-404)Assert(653-656)Assert(865-865)Assert(1171-1174)m_node(1082-1086)m_node(1087-1091)
test/functional/feature_new_quorum_type_activation.py (1)
test/functional/test_framework/test_framework.py (1)
no_op(803-804)
src/test/fuzz/versionbits.cpp (1)
src/versionbits.cpp (4)
pindex(258-261)pindex(258-258)GetStateStatisticsFor(134-180)GetStateStatisticsFor(134-134)
src/script/signingprovider.h (1)
src/script/signingprovider.cpp (2)
Merge(58-65)Merge(58-58)
src/script/signingprovider.cpp (1)
src/script/descriptor.cpp (11)
scripts(759-764)scripts(759-759)pubkeys(512-512)keys(712-712)keys(712-712)keys(723-728)keys(723-723)keys(742-749)keys(742-742)keys(780-793)keys(780-780)
src/util/system.h (2)
src/util/system.cpp (14)
SettingToString(649-652)SettingToString(649-649)SettingToInt(660-663)SettingToInt(660-660)SettingToBool(671-674)SettingToBool(671-671)GetSettingsPath(562-575)GetSettingsPath(562-562)ReadSettingsFile(588-609)ReadSettingsFile(588-588)WriteSettingsFile(611-629)WriteSettingsFile(611-611)GetPersistentSetting(631-636)GetPersistentSetting(631-631)src/node/interfaces.cpp (16)
name(447-456)name(447-447)name(457-457)name(457-457)name(458-468)name(458-458)name(469-478)name(469-469)name(1130-1133)name(1130-1130)name(1134-1137)name(1134-1134)name(1138-1141)name(1138-1138)name(1142-1151)name(1142-1142)
test/functional/wallet_signrawtransactionwithwallet.py (1)
src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
src/versionbits.cpp (2)
src/validation.cpp (14)
pindex(2141-2141)pindex(2141-2141)pindex(2145-2151)pindex(2145-2145)pindex(3067-3074)pindex(3067-3067)params(2139-2139)params(2139-2139)params(2140-2140)params(2140-2140)params(2142-2142)params(2142-2142)params(2143-2143)params(2143-2143)src/test/fuzz/versionbits.cpp (15)
pindex(45-45)pindex(45-45)pindex(47-47)pindex(47-47)pindex(55-55)pindex(55-55)pindex(63-63)pindex(63-63)params(46-46)params(46-46)params(48-48)params(48-48)params(49-49)params(49-49)params(50-50)
src/interfaces/node.h (2)
src/node/interfaces.cpp (16)
name(447-456)name(447-447)name(457-457)name(457-457)name(458-468)name(458-458)name(469-478)name(469-469)name(1130-1133)name(1130-1130)name(1134-1137)name(1134-1134)name(1138-1141)name(1138-1138)name(1142-1151)name(1142-1142)src/util/settings.h (1)
util(16-112)
src/rpc/blockchain.h (2)
src/kernel/coinstats.h (2)
CoinStatsHashType(23-76)node(18-20)src/rpc/blockchain.cpp (3)
GetUTXOStats(1106-1129)GetUTXOStats(1106-1110)view(1340-1340)
test/functional/test_framework/blocktools.py (1)
src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
src/util/settings.h (3)
src/util/settings.cpp (2)
GetSetting(140-195)GetSetting(140-145)src/util/system.cpp (2)
GetSetting(1179-1185)GetSetting(1179-1179)src/node/interfaces.cpp (16)
name(447-456)name(447-447)name(457-457)name(457-457)name(458-468)name(458-458)name(469-478)name(469-469)name(1130-1133)name(1130-1130)name(1134-1137)name(1134-1134)name(1138-1141)name(1138-1138)name(1142-1151)name(1142-1142)
test/functional/feature_dersig.py (2)
test/functional/test_framework/util.py (1)
assert_equal(69-74)src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
src/test/validation_block_tests.cpp (2)
src/net_processing.cpp (3)
pblock(606-606)block(608-608)block(612-612)src/rpc/mining.cpp (2)
block(987-992)block(987-987)
test/functional/wallet_crosschain.py (1)
test/functional/test_framework/test_framework.py (2)
stop_nodes(684-692)start_nodes(660-678)
src/rest.cpp (1)
src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
src/validation.h (1)
src/validation.cpp (11)
ChainstateManager(5985-5995)chainparams(565-575)chainparams(565-567)chainparams(578-587)chainparams(578-579)chainparams(590-599)chainparams(590-591)ProcessNewBlock(4371-4408)ProcessNewBlock(4371-4371)ProcessNewBlockHeaders(4250-4277)ProcessNewBlockHeaders(4250-4250)
src/init.cpp (3)
src/kernel/coinstats.h (1)
CoinStatsHashType(23-76)src/rpc/blockchain.cpp (2)
GetUTXOStats(1106-1129)GetUTXOStats(1106-1110)src/util/system.cpp (2)
CheckDiskSpace(158-164)CheckDiskSpace(158-158)
test/functional/feature_mnehf.py (1)
test/functional/test_framework/test_framework.py (1)
generate(806-809)
test/functional/test_framework/util.py (1)
src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
src/index/coinstatsindex.cpp (1)
src/kernel/coinstats.cpp (5)
CCoinsStats(21-23)GetBogoSize(26-34)GetBogoSize(26-26)TxOutSer(36-42)TxOutSer(36-36)
src/test/descriptor_tests.cpp (1)
src/script/descriptor.cpp (6)
scripts(759-764)scripts(759-759)LEGACY(715-715)LEGACY(731-731)LEGACY(797-797)nullopt(657-657)
src/kernel/coinstats.h (2)
src/rpc/blockchain.h (2)
kernel(26-28)node(33-36)src/kernel/coinstats.cpp (9)
CCoinsStats(21-23)GetBogoSize(26-34)GetBogoSize(26-26)TxOutSer(36-42)TxOutSer(36-36)ComputeUTXOStats(100-137)ComputeUTXOStats(100-100)ComputeUTXOStats(139-165)ComputeUTXOStats(139-139)
src/test/coinstatsindex_tests.cpp (2)
src/kernel/coinstats.cpp (1)
CCoinsStats(21-23)src/kernel/coinstats.h (1)
CoinStatsHashType(23-76)
src/test/validation_chainstate_tests.cpp (3)
src/validation.cpp (6)
chainparams(565-575)chainparams(565-567)chainparams(578-587)chainparams(578-579)chainparams(590-599)chainparams(590-591)src/rpc/mining.cpp (2)
chainparams(134-134)chainparams(359-359)src/test/miner_tests.cpp (1)
chainparams(39-39)
src/validation.cpp (2)
src/kernel/coinstats.cpp (5)
CCoinsStats(21-23)ComputeUTXOStats(100-137)ComputeUTXOStats(100-100)ComputeUTXOStats(139-165)ComputeUTXOStats(139-139)src/kernel/coinstats.h (1)
CoinStatsHashType(23-76)
src/util/system.cpp (1)
src/node/interfaces.cpp (16)
name(447-456)name(447-447)name(457-457)name(457-457)name(458-468)name(458-458)name(469-478)name(469-469)name(1130-1133)name(1130-1130)name(1134-1137)name(1134-1134)name(1138-1141)name(1138-1138)name(1142-1151)name(1142-1142)
src/test/util/setup_common.cpp (1)
src/chainparams.cpp (2)
Params(1354-1357)Params(1354-1354)
src/test/settings_tests.cpp (2)
src/util/settings.cpp (2)
GetSetting(140-195)GetSetting(140-145)src/util/system.cpp (2)
GetSetting(1179-1185)GetSetting(1179-1179)
src/rpc/blockchain.cpp (4)
src/kernel/coinstats.cpp (5)
CCoinsStats(21-23)ComputeUTXOStats(100-137)ComputeUTXOStats(100-100)ComputeUTXOStats(139-165)ComputeUTXOStats(139-139)src/kernel/coinstats.h (2)
CoinStatsHashType(23-76)node(18-20)src/rpc/server.cpp (2)
IsDeprecatedRPCEnabled(367-372)IsDeprecatedRPCEnabled(367-367)src/rest.cpp (1)
getdeploymentinfo(624-624)
🪛 Flake8 (7.3.0)
test/functional/rpc_blockchain.py
[error] 196-196: continuation line under-indented for hanging indent
(E121)
🪛 markdownlint-cli2 (0.18.1)
doc/REST-interface.md
88-88: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
| RPCHelpMan getdeploymentinfo(); | ||
|
|
||
| static bool rest_deploymentinfo(const CoreContext& context, HTTPRequest* req, const std::string& str_uri_part) | ||
| { | ||
| if (!CheckWarmup(req)) return false; | ||
|
|
||
| std::string hash_str; | ||
| const RESTResponseFormat rf = ParseDataFormat(hash_str, str_uri_part); | ||
|
|
||
| switch (rf) { | ||
| case RESTResponseFormat::JSON: { | ||
| JSONRPCRequest jsonRequest; | ||
| jsonRequest.context = context; | ||
| jsonRequest.params = UniValue(UniValue::VARR); | ||
|
|
||
| if (!hash_str.empty()) { | ||
| uint256 hash; | ||
| if (!ParseHashStr(hash_str, hash)) { | ||
| return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hash_str); | ||
| } | ||
|
|
||
| const ChainstateManager* chainman = GetChainman(context, req); | ||
| if (!chainman) return false; | ||
| if (!WITH_LOCK(::cs_main, return chainman->m_blockman.LookupBlockIndex(ParseHashV(hash_str, "blockhash")))) { | ||
| return RESTERR(req, HTTP_BAD_REQUEST, "Block not found"); | ||
| } | ||
|
|
||
| jsonRequest.params.pushKV("blockhash", hash_str); | ||
| } | ||
|
|
||
| req->WriteHeader("Content-Type", "application/json"); | ||
| req->WriteReply(HTTP_OK, getdeploymentinfo().HandleRequest(jsonRequest).write() + "\n"); | ||
| return true; |
There was a problem hiding this comment.
Fix parameter handling and error codes in rest_deploymentinfo
- Use the already parsed hash for LookupBlockIndex (do not call ParseHashV on a string).
- Pass params as a positional array (push_back), not object (pushKV).
- Return 404 for “Block not found” to match other REST endpoints.
- Keep ChainstateManager non-const for consistency with neighboring handlers.
Apply this diff:
-RPCHelpMan getdeploymentinfo();
+RPCHelpMan getdeploymentinfo();
static bool rest_deploymentinfo(const CoreContext& context, HTTPRequest* req, const std::string& str_uri_part)
{
if (!CheckWarmup(req)) return false;
std::string hash_str;
const RESTResponseFormat rf = ParseDataFormat(hash_str, str_uri_part);
switch (rf) {
case RESTResponseFormat::JSON: {
JSONRPCRequest jsonRequest;
jsonRequest.context = context;
- jsonRequest.params = UniValue(UniValue::VARR);
+ jsonRequest.params = UniValue(UniValue::VARR);
if (!hash_str.empty()) {
uint256 hash;
if (!ParseHashStr(hash_str, hash)) {
return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hash_str);
}
- const ChainstateManager* chainman = GetChainman(context, req);
+ ChainstateManager* chainman = GetChainman(context, req);
if (!chainman) return false;
- if (!WITH_LOCK(::cs_main, return chainman->m_blockman.LookupBlockIndex(ParseHashV(hash_str, "blockhash")))) {
- return RESTERR(req, HTTP_BAD_REQUEST, "Block not found");
+ if (!WITH_LOCK(::cs_main, return chainman->m_blockman.LookupBlockIndex(hash))) {
+ return RESTERR(req, HTTP_NOT_FOUND, "Block not found");
}
- jsonRequest.params.pushKV("blockhash", hash_str);
+ jsonRequest.params.push_back(hash_str);
}
req->WriteHeader("Content-Type", "application/json");
req->WriteReply(HTTP_OK, getdeploymentinfo().HandleRequest(jsonRequest).write() + "\n");
return true;
}
default: {
return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: json)");
}
}
}🤖 Prompt for AI Agents
In src/rest.cpp around lines 624 to 656, fix rest_deploymentinfo by using the
parsed uint256 hash (not ParseHashV on the string) when calling
LookupBlockIndex, change the ChainstateManager pointer to non-const
(ChainstateManager* chainman) for consistency, adjust the LookupBlockIndex call
to use the already-parsed hash variable inside the WITH_LOCK, change
jsonRequest.params.pushKV(...) to push a positional param (push_back) so params
are a positional array, and return HTTP_NOT_FOUND (404) instead of
HTTP_BAD_REQUEST when the block is not found.
|
This pull request has conflicts, please rebase. |
bitcoin#22550 adds assertions to validate CSV/CLTV activation and replaces the `generate()` calls with a helper `generate_to_height()` but bitcoin#22818 activates all forks from block 1 by default, so the `generate_to_height()` calls have been dropped. This leaves us with the `generate()` calls being swapped for assertions.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/functional/rpc_blockchain.py (1)
192-250: Fix indentation issue on line 196.There's an indentation issue flagged by flake8 on line 196. The line should be indented to align with the opening bracket.
Apply this diff to fix the indentation:
assert_equal(gdi_result, { - "hash": blockhash, + "hash": blockhash, "height": height, "deployments": {src/kernel/coinstats.cpp (1)
56-72: Bug in serialized-hash height/coinbase encoding (missing parentheses).
?:precedence makes this encode(height*2 + coinbase) ? 1 : 0instead ofheight*2 + (coinbase?1:0), corrupting the UTXO set hash/commitment.Fix:
- ss << VARINT(it->second.nHeight * 2 + it->second.fCoinBase ? 1u : 0u); + ss << VARINT(it->second.nHeight * 2 + (it->second.fCoinBase ? 1u : 0u));
♻️ Duplicate comments (2)
src/test/descriptor_tests.cpp (1)
320-320: Remove stray debug comment.The “/* --- it fails!!! */” line should not be committed in tests.
- /* --- it fails!!! */src/kernel/coinstats.h (1)
14-16: Include explicitly.
std::optionalis used in this header; add the header to avoid relying on transitive includes.#include <cstdint> #include <functional> +#include <optional>
🧹 Nitpick comments (11)
src/test/fuzz/versionbits.cpp (2)
253-259: Add assertion tying signals length to elapsed for tighter invariantsYou already verify numeric equality across calls. Also assert the signals vector length equals stats.elapsed to guard assumptions about ordering/size.
- const BIP9Stats stats = checker.GetStateStatisticsFor(current_block, &signals); + const BIP9Stats stats = checker.GetStateStatisticsFor(current_block, &signals); const BIP9Stats stats_no_signals = checker.GetStateStatisticsFor(current_block); assert(stats.period == stats_no_signals.period && stats.threshold == stats_no_signals.threshold && stats.elapsed == stats_no_signals.elapsed && stats.count == stats_no_signals.count && stats.possible == stats_no_signals.possible); + assert(signals.size() == static_cast<size_t>(stats.elapsed));
287-290: Strengthen final-period checks by validating returned signals vectorAlso capture signals on the last block to ensure:
- signals.size() == period
- true-count equals stats.count
Requires for std::count.
- const BIP9Stats stats = checker.GetStateStatisticsFor(current_block); + std::vector<bool> signals_final; + const BIP9Stats stats = checker.GetStateStatisticsFor(current_block, &signals_final); assert(stats.period == period); assert(stats.threshold == threshold); assert(stats.elapsed == period); assert(stats.count == blocks_sig); assert(stats.possible == (stats.count + period >= stats.elapsed + threshold)); + assert(signals_final.size() == static_cast<size_t>(period)); + assert(std::count(signals_final.begin(), signals_final.end(), true) == stats.count);Additional include (top of file with other headers):
+#include <algorithm>src/Makefile.am (1)
1089-1103: Consider refactoring dash-wallet LDADD to reduce duplication and improve link orderThe dash_wallet block (lines 1089–1103) manually expands libraries that largely overlap with the shared
bitcoin_bin_ldaddvariable. Unlikedashdanddash_nodewhich correctly reuse it,dash_walletcannot usebitcoin_bin_ldadddirectly because it excludes unnecessary node dependencies (LIBBITCOIN_ZMQ, LIBLEVELDB, LIBMEMENV).Recommended approach: Factor wallet-specific libraries into a separate variable (e.g.,
bitcoin_wallet_ldadd) to eliminate duplication, or keep manual expansion but reorder crypto/consensus libs to appear after BLS and secp256k1 primitives (e.g., lines 1095–1098 should move after line 1098) to follow standard link-order conventions on non-Linux targets.test/functional/feature_cltv.py (1)
137-141: Fix comment/semantics mismatch around “active”The comments say “Not active as of current tip…” but test_cltv_info asserts active=True at that point. Update comments to reflect the intended semantics (enforcement state for the next block vs current tip) to avoid confusion. Example:
- self.test_cltv_info(is_active=True) # Not active as of current tip, but next block must obey rules + self.test_cltv_info(is_active=True) # Enforcement for the next block is active (tip at CLTV_HEIGHT-1)Also applies to: 192-195
test/functional/feature_governance.py (1)
176-180: Grammar nit in log messageTiny wording fix.
- self.log.info("v20 is expected to be activate since block 160") + self.log.info("v20 is expected to be active since block 160")test/functional/interface_rest.py (2)
391-402: Nice REST parity test; add positive hash caseGood check that /rest/deploymentinfo matches RPC at tip and error paths. Add a positive hash case to assert /rest/deploymentinfo/ equals getdeploymentinfo():
deployment_info = self.nodes[0].getdeploymentinfo() assert_equal(deployment_info, self.test_rest_request('/deploymentinfo')) + # Also verify explicit blockhash variant matches RPC + tip = self.nodes[0].getbestblockhash() + assert_equal( + self.nodes[0].getdeploymentinfo(tip), + self.test_rest_request(f'/deploymentinfo/{tip}') + )
396-402: HTTP status consistency (optional)Other REST endpoints use 404 for missing blocks (e.g., /block/), while here “Block not found” returns 400. Consider aligning to 404 or documenting the difference in REST docs for predictability.
doc/REST-interface.md (1)
88-96: Docs read well; consider noting error responsesOptionally add a sentence about error codes for invalid hash (400) vs not found (currently tested as 400) to match behavior and reduce ambiguity.
src/test/settings_tests.cpp (1)
109-109: Consider testing the newignore_nonpersistentparameter.All
GetSettingcalls have been correctly updated to passfalsefor the new parameter, preserving existing behavior. However, there's no test coverage for the newignore_nonpersistent=truebehavior.Consider adding a test case that exercises the new functionality, for example:
BOOST_AUTO_TEST_CASE(IgnoreNonPersistent) { util::Settings settings; settings.command_line_options["name"].push_back("cmdline_val"); settings.ro_config["section"]["name"].push_back("config_val"); // With ignore_nonpersistent=false, command line takes precedence BOOST_CHECK_EQUAL(R"("cmdline_val")", GetSetting(settings, "section", "name", false, false, false).write().c_str()); // With ignore_nonpersistent=true, config file value is returned BOOST_CHECK_EQUAL(R"("config_val")", GetSetting(settings, "section", "name", false, true, false).write().c_str()); }Also applies to: 145-147, 228-228
src/init.cpp (2)
834-846: UTXO stats collection path looks good.Locking and optional handling are correct. Optionally, only request index lookup when available to avoid an extra branch.
- const auto maybe_stats = WITH_LOCK(::cs_main, return GetUTXOStats(&chainman.ActiveChainstate().CoinsDB(), chainman.m_blockman, /*hash_type=*/CoinStatsHashType::NONE, node.rpc_interruption_point, chainman.ActiveChain().Tip(), /*index_requested=*/true)); + const bool index_requested = (g_coin_stats_index != nullptr); + const auto maybe_stats = WITH_LOCK(::cs_main, return GetUTXOStats(&chainman.ActiveChainstate().CoinsDB(), chainman.m_blockman, /*hash_type=*/CoinStatsHashType::NONE, node.rpc_interruption_point, chainman.ActiveChain().Tip(), index_requested));
2291-2307: Nice first‑startup disk‑space warning.Logic matches PR intent. Consider casting to uint64_t to avoid accidental overflow when multiplying GiB.
- uint64_t additional_bytes_needed = fPruneMode ? nPruneTarget - : chainparams.AssumedBlockchainSize() * 1024 * 1024 * 1024; + const uint64_t additional_bytes_needed = fPruneMode ? nPruneTarget + : static_cast<uint64_t>(chainparams.AssumedBlockchainSize()) * 1024ULL * 1024ULL * 1024ULL;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
configure.ac(1 hunks)doc/REST-interface.md(1 hunks)doc/release-notes-6901.md(1 hunks)src/Makefile.am(3 hunks)src/index/coinstatsindex.cpp(2 hunks)src/index/coinstatsindex.h(2 hunks)src/init.cpp(4 hunks)src/interfaces/node.h(2 hunks)src/kernel/coinstats.cpp(5 hunks)src/kernel/coinstats.h(3 hunks)src/net_processing.cpp(4 hunks)src/node/interfaces.cpp(1 hunks)src/rest.cpp(2 hunks)src/rpc/blockchain.cpp(15 hunks)src/rpc/blockchain.h(3 hunks)src/rpc/mining.cpp(3 hunks)src/rpc/util.h(0 hunks)src/script/descriptor.cpp(1 hunks)src/script/signingprovider.cpp(1 hunks)src/script/signingprovider.h(2 hunks)src/test/blockfilter_index_tests.cpp(4 hunks)src/test/coinstatsindex_tests.cpp(3 hunks)src/test/descriptor_tests.cpp(5 hunks)src/test/evo_deterministicmns_tests.cpp(9 hunks)src/test/fuzz/coins_view.cpp(0 hunks)src/test/fuzz/rpc.cpp(1 hunks)src/test/fuzz/utxo_snapshot.cpp(1 hunks)src/test/fuzz/versionbits.cpp(4 hunks)src/test/miner_tests.cpp(1 hunks)src/test/settings_tests.cpp(3 hunks)src/test/util/mining.cpp(1 hunks)src/test/util/setup_common.cpp(3 hunks)src/test/validation_block_tests.cpp(4 hunks)src/test/validation_chainstate_tests.cpp(1 hunks)src/util/settings.cpp(2 hunks)src/util/settings.h(1 hunks)src/util/system.cpp(6 hunks)src/util/system.h(2 hunks)src/validation.cpp(28 hunks)src/validation.h(8 hunks)src/versionbits.cpp(3 hunks)src/versionbits.h(2 hunks)src/wallet/rpc/spend.cpp(1 hunks)src/wallet/scriptpubkeyman.cpp(3 hunks)test/functional/feature_asset_locks.py(1 hunks)test/functional/feature_cltv.py(1 hunks)test/functional/feature_dersig.py(1 hunks)test/functional/feature_governance.py(5 hunks)test/functional/feature_mnehf.py(2 hunks)test/functional/feature_new_quorum_type_activation.py(1 hunks)test/functional/interface_rest.py(1 hunks)test/functional/p2p_dos_header_tree.py(4 hunks)test/functional/rpc_blockchain.py(4 hunks)test/functional/test_framework/blocktools.py(1 hunks)test/functional/test_framework/util.py(1 hunks)test/functional/wallet_crosschain.py(3 hunks)test/functional/wallet_importdescriptors.py(1 hunks)test/functional/wallet_signrawtransactionwithwallet.py(2 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
💤 Files with no reviewable changes (2)
- src/rpc/util.h
- src/test/fuzz/coins_view.cpp
🚧 Files skipped from review as they are similar to previous changes (22)
- src/test/fuzz/rpc.cpp
- src/script/descriptor.cpp
- src/test/miner_tests.cpp
- test/functional/test_framework/blocktools.py
- src/wallet/rpc/spend.cpp
- test/functional/wallet_crosschain.py
- src/rest.cpp
- src/versionbits.h
- test/functional/test_framework/util.py
- src/test/util/mining.cpp
- test/functional/p2p_dos_header_tree.py
- src/interfaces/node.h
- src/rpc/blockchain.h
- test/functional/feature_new_quorum_type_activation.py
- src/test/util/setup_common.cpp
- src/util/system.h
- test/functional/feature_dersig.py
- src/util/system.cpp
- src/node/interfaces.cpp
- src/test/blockfilter_index_tests.cpp
- src/net_processing.cpp
- test/functional/feature_mnehf.py
🧰 Additional context used
📓 Path-based instructions (4)
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/REST-interface.mddoc/release-notes-6901.md
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/wallet_signrawtransactionwithwallet.pytest/functional/feature_asset_locks.pytest/functional/rpc_blockchain.pytest/functional/feature_cltv.pytest/functional/feature_governance.pytest/functional/interface_rest.pytest/functional/wallet_importdescriptors.py
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/util/settings.hsrc/test/coinstatsindex_tests.cppsrc/rpc/mining.cppsrc/index/coinstatsindex.cppsrc/versionbits.cppsrc/kernel/coinstats.hsrc/test/evo_deterministicmns_tests.cppsrc/test/fuzz/utxo_snapshot.cppsrc/script/signingprovider.cppsrc/test/fuzz/versionbits.cppsrc/test/settings_tests.cppsrc/wallet/scriptpubkeyman.cppsrc/util/settings.cppsrc/test/descriptor_tests.cppsrc/kernel/coinstats.cppsrc/script/signingprovider.hsrc/index/coinstatsindex.hsrc/validation.hsrc/init.cppsrc/test/validation_chainstate_tests.cppsrc/test/validation_block_tests.cppsrc/validation.cppsrc/rpc/blockchain.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/coinstatsindex_tests.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/fuzz/utxo_snapshot.cppsrc/test/fuzz/versionbits.cppsrc/test/settings_tests.cppsrc/test/descriptor_tests.cppsrc/test/validation_chainstate_tests.cppsrc/test/validation_block_tests.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
🧬 Code graph analysis (22)
test/functional/wallet_signrawtransactionwithwallet.py (1)
src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
test/functional/feature_asset_locks.py (2)
test/functional/test_framework/util.py (1)
assert_equal(69-74)src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
src/util/settings.h (3)
src/util/settings.cpp (2)
GetSetting(140-195)GetSetting(140-145)src/util/system.cpp (2)
GetSetting(1179-1185)GetSetting(1179-1179)src/node/interfaces.cpp (16)
name(447-456)name(447-447)name(457-457)name(457-457)name(458-468)name(458-458)name(469-478)name(469-469)name(1130-1133)name(1130-1130)name(1134-1137)name(1134-1134)name(1138-1141)name(1138-1138)name(1142-1151)name(1142-1142)
src/test/coinstatsindex_tests.cpp (2)
src/kernel/coinstats.cpp (1)
CCoinsStats(21-23)src/kernel/coinstats.h (1)
CoinStatsHashType(23-76)
src/index/coinstatsindex.cpp (1)
src/kernel/coinstats.cpp (5)
CCoinsStats(21-23)GetBogoSize(26-34)GetBogoSize(26-26)TxOutSer(36-42)TxOutSer(36-36)
src/versionbits.cpp (2)
src/validation.cpp (14)
pindex(2205-2205)pindex(2205-2205)pindex(2209-2215)pindex(2209-2209)pindex(3131-3138)pindex(3131-3131)params(2203-2203)params(2203-2203)params(2204-2204)params(2204-2204)params(2206-2206)params(2206-2206)params(2207-2207)params(2207-2207)src/test/fuzz/versionbits.cpp (15)
pindex(45-45)pindex(45-45)pindex(47-47)pindex(47-47)pindex(55-55)pindex(55-55)pindex(63-63)pindex(63-63)params(46-46)params(46-46)params(48-48)params(48-48)params(49-49)params(49-49)params(50-50)
src/kernel/coinstats.h (2)
src/rpc/blockchain.h (2)
kernel(26-28)node(33-36)src/kernel/coinstats.cpp (9)
CCoinsStats(21-23)GetBogoSize(26-34)GetBogoSize(26-26)TxOutSer(36-42)TxOutSer(36-36)ComputeUTXOStats(100-137)ComputeUTXOStats(100-100)ComputeUTXOStats(139-165)ComputeUTXOStats(139-139)
src/script/signingprovider.cpp (1)
src/script/descriptor.cpp (11)
scripts(759-764)scripts(759-759)pubkeys(512-512)keys(712-712)keys(712-712)keys(723-728)keys(723-723)keys(742-749)keys(742-742)keys(780-793)keys(780-780)
src/test/fuzz/versionbits.cpp (1)
src/versionbits.cpp (4)
pindex(258-261)pindex(258-258)GetStateStatisticsFor(134-180)GetStateStatisticsFor(134-134)
src/test/settings_tests.cpp (2)
src/util/settings.cpp (2)
GetSetting(140-195)GetSetting(140-145)src/util/system.cpp (2)
GetSetting(1179-1185)GetSetting(1179-1179)
test/functional/rpc_blockchain.py (2)
test/functional/test_framework/util.py (1)
assert_equal(69-74)src/rpc/blockchain.cpp (4)
getblockchaininfo(1501-1588)getblockchaininfo(1501-1501)getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
src/test/descriptor_tests.cpp (1)
src/script/descriptor.cpp (5)
scripts(759-764)scripts(759-759)LEGACY(715-715)LEGACY(731-731)LEGACY(797-797)
test/functional/feature_cltv.py (2)
test/functional/test_framework/util.py (1)
assert_equal(69-74)src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
src/script/signingprovider.h (1)
src/script/signingprovider.cpp (2)
Merge(58-65)Merge(58-58)
src/index/coinstatsindex.h (3)
src/rpc/blockchain.h (1)
kernel(26-28)src/kernel/coinstats.cpp (1)
CCoinsStats(21-23)src/index/coinstatsindex.cpp (2)
LookUpStats(320-345)LookUpStats(320-320)
src/validation.h (1)
src/validation.cpp (5)
ChainstateManager(6049-6059)ProcessNewBlock(4435-4472)ProcessNewBlock(4435-4435)ProcessNewBlockHeaders(4314-4341)ProcessNewBlockHeaders(4314-4314)
src/init.cpp (4)
src/kernel/coinstats.h (1)
CoinStatsHashType(23-76)src/rpc/blockchain.cpp (2)
GetUTXOStats(1106-1129)GetUTXOStats(1106-1110)src/util/system.cpp (2)
CheckDiskSpace(158-164)CheckDiskSpace(158-158)src/fs.h (1)
quoted(94-97)
test/functional/feature_governance.py (1)
src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
src/test/validation_chainstate_tests.cpp (3)
src/validation.cpp (6)
chainparams(569-580)chainparams(569-571)chainparams(583-593)chainparams(583-584)chainparams(596-606)chainparams(596-597)src/rpc/mining.cpp (2)
chainparams(134-134)chainparams(359-359)src/test/miner_tests.cpp (3)
chainparams(39-39)chainparams(40-40)chainparams(41-41)
test/functional/interface_rest.py (2)
src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)test/functional/test_framework/util.py (1)
assert_equal(69-74)
src/validation.cpp (2)
src/kernel/coinstats.cpp (5)
CCoinsStats(21-23)ComputeUTXOStats(100-137)ComputeUTXOStats(100-100)ComputeUTXOStats(139-165)ComputeUTXOStats(139-139)src/kernel/coinstats.h (1)
CoinStatsHashType(23-76)
src/rpc/blockchain.cpp (4)
src/kernel/coinstats.cpp (5)
CCoinsStats(21-23)ComputeUTXOStats(100-137)ComputeUTXOStats(100-100)ComputeUTXOStats(139-165)ComputeUTXOStats(139-139)src/kernel/coinstats.h (2)
CoinStatsHashType(23-76)node(18-20)src/rpc/server.cpp (2)
IsDeprecatedRPCEnabled(367-372)IsDeprecatedRPCEnabled(367-367)src/deploymentinfo.cpp (2)
DeploymentName(20-56)DeploymentName(20-20)
🪛 Flake8 (7.3.0)
test/functional/rpc_blockchain.py
[error] 196-196: continuation line under-indented for hanging indent
(E121)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (58)
test/functional/wallet_importdescriptors.py (1)
131-131: LGTM! Good simplification.Directly using
key.p2pkh_addrremoves redundant address computation and aligns with the usage pattern throughout the rest of the file (lines 97, 107, 117, 180).src/versionbits.cpp (2)
134-180: LGTM! Clean implementation of per-block signalling statistics.The changes correctly implement the optional
signalling_blocksparameter with sound loop indexing and elapsed counter logic. The retention ofpindexEndOfPrevPeriodfor Dash-specific dynamic threshold calculation (lines 152-159) while using the new elapsed counter approach (lines 162-175) properly aligns with the PR objectives.
276-280: LGTM! Parameter forwarding is correct.The updated signature and forwarding of the
signalling_blocksparameter toGetStateStatisticsForis properly implemented.configure.ac (1)
1505-1509: LGTM! Reasonable build optimization.This gating logic correctly disables networking features (UPnP, NAT-PMP, ZMQ) when none of the components that require them are being built. The condition appropriately checks the daemon, GUI, benchmarks, and tests—the primary consumers of these networking capabilities.
The pattern is consistent with the existing fuzzing mode logic (lines 1401-1417) and the placement before the actual library detection routines (lines 1512+) properly prevents unnecessary dependency checks.
src/test/fuzz/versionbits.cpp (3)
55-55: Adapter overload matches updated APIWrapper cleanly forwards to AbstractThresholdConditionChecker::GetStateStatisticsFor with optional signals capture. No issues.
225-231: Baseline stats initialization at period boundary looks correctSetting elapsed/count to 0 and possible = (period >= threshold) matches the upstream formula at the start of a period. Good.
267-270: Per-block signalling history checks are consistentThe sequence-size, last-bit, and full-sequence equality checks align with the signalling_blocks semantics. LGTM.
test/functional/wallet_signrawtransactionwithwallet.py (2)
133-133: Switch to getdeploymentinfo for CSV activation — LGTMAccurately reflects the new RPC and removes brittle block-gen assumptions.
162-167: CLTV check/operand adjustment — LGTMUsing getdeploymentinfo and dropping CLTV operand to 100 is consistent with upstream; passing current height as locktime keeps the tx final.
src/Makefile.am (2)
262-262: Public header move to kernel/coinstats.h — LGTMMatches the code relocation.
532-532: Add kernel/coinstats.cpp to node sources — LGTMConsistent with the namespace move.
src/rpc/mining.cpp (3)
148-149: Update ProcessNewBlock signature usage — LGTMCorrectly removes Params() per the new API.
1044-1044: submitblock: ProcessNewBlock call updated — LGTMMatches the refactored ChainstateManager interface.
1085-1085: submitheader: ProcessNewBlockHeaders call updated — LGTMSignature change is applied correctly.
test/lint/lint-circular-dependencies.py (1)
22-22: Update expected cycle: kernel/coinstats ↔ validation — LGTMKeeps the linter aligned with the namespace move.
test/functional/feature_asset_locks.py (1)
251-251: Use getdeploymentinfo for v20 activation — LGTMAvoids deprecated getblockchaininfo['softforks'] and aligns tests with the new RPC.
test/functional/feature_cltv.py (1)
101-107: Good migration to getdeploymentinfoSwitching to getdeploymentinfo() for buried deployment checks is correct and future‑proof. Keys asserted here match the buried shape.
test/functional/feature_governance.py (1)
27-29: Consistent use of getdeploymentinfo for v20 stateAll call sites now query v20 via getdeploymentinfo(). Looks consistent with the deprecation of softforks in getblockchaininfo.
Also applies to: 98-99, 105-106, 224-226, 364-365, 378-380
doc/release-notes-6901.md (2)
4-11: Clear note on softforks relocationRelease note accurately describes the move to getdeploymentinfo and the deprecation flag. LGTM.
16-17: REST endpoint mentionGood addition; no changes needed.
src/util/settings.cpp (2)
144-144: LGTM! Parameter addition is clean.The new
ignore_nonpersistentparameter is well-placed in the signature and aligns with the function's existing design pattern.
180-181: LGTM! Implementation is correct.The logic correctly skips non-persistent settings (COMMAND_LINE and FORCED) when requested, and the placement in the control flow is appropriate.
src/test/fuzz/utxo_snapshot.cpp (1)
60-60: LGTM! API usage updated correctly.The removal of the
::Params()argument fromProcessNewBlockHeadersis consistent with the ChainstateManager API changes where chain parameters are now held by the manager itself.src/test/validation_chainstate_tests.cpp (1)
27-28: LGTM! ChainstateManager initialization updated correctly.The explicit chain parameters binding and passing to the constructor aligns with the PR's API changes where ChainstateManager now holds
CChainParamsand requires explicit initialization.src/script/signingprovider.h (2)
9-9: LGTM! Appropriate include for LIFETIMEBOUND.The
attributes.hinclude is necessary for theLIFETIMEBOUNDattribute used in the newMergemember function.
59-60: Excellent refactor to member function with move semantics.The conversion from a free function to a member function
Merge(FlatSigningProvider&& b)offers several improvements:
- Move semantics avoid unnecessary copies
- In-place mutation is more efficient
- Return reference enables method chaining
LIFETIMEBOUNDattribute prevents lifetime issuessrc/util/settings.h (1)
58-62: LGTM! Clear documentation and consistent API.The new
ignore_nonpersistentparameter is well-documented and the declaration correctly matches the implementation insrc/util/settings.cpp.Also applies to: 69-69
test/functional/rpc_blockchain.py (1)
252-283: LGTM! Comprehensive test coverage for getdeploymentinfo RPC.The test method properly validates:
- Deployment info at the current tip
- Deployment info at a specific height
- Historical queries using explicit block hash
- Status transitions (started → locked_in)
The test setup with various activation heights and the validation helper provide good coverage of the new RPC endpoint.
src/index/coinstatsindex.h (2)
12-12: LGTM! Migration to kernel namespace.The include change from
node/coinstats.htokernel/coinstats.hcorrectly reflects the UTXO statistics migration to the kernel namespace as outlined in the PR objectives.
59-59: Excellent API modernization with std::optional.The signature change from an out-parameter pattern to returning
std::optional<kernel::CCoinsStats>is a significant improvement:
- Clearer semantics:
std::nulloptexplicitly indicates "not found"- Eliminates need for separate boolean return + out-parameter
- More idiomatic modern C++
- Safer: impossible to use uninitialized data on failure
src/wallet/scriptpubkeyman.cpp (3)
2212-2212: LGTM: Correct in-place merge pattern.The change from
*out_keys = Merge(*out_keys, it->second)toout_keys->Merge(FlatSigningProvider{it->second})correctly uses the new in-place member function. Creating a temporary via brace-initialization is appropriate for merging cached data.
2249-2249: LGTM: Appropriate use of move semantics.The in-place merge with
std::move(*coin_keys)is correct. Sincecoin_keysis a temporary signing provider that won't be reused after this merge, moving it is efficient and appropriate.
2321-2321: LGTM: Consistent in-place merge pattern in PSBT filling.Both merge calls correctly use the new in-place member function with move semantics. Since
script_keysandpk_keysare temporary providers used only for accumulation, moving them into the destination is efficient and correct.Also applies to: 2329-2329
src/test/validation_block_tests.cpp (1)
102-102: LGTM: Consistent API migration removing Params() arguments.All call sites correctly updated to omit the
Params()argument fromProcessNewBlockHeadersandProcessNewBlock. This aligns with the PR's broader change whereChainstateManagernow holdsCChainParamsinternally, eliminating the need to pass it explicitly.Also applies to: 159-159, 181-181, 187-187, 226-226
src/test/coinstatsindex_tests.cpp (2)
15-16: LGTM: Correct namespace migration for coin stats types.Using declarations properly updated to reference
kernel::CCoinsStatsandkernel::CoinStatsHashType, aligning with the broader refactor moving these types into the kernel namespace.
31-31: LGTM: Clean migration to optional return pattern.All
LookUpStatscall sites correctly updated to:
- Call with only
block_indexparameter (no out-parameter)- Check the returned
std::optionalfor presence/absence- Rely on the optional's boolean conversion in
BOOST_CHECKThis is a cleaner API that makes the miss-vs-hit distinction explicit.
Also applies to: 47-47, 50-50, 64-64
src/test/evo_deterministicmns_tests.cpp (1)
266-266: LGTM: Consistent ProcessNewBlock API migration across Dash tests.All
ProcessNewBlockcall sites correctly updated to remove theParams()argument. The changes are consistent with the broader API refactor whereChainstateManagernow internally holdsCChainParams. Test behavior and assertions remain unchanged.Also applies to: 276-276, 304-304, 322-322, 342-342, 642-642, 788-788, 821-821, 834-834
src/script/signingprovider.cpp (1)
58-65: LGTM: Clean in-place merge implementation.The new member function correctly:
- Uses
std::map::mergeto transfer elements from source to destination (preserving existing entries)- Accepts rvalue reference to enable move semantics
- Merges all four member containers (
scripts,pubkeys,keys,origins)- Returns
*thisto enable method chainingThis is a cleaner API than the previous free function, making the in-place mutation explicit.
src/index/coinstatsindex.cpp (2)
17-20: LGTM: Correct kernel namespace usage.Using declarations properly reference
kernel::CCoinsStats,kernel::GetBogoSize, andkernel::TxOutSer, aligning with the coin stats types migration to the kernel namespace.
320-345: LGTM: Clean migration to optional return pattern.The refactored
LookUpStatscorrectly:
- Returns
std::optional<CCoinsStats>instead of using an out-parameter- Constructs
CCoinsStatslocally withblock_heightandblock_hashconstructor- Sets
index_used = trueto indicate index was queried- Populates all 13 stats fields from the DB entry
- Returns
std::nullopton DB miss, making the miss-vs-hit case explicitThis is a cleaner, more modern API that eliminates the need for callers to pre-construct a stats object.
src/init.cpp (1)
1938-1939: Ctor update is consistent with new API.Instantiation with
chainparamsaligns with the refactor.Please verify all other
ChainstateManagerconstructions passCChainParams.src/test/descriptor_tests.cpp (4)
78-80: Signature refactor: norm_pub only.Updated DoCheck signature is consistent and clear.
237-244: New Merge usage: verify return type expectations.
FlatSigningProvider{...}.Merge(...)is used as an rvalue where aconst SigningProvider&is expected. EnsureMergereturns a reference/value usable in this context.Run build/tests to confirm no lifetime issues.
271-295: Wrapper Check updates look consistent.Apostrophe replacement paths remain covered; passing
norm_pubthroughout is correct.
301-346: Bulk test vector updates.Vectors reflect the new provider‑merge approach and normalization; looks fine.
src/kernel/coinstats.cpp (1)
139-165: Dispatcher looks correct.Optional handling and block index lookup align with callers.
src/validation.h (3)
101-110: Prune target bump to 945 MiB.Matches PR rationale for Dash disk space; OK.
1082-1100: Deployment helpers passthrough.Thin wrappers are fine and reduce param threading.
908-931: All call sites correctly updated; chainparams successfully embedded in ChainstateManager.Verification confirms the changes are complete:
- Constructor is explicit and embeds
CChainParamsvia member initialization- Accessors (
GetParams(),GetConsensus()) are clean and functionalProcessNewBlockandProcessNewBlockHeaderssignatures no longer takeCChainParamsparameters—they now access it viathis->m_chainparams- All call sites (across
net_processing.cpp,rpc/mining.cpp, test files) correctly invoke methods on thechainmaninstance without passingCChainParamsdirectlysrc/rpc/blockchain.cpp (9)
23-23: LGTM: Clean migration to kernel namespace.The header include change from
node/coinstats.htokernel/coinstats.hand the corresponding using declarations properly reflect the migration of UTXO statistics APIs to the kernel namespace, as confirmed by the relevant code snippets.Also applies to: 66-68
1106-1129: LGTM: Well-structured UTXO stats retrieval function.The GetUTXOStats wrapper properly handles two paths:
- Using CoinStatsIndex when available and requested (for MUHASH/NONE)
- Computing stats directly via kernel::ComputeUTXOStats otherwise
The CHECK_NONFATAL at line 1126 correctly validates that without the index, pindex must be null or equal to the view's best block, which is a necessary constraint.
1186-1186: LGTM: Proper optional handling for GetUTXOStats.The updates correctly adapt to the new GetUTXOStats API that returns
std::optional<kernel::CCoinsStats>:
- Lines 1229-1231: Properly checks
has_value()before accessing stats- Lines 1250-1256: Correctly handles the case where previous block stats cannot be retrieved, throwing an appropriate error
- Consistent use of the
index_requestedparameter throughoutAlso applies to: 1207-1217, 1229-1276
1400-1492: LGTM: Clean refactoring to use ChainstateManager.The SoftForkDescPushBack functions have been properly refactored to accept
ChainstateManagerinstead of separate consensus parameters:
- Both buried and BIP9 deployment versions now consistently use
chainman.GetConsensus()to access consensus parameters- Line 1420: Sensible early return when blockindex is null for BIP9 deployments
- Lines 1461-1477: Signalling bitstring generation is correct (using '#' for signaling blocks and '-' for non-signaling)
This aligns with the architectural change of ChainstateManager holding CChainParams as described in the AI summary.
1494-1498: LGTM: Proper deprecation handling for softforks reporting.The changes correctly implement the deprecation of softforks reporting in getblockchaininfo:
- Lines 1494-1498: Forward declarations properly handle the anonymous namespace structure
- Lines 1579-1582: Softforks are only included when
-deprecatedrpc=softforksis enabled, maintaining backward compatibility while encouraging migration to getdeploymentinfo- The help text clearly documents the deprecation at lines 1526-1531
This aligns with the PR objectives stating that requesting deployment info from getblockchaininfo is deprecated in favor of getdeploymentinfo.
Also applies to: 1501-1588
1590-1696: LGTM: Well-implemented getdeploymentinfo RPC.The new getdeploymentinfo RPC provides a clean interface for querying deployment information:
- Lines 1655, 1676-1685: Properly handles optional blockhash parameter, defaulting to chain tip
- Line 1681: Validates block existence before proceeding
- Lines 1619-1647: DeploymentInfo function correctly iterates through both buried deployments and EHF-enabled deployments (V24, TESTDUMMY)
- The implementation properly separates concerns by delegating to the refactored SoftForkDescPushBack functions
This is the recommended replacement for the deprecated softforks field in getblockchaininfo.
2825-2825: LGTM: Consistent optional handling in CreateUTXOSnapshot.The changes properly adapt CreateUTXOSnapshot to use the new GetUTXOStats API:
- Line 2825: Declares
maybe_statsasstd::optional<CCoinsStats>- Lines 2845-2848: Calls GetUTXOStats and validates the result before proceeding
- Lines 2851, 2858, 2880, 2884: All accesses to stats fields use the
->operator, which is safe after the validation checkThe error handling is appropriate and consistent with the RPC error reporting pattern used elsewhere in this file.
Also applies to: 2845-2889
2907-2907: LGTM: Proper RPC registration.The getdeploymentinfo RPC is correctly registered in the blockchain category at line 2907, maintaining alphabetical order with other commands.
1106-1129: No changes required—GetUTXOStats visibility is correctly implemented.The function is properly declared in
src/rpc/blockchain.hand used externally insrc/init.cpp, confirming it is correctly exposed as a public API. The code follows the appropriate visibility pattern for functions that need to be called across module boundaries.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/init.cpp (1)
834-846: Don't hold cs_main while computing UTXO stats.
GetUTXOStats(...)can index/scan LevelDB and block for a while. Holdingcs_mainhere increases contention and risks deadlocks. Capture the DB pointer under lock, release it, and call withoutcs_main; passpindex=nullptrto avoid CHECK_NONFATAL trips when the index is unavailable.- const auto maybe_stats = WITH_LOCK(::cs_main, return GetUTXOStats(&chainman.ActiveChainstate().CoinsDB(), chainman.m_blockman, /*hash_type=*/CoinStatsHashType::NONE, node.rpc_interruption_point, chainman.ActiveChain().Tip(), /*index_requested=*/true)); + CCoinsView* coinsdb{nullptr}; + { + LOCK(::cs_main); + coinsdb = &chainman.ActiveChainstate().CoinsDB(); + } + const auto maybe_stats = GetUTXOStats( + coinsdb, + chainman.m_blockman, + /*hash_type=*/CoinStatsHashType::NONE, + node.rpc_interruption_point, + /*pindex=*/nullptr, + /*index_requested=*/true);src/kernel/coinstats.cpp (1)
56-72: Critical: precedence bug corrupts UTXO stats hash.
?:binds to the sum; this changes the commitment format. Must wrap the conditional.- ss << VARINT(it->second.nHeight * 2 + it->second.fCoinBase ? 1u : 0u); + ss << VARINT(it->second.nHeight * 2u + (it->second.fCoinBase ? 1u : 0u));
♻️ Duplicate comments (3)
src/test/descriptor_tests.cpp (1)
320-320: Remove stray debug comment.The comment
/* --- it fails!!! */appears to be leftover debug noise that should be removed to keep the test file clean.Apply this diff:
- /* --- it fails!!! */src/init.cpp (1)
34-34: Resolved: coinstats types are correctly included.Adding
<kernel/coinstats.h>fixes prior missing-declaration issues when using kernel coinstats types.src/kernel/coinstats.h (1)
16-16: Resolved: explicit<optional>include.Good fix to avoid relying on transitive headers.
🧹 Nitpick comments (5)
test/functional/rpc_blockchain.py (1)
195-250: Fix indentation in dictionary literal.The continuation lines in the dictionary literal are under-indented. The keys should align with the opening brace or use consistent 4-space indentation.
Apply this diff to fix the indentation:
assert_equal(gdi_result, { - "hash": blockhash, - "height": height, - "deployments": { + "hash": blockhash, + "height": height, + "deployments": { 'bip34': {'type': 'buried', 'active': True, 'height': 2},(Similar adjustments should be made to all continuation lines in this dictionary to maintain consistent indentation throughout the structure.)
src/util/system.cpp (1)
643-674: Excellent refactoring! Helper functions improve maintainability.Extracting
SettingToString,SettingToInt, andSettingToBoolas standalone functions:
- Eliminates code duplication from the original
Get*Argimplementations- Enables reuse by other parts of the codebase (as seen in the node interfaces per the relevant code snippets)
- Preserves the original conversion logic exactly
This is a well-executed refactoring that enhances code clarity without changing behavior.
src/validation.cpp (3)
4056-4076: ContextualCheckBlockHeader switched to ChainstateManager — small nit.Logic is intact. Prefer std::abs over abs for doubles to avoid overload ambiguity.
- if (abs(n1-n2) > n1*0.5) { + if (std::abs(n1 - n2) > n1 * 0.5) {
5939-5955: Snapshot validation now uses kernel::ComputeUTXOStats (optional) and verifies assumeutxo hash — solid.Optional: add a brief comment noting HASH_SERIALIZED is required for assumeutxo verification.
Also applies to: 5956-5964, 5996-6003
146-147: Remaining direct Params() usages (informational).A few legacy paths still call Params() (e.g., subsidy helpers). Given backport scope, OK to defer consolidating to chainman/m_params.
Based on learnings
Also applies to: 317-350, 4098-4104
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
configure.ac(1 hunks)doc/REST-interface.md(1 hunks)doc/release-notes-6901.md(1 hunks)src/Makefile.am(3 hunks)src/index/coinstatsindex.cpp(2 hunks)src/index/coinstatsindex.h(2 hunks)src/init.cpp(5 hunks)src/interfaces/node.h(2 hunks)src/kernel/coinstats.cpp(5 hunks)src/kernel/coinstats.h(3 hunks)src/net_processing.cpp(4 hunks)src/node/interfaces.cpp(1 hunks)src/rest.cpp(2 hunks)src/rpc/blockchain.cpp(15 hunks)src/rpc/blockchain.h(3 hunks)src/rpc/mining.cpp(3 hunks)src/rpc/util.h(0 hunks)src/script/descriptor.cpp(1 hunks)src/script/signingprovider.cpp(1 hunks)src/script/signingprovider.h(2 hunks)src/test/blockfilter_index_tests.cpp(4 hunks)src/test/coinstatsindex_tests.cpp(3 hunks)src/test/descriptor_tests.cpp(5 hunks)src/test/evo_deterministicmns_tests.cpp(9 hunks)src/test/fuzz/coins_view.cpp(0 hunks)src/test/fuzz/rpc.cpp(1 hunks)src/test/fuzz/utxo_snapshot.cpp(1 hunks)src/test/fuzz/versionbits.cpp(4 hunks)src/test/miner_tests.cpp(1 hunks)src/test/settings_tests.cpp(3 hunks)src/test/util/mining.cpp(1 hunks)src/test/util/setup_common.cpp(3 hunks)src/test/validation_block_tests.cpp(4 hunks)src/test/validation_chainstate_tests.cpp(1 hunks)src/util/settings.cpp(2 hunks)src/util/settings.h(1 hunks)src/util/system.cpp(6 hunks)src/util/system.h(2 hunks)src/validation.cpp(28 hunks)src/validation.h(8 hunks)src/versionbits.cpp(3 hunks)src/versionbits.h(3 hunks)src/wallet/rpc/spend.cpp(1 hunks)src/wallet/scriptpubkeyman.cpp(3 hunks)test/functional/feature_asset_locks.py(1 hunks)test/functional/feature_cltv.py(1 hunks)test/functional/feature_dersig.py(1 hunks)test/functional/feature_governance.py(5 hunks)test/functional/feature_mnehf.py(2 hunks)test/functional/feature_new_quorum_type_activation.py(1 hunks)test/functional/interface_rest.py(1 hunks)test/functional/p2p_dos_header_tree.py(4 hunks)test/functional/rpc_blockchain.py(4 hunks)test/functional/test_framework/blocktools.py(1 hunks)test/functional/test_framework/util.py(1 hunks)test/functional/wallet_crosschain.py(3 hunks)test/functional/wallet_importdescriptors.py(1 hunks)test/functional/wallet_signrawtransactionwithwallet.py(2 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
💤 Files with no reviewable changes (2)
- src/rpc/util.h
- src/test/fuzz/coins_view.cpp
🚧 Files skipped from review as they are similar to previous changes (33)
- test/functional/test_framework/blocktools.py
- test/functional/feature_asset_locks.py
- test/functional/test_framework/util.py
- test/functional/feature_dersig.py
- src/test/validation_chainstate_tests.cpp
- src/script/descriptor.cpp
- src/script/signingprovider.h
- src/test/fuzz/rpc.cpp
- test/functional/p2p_dos_header_tree.py
- src/test/validation_block_tests.cpp
- test/functional/wallet_signrawtransactionwithwallet.py
- test/functional/wallet_importdescriptors.py
- src/node/interfaces.cpp
- src/interfaces/node.h
- test/lint/lint-circular-dependencies.py
- src/util/settings.h
- src/versionbits.h
- src/test/fuzz/utxo_snapshot.cpp
- src/util/system.h
- test/functional/interface_rest.py
- src/Makefile.am
- doc/release-notes-6901.md
- src/rpc/mining.cpp
- src/util/settings.cpp
- test/functional/wallet_crosschain.py
- src/wallet/rpc/spend.cpp
- test/functional/feature_cltv.py
- src/rpc/blockchain.h
- src/index/coinstatsindex.cpp
- src/test/util/mining.cpp
- src/wallet/scriptpubkeyman.cpp
- src/rest.cpp
- src/test/miner_tests.cpp
🧰 Additional context used
📓 Path-based instructions (4)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/rpc_blockchain.pytest/functional/feature_governance.pytest/functional/feature_new_quorum_type_activation.pytest/functional/feature_mnehf.py
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/test/util/setup_common.cppsrc/validation.hsrc/kernel/coinstats.hsrc/index/coinstatsindex.hsrc/script/signingprovider.cppsrc/kernel/coinstats.cppsrc/test/descriptor_tests.cppsrc/util/system.cppsrc/versionbits.cppsrc/net_processing.cppsrc/test/fuzz/versionbits.cppsrc/test/blockfilter_index_tests.cppsrc/test/coinstatsindex_tests.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/settings_tests.cppsrc/rpc/blockchain.cppsrc/init.cppsrc/validation.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/util/setup_common.cppsrc/test/descriptor_tests.cppsrc/test/fuzz/versionbits.cppsrc/test/blockfilter_index_tests.cppsrc/test/coinstatsindex_tests.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/settings_tests.cpp
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/REST-interface.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
🧬 Code graph analysis (18)
test/functional/rpc_blockchain.py (3)
test/functional/test_framework/util.py (1)
assert_equal(69-74)test/functional/test_framework/test_framework.py (1)
start_node(649-658)src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
src/test/util/setup_common.cpp (2)
src/validation.cpp (6)
chainparams(569-580)chainparams(569-571)chainparams(583-593)chainparams(583-584)chainparams(596-606)chainparams(596-597)src/chainparams.cpp (2)
Params(1354-1357)Params(1354-1354)
src/validation.h (1)
src/validation.cpp (5)
ChainstateManager(6049-6059)ProcessNewBlock(4435-4472)ProcessNewBlock(4435-4435)ProcessNewBlockHeaders(4314-4341)ProcessNewBlockHeaders(4314-4314)
src/kernel/coinstats.h (2)
src/rpc/blockchain.h (2)
kernel(26-28)node(33-36)src/kernel/coinstats.cpp (9)
CCoinsStats(21-23)GetBogoSize(26-34)GetBogoSize(26-26)TxOutSer(36-42)TxOutSer(36-36)ComputeUTXOStats(100-137)ComputeUTXOStats(100-100)ComputeUTXOStats(139-165)ComputeUTXOStats(139-139)
src/index/coinstatsindex.h (2)
src/kernel/coinstats.cpp (1)
CCoinsStats(21-23)src/index/coinstatsindex.cpp (2)
LookUpStats(320-345)LookUpStats(320-320)
src/script/signingprovider.cpp (1)
src/script/descriptor.cpp (11)
scripts(759-764)scripts(759-759)pubkeys(512-512)keys(712-712)keys(712-712)keys(723-728)keys(723-723)keys(742-749)keys(742-742)keys(780-793)keys(780-780)
src/test/descriptor_tests.cpp (1)
src/script/descriptor.cpp (6)
scripts(759-764)scripts(759-759)LEGACY(715-715)LEGACY(731-731)LEGACY(797-797)nullopt(657-657)
src/util/system.cpp (2)
src/node/interfaces.cpp (16)
name(447-456)name(447-447)name(457-457)name(457-457)name(458-468)name(458-458)name(469-478)name(469-469)name(1130-1133)name(1130-1130)name(1134-1137)name(1134-1134)name(1138-1141)name(1138-1138)name(1142-1151)name(1142-1142)src/util/settings.cpp (2)
GetSetting(140-195)GetSetting(140-145)
src/versionbits.cpp (2)
src/validation.cpp (14)
pindex(2205-2205)pindex(2205-2205)pindex(2209-2215)pindex(2209-2209)pindex(3131-3138)pindex(3131-3131)params(2203-2203)params(2203-2203)params(2204-2204)params(2204-2204)params(2206-2206)params(2206-2206)params(2207-2207)params(2207-2207)src/test/fuzz/versionbits.cpp (15)
pindex(45-45)pindex(45-45)pindex(47-47)pindex(47-47)pindex(55-55)pindex(55-55)pindex(63-63)pindex(63-63)params(46-46)params(46-46)params(48-48)params(48-48)params(49-49)params(49-49)params(50-50)
test/functional/feature_governance.py (1)
src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
src/test/fuzz/versionbits.cpp (1)
src/versionbits.cpp (4)
pindex(258-261)pindex(258-258)GetStateStatisticsFor(134-180)GetStateStatisticsFor(134-134)
src/test/coinstatsindex_tests.cpp (2)
src/kernel/coinstats.cpp (1)
CCoinsStats(21-23)src/kernel/coinstats.h (1)
CoinStatsHashType(24-77)
src/test/settings_tests.cpp (2)
src/util/settings.cpp (2)
GetSetting(140-195)GetSetting(140-145)src/util/system.cpp (2)
GetSetting(1179-1185)GetSetting(1179-1179)
src/rpc/blockchain.cpp (6)
src/kernel/coinstats.cpp (5)
CCoinsStats(21-23)ComputeUTXOStats(100-137)ComputeUTXOStats(100-100)ComputeUTXOStats(139-165)ComputeUTXOStats(139-139)src/kernel/coinstats.h (2)
CoinStatsHashType(24-77)node(19-21)src/rpc/server.cpp (2)
IsDeprecatedRPCEnabled(367-372)IsDeprecatedRPCEnabled(367-367)src/deploymentinfo.cpp (2)
DeploymentName(20-56)DeploymentName(20-20)src/rpc/server_util.cpp (4)
EnsureAnyNodeContext(28-45)EnsureAnyNodeContext(28-28)EnsureChainman(87-93)EnsureChainman(87-87)src/rest.cpp (1)
getdeploymentinfo(624-624)
src/init.cpp (4)
src/kernel/coinstats.h (1)
CoinStatsHashType(24-77)src/rpc/blockchain.cpp (2)
GetUTXOStats(1106-1129)GetUTXOStats(1106-1110)src/util/system.cpp (2)
CheckDiskSpace(158-164)CheckDiskSpace(158-158)src/fs.h (1)
quoted(94-97)
test/functional/feature_new_quorum_type_activation.py (1)
test/functional/test_framework/test_framework.py (1)
no_op(803-804)
test/functional/feature_mnehf.py (1)
test/functional/test_framework/test_framework.py (1)
generate(806-809)
src/validation.cpp (2)
src/kernel/coinstats.cpp (5)
CCoinsStats(21-23)ComputeUTXOStats(100-137)ComputeUTXOStats(100-100)ComputeUTXOStats(139-165)ComputeUTXOStats(139-139)src/kernel/coinstats.h (1)
CoinStatsHashType(24-77)
🪛 Flake8 (7.3.0)
test/functional/rpc_blockchain.py
[error] 196-196: continuation line under-indented for hanging indent
(E121)
🪛 markdownlint-cli2 (0.18.1)
doc/REST-interface.md
88-88: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: predict_conflicts
- GitHub Check: Lint / Run linters
🔇 Additional comments (56)
test/functional/feature_new_quorum_type_activation.py (1)
25-25: Correct adjustment for upstream behavior change.The increment from 9 to 10 blocks is necessary due to bitcoin#23508's change in elapsed block computation. With a window of 10 blocks, the 'started' status is now visible at block 10 (start of period 1) rather than block 9 (end of period 0), aligning with upstream behavior while preserving Dash's dynamic activation threshold logic.
src/script/signingprovider.cpp (1)
58-65: LGTM! Clean refactor to in-place member function.The conversion from a free function to an in-place member function using move semantics is well-implemented. The use of
container.merge()efficiently moves elements frombintothis, preserving existing entries when keys conflict, and the returned reference enables method chaining.src/test/descriptor_tests.cpp (3)
78-78: LGTM! Signature change aligns with upstream.The parameter change from
norm_prvtonorm_pubis semantically correct—normalized descriptors should represent the public form. All call sites have been updated consistently throughout the test file.
237-243: LGTM! Temporary lifetime is safe.The new merge pattern
FlatSigningProvider{A}.Merge(FlatSigningProvider{B})correctly creates temporaries that live until the end of the full expression, ensuring safe reference passing toIsSolvableandSignSignature. The rvalue reference binding inMergeworks correctly with the temporary objects.
304-333: LGTM! Test cases updated consistently.All test cases have been systematically updated to pass
norm_pubas the third parameter, maintaining consistency with the new function signatures. The test data correctly provides normalized public descriptor forms.src/test/fuzz/versionbits.cpp (4)
55-55: LGTM! Function signature correctly updated.The signature change adds the optional
signalsparameter and renamespindexPrevtopindex, matching the updated implementation insrc/versionbits.cpp(line 134).
224-231: LGTM! Proper initialization of statistics tracking.The initialization correctly sets up baseline statistics reflecting the end of the previous period, with
countandelapsedat zero andpossiblecomputed as(period >= threshold). The emptylast_signalsvector is properly initialized for per-block signalling history.
253-270: LGTM! Comprehensive validation of statistics collection.The test thoroughly validates the new signalling API by:
- Collecting statistics both with and without the
signalsparameter and asserting consistency (lines 254-258)- Validating per-block signal vector evolution: size growth, current signal value, and cumulative history (lines 267-270)
This ensures the optional
signalling_blocksparameter doesn't affect core statistics computation.
287-289: LGTM! Final period-end assertions are correct.The assertions properly validate that at the period boundary:
elapsedequals the full period lengthcountmatches the total number of signalling blockspossibleis correctly computed using the formula(count + period >= elapsed + threshold)src/versionbits.cpp (6)
134-134: LGTM! Function signature correctly extended.The signature now accepts an optional
signalling_blocksparameter to enable per-block signalling data collection, consistent with the upstream backport (bitcoin#23508).
141-149: LGTM! Proper initialization of period tracking.The code correctly:
- Returns early with zero-initialized stats when
pindexis nullptr- Calculates
blocks_in_periodas the number of blocks from the start of the current period throughpindex(inclusive)- Initializes the
signalling_blocksvector with the appropriate size
152-159: LGTM! Preserves Dash-specific threshold logic.The code correctly retains
pindexEndOfPrevPeriodfor dynamic threshold calculation (Dash-specific logic from dash#3692) while no longer using it to computeelapsed, aligning with the PR objective and upstream behavior change from bitcoin#23508.
162-173: LGTM! Loop logic correctly counts blocks and populates signalling data.The do-while loop properly:
- Counts blocks in the current period by incrementing
elapsed- Uses
blocks_in_periodas a countdown index for thesignalling_blocksvector- Populates
signalling_blocksin chronological order (index 0 = earliest block in period)- Terminates when all blocks in the period have been processed
The indexing is safe:
blocks_in_periodis decremented before use (line 167), ensuring access within valid range[0, blocks_in_period-1].
175-175: LGTM! Elapsed now computed directly from block counting.Setting
stats.elapsedfrom the loop counter rather than deriving it frompindexEndOfPrevPeriodimplements the upstream behavior change from bitcoin#23508. This approach is more explicit and straightforward.
276-279: LGTM! Statistics wrapper correctly updated.The
VersionBitsCache::Statisticsmethod signature is properly extended to accept thesignalling_blocksparameter and forwards it to the underlyingGetStateStatisticsForimplementation. The parameter name change frompindexPrevtopindexmaintains consistency with the implementation.src/test/coinstatsindex_tests.cpp (2)
15-16: LGTM! Namespace migration correctly applied.The using declarations properly reference the kernel namespace for CCoinsStats and CoinStatsHashType, aligning with the API relocation described in the PR objectives.
31-31: LGTM! Test correctly adapted to optional-returning API.The test properly handles the new
LookUpStatssignature that returnsstd::optional<kernel::CCoinsStats>instead of taking an out-parameter. The boolean checks leverage implicit conversion from optional (nullopt converts to false, populated optional converts to true).Also applies to: 47-47, 50-50, 64-64
src/test/evo_deterministicmns_tests.cpp (1)
266-266: LGTM! ProcessNewBlock calls consistently updated.All 9 call sites correctly adapted to the new signature that no longer requires the
Params()argument. The ChainstateManager now internally manages chain parameters, simplifying the call sites.Also applies to: 276-276, 304-304, 322-322, 342-342, 642-642, 788-788, 821-821, 833-833
src/test/blockfilter_index_tests.cpp (2)
105-105: LGTM! ProcessNewBlockHeaders signature correctly updated.The call properly omits the
Params()argument, aligning with the API change where ChainstateManager now manages chain parameters internally.
177-177: LGTM! ProcessNewBlock calls consistently updated.All three call sites correctly adapted to remove the
Params()argument, consistent with the signature changes throughout the codebase.Also applies to: 195-195, 226-226
src/util/system.cpp (4)
562-575: LGTM! Clean extension for backup file support.The addition of the
backupparameter with conditional ".bak" suffix appending is straightforward and maintains backward compatibility with the defaultfalsevalue.
611-629: LGTM! Backup support correctly propagated.The
WriteSettingsFilemethod properly passes the backup flag to bothGetSettingsPathcalls (for the target path and temporary path), ensuring consistent backup file naming.
631-636: LGTM! GetPersistentSetting correctly filters transient settings.The new method appropriately calls
util::GetSettingwithignore_nonpersistent=true, ensuring that only configuration file and read/write settings file values are returned, excluding command-line arguments and forced settings. This is useful for distinguishing persistent configuration from runtime overrides.
1114-1114: LGTM! GetSetting calls correctly use extended signature.The additional boolean parameters are properly set:
- Line 1114 (
GetChainName): Usesignore_nonpersistent=falseto include command-line chain selection arguments- Lines 1183-1184 (
GetSetting): Usesignore_nonpersistent=false, get_chain_name=falsefor standard setting retrievalThese parameter values align with the intended behavior for each context.
Also applies to: 1183-1184
src/net_processing.cpp (4)
2768-2770: Rename to tx_iter is fine; semantics unchanged.Locked access, null/empty check, and deref remain correct. No action needed.
3502-3503: Updated ProcessNewBlock call aligns with new signature.Force-processing and new_block semantics unchanged; mapBlockSource cleanup remains intact.
4731-4732: CMPCTBLOCK path: header processing migrated to new API.Consistent with earlier change; invalid-header punishment remains correct.
3197-3198: ProcessNewBlockHeaders and ProcessNewBlock API migration verified across all call sites.Verification confirms all 15+ call sites (net_processing.cpp, test files, rpc/mining.cpp) use the new 3-parameter signatures without chainparams. The target calls at lines 3197–3198 and 3502 match the updated API. Invalid state handling preserved. No old-style 4-parameter calls remain.
src/init.cpp (2)
137-137: Namespace swap acknowledged.Switching to
using kernel::CoinStatsHashType;aligns with the move from node:: to kernel::.
1938-1939: Ctor update looks correct.Constructing
ChainstateManager(chainparams)matches the new API and centralizes param access.src/kernel/coinstats.h (1)
23-30: API move to kernel namespace LGTM.
CoinStatsHashTypeandCCoinsStatsrelocation is consistent with callers.src/kernel/coinstats.cpp (1)
139-165: ComputeUTXOStats dispatcher looks good.Height/hash initialization and hash-type routing align with upstream patterns.
If you want, we can sanity-check that all call sites expect
std::optional<CCoinsStats>.src/validation.h (4)
17-20: Include additions acceptable.Pulling in
chainparams.handdeploymentstatus.hhere is consistent with the new inline wrappers.
908-931: ChainstateManager now owns params — good direction.
m_chainparams+GetParams()/GetConsensus()centralize access and simplify callers.
1082-1100: Wrapper Deployment helpers LGTM.*Inline wrappers delegate to
deploymentstatus.husing consensus fromchainman.
1046-1059: All call sites correctly use ChainstateManager — no stale or unqualified calls found.Verification of
ProcessNewBlockandProcessNewBlockHeaderscall sites shows all usages are properly qualified:
- Method definitions in src/validation.cpp correctly implement
ChainstateManager::ProcessNewBlockandChainstateManager::ProcessNewBlockHeaders- All 25 ProcessNewBlock call sites use qualified member access (e.g.,
m_chainman.ProcessNewBlock(),chainman.ProcessNewBlock())- All 8 ProcessNewBlockHeaders call sites similarly use qualified member access
- No stale free-function calls or unqualified invocations detected
The signature changes have been successfully propagated throughout the codebase.
src/rpc/blockchain.cpp (7)
23-23: LGTM: Namespace migration properly implemented.The include and using declarations correctly reflect the migration of CCoinsStats and CoinStatsHashType to the kernel namespace.
Also applies to: 66-68
1106-1129: LGTM: Well-structured optional return pattern.The GetUTXOStats function correctly implements the optional return pattern with proper branching for index vs. direct computation paths. The CHECK_NONFATAL on line 1126 appropriately enforces the invariant that without the coinstats index, the pindex must match the view's best block.
1229-1276: LGTM: Proper optional handling with error reporting.The optional return values from GetUTXOStats are handled correctly with appropriate error messages when stats cannot be retrieved. The nested optional handling for prev_stats (lines 1250-1257) is also correct.
1579-1582: LGTM: Proper deprecation handling.The conditional inclusion of softforks data based on the
-deprecatedrpc=softforksflag correctly implements the breaking change mentioned in the PR objectives. The help text appropriately marks the field as optional and deprecated.
1650-1696: LGTM: New getdeploymentinfo RPC properly implemented.The new RPC provides a clean replacement for the deprecated softforks field in getblockchaininfo. The implementation correctly:
- Accepts an optional blockhash parameter
- Fetches EHF signals from mnhf_manager
- Returns comprehensive deployment information via DeploymentInfo
2825-2889: LGTM: Correct optional handling in CreateUTXOSnapshot.The optional CCoinsStats is properly checked at line 2846 before being accessed throughout the rest of the function. All member accesses use the
->operator on the optional after confirming it has a value.
2907-2907: LGTM: RPC command properly registered.The getdeploymentinfo RPC is correctly added to the blockchain command table.
src/validation.cpp (13)
22-22: Kernel coinstats migration looks correct.Header and using-decls align with the new ComputeUTXOStats API. No issues.
Also applies to: 69-72
1023-1027: Consensus checks use chainman-driven flags — LGTM.Using GetBlockScriptFlags(tip, m_chainman) keeps mempool cache checks consistent with active consensus.
1432-1441: Using active_chainstate.m_params in ProcessNewPackage.Good removal of ::Params() here; consistent with storing CChainParams in CChainState.
1656-1658: CChainState now caches params from ChainstateManager.Solid dependency direction; avoids globals. No concerns.
2416-2421: CSV enforcement and script flags now use ChainstateManager.This brings ConnectBlock in line with the new deployment helpers. Looks good.
4084-4104: Checkpoints and version gating via chainman — LGTM.Uses blockman + chainman params correctly.
4183-4311: ChainstateManager::AcceptBlockHeader updated to use internal params/consensus.Flow and failure handling preserved. Good.
4315-4341: ProcessNewBlockHeaders refactor — progress log OK.Minor: progress estimation can be noisy early; optional clamp already present. No action required.
4356-4433: AcceptBlock switched to ContextualCheckBlock(chainman).Consistent with header changes; state updates maintained.
4435-4472: ProcessNewBlock uses GetConsensus() and AcceptBlock flow unchanged.Good lock scoping and signals. No issues.
4519-4540: TestBlockValidity updated to chainman-based context.Matches new signatures; good.
4115-4181: ContextualCheckBlock migration verified — all call sites updated correctly.Search confirms that both call sites (lines 4396 and 4526 in src/validation.cpp) pass the new
chainmanparameter. No lingering old-style calls exist.
352-354: GetBlockScriptFlags refactor verified — all call sites properly updated.Comprehensive search of the C++ codebase confirms all GetBlockScriptFlags references are using the new signature with ChainstateManager parameter. Both call sites (lines 1022 and 2420) are correctly passing chainman arguments. No stale calls remain.
| int chain_active_height = WITH_LOCK(cs_main, return chainman.ActiveChain().Height()); | ||
|
|
||
| // On first startup, warn on low block storage space | ||
| if (!fReindex && !fReindexChainState && chain_active_height <= 1) { | ||
| uint64_t additional_bytes_needed = fPruneMode ? nPruneTarget | ||
| : chainparams.AssumedBlockchainSize() * 1024 * 1024 * 1024; | ||
|
|
||
| if (!CheckDiskSpace(args.GetBlocksDirPath(), additional_bytes_needed)) { | ||
| InitWarning(strprintf(_( | ||
| "Disk space for %s may not accommodate the block files. " \ | ||
| "Approximately %u GB of data will be stored in this directory." | ||
| ), | ||
| fs::quoted(fs::PathToString(args.GetBlocksDirPath())), | ||
| chainparams.AssumedBlockchainSize() | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix first‑startup disk‑space warning for -prune=1.
For manual prune (-prune=1) nPruneTarget == UINT64_MAX, so this branch always warns. Use the smaller of nPruneTarget and AssumedBlockchainSize().
- // On first startup, warn on low block storage space
+ // On first startup, warn on low block storage space
if (!fReindex && !fReindexChainState && chain_active_height <= 1) {
- uint64_t additional_bytes_needed = fPruneMode ? nPruneTarget
- : chainparams.AssumedBlockchainSize() * 1024 * 1024 * 1024;
+ const uint64_t approx_chain_bytes =
+ static_cast<uint64_t>(chainparams.AssumedBlockchainSize()) * 1024 * 1024 * 1024;
+ uint64_t additional_bytes_needed =
+ fPruneMode ? std::min(nPruneTarget, approx_chain_bytes) : approx_chain_bytes;
- if (!CheckDiskSpace(args.GetBlocksDirPath(), additional_bytes_needed)) {
- InitWarning(strprintf(_(
- "Disk space for %s may not accommodate the block files. " \
- "Approximately %u GB of data will be stored in this directory."
- ),
- fs::quoted(fs::PathToString(args.GetBlocksDirPath())),
- chainparams.AssumedBlockchainSize()
- ));
- }
+ if (!CheckDiskSpace(args.GetBlocksDirPath(), additional_bytes_needed)) {
+ InitWarning(strprintf(_("Disk space for %s may not accommodate the block files. Approximately %u GB of data will be stored in this directory."),
+ fs::quoted(fs::PathToString(args.GetBlocksDirPath())),
+ chainparams.AssumedBlockchainSize()));
+ }
}🤖 Prompt for AI Agents
In src/init.cpp around lines 2291-2307, the first-startup disk-space warning
uses nPruneTarget directly when fPruneMode is true, but nPruneTarget ==
UINT64_MAX for manual prune (-prune=1) so the code always warns; compute the
assumed blockchain size in bytes (chainparams.AssumedBlockchainSize() * 1024 *
1024 * 1024) and when fPruneMode is true set additional_bytes_needed to the
smaller of nPruneTarget and that assumed size (use std::min), otherwise use the
assumed size; keep the displayed GB value as
chainparams.AssumedBlockchainSize().
|
This PR includes breaking changes. Breaking changes cannot be included in minor versions please add a commit to not deprecate anything yet. |
Breaking changes aren't permitted in minor versions, should be reintroduced in v24 merge cycle.
…couple from `coinstatsindex`
…is less than necessary
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/index/coinstatsindex.h (1)
12-60: Include<optional>forstd::optional.
LookUpStatsnow returnsstd::optional, but this header doesn’t include<optional>. Any TU including just this header will fail to compile becausestd::optionalis undefined. Please add the missing include.#include <index/base.h> #include <kernel/coinstats.h> +#include <optional>src/kernel/coinstats.cpp (1)
61-61: Fix precedence bug: height/coinbase varint is wrong.The ternary applies to the entire sum. It should add 0/1 for coinbase, not collapse to 0/1 for the whole expression. This corrupts the serialized-hash input and invalidates UTXO commitments.
- ss << VARINT(it->second.nHeight * 2 + it->second.fCoinBase ? 1u : 0u); + ss << VARINT(it->second.nHeight * 2 + (it->second.fCoinBase ? 1u : 0u));src/validation.cpp (1)
4315-4338: Progress calc can underflow/zero-divide; clamp inputsIf local clock skew makes (GetTime() - block_time) <= 0, blocks_left can be 0/negative. Clamp age ≥ 0 and blocks_left ≥ 1.
- const int64_t blocks_left{(GetTime() - last_accepted.GetBlockTime()) / GetConsensus().nPowTargetSpacing}; - const double progress{100.0 * last_accepted.nHeight / (last_accepted.nHeight + blocks_left)}; + const int64_t target = std::max<int64_t>(1, GetConsensus().nPowTargetSpacing); + const int64_t age = std::max<int64_t>(0, GetTime() - last_accepted.GetBlockTime()); + const int64_t blocks_left = std::max<int64_t>(1, age / target); + const double progress = 100.0 * last_accepted.nHeight / (last_accepted.nHeight + blocks_left);
♻️ Duplicate comments (3)
src/test/descriptor_tests.cpp (1)
320-320: Remove stray debug comment.Leftover “/* --- it fails!!! */” should be deleted to keep tests clean.
- /* --- it fails!!! */src/init.cpp (1)
2291-2307: Fix first-startup disk-space warning for -prune=1 (UINT64_MAX).This was previously flagged; still computes additional_bytes_needed = nPruneTarget, which over-warns for manual prune. Use min(nPruneTarget, approx_chain_bytes).
- uint64_t additional_bytes_needed = fPruneMode ? nPruneTarget - : chainparams.AssumedBlockchainSize() * 1024 * 1024 * 1024; + const uint64_t approx_chain_bytes = + static_cast<uint64_t>(chainparams.AssumedBlockchainSize()) * 1024 * 1024 * 1024; + uint64_t additional_bytes_needed = + fPruneMode ? std::min(nPruneTarget, approx_chain_bytes) : approx_chain_bytes;src/rpc/blockchain.cpp (1)
1416-1493: Add null check forpprevto prevent potential crash.Lines 1437 and 1455 access
blockindex->pprevwithout checking if it's null. If these functions are invoked with the genesis block (wherepprevis null), they will cause a crash wheng_versionbitscache.State()andStateSinceHeight()attempt to dereference the null pointer.While the functions check
if (blockindex == nullptr)at lines 1405 and 1421, there's no check forblockindex->pprev == nullptrbefore usage.Add a defensive check:
if (blockindex->pprev == nullptr) return;after line 1421 (for the BIP9 version) and after line 1405 (for the buried deployment version if pprev is used there).
Note: This issue was previously flagged and marked as resolved, but the fix does not appear to be present in the current code.
🧹 Nitpick comments (7)
test/functional/wallet_crosschain.py (1)
68-69: Correct conditional logic, minor simplification possible.The conditional stderr handling correctly distinguishes between descriptor/non-descriptor modes and node configurations. The logic is functionally correct.
For improved readability, the conditional on line 69 can be simplified:
- self.nodes[idx].stop_node(expected_stderr=EXPECTED_STDERR_NO_GOV_PRUNE if not self.options.descriptors or (self.options.descriptors and idx == 1) else "") + self.nodes[idx].stop_node(expected_stderr=EXPECTED_STDERR_NO_GOV_PRUNE if not self.options.descriptors or idx == 1 else "")The condition
(self.options.descriptors and idx == 1)can be shortened toidx == 1when combined withor, since the first part already handles thenot self.options.descriptorscase.test/functional/rpc_blockchain.py (1)
192-251: Fix flake8 E121 and improve readability of large expected dictflake8 reports E121 at Line 196. Avoid hanging‑indent sensitivity by assigning the expected object to a variable, then asserting.
- def check_signalling_deploymentinfo_result(self, gdi_result, height, blockhash, status_next): + def check_signalling_deploymentinfo_result(self, gdi_result, height, blockhash, status_next): assert height >= 144 and height <= 287 - - assert_equal(gdi_result, { - "hash": blockhash, - "height": height, - "deployments": { + expected = { + "hash": blockhash, + "height": height, + "deployments": { 'bip34': {'type': 'buried', 'active': True, 'height': 2}, 'bip66': {'type': 'buried', 'active': True, 'height': 3}, 'bip65': {'type': 'buried', 'active': True, 'height': 4}, 'csv': {'type': 'buried', 'active': True, 'height': 5}, 'bip147': {'type': 'buried', 'active': True, 'height': 6}, 'dip0001': { 'type': 'buried', 'active': True, 'height': 10}, 'dip0003': { 'type': 'buried', 'active': False, 'height': 411}, 'dip0008': { 'type': 'buried', 'active': True, 'height': 12}, 'dip0020': { 'type': 'buried', 'active': True, 'height': 1}, 'dip0024': { 'type': 'buried', 'active': True, 'height': 13}, 'realloc': { 'type': 'buried', 'active': True, 'height': 14}, 'v19': { 'type': 'buried', 'active': True, 'height': 15}, 'v20': { 'type': 'buried', 'active': False, 'height': 412}, 'mn_rr': { 'type': 'buried', 'active': False, 'height': 413}, 'withdrawals': { 'type': 'buried', 'active': False, 'height': 600}, 'v24': { 'type': 'bip9', 'bip9': { 'start_time': 0, 'timeout': 9223372036854775807, # "v24" does not have a timeout so is set to the max int64 value 'min_activation_height': 0, 'since': 0, 'status': 'defined', 'status_next': 'defined', 'ehf': True, }, 'active': False }, 'testdummy': { 'type': 'bip9', 'bip9': { 'bit': 28, 'start_time': 0, 'timeout': 9223372036854775807, # testdummy does not have a timeout so is set to the max int64 value 'min_activation_height': 0, 'since': 144, 'status': 'started', 'status_next': status_next, 'statistics': { 'period': 144, 'threshold': 108, 'elapsed': height - 143, 'count': height - 143, 'possible': True, }, 'ehf': False, 'signalling': '#'*(height-143), }, 'active': False } - } - }) + } + } + assert_equal(gdi_result, expected)src/versionbits.cpp (1)
146-150: Document signalling_blocks indexing.Indexing uses 0 = first block of period, last index = current block. Add a brief comment to avoid misuse by callers.
- if (signalling_blocks) { + if (signalling_blocks) { + // Indexing: position 0 corresponds to the first block of the current period, + // and position (elapsed-1) corresponds to the current block. signalling_blocks->assign(blocks_in_period, false); } @@ - if (Condition(currentIndex, params)) { + if (Condition(currentIndex, params)) { ++count; if (signalling_blocks) signalling_blocks->at(blocks_in_period) = true; }Also applies to: 166-174
src/kernel/coinstats.cpp (1)
139-165: New optional ComputeUTXOStats API: LGTM; prefer graceful null pindex handling.Looks good. Minor: instead of asserting when LookupBlockIndex fails, return std::nullopt.
- CCoinsStats stats{Assert(pindex)->nHeight, pindex->GetBlockHash()}; + if (!pindex) return std::nullopt; + CCoinsStats stats{pindex->nHeight, pindex->GetBlockHash()};src/init.cpp (1)
834-846: Avoid holding cs_main during full UTXO stats scan.Current WITH_LOCK wraps GetUTXOStats, potentially locking cs_main while iterating LevelDB. Capture dependencies under cs_main, then call GetUTXOStats without the lock.
- const auto maybe_stats = WITH_LOCK(::cs_main, return GetUTXOStats(&chainman.ActiveChainstate().CoinsDB(), chainman.m_blockman, /*hash_type=*/CoinStatsHashType::NONE, node.rpc_interruption_point, chainman.ActiveChain().Tip(), /*index_requested=*/true)); + CCoinsView* view = WITH_LOCK(::cs_main, return &chainman.ActiveChainstate().CoinsDB()); + const CBlockIndex* tip = WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip()); + const auto maybe_stats = GetUTXOStats(view, chainman.m_blockman, + /*hash_type=*/CoinStatsHashType::NONE, node.rpc_interruption_point, + tip, /*index_requested=*/true);src/validation.cpp (2)
353-353: Add cs_main contract + null-guard to GetBlockScriptFlagsCallers already hold cs_main; encode it to prevent misuse and assert non-null pindex.
-static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const ChainstateManager& chainman); +static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const ChainstateManager& chainman) + EXCLUSIVE_LOCKS_REQUIRED(::cs_main);-static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const ChainstateManager& chainman) +static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const ChainstateManager& chainman) + EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + assert(pindex != nullptr); unsigned int flags = SCRIPT_VERIFY_NONE; ... }Also applies to: 2201-2249
1023-1028: Guard against null Tip() in early startupRare but possible before genesis/m_chain init; add an assert to document the expectation.
- unsigned int currentBlockScriptVerifyFlags{GetBlockScriptFlags(m_active_chainstate.m_chain.Tip(), m_active_chainstate.m_chainman)}; + Assert(m_active_chainstate.m_chain.Tip() != nullptr); + unsigned int currentBlockScriptVerifyFlags{ + GetBlockScriptFlags(m_active_chainstate.m_chain.Tip(), m_active_chainstate.m_chainman) + };Would you like me to scan other call sites for similar assumptions? I can provide a repo-wide check script.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
configure.ac(1 hunks)doc/REST-interface.md(1 hunks)doc/release-notes-6901.md(1 hunks)src/Makefile.am(3 hunks)src/index/coinstatsindex.cpp(2 hunks)src/index/coinstatsindex.h(2 hunks)src/init.cpp(5 hunks)src/interfaces/node.h(2 hunks)src/kernel/coinstats.cpp(5 hunks)src/kernel/coinstats.h(3 hunks)src/net_processing.cpp(4 hunks)src/node/interfaces.cpp(1 hunks)src/rest.cpp(2 hunks)src/rpc/blockchain.cpp(15 hunks)src/rpc/blockchain.h(3 hunks)src/rpc/mining.cpp(3 hunks)src/rpc/util.h(0 hunks)src/script/descriptor.cpp(1 hunks)src/script/signingprovider.cpp(1 hunks)src/script/signingprovider.h(2 hunks)src/test/blockfilter_index_tests.cpp(4 hunks)src/test/coinstatsindex_tests.cpp(3 hunks)src/test/descriptor_tests.cpp(5 hunks)src/test/evo_deterministicmns_tests.cpp(9 hunks)src/test/fuzz/coins_view.cpp(0 hunks)src/test/fuzz/rpc.cpp(1 hunks)src/test/fuzz/utxo_snapshot.cpp(1 hunks)src/test/fuzz/versionbits.cpp(4 hunks)src/test/miner_tests.cpp(1 hunks)src/test/settings_tests.cpp(3 hunks)src/test/util/mining.cpp(1 hunks)src/test/util/setup_common.cpp(3 hunks)src/test/validation_block_tests.cpp(4 hunks)src/test/validation_chainstate_tests.cpp(1 hunks)src/util/settings.cpp(2 hunks)src/util/settings.h(1 hunks)src/util/system.cpp(6 hunks)src/util/system.h(2 hunks)src/validation.cpp(28 hunks)src/validation.h(8 hunks)src/versionbits.cpp(3 hunks)src/versionbits.h(3 hunks)src/wallet/rpc/spend.cpp(1 hunks)src/wallet/scriptpubkeyman.cpp(3 hunks)test/functional/feature_asset_locks.py(1 hunks)test/functional/feature_cltv.py(1 hunks)test/functional/feature_dersig.py(1 hunks)test/functional/feature_governance.py(5 hunks)test/functional/feature_mnehf.py(2 hunks)test/functional/feature_new_quorum_type_activation.py(1 hunks)test/functional/interface_rest.py(1 hunks)test/functional/p2p_dos_header_tree.py(4 hunks)test/functional/rpc_blockchain.py(4 hunks)test/functional/test_framework/blocktools.py(1 hunks)test/functional/test_framework/util.py(1 hunks)test/functional/wallet_crosschain.py(3 hunks)test/functional/wallet_importdescriptors.py(1 hunks)test/functional/wallet_signrawtransactionwithwallet.py(2 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
💤 Files with no reviewable changes (2)
- src/test/fuzz/coins_view.cpp
- src/rpc/util.h
🚧 Files skipped from review as they are similar to previous changes (24)
- src/test/fuzz/rpc.cpp
- test/functional/wallet_signrawtransactionwithwallet.py
- test/lint/lint-circular-dependencies.py
- doc/release-notes-6901.md
- src/Makefile.am
- test/functional/test_framework/util.py
- src/util/settings.cpp
- src/script/descriptor.cpp
- src/test/validation_chainstate_tests.cpp
- src/script/signingprovider.cpp
- src/rest.cpp
- src/wallet/rpc/spend.cpp
- src/wallet/scriptpubkeyman.cpp
- test/functional/interface_rest.py
- src/test/blockfilter_index_tests.cpp
- test/functional/feature_mnehf.py
- src/test/fuzz/utxo_snapshot.cpp
- src/node/interfaces.cpp
- src/interfaces/node.h
- test/functional/feature_asset_locks.py
- src/util/settings.h
- test/functional/feature_governance.py
- test/functional/test_framework/blocktools.py
- src/test/miner_tests.cpp
🧰 Additional context used
📓 Path-based instructions (4)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/wallet_importdescriptors.pytest/functional/feature_dersig.pytest/functional/rpc_blockchain.pytest/functional/p2p_dos_header_tree.pytest/functional/feature_cltv.pytest/functional/feature_new_quorum_type_activation.pytest/functional/wallet_crosschain.py
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/script/signingprovider.hsrc/versionbits.hsrc/rpc/mining.cppsrc/index/coinstatsindex.cppsrc/test/settings_tests.cppsrc/index/coinstatsindex.hsrc/kernel/coinstats.cppsrc/test/util/setup_common.cppsrc/test/coinstatsindex_tests.cppsrc/test/descriptor_tests.cppsrc/test/fuzz/versionbits.cppsrc/util/system.cppsrc/util/system.hsrc/test/validation_block_tests.cppsrc/validation.hsrc/test/evo_deterministicmns_tests.cppsrc/test/util/mining.cppsrc/rpc/blockchain.cppsrc/net_processing.cppsrc/kernel/coinstats.hsrc/rpc/blockchain.hsrc/versionbits.cppsrc/init.cppsrc/validation.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/settings_tests.cppsrc/test/util/setup_common.cppsrc/test/coinstatsindex_tests.cppsrc/test/descriptor_tests.cppsrc/test/fuzz/versionbits.cppsrc/test/validation_block_tests.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/util/mining.cpp
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/REST-interface.md
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
PR: dashpay/dash#6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
PR: dashpay/dash#6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/rpc/blockchain.cpp
🧬 Code graph analysis (24)
src/script/signingprovider.h (1)
src/script/signingprovider.cpp (2)
Merge(58-65)Merge(58-58)
src/versionbits.h (2)
src/versionbits.cpp (18)
GetStateStatisticsFor(134-180)GetStateStatisticsFor(134-134)pindex(258-261)pindex(258-258)params(227-227)params(227-227)params(243-243)params(243-243)params(244-244)params(244-244)params(245-245)params(245-245)params(246-256)params(246-246)params(265-265)params(265-265)Statistics(276-280)Statistics(276-276)src/test/fuzz/versionbits.cpp (12)
pindex(45-45)pindex(45-45)pindex(47-47)pindex(47-47)pindex(55-55)pindex(55-55)pindex(63-63)pindex(63-63)params(46-46)params(46-46)params(48-48)params(48-48)
test/functional/feature_dersig.py (2)
test/functional/test_framework/util.py (1)
assert_equal(69-74)src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
test/functional/rpc_blockchain.py (3)
test/functional/test_framework/util.py (1)
assert_equal(69-74)test/functional/test_framework/test_framework.py (1)
start_node(649-658)src/rpc/blockchain.cpp (4)
getblockchaininfo(1502-1588)getblockchaininfo(1502-1502)getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
src/index/coinstatsindex.cpp (1)
src/kernel/coinstats.cpp (5)
CCoinsStats(21-23)GetBogoSize(26-34)GetBogoSize(26-26)TxOutSer(36-42)TxOutSer(36-36)
test/functional/feature_cltv.py (2)
test/functional/test_framework/util.py (1)
assert_equal(69-74)src/rpc/blockchain.cpp (2)
getdeploymentinfo(1650-1696)getdeploymentinfo(1650-1650)
src/test/settings_tests.cpp (2)
src/util/settings.cpp (2)
GetSetting(140-195)GetSetting(140-145)src/util/system.cpp (2)
GetSetting(1179-1185)GetSetting(1179-1179)
test/functional/feature_new_quorum_type_activation.py (1)
test/functional/test_framework/test_framework.py (1)
no_op(803-804)
src/index/coinstatsindex.h (3)
src/rpc/blockchain.h (1)
kernel(26-28)src/kernel/coinstats.cpp (1)
CCoinsStats(21-23)src/index/coinstatsindex.cpp (2)
LookUpStats(320-345)LookUpStats(320-320)
src/test/util/setup_common.cpp (2)
src/validation.cpp (6)
chainparams(569-580)chainparams(569-571)chainparams(583-593)chainparams(583-584)chainparams(596-606)chainparams(596-597)src/chainparams.cpp (2)
Params(1354-1357)Params(1354-1354)
src/test/coinstatsindex_tests.cpp (2)
src/kernel/coinstats.cpp (1)
CCoinsStats(21-23)src/kernel/coinstats.h (1)
CoinStatsHashType(24-77)
src/test/descriptor_tests.cpp (1)
src/script/descriptor.cpp (2)
scripts(759-764)scripts(759-759)
test/functional/wallet_crosschain.py (1)
test/functional/test_framework/test_framework.py (2)
stop_nodes(684-692)start_nodes(660-678)
src/test/fuzz/versionbits.cpp (1)
src/versionbits.cpp (4)
pindex(258-261)pindex(258-258)GetStateStatisticsFor(134-180)GetStateStatisticsFor(134-134)
src/util/system.cpp (2)
src/node/interfaces.cpp (16)
name(447-456)name(447-447)name(457-457)name(457-457)name(458-468)name(458-458)name(469-478)name(469-469)name(1130-1133)name(1130-1130)name(1134-1137)name(1134-1134)name(1138-1141)name(1138-1138)name(1142-1151)name(1142-1142)src/util/settings.cpp (2)
GetSetting(140-195)GetSetting(140-145)
src/util/system.h (2)
src/util/system.cpp (14)
SettingToString(649-652)SettingToString(649-649)SettingToInt(660-663)SettingToInt(660-660)SettingToBool(671-674)SettingToBool(671-671)GetSettingsPath(562-575)GetSettingsPath(562-562)ReadSettingsFile(588-609)ReadSettingsFile(588-588)WriteSettingsFile(611-629)WriteSettingsFile(611-611)GetPersistentSetting(631-636)GetPersistentSetting(631-631)src/node/interfaces.cpp (16)
name(447-456)name(447-447)name(457-457)name(457-457)name(458-468)name(458-458)name(469-478)name(469-469)name(1130-1133)name(1130-1130)name(1134-1137)name(1134-1134)name(1138-1141)name(1138-1138)name(1142-1151)name(1142-1142)
src/validation.h (1)
src/validation.cpp (11)
ChainstateManager(6049-6059)chainparams(569-580)chainparams(569-571)chainparams(583-593)chainparams(583-584)chainparams(596-606)chainparams(596-597)ProcessNewBlock(4435-4472)ProcessNewBlock(4435-4435)ProcessNewBlockHeaders(4314-4341)ProcessNewBlockHeaders(4314-4314)
src/test/util/mining.cpp (1)
src/node/interfaces.cpp (9)
Assert(93-93)Assert(94-94)Assert(118-118)Assert(259-259)Assert(282-282)Assert(404-404)Assert(653-656)Assert(865-865)Assert(1171-1174)
src/rpc/blockchain.cpp (4)
src/kernel/coinstats.cpp (5)
CCoinsStats(21-23)ComputeUTXOStats(100-137)ComputeUTXOStats(100-100)ComputeUTXOStats(139-165)ComputeUTXOStats(139-139)src/kernel/coinstats.h (2)
CoinStatsHashType(24-77)node(19-21)src/deploymentinfo.cpp (2)
DeploymentName(20-56)DeploymentName(20-20)src/rest.cpp (1)
getdeploymentinfo(624-624)
src/kernel/coinstats.h (2)
src/rpc/blockchain.h (2)
kernel(26-28)node(33-36)src/kernel/coinstats.cpp (9)
CCoinsStats(21-23)GetBogoSize(26-34)GetBogoSize(26-26)TxOutSer(36-42)TxOutSer(36-36)ComputeUTXOStats(100-137)ComputeUTXOStats(100-100)ComputeUTXOStats(139-165)ComputeUTXOStats(139-139)
src/rpc/blockchain.h (2)
src/kernel/coinstats.h (2)
CoinStatsHashType(24-77)node(19-21)src/rpc/blockchain.cpp (3)
GetUTXOStats(1106-1129)GetUTXOStats(1106-1110)view(1340-1340)
src/versionbits.cpp (2)
src/validation.cpp (14)
pindex(2205-2205)pindex(2205-2205)pindex(2209-2215)pindex(2209-2209)pindex(3131-3138)pindex(3131-3131)params(2203-2203)params(2203-2203)params(2204-2204)params(2204-2204)params(2206-2206)params(2206-2206)params(2207-2207)params(2207-2207)src/test/fuzz/versionbits.cpp (15)
pindex(45-45)pindex(45-45)pindex(47-47)pindex(47-47)pindex(55-55)pindex(55-55)pindex(63-63)pindex(63-63)params(46-46)params(46-46)params(48-48)params(48-48)params(49-49)params(49-49)params(50-50)
src/init.cpp (4)
src/kernel/coinstats.h (1)
CoinStatsHashType(24-77)src/rpc/blockchain.cpp (2)
GetUTXOStats(1106-1129)GetUTXOStats(1106-1110)src/util/system.cpp (2)
CheckDiskSpace(158-164)CheckDiskSpace(158-158)src/fs.h (1)
quoted(94-97)
src/validation.cpp (2)
src/kernel/coinstats.cpp (5)
CCoinsStats(21-23)ComputeUTXOStats(100-137)ComputeUTXOStats(100-100)ComputeUTXOStats(139-165)ComputeUTXOStats(139-139)src/kernel/coinstats.h (1)
CoinStatsHashType(24-77)
🪛 Flake8 (7.3.0)
test/functional/rpc_blockchain.py
[error] 196-196: continuation line under-indented for hanging indent
(E121)
🪛 markdownlint-cli2 (0.18.1)
doc/REST-interface.md
88-88: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (76)
test/functional/feature_new_quorum_type_activation.py (1)
25-25: LGTM! Test adjustment aligns with upstream behavior change.The increase from 9 to 10 blocks correctly compensates for the behavior change in
AbstractThresholdConditionChecker::GetStateStatisticsFor()introduced by bitcoin#23508, ensuring the BIP9 deployment status transitions at the expected point.test/functional/wallet_importdescriptors.py (1)
131-131: LGTM! Consistent simplification.The change replaces manual address derivation with a direct property access (
key.p2pkh_addr), which is consistent with the pattern used throughout this file at lines 97, 107, 117, and 180. This simplification improves maintainability and reduces the chance of derivation errors.test/functional/p2p_dos_header_tree.py (4)
9-9: LGTM: Import aligns with pruning changes.The import of
EXPECTED_STDERR_NO_GOV_PRUNEis necessary to handle expected governance-related stderr output when running nodes with pruning enabled.
27-27: LGTM: Prune height correctly adjusted for Dash.The prune value of 945 correctly reflects Dash's adjusted minimum prune height (vs upstream 550), necessitated by the increased
MIN_DISK_SPACE_FOR_BLOCK_FILESin dash#1621.
69-69: LGTM: Node restart properly configured.The restart correctly applies the
-prune=945flag consistent with the initial node configuration, and properly handles expected stderr output withexpected_stderr=EXPECTED_STDERR_NO_GOV_PRUNE.
89-90: LGTM: Proper cleanup added for pruned nodes.The cleanup step correctly stops all nodes with
expected_stderr=EXPECTED_STDERR_NO_GOV_PRUNE, ensuring governance-related stderr output during shutdown doesn't cause test failures.test/functional/wallet_crosschain.py (4)
8-8: LGTM: Import aligns with governance/prune expectations.The import is necessary for handling expected stderr output when stopping pruned nodes.
25-25: LGTM: Prune value aligns with Dash requirements.The prune value of 945 is consistent with the PR objectives, reflecting Dash's increased MIN_DISK_SPACE_FOR_BLOCK_FILES from dash#1621.
62-63: LGTM: Correct handling of expected stderr.The explicit stop of node 1 with
expected_stderrbefore callingstop_nodes()is necessary becausestop_nodes()doesn't support per-node stderr expectations. While this results in attempting to stop node 1 twice, the implementation handles this gracefully.
64-64: LGTM: Restart configuration is correct.Both nodes are restarted with consistent pruning configuration, enabling the cross-chain wallet override test.
configure.ac (1)
1505-1509: No issues identified; the narrower gating scope is correct.ZMQ is used for publishing block/transaction events from the daemon to subscribers, and port forwarding (UPnP/NAT-PMP) is needed for the daemon to accept incoming connections—both are daemon-specific features. CLI tools (dash-cli, dash-tx, dash-wallet) don't reference these networking features, confirming they are unnecessary for command-line utilities. The condition correctly disables these libraries only when no network-facing components (
build_bitcoind,bitcoin_enable_qt,use_bench,use_tests) are built, which is intentional and matches expected behavior.src/script/signingprovider.h (1)
9-9: LGTM! Clean API refactor from free function to member function.The migration from a free function
Merge(a, b)to an in-place member functionMerge(FlatSigningProvider&&)with move semantics is well-structured. TheLIFETIMEBOUNDattribute correctly requires theattributes.hinclude.Also applies to: 59-60
src/test/util/mining.cpp (1)
73-73: LGTM! Signature change aligns with updated ProcessNewBlock API.The removal of the
Params()argument is consistent with the broader repository changes whereProcessNewBlocknow derives parameters fromChainstateManagerrather than requiring them explicitly.src/versionbits.h (1)
13-13: LGTM! Header is now self-contained with per-block signalling support.The
<vector>include resolves the previous non-self-contained header issue. The optionalsignalling_blocksparameter enables callers to retrieve detailed per-block signalling history within a deployment period, which is useful for diagnostic and testing purposes.Also applies to: 70-73, 90-93
src/util/system.cpp (6)
562-575: LGTM! Clean backup file path support.The addition of the
backupparameter to generate.baksuffixed paths is straightforward and follows a clear naming convention.
611-629: LGTM! Backup parameter correctly propagated.The
backupparameter is consistently passed through to bothGetSettingsPathcalls, enabling backup file writes when requested.
631-636: LGTM! New GetPersistentSetting API is well-defined.The method correctly retrieves settings while ignoring non-persistent sources (command-line and forced settings) by passing
ignore_nonpersistent=trueto the underlyingGetSettingcall.
643-652: LGTM! Helpful extraction of conversion logic.Extracting
SettingToStringreduces code duplication and improves testability by separating the conversion logic from the settings retrieval.
654-663: LGTM! Consistent refactoring pattern.The extraction of
SettingToIntfollows the same clean pattern asSettingToString, improving code organization and reusability.
665-674: LGTM! Consistent helper extraction and explicit flag passing.The
SettingToBoolextraction completes the pattern, and the explicitignore_nonpersistent=falseflags inGetChainNameandGetSettingclarify that these methods consider all setting sources (including command-line and forced settings).Also applies to: 1108-1135, 1179-1185
test/functional/feature_cltv.py (1)
102-107: LGTM! Test updated for the new deployment info API.The migration from
getblockchaininfo()['softforks']togetdeploymentinfo()['deployments']reflects the PR's documented breaking change. The returned object structure (active,height,type) remains unchanged, ensuring test expectations are preserved.test/functional/feature_dersig.py (1)
67-73: LGTM! Migration to getdeploymentinfo RPC looks correct.The change correctly migrates from querying
getblockchaininfo()['softforks']to the newgetdeploymentinfo()['deployments']endpoint, and the expected structure appropriately includes the additional"type": "buried"field.doc/REST-interface.md (1)
88-96: LGTM! Documentation for new deployment info REST endpoints is clear.The documentation correctly describes the new REST endpoints and appropriately references the
getdeploymentinfoRPC help for details. The heading style (ATX with####) is consistent with all other headings in this file, so the static analysis hint about heading style is a false positive.src/test/settings_tests.cpp (3)
109-109: LGTM! GetSetting call correctly updated for new signature.The call has been properly updated to include the new
ignore_nonpersistentparameter (passed asfalse), which preserves the existing test behavior while matching the updated function signature.
145-147: LGTM! GetSetting calls correctly updated for new signature.Both calls have been properly updated to include the new
ignore_nonpersistentparameter (passed asfalse), preserving the existing test behavior.
228-228: LGTM! GetSetting call correctly updated for new signature.The call has been properly updated to include the new
ignore_nonpersistentparameter (passed asfalse), matching the updated function signature.src/test/evo_deterministicmns_tests.cpp (9)
266-266: LGTM! ProcessNewBlock call correctly updated.The call has been properly updated to remove the
Params()parameter, consistent with the refactored API whereChainstateManagernow manages chain parameters internally.
276-276: LGTM! ProcessNewBlock call correctly updated.The call has been properly updated to match the new API signature.
304-304: LGTM! ProcessNewBlock call correctly updated.The call has been properly updated to match the new API signature.
322-322: LGTM! ProcessNewBlock call correctly updated.The call has been properly updated to match the new API signature.
342-342: LGTM! ProcessNewBlock call correctly updated.The call has been properly updated to match the new API signature.
642-642: LGTM! ProcessNewBlock call correctly updated.The call has been properly updated to match the new API signature.
788-788: LGTM! ProcessNewBlock call correctly updated.The call has been properly updated to match the new API signature.
821-821: LGTM! ProcessNewBlock call correctly updated.The call has been properly updated to match the new API signature.
833-833: LGTM! ProcessNewBlock call correctly updated.The call has been properly updated to match the new API signature.
src/index/coinstatsindex.cpp (1)
17-19: API migration to kernel:: and std::optional is sound
- Using kernel::CCoinsStats/GetBogoSize/TxOutSer and returning std::optional is consistent with upstream refactor. Initialization sets index_used and copies all index-only totals. No functional regressions spotted.
Also applies to: 320-345
test/functional/rpc_blockchain.py (3)
9-9: Add getdeploymentinfo to the covered RPC list — OKScope expansion looks correct.
96-97: Wire up _test_getdeploymentinfo — OKPlacement after _test_stopatheight avoids premature shutdown.
252-284: getdeploymentinfo scenario coverage — OKCovers default tip query, explicit hash, and boundary near lock‑in. Good end‑to‑end signal checks.
src/test/coinstatsindex_tests.cpp (1)
15-16: Tests aligned to optional LookUpStats — OKSwitched to kernel:: types and std::optional semantics; assertions verify presence at genesis, tip, and after new block.
Also applies to: 31-32, 47-48, 50-51, 64-65
src/test/util/setup_common.cpp (2)
248-249: ChainstateManager now requires CChainParams — OKConstructing ChainstateManager with Params() matches the refactor and keeps dependencies explicit.
Also applies to: 261-262
482-483: ProcessNewBlock call site updated — OKRemoving the chainparams argument aligns with the new API.
src/test/validation_block_tests.cpp (1)
103-103: No lingering legacy call sites detected — code changes verifiedThe verification confirms all ProcessNewBlock and ProcessNewBlockHeaders calls in the file (lines 102, 159, 181, 187, 226) use consistent new signatures without passing Params() as a function argument. The one occurrence of Params() on line 159 is used internally to construct the genesis block object, not as a direct function argument. The refactoring is complete.
src/versionbits.cpp (2)
276-280: Forwarder matches new signature.
VersionBitsCache::Statistics(..., std::vector<bool>* signalling_blocks)correctly forwards to the updated checker.
152-159: The review comment is incorrect; the code is safe as-is.GetAncestor() safely handles negative heights by returning nullptr rather than asserting, as shown in the implementation (src/chain.cpp:93-96):
if (height > nHeight || height < 0) { return nullptr; // Safe return, no assert }For early block heights where
pindex->nHeight - ((pindex->nHeight + 1) % stats.period)evaluates to −1, the call returns nullptr. Subsequently,GetStateFor(nullptr, ...)deterministically returnsThresholdState::DEFINED(src/versionbits.cpp:59-62), making the condition at line 155 (if (state == ThresholdState::STARTED)) false. Line 157's dereference ofpindexEndOfPrevPeriod->nHeightis therefore never reached when the pointer is nullptr—the guard is implicit in the state check.The proposed diff is unnecessary.
Likely an incorrect or invalid review comment.
src/test/descriptor_tests.cpp (2)
78-83: Arg order updates are consistent.
DoCheck/Checknow takenorm_pubas third parameter; call sites updated accordingly.Also applies to: 271-295
237-238: Merge usage reads clean.Passing
FlatSigningProvider{A}.Merge(FlatSigningProvider{B})intoIsSolvable/SignSignatureis concise and correct given the updated API.Also applies to: 244-244
src/test/fuzz/versionbits.cpp (1)
55-56: Fuzz harness aligns with new stats API.Signals vector propagation and boundary assertions match the updated semantics; dual call (with/without signals) parity checks are a nice guard.
Also applies to: 253-271, 287-290
src/rpc/mining.cpp (1)
148-148: API call-site updates confirmed correct.Verification results show all ProcessNewBlock and ProcessNewBlockHeaders call sites in src/rpc/mining.cpp (lines 148, 1043, 1085) and throughout the codebase use the new signatures without Params(). No lingering old-signature calls detected.
src/rpc/blockchain.h (1)
11-18: Public API changes verified — all call sites correctly updated to std::optional.GetUTXOStats callers (src/init.cpp:834, src/rpc/blockchain.cpp:1229, 1252, 2845) all use
has_value()checks; CoinStatsHashType consistently qualified askernel::CoinStatsHashTypein signatures withusingdirectives for unqualified access. No stale out-parameter patterns or legacy return handling detected.src/init.cpp (3)
34-34: Include for kernel coinstats: LGTM.Resolves missing type exposure locally.
137-137: Namespace alias for CoinStatsHashType: LGTM.
1938-1939: Construct ChainstateManager with params: LGTM.Matches new API surface and simplifies downstream calls.
src/kernel/coinstats.h (2)
68-70: CCoinsStats constructor exposure: LGTM.
76-77: ComputeUTXOStats declaration with std::optional: LGTM.src/util/system.h (2)
177-180: Settings conversion helpers declarations: LGTM.
475-487: Settings path/backup and persistent setting API: LGTM.Also applies to: 488-493
src/validation.h (3)
908-931: Embed chain params in ChainstateManager: LGTM.Constructor + getters simplify param plumbing.
1046-1059: ProcessNewBlock/Headers signatures without params: LGTM.Aligns with internal accessors; matches validation.cpp.
1082-1099: Deployment wrappers via ChainstateManager: LGTM.*Overload selection targets consensus variants; no recursion.
src/rpc/blockchain.cpp (5)
23-23: LGTM! Namespace migration properly implemented.The include path and using declarations are correctly updated to reflect the move of coinstats types from
node::tokernel::namespace.Also applies to: 66-68
1106-1129: LGTM! Well-structured optional return implementation.The new
GetUTXOStatsfunction properly returnsstd::optional<kernel::CCoinsStats>and includes appropriate validation withCHECK_NONFATALto ensure consistency betweenpindexand the view's best block when not using the coinstats index.
1186-1186: LGTM! Proper optional handling throughout.The updates to
gettxoutsetinfocorrectly handle the new optional return type fromGetUTXOStats, including:
- Proper checks with
has_value()before accessing the value- Appropriate error handling when stats cannot be retrieved
- Correct nested optional handling for
prev_statsAlso applies to: 1207-1213, 1229-1257
1495-1499: LGTM! Well-implemented new RPC command.The new
getdeploymentinfoRPC command is properly implemented with:
- Clear help documentation in
RPCHelpForDeployment- Proper parameter handling (optional blockhash with defaults to tip)
- Appropriate error handling for invalid blocks
- Clean separation of concerns with
DeploymentInfofunctionAlso applies to: 1591-1617, 1619-1648, 1650-1696
2825-2825: LGTM! Safe optional handling in CreateUTXOSnapshot.The updates properly handle the optional return from
GetUTXOStats:
- Clear null check with informative error message if stats unavailable
- All subsequent accesses to
maybe_statsare safe after validation- Maintains consistency with the new optional-based API
Also applies to: 2845-2889
src/validation.cpp (11)
22-22: Kernel coinstats migration: LGTMImport and aliases align with the kernel::ComputeUTXOStats/CCoinsStats move. No issues.
Also applies to: 69-72
1432-1433: Using chainstate-bound params: LGTMSwitching to active_chainstate.m_params ensures consistency across snapshot/IBD contexts.
1656-1658: Initialize CChainState params from ChainstateManager: LGTMKeeps params source-of-truth centralized.
2416-2421: Versionbits via ChainstateManager: LGTMUsing DeploymentActiveAt(*pindex, m_chainman, …) and wiring flags through GetBlockScriptFlags is correct and consistent.
4056-4076: Contextual header checks refactor: LGTM; confirm difficulty pathRefactor to BlockManager/ChainstateManager reads well. Please confirm tests cover the pre-DGW fork path (Line 4063~) after the refactor.
Also applies to: 4084-4105
4115-4173: Contextual block checks via ChainstateManager: LGTMConsensus params/height gating now sourced from chainman; no issues spotted.
Also applies to: 4149-4154, 4166-4172
4183-4285: AcceptBlockHeader moved under ChainstateManager: LGTMScope and invariants remain intact; duplicate/failed/conflict handling unchanged semantically.
4356-4403: AcceptBlock contextualization via ChainstateManager: LGTMHeader acceptance and subsequent ContextualCheckBlock(chainman) look consistent.
4454-4461: ProcessNewBlock refactor: LGTMUsing GetConsensus() from ChainstateManager is appropriate; preserves existing locking.
4522-4529: TestBlockValidity refactor: LGTMContextual checks now consistently use chainstate.m_chainman/m_blockman; no issues.
5837-5844: Snapshot validation with kernel::ComputeUTXOStats and AssumeutxoHash: LGTM; verify hash variantThe HASH_SERIALIZED path and AssumeutxoHash comparison match upstream expectations. Please confirm the assumeutxo entries for this network/height reflect HASH_SERIALIZED (not MUHASH) to avoid mismatches.
Also applies to: 5944-5955
|
#6888 has been repurposed to include |
|
This pull request has conflicts, please rebase. |
2 participants
Additional Information
Depends on backport: bitcoin#23508, #24187, #24528, #24579, #25412 (deployment backports) #6888
Dependency for backport: merge bitcoin#25065, #25290, #25494, #25439, #25648, #25772, #25222 (kernel backports: part 1) #6904
bitcoin#24595 is partial as Dash uses
DeploymentActive{After,At}()more extensively than upstream, it is completed in dash#6888 after all Dash-specific usage is migrated over.The minimum acceptable prune height in bitcoin#25315 is higher than upstream (
945vs550) because minimum height is calculated usingMIN_DISK_SPACE_FOR_BLOCK_FILESwhich was bumped in dash#1621, this has been reflected in functional tests.Breaking Changes
Work in progress.
Checklist