backport: bitcoin#23508, #24187, #24528, #24579, #25412, #27853 (deployment backports)#6888
backport: bitcoin#23508, #24187, #24528, #24579, #25412, #27853 (deployment backports)#6888kwvg wants to merge 7 commits intodashpay:developfrom
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
This pull request has conflicts, please rebase. |
kwvg
changed the title
kwvg
force-pushed
the
deploy_bps
branch
2 times, most recently
from
f57c351 to
ffc7fdd
Compare
|
This pull request has conflicts, please rebase. |
kwvg
changed the title
|
This pull request has conflicts, please rebase. |
WalkthroughAdds a new RPC getdeploymentinfo and REST endpoints (/rest/deploymentinfo and /rest/deploymentinfo/.json) to return deployment (softfork) state at the chain tip or at a supplied block. VersionBits statistics were extended to optionally return per-period signalling history (signalling_blocks). A DeploymentInfo abstraction was introduced and wired into getdeploymentinfo and conditionally into getblockchaininfo (when -deprecatedrpc=softforks). Tests and REST documentation were added or updated to exercise the new interfaces. Sequence DiagramsequenceDiagram
participant Client
participant REST_Handler as REST Handler
participant RPC as getdeploymentinfo RPC
participant Chain as Chain / CBlockIndex
participant VB as VersionBits / Cache
Client->>REST_Handler: GET /rest/deploymentinfo[/{blockhash}]
REST_Handler->>Chain: validate/lookup blockhash (optional)
alt block found
Chain-->>REST_Handler: CBlockIndex
REST_Handler->>RPC: getdeploymentinfo(CBlockIndex)
RPC->>VB: Statistics / GetStateStatisticsFor(CBlockIndex, &signalling_blocks)
VB-->>RPC: BIP9Stats + signalling_blocks
RPC-->>REST_Handler: DeploymentInfo JSON
REST_Handler-->>Client: HTTP 200 JSON {blockhash, height, deployments}
else error
Chain-->>REST_Handler: error (invalid or not found)
REST_Handler-->>Client: HTTP 400/404 error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/rpc/blockchain.cpp (1)
1456-1462:⚠️ Potential issue | 🟠 MajorRemove duplicate
min_activation_heightfield inbip9output.Line 1456 already emits
min_activation_height, and Line 1461 emits it again. This produces duplicate JSON keys in the same object, which is ambiguous for clients.Proposed fix
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); } - bip9.pushKV("min_activation_height", chainman.GetConsensus().vDeployments[id].min_activation_height);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/blockchain.cpp` around lines 1456 - 1462, The bip9 JSON object is emitting "min_activation_height" twice (via bip9.pushKV("min_activation_height", chainman.GetConsensus().vDeployments[id].min_activation_height)); remove the duplicate pushKV call so the field is only added once; locate the repeated call near the block that also sets "ehf" and optional "ehf_height" (references: bip9.pushKV, chainman.GetConsensus().vDeployments[id].min_activation_height, signals) and delete the second occurrence to avoid duplicate JSON keys.test/functional/rpc_blockchain.py (1)
194-252:⚠️ Potential issue | 🟡 MinorFix Flake8 E121 — continuation line under-indented for hanging indent (Line 198)
The dict literal passed to
assert_equalstarting at Line 197 has inconsistent hanging indent, triggering Flake8 E121.🔧 Proposed fix
- assert_equal(gdi_result, { - "hash": blockhash, - "height": height, - "deployments": { + assert_equal(gdi_result, { + "hash": blockhash, + "height": height, + "deployments": {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/functional/rpc_blockchain.py` around lines 194 - 252, The dict literal in check_signalling_deploymentinfo_result passed to assert_equal has inconsistent hanging indentation causing Flake8 E121; reformat the multi-line dict so the continuation lines are aligned under the opening parenthesis (or the first key) of the assert_equal call and ensure nested dicts maintain consistent indentation; update the block inside check_signalling_deploymentinfo_result (the big "deployments" dict and its nested bip9/testdummy entries) so all continuation lines use the same indent level and closing braces line up to remove the E121 violation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rest.cpp`:
- Around line 639-653: jsonRequest.params is created as an array
(UniValue::VARR) but the code uses pushKV("blockhash", ...) which requires an
object and will throw; change the code that adds the optional blockhash
positional RPC parameter to use jsonRequest.params.push_back(...) with the
string/hash value instead of pushKV, keeping the existing validation flow
(ParseHashStr / ParseHashV checks, chainman lookup via WITH_LOCK(::cs_main,
...)) and ensuring the RPC call to getdeploymentinfo receives the positional
argument as expected.
---
Outside diff comments:
In `@src/rpc/blockchain.cpp`:
- Around line 1456-1462: The bip9 JSON object is emitting
"min_activation_height" twice (via bip9.pushKV("min_activation_height",
chainman.GetConsensus().vDeployments[id].min_activation_height)); remove the
duplicate pushKV call so the field is only added once; locate the repeated call
near the block that also sets "ehf" and optional "ehf_height" (references:
bip9.pushKV, chainman.GetConsensus().vDeployments[id].min_activation_height,
signals) and delete the second occurrence to avoid duplicate JSON keys.
In `@test/functional/rpc_blockchain.py`:
- Around line 194-252: The dict literal in
check_signalling_deploymentinfo_result passed to assert_equal has inconsistent
hanging indentation causing Flake8 E121; reformat the multi-line dict so the
continuation lines are aligned under the opening parenthesis (or the first key)
of the assert_equal call and ensure nested dicts maintain consistent
indentation; update the block inside check_signalling_deploymentinfo_result (the
big "deployments" dict and its nested bip9/testdummy entries) so all
continuation lines use the same indent level and closing braces line up to
remove the E121 violation.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
doc/REST-interface.mddoc/release-notes-6888.mdsrc/rest.cppsrc/rpc/blockchain.cppsrc/test/fuzz/rpc.cppsrc/test/fuzz/versionbits.cppsrc/versionbits.cppsrc/versionbits.htest/functional/feature_asset_locks.pytest/functional/feature_cltv.pytest/functional/feature_dersig.pytest/functional/feature_governance.pytest/functional/feature_mnehf.pytest/functional/feature_new_quorum_type_activation.pytest/functional/interface_rest.pytest/functional/rpc_blockchain.pytest/functional/test_framework/blocktools.pytest/functional/test_framework/util.pytest/functional/wallet_signrawtransactionwithwallet.py
kwvg
requested review from
PastaPastaPasta,
UdjinM6 and
knst
and removed request for
PastaPastaPasta,
UdjinM6 and
knst
| obj.pushKV("softforks", softforks); | ||
| return softforks; | ||
| } | ||
| } // anonymous namespace |
There was a problem hiding this comment.
nit: bitcoin says anon namespace
| } | ||
| currentIndex = currentIndex->pprev; | ||
| } | ||
| } while (blocks_in_period > 0); |
There was a problem hiding this comment.
nit: remove extra space } while(blocks_in_period > 0);
kwvg
changed the title
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)
test/functional/rpc_blockchain.py (1)
197-252:⚠️ Potential issue | 🟡 MinorFix the hanging-indent in the expected
getdeploymentinfofixture.Flake8 is already flagging Line 198 with
E121, so this test file won't pass lint as written. Please reindent the multilineassert_equal(...)payload consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/functional/rpc_blockchain.py` around lines 197 - 252, The multi-line expected payload passed to assert_equal for gdi_result has inconsistent/hanging indentation causing flake8 E121; reformat the dictionary so all continuation lines are consistently aligned (e.g., align nested dict entries under the opening brace or align the first key line after the opening parenthesis), ensuring the block in the assert_equal(gdi_result, {...}) call (including all deployment entries like 'v24' and 'testdummy') uses consistent indentation level throughout.src/versionbits.cpp (1)
158-165:⚠️ Potential issue | 🟠 MajorUse the current period when deriving dynamic thresholds.
Line 158 aligns to the end of the current period. On the last block of a period that makes
GetStateFor()/GetStateSinceHeightFor()observe the next period, sonAttemptincrements one period too early. For deployments with a falling threshold,statistics.thresholdandstatistics.possibleare wrong exactly at the rollover block.Suggested fix
- const CBlockIndex* pindexEndOfPrevPeriod = pindex->GetAncestor(pindex->nHeight - ((pindex->nHeight + 1) % stats.period)); + const CBlockIndex* pindexPrevForCurrentState = pindex->pprev; + if (pindexPrevForCurrentState != nullptr) { + pindexPrevForCurrentState = pindexPrevForCurrentState->GetAncestor( + pindexPrevForCurrentState->nHeight - ((pindexPrevForCurrentState->nHeight + 1) % stats.period)); + } int nAttempt{0}; - const ThresholdState state = GetStateFor(pindexEndOfPrevPeriod, params, cache); + const ThresholdState state = GetStateFor(pindexPrevForCurrentState, params, cache); if (state == ThresholdState::STARTED) { - int nStartHeight = GetStateSinceHeightFor(pindexEndOfPrevPeriod, params, cache); - nAttempt = (pindexEndOfPrevPeriod->nHeight + 1 - nStartHeight)/stats.period; + const int nStartHeight = GetStateSinceHeightFor(pindexPrevForCurrentState, params, cache); + nAttempt = (pindex->nHeight - nStartHeight) / stats.period; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/versionbits.cpp` around lines 158 - 165, The code currently queries the period boundary using pindexEndOfPrevPeriod (computed with (nHeight + 1) % period) which points to the end of the previous period on the last block of a period and causes nAttempt to be advanced one period too early; change the ancestor calculation to use the current period boundary (use pindex->GetAncestor(pindex->nHeight - (pindex->nHeight % stats.period)) ), call GetStateFor and GetStateSinceHeightFor against that current-period index (update the variable name accordingly), then compute nAttempt from that result before calling Threshold(params, nAttempt) so statistics.threshold/possible reflect the current period instead of the next one.
🧹 Nitpick comments (2)
doc/release-notes-6888.md (1)
4-17: Keep the backported release-note text verbatim.The added
(#6888)suffixes make this copied upstream text diverge from Bitcoin Core and will add noise to future backports/diff reviews. If you want local attribution, add it as a separate note outside the copied bullets instead of editing the imported prose.Based on learnings, preserve content copied from Bitcoin Core upstream verbatim to maintain accurate diffs and attribution. Do not edit for grammar/style; only modify if the upstream content changes and ensure attribution remains clear.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/release-notes-6888.md` around lines 4 - 17, The release-note bullets were modified by adding "(`#6888`)" suffixes, diverging from upstream text; revert the two bullets that mention getblockchaininfo/getdeploymentinfo and the new /rest/deploymentinfo endpoint to match the original upstream wording exactly (remove the "(`#6888`)" annotations), and if local attribution is required add a separate short note outside the quoted bullets (not inline) stating the backport reference; locate and update the lines containing the strings "getblockchaininfo", "getdeploymentinfo", and "/rest/deploymentinfo" to perform the change.src/test/fuzz/versionbits.cpp (1)
284-289: Exercise the boundary block withsignalstoo.The last block of the period is where rollover/off-by-one bugs tend to hide, but this branch only checks the no-signals overload. Requesting
signalshere as well would cover the new indexing path at the most interesting boundary.Possible test extension
- const BIP9Stats stats = checker.GetStateStatisticsFor(current_block); + std::vector<bool> 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); + assert(stats.threshold == stats_no_signals.threshold); + assert(stats.elapsed == stats_no_signals.elapsed); + assert(stats.count == stats_no_signals.count); + assert(stats.possible == stats_no_signals.possible); 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)); + last_signals.push_back(signal); + assert(signals == last_signals);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/versionbits.cpp` around lines 284 - 289, The test currently calls checker.GetStateStatisticsFor(current_block) and asserts BIP9Stats fields for the final block of a period but misses the overload that accepts a signals vector; update the test to also call checker.GetStateStatisticsFor(current_block, signals) (using the same signals used earlier in the test or constructing the boundary signals) and assert the same properties (stats.period, stats.threshold, stats.elapsed, stats.count, and stats.possible) to exercise the signals-indexing path for the boundary block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rpc/blockchain.cpp`:
- Line 1612: The RPC help currently lists the RPCResult entry
{RPCResult::Type::NUM, "activation_height", "..."} as required; mark this field
optional since it's only returned for the "locked_in" status by changing the
RPCResult entry for "activation_height" to include the optional flag (e.g. add
the optional true parameter or equivalent API flag used by RPCResult) in the
same RPCResult initializer where "activation_height" is defined.
- Around line 1489-1491: The activation_height calculation uses
vDeployments[id].nWindowSize directly, which is wrong when that field is zero;
instead compute the effective signalling period via the
VersionBitsConditionChecker::Period() (which falls back to
params.nMinerConfirmationWindow) for the deployment and add that period to
since_height before pushing "activation_height" into bip9; update the block that
checks ThresholdState::LOCKED_IN to use the VersionBitsConditionChecker-derived
period for vDeployments[id] rather than vDeployments[id].nWindowSize.
---
Outside diff comments:
In `@src/versionbits.cpp`:
- Around line 158-165: The code currently queries the period boundary using
pindexEndOfPrevPeriod (computed with (nHeight + 1) % period) which points to the
end of the previous period on the last block of a period and causes nAttempt to
be advanced one period too early; change the ancestor calculation to use the
current period boundary (use pindex->GetAncestor(pindex->nHeight -
(pindex->nHeight % stats.period)) ), call GetStateFor and GetStateSinceHeightFor
against that current-period index (update the variable name accordingly), then
compute nAttempt from that result before calling Threshold(params, nAttempt) so
statistics.threshold/possible reflect the current period instead of the next
one.
In `@test/functional/rpc_blockchain.py`:
- Around line 197-252: The multi-line expected payload passed to assert_equal
for gdi_result has inconsistent/hanging indentation causing flake8 E121;
reformat the dictionary so all continuation lines are consistently aligned
(e.g., align nested dict entries under the opening brace or align the first key
line after the opening parenthesis), ensuring the block in the
assert_equal(gdi_result, {...}) call (including all deployment entries like
'v24' and 'testdummy') uses consistent indentation level throughout.
---
Nitpick comments:
In `@doc/release-notes-6888.md`:
- Around line 4-17: The release-note bullets were modified by adding "(`#6888`)"
suffixes, diverging from upstream text; revert the two bullets that mention
getblockchaininfo/getdeploymentinfo and the new /rest/deploymentinfo endpoint to
match the original upstream wording exactly (remove the "(`#6888`)" annotations),
and if local attribution is required add a separate short note outside the
quoted bullets (not inline) stating the backport reference; locate and update
the lines containing the strings "getblockchaininfo", "getdeploymentinfo", and
"/rest/deploymentinfo" to perform the change.
In `@src/test/fuzz/versionbits.cpp`:
- Around line 284-289: The test currently calls
checker.GetStateStatisticsFor(current_block) and asserts BIP9Stats fields for
the final block of a period but misses the overload that accepts a signals
vector; update the test to also call
checker.GetStateStatisticsFor(current_block, signals) (using the same signals
used earlier in the test or constructing the boundary signals) and assert the
same properties (stats.period, stats.threshold, stats.elapsed, stats.count, and
stats.possible) to exercise the signals-indexing path for the boundary block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a44608c4-a5fd-4e26-a5f2-feaaebd2fe6d
📒 Files selected for processing (19)
doc/REST-interface.mddoc/release-notes-6888.mdsrc/rest.cppsrc/rpc/blockchain.cppsrc/test/fuzz/rpc.cppsrc/test/fuzz/versionbits.cppsrc/versionbits.cppsrc/versionbits.htest/functional/feature_asset_locks.pytest/functional/feature_cltv.pytest/functional/feature_dersig.pytest/functional/feature_governance.pytest/functional/feature_mnehf.pytest/functional/feature_new_quorum_type_activation.pytest/functional/interface_rest.pytest/functional/rpc_blockchain.pytest/functional/test_framework/blocktools.pytest/functional/test_framework/util.pytest/functional/wallet_signrawtransactionwithwallet.py
🚧 Files skipped from review as they are similar to previous changes (7)
- src/test/fuzz/rpc.cpp
- test/functional/test_framework/blocktools.py
- test/functional/feature_dersig.py
- doc/REST-interface.md
- test/functional/feature_asset_locks.py
- test/functional/test_framework/util.py
- test/functional/feature_new_quorum_type_activation.py
| if (ThresholdState::LOCKED_IN == current_state) { | ||
| bip9.pushKV("activation_height", since_height + static_cast<int>(chainman.GetConsensus().vDeployments[id].nWindowSize)); | ||
| } |
There was a problem hiding this comment.
Use the effective signalling period for activation_height.
Line 1490 adds raw vDeployments[id].nWindowSize, but VersionBitsConditionChecker::Period() falls back to params.nMinerConfirmationWindow when that field is zero. For deployments using the default window, getdeploymentinfo will report activation_height == since_height instead of one full period later.
Suggested fix
if (ThresholdState::LOCKED_IN == current_state) {
- bip9.pushKV("activation_height", since_height + static_cast<int>(chainman.GetConsensus().vDeployments[id].nWindowSize));
+ const int period = chainman.GetConsensus().vDeployments[id].nWindowSize
+ ? static_cast<int>(chainman.GetConsensus().vDeployments[id].nWindowSize)
+ : chainman.GetConsensus().nMinerConfirmationWindow;
+ bip9.pushKV("activation_height", since_height + period);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (ThresholdState::LOCKED_IN == current_state) { | |
| bip9.pushKV("activation_height", since_height + static_cast<int>(chainman.GetConsensus().vDeployments[id].nWindowSize)); | |
| } | |
| if (ThresholdState::LOCKED_IN == current_state) { | |
| const int period = chainman.GetConsensus().vDeployments[id].nWindowSize | |
| ? static_cast<int>(chainman.GetConsensus().vDeployments[id].nWindowSize) | |
| : chainman.GetConsensus().nMinerConfirmationWindow; | |
| bip9.pushKV("activation_height", since_height + period); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rpc/blockchain.cpp` around lines 1489 - 1491, The activation_height
calculation uses vDeployments[id].nWindowSize directly, which is wrong when that
field is zero; instead compute the effective signalling period via the
VersionBitsConditionChecker::Period() (which falls back to
params.nMinerConfirmationWindow) for the deployment and add that period to
since_height before pushing "activation_height" into bip9; update the block that
checks ThresholdState::LOCKED_IN to use the VersionBitsConditionChecker-derived
period for vDeployments[id] rather than vDeployments[id].nWindowSize.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
We always set nWindowSize anyway so not a real issue imo. Also, nMinerConfirmationWindow values are off I think. All of it can be fixed in a separate PR if needed.
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 (1)
src/versionbits.cpp (1)
158-164:⚠️ Potential issue | 🟠 MajorFix period-boundary ancestor selection for
nAttemptcalculation.At Line 158, the current formula uses current-block
pindexbut still applies apindexPrev-style boundary expression. On boundary heights, this selects the current period tip, so Lines 161-164 can computenAttemptfor the next period and misreport threshold statistics for the last block of a STARTED period.🐛 Proposed fix
- const CBlockIndex* pindexEndOfPrevPeriod = pindex->GetAncestor(pindex->nHeight - ((pindex->nHeight + 1) % stats.period)); + const int prev_period_end_height = pindex->nHeight - (pindex->nHeight % stats.period) - 1; + const CBlockIndex* pindexEndOfPrevPeriod = pindex->GetAncestor(prev_period_end_height);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/versionbits.cpp` around lines 158 - 164, The period-boundary ancestor selection for pindexEndOfPrevPeriod is off-by-one: replace the ancestor offset that uses (pindex->nHeight + 1) % stats.period with one based on pindex->nHeight % stats.period so pindexEndOfPrevPeriod truly points to the tip of the previous completed period (use pindex->GetAncestor(pindex->nHeight - (pindex->nHeight % stats.period))). Keep the rest of the logic (GetStateFor, ThresholdState::STARTED, GetStateSinceHeightFor and nAttempt calculation) unchanged but ensure they operate on that corrected pindexEndOfPrevPeriod so nAttempt reflects the correct current period rather than the next one.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/REST-interface.md`:
- Line 89: Replace the inconsistent heading token "#### Deployment info" with
the repository's canonical heading style to satisfy MD003 — specifically update
that line to match surrounding headings (e.g., change it to "### Deployment
info" if other sections use three-level ATX headings) so the heading style is
consistent across doc/REST-interface.md.
In `@test/functional/rpc_blockchain.py`:
- Around line 197-200: The multi-line assert_equal call is mis-indented (Flake8
E121); align the hanging indent so the continued lines are indented to match the
opening parenthesis of assert_equal(gdi_result, { ... }); adjust the lines
containing "hash": blockhash, "height": height, and "deployments": { (and
subsequent nested lines) to use a consistent extra indent level under
assert_equal to remove the under-indentation while preserving the existing
dictionary structure and keys.
---
Outside diff comments:
In `@src/versionbits.cpp`:
- Around line 158-164: The period-boundary ancestor selection for
pindexEndOfPrevPeriod is off-by-one: replace the ancestor offset that uses
(pindex->nHeight + 1) % stats.period with one based on pindex->nHeight %
stats.period so pindexEndOfPrevPeriod truly points to the tip of the previous
completed period (use pindex->GetAncestor(pindex->nHeight - (pindex->nHeight %
stats.period))). Keep the rest of the logic (GetStateFor,
ThresholdState::STARTED, GetStateSinceHeightFor and nAttempt calculation)
unchanged but ensure they operate on that corrected pindexEndOfPrevPeriod so
nAttempt reflects the correct current period rather than the next one.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6fe83ee8-b5dc-43d1-9fe7-ccf2b3e79c20
📒 Files selected for processing (19)
doc/REST-interface.mddoc/release-notes-6888.mdsrc/rest.cppsrc/rpc/blockchain.cppsrc/test/fuzz/rpc.cppsrc/test/fuzz/versionbits.cppsrc/versionbits.cppsrc/versionbits.htest/functional/feature_asset_locks.pytest/functional/feature_cltv.pytest/functional/feature_dersig.pytest/functional/feature_governance.pytest/functional/feature_mnehf.pytest/functional/feature_new_quorum_type_activation.pytest/functional/interface_rest.pytest/functional/rpc_blockchain.pytest/functional/test_framework/blocktools.pytest/functional/test_framework/util.pytest/functional/wallet_signrawtransactionwithwallet.py
✅ Files skipped from review due to trivial changes (1)
- doc/release-notes-6888.md
🚧 Files skipped from review as they are similar to previous changes (4)
- test/functional/feature_asset_locks.py
- src/rest.cpp
- test/functional/feature_dersig.py
- test/functional/feature_cltv.py
| Only supports JSON as output format. | ||
| Refer to the `getblockchaininfo` RPC help for details. | ||
|
|
||
| #### Deployment info |
There was a problem hiding this comment.
Fix heading style to satisfy markdownlint MD003.
This new heading currently triggers the configured heading-style rule; please align it with the repository’s markdownlint style to avoid lint noise.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 89-89: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/REST-interface.md` at line 89, Replace the inconsistent heading token
"#### Deployment info" with the repository's canonical heading style to satisfy
MD003 — specifically update that line to match surrounding headings (e.g.,
change it to "### Deployment info" if other sections use three-level ATX
headings) so the heading style is consistent across doc/REST-interface.md.
| assert_equal(gdi_result, { | ||
| "hash": blockhash, | ||
| "height": height, | ||
| "deployments": { |
There was a problem hiding this comment.
Flake8 E121: hanging indent is under-indented.
Line 198 is under-indented in the multi-line assert_equal(...) expression and will fail lint checks.
💡 Proposed fix
- assert_equal(gdi_result, {
- "hash": blockhash,
- "height": height,
- "deployments": {
+ expected = {
+ "hash": blockhash,
+ "height": height,
+ "deployments": {
@@
- }
- })
+ }
+ }
+ assert_equal(gdi_result, expected)🧰 Tools
🪛 Flake8 (7.3.0)
[error] 198-198: continuation line under-indented for hanging indent
(E121)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/functional/rpc_blockchain.py` around lines 197 - 200, The multi-line
assert_equal call is mis-indented (Flake8 E121); align the hanging indent so the
continued lines are indented to match the opening parenthesis of
assert_equal(gdi_result, { ... }); adjust the lines containing "hash":
blockhash, "height": height, and "deployments": { (and subsequent nested lines)
to use a consistent extra indent level under assert_equal to remove the
under-indentation while preserving the existing dictionary structure and keys.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The backport is well-adapted for Dash. No blocking issues found. Claude's variable shadowing finding is confirmed and worth addressing. The help field-order mismatch is real but cosmetic. The feature_mnehf.py generate adjustments finding is valid but trivial.
Reviewed commit: e5688b2
🟡 2 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/rpc/blockchain.cpp`:
- [SUGGESTION] lines 1470-1486: Variable `signals` shadows function parameter
The local `std::vector<bool> signals` at line 1470 shadows the function parameter `const std::unordered_map<uint8_t, int>& signals` declared at line 1425. The parameter is last used at line 1457 (before the shadow begins), so there is no functional bug, but this will trigger `-Wshadow` compiler warnings and is confusing to read.
This is Dash-specific — upstream Bitcoin Core doesn't have a `signals` parameter on this function (it's the EHF signals map). Renaming the local variable to `signalling_blocks` would resolve the shadow and align with upstream naming conventions.
- [SUGGESTION] lines 1601-1627: RPC help field order does not match actual output order for bip9 object
The `RPCHelpForDeployment` vector declares `activation_height` and `min_activation_height` at lines 1612-1613 (before `status`), but the actual code emits them at lines 1489-1491 (after `signalling`, at the end of the bip9 object).
Help order: `bit, start_time, timeout, ehf, ehf_height, activation_height, min_activation_height, status, since, status_next, statistics, signalling`
Actual order: `bit, start_time, timeout, ehf, ehf_height, status, since, status_next, statistics, signalling, activation_height, min_activation_height`
JSON objects are unordered by spec, and the `MatchesType` RPC doc check only validates types (not key ordering), so this causes no runtime failure. It's a cosmetic documentation inaccuracy.
| std::vector<bool> signals; | ||
| BIP9Stats statsStruct = chainman.m_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); |
There was a problem hiding this comment.
🟡 Suggestion: Variable signals shadows function parameter
The local std::vector<bool> signals at line 1470 shadows the function parameter const std::unordered_map<uint8_t, int>& signals declared at line 1425. The parameter is last used at line 1457 (before the shadow begins), so there is no functional bug, but this will trigger -Wshadow compiler warnings and is confusing to read.
This is Dash-specific — upstream Bitcoin Core doesn't have a signals parameter on this function (it's the EHF signals map). Renaming the local variable to signalling_blocks would resolve the shadow and align with upstream naming conventions.
💡 Suggested change
| std::vector<bool> signals; | |
| BIP9Stats statsStruct = chainman.m_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); | |
| std::vector<bool> signalling_blocks; | |
| BIP9Stats statsStruct = chainman.m_versionbitscache.Statistics(blockindex, chainman.GetConsensus(), id, &signalling_blocks); | |
| statsUV.pushKV("period", statsStruct.period); | |
| statsUV.pushKV("elapsed", statsStruct.elapsed); | |
| statsUV.pushKV("count", statsStruct.count); | |
| 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(signalling_blocks.size()); | |
| for (const bool s : signalling_blocks) { | |
| sig.push_back(s ? '#' : '-'); | |
| } | |
| bip9.pushKV("signalling", sig); |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/rpc/blockchain.cpp`:
- [SUGGESTION] lines 1470-1486: Variable `signals` shadows function parameter
The local `std::vector<bool> signals` at line 1470 shadows the function parameter `const std::unordered_map<uint8_t, int>& signals` declared at line 1425. The parameter is last used at line 1457 (before the shadow begins), so there is no functional bug, but this will trigger `-Wshadow` compiler warnings and is confusing to read.
This is Dash-specific — upstream Bitcoin Core doesn't have a `signals` parameter on this function (it's the EHF signals map). Renaming the local variable to `signalling_blocks` would resolve the shadow and align with upstream naming conventions.
| const std::vector<RPCResult> RPCHelpForDeployment{ | ||
| {RPCResult::Type::STR, "type", "one of \"buried\", \"bip9\""}, | ||
| {RPCResult::Type::NUM, "height", /*optional=*/true, "height of the first block which the rules are or will be enforced (only for \"buried\" type, or \"bip9\" type with \"active\" status)"}, | ||
| {RPCResult::Type::BOOL, "active", "true if the rules are enforced for the mempool and the next block"}, | ||
| {RPCResult::Type::OBJ, "bip9", /*optional=*/true, "status of bip9 softforks (only for \"bip9\" type)", | ||
| { | ||
| {RPCResult::Type::NUM, "bit", /*optional=*/true, "the bit (0-28) in the block version field used to signal this softfork (only for \"started\" and \"locked_in\" status)"}, | ||
| {RPCResult::Type::NUM_TIME, "start_time", "the minimum median time past of a block at which the bit gains its meaning"}, | ||
| {RPCResult::Type::NUM_TIME, "timeout", "the median time past of a block at which the deployment is considered failed if not yet locked in"}, | ||
| {RPCResult::Type::BOOL, "ehf", "returns true for EHF activated forks"}, | ||
| {RPCResult::Type::NUM, "ehf_height", /*optional=*/true, "the minimum height when miner's signals for the deployment matter. Below this height miner signaling cannot trigger hard fork lock-in. Not specified for non-EHF forks"}, | ||
| {RPCResult::Type::NUM, "activation_height", /*optional=*/true, "expected activation height for this softfork (only for \"locked_in\" status)"}, | ||
| {RPCResult::Type::NUM, "min_activation_height", "minimum height of blocks for which the rules may be enforced"}, | ||
| {RPCResult::Type::STR, "status", "status of deployment at specified block (one of \"defined\", \"started\", \"locked_in\", \"active\", \"failed\")"}, | ||
| {RPCResult::Type::NUM, "since", "height of the first block to which the status applies"}, | ||
| {RPCResult::Type::STR, "status_next", "status of deployment at the next block"}, | ||
| {RPCResult::Type::OBJ, "statistics", /*optional=*/true, "numeric statistics about signalling for a softfork (only for \"started\" and \"locked_in\" status)", | ||
| { | ||
| {RPCResult::Type::NUM, "period", "the length in blocks of the signalling period"}, | ||
| {RPCResult::Type::NUM, "threshold", /*optional=*/true, "the number of blocks with the version bit set required to activate the feature (only for \"started\" status)"}, | ||
| {RPCResult::Type::NUM, "elapsed", "the number of blocks elapsed since the beginning of the current period"}, | ||
| {RPCResult::Type::NUM, "count", "the number of blocks with the version bit set in the current period"}, | ||
| {RPCResult::Type::BOOL, "possible", /*optional=*/true, "returns false if there are not enough blocks left in this period to pass activation threshold (only for \"started\" status)"}, | ||
| }}, | ||
| {RPCResult::Type::STR, "signalling", /*optional=*/true, "indicates blocks that signalled with a # and blocks that did not with a -"}, | ||
| }}, | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: RPC help field order does not match actual output order for bip9 object
The RPCHelpForDeployment vector declares activation_height and min_activation_height at lines 1612-1613 (before status), but the actual code emits them at lines 1489-1491 (after signalling, at the end of the bip9 object).
Help order: bit, start_time, timeout, ehf, ehf_height, activation_height, min_activation_height, status, since, status_next, statistics, signalling
Actual order: bit, start_time, timeout, ehf, ehf_height, status, since, status_next, statistics, signalling, activation_height, min_activation_height
JSON objects are unordered by spec, and the MatchesType RPC doc check only validates types (not key ordering), so this causes no runtime failure. It's a cosmetic documentation inaccuracy.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/rpc/blockchain.cpp`:
- [SUGGESTION] lines 1601-1627: RPC help field order does not match actual output order for bip9 object
The `RPCHelpForDeployment` vector declares `activation_height` and `min_activation_height` at lines 1612-1613 (before `status`), but the actual code emits them at lines 1489-1491 (after `signalling`, at the end of the bip9 object).
Help order: `bit, start_time, timeout, ehf, ehf_height, activation_height, min_activation_height, status, since, status_next, statistics, signalling`
Actual order: `bit, start_time, timeout, ehf, ehf_height, status, since, status_next, statistics, signalling, activation_height, min_activation_height`
JSON objects are unordered by spec, and the `MatchesType` RPC doc check only validates types (not key ordering), so this causes no runtime failure. It's a cosmetic documentation inaccuracy.
| @@ -197,7 +200,7 @@ def run_test(self): | |||
| assert tx_sent_2 in node.getblock(ehf_blockhash_2)['tx'] | |||
|
|
|||
| self.log.info(f"Generate some more block to jump to `started` status") | |||
| self.generate(node, 4) | |||
| self.generate(node, 5) | |||
There was a problem hiding this comment.
💬 Nitpick: Extra generate calls lack comments explaining the BIP9 semantic adjustment
Three self.generate(node, 1) calls were added (at lines 168, 174, 182) and one block count was changed from 4 to 5 (line 203) to compensate for the BIP9 semantic change where status now reports the current block's state rather than the next block's. A brief inline comment (e.g., # extra block needed: status is now current-block, not next-block) would help future readers understand why the counts changed.
source: ['claude']
There was a problem hiding this comment.
extra block needed: status is now current-block, not next-block) I belive commit message explain it already if anyone will look to 'git blame`
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Backport is largely correct. The variable shadowing of signals is a Dash-specific merge artifact that is functionally harmless. The activation_height calculation has a Dash-specific adaptation error where it uses raw nWindowSize instead of matching the fallback logic in Period(), affecting deployments with nWindowSize==0 (currently only TESTDUMMY on regtest). The versionbits boundary finding is a false positive confirmed by unit tests.
Reviewed commit: e5688b2
🟡 1 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/rpc/blockchain.cpp`:
- [SUGGESTION] line 1489: `activation_height` uses raw `nWindowSize` without the `Period()` fallback
When `nWindowSize == 0`, `Period()` (versionbits.cpp:251) falls back to `nMinerConfirmationWindow`, but `activation_height` here uses `nWindowSize` directly, yielding `since_height + 0 = since_height`.
`DEPLOYMENT_TESTDUMMY` on regtest has `nWindowSize == 0` (default) and `nStartTime == 0`, so it can reach `LOCKED_IN`. The reported `activation_height` would be wrong. Production deployments (`V24`) always set `nWindowSize` explicitly, so this only affects test scenarios.
This is a Dash-specific adaptation error: upstream bitcoin#23508 uses `nMinerConfirmationWindow` (a single global value), but the Dash adaptation substituted `nWindowSize` without the per-deployment fallback logic.
| } | ||
| if (ThresholdState::LOCKED_IN == thresholdState) { | ||
| if (ThresholdState::LOCKED_IN == current_state) { | ||
| bip9.pushKV("activation_height", since_height + static_cast<int>(chainman.GetConsensus().vDeployments[id].nWindowSize)); |
There was a problem hiding this comment.
🟡 Suggestion: activation_height uses raw nWindowSize without the Period() fallback
When nWindowSize == 0, Period() (versionbits.cpp:251) falls back to nMinerConfirmationWindow, but activation_height here uses nWindowSize directly, yielding since_height + 0 = since_height.
DEPLOYMENT_TESTDUMMY on regtest has nWindowSize == 0 (default) and nStartTime == 0, so it can reach LOCKED_IN. The reported activation_height would be wrong. Production deployments (V24) always set nWindowSize explicitly, so this only affects test scenarios.
This is a Dash-specific adaptation error: upstream bitcoin#23508 uses nMinerConfirmationWindow (a single global value), but the Dash adaptation substituted nWindowSize without the per-deployment fallback logic.
💡 Suggested change
| bip9.pushKV("activation_height", since_height + static_cast<int>(chainman.GetConsensus().vDeployments[id].nWindowSize)); | |
| bip9.pushKV("activation_height", since_height + static_cast<int>(chainman.GetConsensus().vDeployments[id].nWindowSize ? chainman.GetConsensus().vDeployments[id].nWindowSize : chainman.GetConsensus().nMinerConfirmationWindow)); |
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/rpc/blockchain.cpp`:
- [SUGGESTION] line 1489: `activation_height` uses raw `nWindowSize` without the `Period()` fallback
When `nWindowSize == 0`, `Period()` (versionbits.cpp:251) falls back to `nMinerConfirmationWindow`, but `activation_height` here uses `nWindowSize` directly, yielding `since_height + 0 = since_height`.
`DEPLOYMENT_TESTDUMMY` on regtest has `nWindowSize == 0` (default) and `nStartTime == 0`, so it can reach `LOCKED_IN`. The reported `activation_height` would be wrong. Production deployments (`V24`) always set `nWindowSize` explicitly, so this only affects test scenarios.
This is a Dash-specific adaptation error: upstream bitcoin#23508 uses `nMinerConfirmationWindow` (a single global value), but the Dash adaptation substituted `nWindowSize` without the per-deployment fallback logic.
| if (has_signal) { | ||
| UniValue statsUV(UniValue::VOBJ); | ||
| BIP9Stats statsStruct = chainman.m_versionbitscache.Statistics(active_chain_tip, chainman.GetConsensus(), id); | ||
| std::vector<bool> signals; |
There was a problem hiding this comment.
💬 Nitpick: Local signals shadows the function parameter signals
The std::vector<bool> signals at line 1470 shadows the function parameter const std::unordered_map<uint8_t, int>& signals at line 1425. The parameter is used at line 1457 (before this scope), so the code is functionally correct — the shadow only exists within the if (has_signal) block where the parameter isn't needed.
This is a Dash-specific merge artifact: the signals parameter comes from Dash's EHF system, while the local signals variable comes from upstream Bitcoin Core's BIP9 statistics code.
💡 Suggested change
| std::vector<bool> signals; | |
| std::vector<bool> signalling_blocks; | |
| BIP9Stats statsStruct = chainman.m_versionbitscache.Statistics(blockindex, chainman.GetConsensus(), id, &signalling_blocks); |
source: ['claude']
| # block just prior to lock in | ||
| self.generate(self.wallet, 287 - gbci207["blocks"]) | ||
| gbci287 = self.nodes[0].getblockchaininfo() | ||
| self.check_signalling_deploymentinfo_result(self.nodes[0].getdeploymentinfo(), gbci287["blocks"], gbci287["bestblockhash"], "locked_in") |
There was a problem hiding this comment.
💬 Nitpick: Test never exercises the activation_height output path
The test advances testdummy to status_next == 'locked_in' but never to current_state == LOCKED_IN. The activation_height field is only emitted when current_state == ThresholdState::LOCKED_IN (blockchain.cpp:1488), so this code path is untested. This means the nWindowSize == 0 bug above wouldn't be caught by the test suite.
source: ['codex']
4 participants
Additional Information
Depends on backport: merge bitcoin#23345, #24410, #15936, #25748, #25863, #24051, #25315, #26624, #26894, #26673, #24462, #25815, #22949, #26723, #23395, #27016, #27189, #27317 (auxiliary backports: part 28) #6901
Depends on backport: merge bitcoin#24595 (move g_versionbitscache global to ChainstateManager) #6933
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.Breaking Changes
Requesting deployment information from
getblockchaininfois now deprecated (replaced bygetdeploymentinfo) and restoring old behaviour will require the runtime flag-deprecatedrpc=softforks.Checklist