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 APIs from node:: to kernel:: (types CCoinsStats, CoinStatsHashType) and introduces ComputeUTXOStats returning std::optional. CoinStatsIndex::LookUpStats now returns an optional. ChainstateManager now stores CChainParams, its constructor requires const CChainParams&, and several validation/block-processing APIs (AcceptBlockHeader, ProcessNewBlockHeaders, ProcessNewBlock, contextual checks, deployment helpers) were changed to use ChainstateManager rather than passing Params. A new getdeploymentinfo RPC and /rest/deploymentinfo REST endpoints were added. Settings persistence APIs and helpers were extended (ArgsManager, util::settings), FlatSigningProvider::Merge became an in-place member, many call sites and tests updated, and numerous signature changes propagated across rpc, validation, index, kernel, util, and test code. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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.
…couple from `coinstatsindex`
…is less than necessary
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().
| static void SoftForkDescPushBack(const CBlockIndex* blockindex, const std::unordered_map<uint8_t, int>& signals, UniValue& softforks, const ChainstateManager& chainman, Consensus::DeploymentPos id) | ||
| { | ||
| // For BIP9 deployments. | ||
|
|
||
| if (!DeploymentEnabled(consensusParams, id)) return; | ||
| if (!DeploymentEnabled(chainman, id)) return; | ||
| if (blockindex == nullptr) return; | ||
|
|
||
| auto get_state_name = [](const ThresholdState state) -> std::string { | ||
| switch (state) { | ||
| case ThresholdState::DEFINED: return "defined"; | ||
| case ThresholdState::STARTED: return "started"; | ||
| case ThresholdState::LOCKED_IN: return "locked_in"; | ||
| case ThresholdState::ACTIVE: return "active"; | ||
| case ThresholdState::FAILED: return "failed"; | ||
| } | ||
| return "invalid"; | ||
| }; | ||
|
|
||
| UniValue bip9(UniValue::VOBJ); | ||
| const ThresholdState thresholdState = g_versionbitscache.State(active_chain_tip, consensusParams, id); | ||
| switch (thresholdState) { | ||
| case ThresholdState::DEFINED: bip9.pushKV("status", "defined"); break; | ||
| case ThresholdState::STARTED: bip9.pushKV("status", "started"); break; | ||
| case ThresholdState::LOCKED_IN: bip9.pushKV("status", "locked_in"); break; | ||
| case ThresholdState::ACTIVE: bip9.pushKV("status", "active"); break; | ||
| case ThresholdState::FAILED: bip9.pushKV("status", "failed"); break; | ||
| } | ||
| const bool has_signal = (ThresholdState::STARTED == thresholdState || ThresholdState::LOCKED_IN == thresholdState); | ||
|
|
||
| const ThresholdState next_state = g_versionbitscache.State(blockindex, chainman.GetConsensus(), id); | ||
| const ThresholdState current_state = g_versionbitscache.State(blockindex->pprev, chainman.GetConsensus(), id); | ||
|
|
||
| const bool has_signal = (ThresholdState::STARTED == current_state || ThresholdState::LOCKED_IN == current_state); | ||
|
|
||
| // BIP9 parameters | ||
| if (has_signal) { | ||
| bip9.pushKV("bit", consensusParams.vDeployments[id].bit); | ||
| bip9.pushKV("bit", chainman.GetConsensus().vDeployments[id].bit); | ||
| } | ||
| bip9.pushKV("start_time", consensusParams.vDeployments[id].nStartTime); | ||
| bip9.pushKV("timeout", consensusParams.vDeployments[id].nTimeout); | ||
| bip9.pushKV("ehf", consensusParams.vDeployments[id].useEHF); | ||
| if (auto it = signals.find(consensusParams.vDeployments[id].bit); it != signals.end()) { | ||
| bip9.pushKV("start_time", chainman.GetConsensus().vDeployments[id].nStartTime); | ||
| bip9.pushKV("timeout", chainman.GetConsensus().vDeployments[id].nTimeout); | ||
| bip9.pushKV("min_activation_height", chainman.GetConsensus().vDeployments[id].min_activation_height); | ||
| bip9.pushKV("ehf", chainman.GetConsensus().vDeployments[id].useEHF); | ||
| if (auto it = signals.find(chainman.GetConsensus().vDeployments[id].bit); it != signals.end()) { | ||
| bip9.pushKV("ehf_height", it->second); | ||
| } | ||
| int64_t since_height = g_versionbitscache.StateSinceHeight(active_chain_tip, consensusParams, id); | ||
|
|
||
| // BIP9 status | ||
| bip9.pushKV("status", get_state_name(current_state)); | ||
| int64_t since_height = g_versionbitscache.StateSinceHeight(blockindex->pprev, chainman.GetConsensus(), id); | ||
| bip9.pushKV("since", since_height); | ||
| bip9.pushKV("status_next", get_state_name(next_state)); | ||
|
|
||
| // BIP9 signalling status, if applicable | ||
| if (has_signal) { | ||
| UniValue statsUV(UniValue::VOBJ); | ||
| BIP9Stats statsStruct = g_versionbitscache.Statistics(active_chain_tip, consensusParams, id); | ||
| std::vector<bool> signals; | ||
| BIP9Stats statsStruct = g_versionbitscache.Statistics(blockindex, chainman.GetConsensus(), id, &signals); | ||
| statsUV.pushKV("period", statsStruct.period); | ||
| statsUV.pushKV("elapsed", statsStruct.elapsed); | ||
| statsUV.pushKV("count", statsStruct.count); | ||
| if (ThresholdState::LOCKED_IN != thresholdState) { | ||
| if (ThresholdState::LOCKED_IN != current_state) { | ||
| statsUV.pushKV("threshold", statsStruct.threshold); | ||
| statsUV.pushKV("possible", statsStruct.possible); | ||
| } | ||
| bip9.pushKV("statistics", statsUV); | ||
|
|
||
| std::string sig; | ||
| sig.reserve(signals.size()); | ||
| for (const bool s : signals) { | ||
| sig.push_back(s ? '#' : '-'); | ||
| } | ||
| bip9.pushKV("signalling", sig); | ||
| } | ||
| if (ThresholdState::LOCKED_IN == thresholdState) { | ||
| bip9.pushKV("activation_height", since_height + static_cast<int>(consensusParams.vDeployments[id].nWindowSize)); | ||
| if (ThresholdState::LOCKED_IN == current_state) { | ||
| bip9.pushKV("activation_height", since_height + static_cast<int>(chainman.GetConsensus().vDeployments[id].nWindowSize)); | ||
| } | ||
| bip9.pushKV("min_activation_height", consensusParams.vDeployments[id].min_activation_height); | ||
|
|
||
| UniValue rv(UniValue::VOBJ); | ||
| rv.pushKV("type", "bip9"); | ||
| rv.pushKV("bip9", bip9); | ||
| if (ThresholdState::ACTIVE == thresholdState) { | ||
| rv.pushKV("height", since_height); | ||
| if (ThresholdState::ACTIVE == next_state) { | ||
| rv.pushKV("height", g_versionbitscache.StateSinceHeight(blockindex, chainman.GetConsensus(), id)); | ||
| } | ||
| rv.pushKV("active", ThresholdState::ACTIVE == thresholdState); | ||
| rv.pushKV("active", ThresholdState::ACTIVE == next_state); | ||
| rv.pushKV("bip9", bip9); | ||
|
|
||
| softforks.pushKV(DeploymentName(id), rv); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify null safety for pprev access.
Lines 1436 and 1454 access blockindex->pprev without checking if it's null. While BIP9 deployments typically activate well after the genesis block (where pprev would be null), this could still cause issues if:
- The function is called with a genesis block index
g_versionbitscache.State()orStateSinceHeight()don't handle nullptr properly
Consider adding a defensive check:
if (!DeploymentEnabled(chainman, id)) return;
if (blockindex == nullptr) return;
if (blockindex->pprev == nullptr) return; // Add this checkOr verify that the versionbitscache functions safely handle nullptr for the block parameter.
🏁 Script executed:
#!/bin/bash
# Check if State and StateSinceHeight handle nullptr blocks
rg -n "ThresholdState.*State\s*\(" --type cpp -A 10 | head -30
rg -n "StateSinceHeight\s*\(" --type cpp -A 10 | head -30Length of output: 4422
🏁 Script executed:
#!/bin/bash
# Find GetStateFor implementation and check how it handles nullptr
rg -n "GetStateFor\s*\(" --type cpp -A 15 | head -50Length of output: 3859
🏁 Script executed:
#!/bin/bash
# Find StateSinceHeight implementation
rg -n "int\s+StateSinceHeight\s*\(" --type cpp -A 15Length of output: 894
🏁 Script executed:
#!/bin/bash
# Check if there are any existing null checks for pindexPrev in versionbits
rg -n "pindexPrev.*nullptr\|nullptr.*pindexPrev" --type cpp -B 2 -A 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for how genesis block is typically handled (pprev is nullptr)
rg -n "genesis\|pprev.*nullptr" --type cpp --type h | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check callers of State() and StateSinceHeight() to see if they guard against nullptr pprev
rg -n "\.State\(.*pprev" --type cpp -B 3 -A 1Length of output: 396
🏁 Script executed:
#!/bin/bash
# Look for practical usage patterns - does code generally pass nullptr?
rg -n "\.State\(\s*nullptr" --type cpp -B 2 -A 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if blockindex->pprev is validated before being passed to these functions elsewhere
rg -n "pprev\s*==\s*nullptr\|nullptr.*pprev" --type cpp src/rpc/ -B 2 -A 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the full SoftForkDescPushBack to understand control flow better
sed -n '1415,1492p' src/rpc/blockchain.cppLength of output: 3444
🏁 Script executed:
#!/bin/bash
# Check versionbits.cpp more carefully for null handling in State
sed -n '270,290p' src/versionbits.cppLength of output: 971
🏁 Script executed:
#!/bin/bash
# Look for StateSinceHeight implementation in versionbits.cpp
rg -n "StateSinceHeight" src/versionbits.cpp -A 10Length of output: 1695
🏁 Script executed:
#!/bin/bash
# Check VersionBitsConditionChecker to understand the actual implementation
rg -n "class VersionBitsConditionChecker" --type cpp -A 30Length of output: 2461
🏁 Script executed:
#!/bin/bash
# Look for GetStateStatisticsFor to understand patterns
rg -n "GetStateStatisticsFor" src/versionbits.cpp -A 15 | head -40Length of output: 1380
🏁 Script executed:
#!/bin/bash
# Check actual implementation of GetStateFor
rg -n "GetStateFor\s*\(" src/versionbits.cpp -A 20 | head -50Length of output: 2280
🏁 Script executed:
#!/bin/bash
# Check if there's a guard for nullptr in the base implementation
rg -n "class.*Checker.*{" src/versionbits.cpp -A 50 | head -100Length of output: 97
🏁 Script executed:
#!/bin/bash
# Look directly at versionbits.cpp for the complete function implementations
wc -l src/versionbits.cppLength of output: 23
🏁 Script executed:
#!/bin/bash
# Check the actual implementation line by line
sed -n '260,300p' src/versionbits.cppLength of output: 1782
🏁 Script executed:
#!/bin/bash
# Get full implementation of State and related functions
sed -n '1,100p' src/versionbits.cppLength of output: 4257
🏁 Script executed:
#!/bin/bash
# Look for how pprev nullptr is typically handled in consensus code
rg -n "pprev.*nullptr\|if.*pprev" --type cpp src/consensus/ -B 1 -A 1 | head -30Length of output: 0
🏁 Script executed:
#!/bin/bash
# Specifically search for GetStateFor in versionbits
grep -n "GetStateFor" src/versionbits.cppLength of output: 668
🏁 Script executed:
#!/bin/bash
# Find the actual class definition with GetStateFor implementation
cat src/versionbits.cpp | grep -n "GetStateFor\|ThresholdState" | head -30Length of output: 1972
🏁 Script executed:
#!/bin/bash
# Look at versionbits implementation with extended context
head -400 src/versionbits.cpp | tail -150Length of output: 6668
🏁 Script executed:
#!/bin/bash
# Search for where nullptr is explicitly tested with blocks
rg -n "if\s*\(\s*p.*[Bb]lock.*==\s*nullptr\|if\s*\(\s*!p.*[Bb]lock" --type cpp src/versionbits.cppLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check simple practical thing - where is State() actually used with pprev?
rg -n "State\(" src/rpc/blockchain.cpp -B 2 -A 2Length of output: 404
🏁 Script executed:
#!/bin/bash
# Check if this is a known issue or pattern
rg -n "blockindex->pprev" src/rpc/blockchain.cpp -B 2 -A 2Length of output: 964
🏁 Script executed:
#!/bin/bash
# Try to get VersionBitsConditionChecker implementation
rg -n "class VersionBitsConditionChecker" src/versionbits.cpp -A 100 | head -120Length of output: 4641
🏁 Script executed:
#!/bin/bash
# Final check - check if there are conditionals that prevent pprev nullptr
sed -n '1415,1465p' src/rpc/blockchain.cppLength of output: 2450
🏁 Script executed:
#!/bin/bash
# Look at actual usage and conditionals - extended view
sed -n '1410,1500p' src/rpc/blockchain.cppLength of output: 3962
🏁 Script executed:
#!/bin/bash
# Look at what calls this function to understand context better
rg -n "SoftForkDescPushBack" src/rpc/blockchain.cpp -B 5 -A 2Length of output: 1537
🏁 Script executed:
#!/bin/bash
# Check if there's context about what blockindex values are passed
grep -n "SoftForkDescPushBack\|GetTip\|LookupBlockIndex" src/rpc/blockchain.cpp | head -20Length of output: 1914
🏁 Script executed:
#!/bin/bash
# Look for the callers context - what calls SoftForkDescPushBack
rg -n "SoftForkDescPushBack\(" src/rpc/blockchain.cpp -B 10 | head -40Length of output: 2133
🏁 Script executed:
#!/bin/bash
# Check RPC method that calls this
rg -n "getblockchaininfo\|softfork" src/rpc/blockchain.cpp -B 3 -A 3 | head -50Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at where this function is invoked
sed -n '1500,1600p' src/rpc/blockchain.cpp | head -50Length of output: 3358
🏁 Script executed:
#!/bin/bash
# Find the actual caller and what blockindex is passed
rg -n "void.*BlockchainInfo\|getblockchaininfo" src/rpc/blockchain.cpp -A 50 | head -60Length of output: 0
Add null check for genesis block to prevent crash in StateSinceHeight call.
Lines 1436 and 1454 pass blockindex->pprev to g_versionbitscache.State() and StateSinceHeight() without verifying it's not null. If this function is called with the genesis block (where pprev is null), GetStateSinceHeightFor() will dereference nullptr at versionbits.cpp:213 when accessing pindexPrev->nHeight.
Add a defensive check after line 1420:
if (blockindex->pprev == nullptr) return;This prevents crashes when the RPC is invoked on the genesis block index, which while uncommon, is technically valid.
🤖 Prompt for AI Agents
In src/rpc/blockchain.cpp around lines 1415 to 1492, the code calls
g_versionbitscache.State(...) and StateSinceHeight(...) with blockindex->pprev
without ensuring pprev is non-null; this can crash when blockindex is the
genesis block. Add a defensive null check immediately after the early-return
checks (after the null check for blockindex) to return early if
blockindex->pprev == nullptr, preventing dereference of a null previous index
before any calls that use blockindex->pprev.
1 participant
Additional Information
Dependency for backport: merge bitcoin#24595, 25830 (deployment backports) #6888
Dependency for backport: merge bitcoin#25065, #25290, #25494, #25439, #25648, #25772, #25222 (kernel backports: part 1) #6904
The help text for
getblockchaininfo['softforks']was introduced in bitcoin#16060 (backported in dash#5255) but theRPCResulthelp text was introduced in bitcoin#17809, backported before bitcoin#16060. This resulted in the help text forsoftforksdeviating from upstream without the output to match (which is aligned with upstream).This has since been resolved as the output structure hasn't changed, only the help text.
bitcoin#22818 (backported in dash#6214) activated many softforks at height 1, which rendered the block generation calls in
wallet_signrawtransactionwithwallet.py(source) no longer necessary, additionally bitcoin#22550 (backported in dash#6189) added assertions to validate CSV/CLTV activation that were not included in the backport.This has since been resolved, replacing the
generatecalls with assertions.1000to100(matching with upstream, source) as we are no longer generating enough blocks to reach that height anymore.In bitcoin#23508,
AbstractThresholdConditionChecker::GetStateStatisticsFor()retainspindexEndOfPrevPeriodunlike upstream as it's used to calculate dynamic activation threshold (see dash#3692 for more information) but it isn't used to calculateelapsedas bitcoin#23508 no longer calculates the status of the next block but the current block, which prevents complete preservation of old logic.feature_mnehf.pyandfeature_new_quorum_type_activation.pyto mine one more block to get status information expected by the tests.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
Requesting deployment information from
getblockchaininfois now deprecated (replaced bygetdeploymentinfo) and restoring old behaviour will require the runtime flag-deprecatedrpc=softforks.Checklist