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. |
f57c351 to
ffc7fdd
Compare
|
This pull request has conflicts, please rebase. |
|
This pull request has conflicts, please rebase. |
WalkthroughAdds a new RPC getdeploymentinfo and REST endpoints (/rest/deploymentinfo and /rest/deploymentinfo/.json) to expose deployment (softfork) state at the chain tip or at a supplied block. VersionBits logic was extended to provide per-block signalling history (optional signalling_blocks vector) and DeploymentInfo was introduced to compute deployment state from an arbitrary CBlockIndex. getblockchaininfo was adjusted to optionally emit legacy softforks via DeploymentInfo when deprecated RPCs are enabled. Tests and REST documentation were added/updated to exercise the new interfaces. Sequence DiagramsequenceDiagram
participant Client
participant REST_Handler
participant RPC as getdeploymentinfo RPC
participant Chain as Chain/BlockIndex
participant VB as VersionBits/Cache
Client->>REST_Handler: GET /rest/deploymentinfo[/{blockhash}]
REST_Handler->>Chain: validate/lookup blockhash (optional)
Chain-->>REST_Handler: CBlockIndex (or error)
REST_Handler->>RPC: getdeploymentinfo(CBlockIndex)
RPC->>VB: Statistics/GetStateStatisticsFor(CBlockIndex, &signalling_blocks)
VB-->>RPC: BIP9Stats + signalling_blocks
RPC-->>REST_Handler: aggregated DeploymentInfo JSON
REST_Handler-->>Client: HTTP 200 JSON {blockhash, height, deployments}
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 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
| 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);
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!
| {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", "expected activation height for this softfork (only for \"locked_in\" status)"}, |
There was a problem hiding this comment.
Mark activation_height as optional in the RPC help.
This field is only pushed in the LOCKED_IN branch above, so the help schema currently advertises it as always present.
Suggested fix
- {RPCResult::Type::NUM, "activation_height", "expected activation height for this softfork (only for \"locked_in\" status)"},
+ {RPCResult::Type::NUM, "activation_height", /*optional=*/true, "expected activation height for this softfork (only for \"locked_in\" status)"},📝 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.
| {RPCResult::Type::NUM, "activation_height", "expected activation height for this softfork (only for \"locked_in\" status)"}, | |
| {RPCResult::Type::NUM, "activation_height", /*optional=*/true, "expected activation height for this softfork (only for \"locked_in\" status)"}, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rpc/blockchain.cpp` at 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.
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