Skip to content

backport: bitcoin#23508, #24187, #24528, #24579, #25412, #27853 (deployment backports)#6888

Open
kwvg wants to merge 7 commits intodashpay:developfrom
kwvg:deploy_bps
Open

backport: bitcoin#23508, #24187, #24528, #24579, #25412, #27853 (deployment backports)#6888
kwvg wants to merge 7 commits intodashpay:developfrom
kwvg:deploy_bps

Conversation

@kwvg
Copy link
Collaborator

@kwvg kwvg commented Oct 14, 2025

Additional Information

Breaking Changes

Requesting deployment information from getblockchaininfo is now deprecated (replaced by getdeploymentinfo) and restoring old behaviour will require the runtime flag -deprecatedrpc=softforks.

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas (note: N/A)
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@kwvg kwvg added this to the 23 milestone Oct 14, 2025
@github-actions
Copy link

github-actions bot commented Oct 14, 2025

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@github-actions
Copy link

This pull request has conflicts, please rebase.

@kwvg kwvg changed the title backport: merge bitcoin#23508, #24187, #24528, #24579, #24595, #25830, #25412 (deployment backports) backport: merge bitcoin#24595, 25830 (deployment backports) Oct 19, 2025
@kwvg kwvg force-pushed the deploy_bps branch 2 times, most recently from f57c351 to ffc7fdd Compare October 19, 2025 19:11
@github-actions
Copy link

This pull request has conflicts, please rebase.

@kwvg kwvg modified the milestones: 23, 23.1 Oct 21, 2025
@kwvg kwvg changed the title backport: merge bitcoin#24595, 25830 (deployment backports) backport: bitcoin#23508, #24187, #24528, #24579, #25412 (deployment backports) Oct 23, 2025
@kwvg kwvg added the RPC Some notable changes to RPC params/behaviour/descriptions label Oct 23, 2025
@kwvg kwvg requested a review from PastaPastaPasta October 23, 2025 15:51
@github-actions
Copy link

This pull request has conflicts, please rebase.

@kwvg kwvg marked this pull request as ready for review February 25, 2026 01:56
@coderabbitai
Copy link

coderabbitai bot commented Feb 25, 2026

Walkthrough

Adds 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 Diagram

sequenceDiagram
    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}
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.28% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the PR as a backport of multiple Bitcoin PRs related to deployment information and RPC changes, which aligns with the substantial changes across RPC, REST, and test files.
Description check ✅ Passed The description provides relevant context about the backported changes, dependencies, breaking changes, test adjustments, and includes a completed checklist, all of which relate to the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟠 Major

Remove duplicate min_activation_height field in bip9 output.

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 | 🟡 Minor

Fix Flake8 E121 — continuation line under-indented for hanging indent (Line 198)

The dict literal passed to assert_equal starting 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

📥 Commits

Reviewing files that changed from the base of the PR and between d71b89c and 669af55.

📒 Files selected for processing (19)
  • doc/REST-interface.md
  • doc/release-notes-6888.md
  • src/rest.cpp
  • src/rpc/blockchain.cpp
  • src/test/fuzz/rpc.cpp
  • src/test/fuzz/versionbits.cpp
  • src/versionbits.cpp
  • src/versionbits.h
  • test/functional/feature_asset_locks.py
  • test/functional/feature_cltv.py
  • test/functional/feature_dersig.py
  • test/functional/feature_governance.py
  • test/functional/feature_mnehf.py
  • test/functional/feature_new_quorum_type_activation.py
  • test/functional/interface_rest.py
  • test/functional/rpc_blockchain.py
  • test/functional/test_framework/blocktools.py
  • test/functional/test_framework/util.py
  • test/functional/wallet_signrawtransactionwithwallet.py

@kwvg kwvg requested review from PastaPastaPasta, UdjinM6 and knst and removed request for PastaPastaPasta, UdjinM6 and knst March 9, 2026 21:52
knst
knst previously approved these changes Mar 10, 2026
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 1019d64

obj.pushKV("softforks", softforks);
return softforks;
}
} // anonymous namespace
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: bitcoin says anon namespace

}
currentIndex = currentIndex->pprev;
}
} while (blocks_in_period > 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove extra space } while(blocks_in_period > 0);

@kwvg kwvg changed the title backport: bitcoin#23508, #24187, #24528, #24579, #25412 (deployment backports) backport: bitcoin#23508, #24187, #24528, #24579, #25412, #27853 (deployment backports) Mar 11, 2026
@kwvg kwvg requested review from UdjinM6 and knst March 11, 2026 17:14
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟡 Minor

Fix the hanging-indent in the expected getdeploymentinfo fixture.

Flake8 is already flagging Line 198 with E121, so this test file won't pass lint as written. Please reindent the multiline assert_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 | 🟠 Major

Use 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, so nAttempt increments one period too early. For deployments with a falling threshold, statistics.threshold and statistics.possible are 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 with signals too.

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 signals here 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1019d64 and 1ead41a.

📒 Files selected for processing (19)
  • doc/REST-interface.md
  • doc/release-notes-6888.md
  • src/rest.cpp
  • src/rpc/blockchain.cpp
  • src/test/fuzz/rpc.cpp
  • src/test/fuzz/versionbits.cpp
  • src/versionbits.cpp
  • src/versionbits.h
  • test/functional/feature_asset_locks.py
  • test/functional/feature_cltv.py
  • test/functional/feature_dersig.py
  • test/functional/feature_governance.py
  • test/functional/feature_mnehf.py
  • test/functional/feature_new_quorum_type_activation.py
  • test/functional/interface_rest.py
  • test/functional/rpc_blockchain.py
  • test/functional/test_framework/blocktools.py
  • test/functional/test_framework/util.py
  • test/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

Comment on lines +1489 to 1491
if (ThresholdState::LOCKED_IN == current_state) {
bip9.pushKV("activation_height", since_height + static_cast<int>(chainman.GetConsensus().vDeployments[id].nWindowSize));
}
Copy link

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@UdjinM6 thoughts?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)"},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
{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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPC Some notable changes to RPC params/behaviour/descriptions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants