feat: remove SPORK_3_INSTANTSEND_BLOCK_FILTERING and SPORK_9_SUPERBLOCKS_ENABLED#7278
feat: remove SPORK_3_INSTANTSEND_BLOCK_FILTERING and SPORK_9_SUPERBLOCKS_ENABLED#7278UdjinM6 wants to merge 5 commits intodashpay:developfrom
Conversation
…OCKS_ENABLED Both sporks have been effectively permanent fixtures of the network and their gating logic is no longer needed. Drop the enum entries, spork definitions, the AreSuperblocksEnabled() helper, and all associated branches in governance, InstantSend, masternode payments and mining RPC. getblocktemplate now always reports superblocks_enabled as true. Functional tests are updated to stop toggling the removed sporks; feature_multikeysporks.py now uses SPORK_19_CHAINLOCKS_ENABLED as its second spork under test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
✅ Review complete (commit 18b990b) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR removes SPORK_3_INSTANTSEND_BLOCK_FILTERING and SPORK_9_SUPERBLOCKS_ENABLED and makes the behaviors they gated always active. It deletes the spork enum entries and definitions, removes spork-based guards and the AreSuperblocksEnabled helper, updates CMNPaymentsProcessor/CChainstateHelper and related constructors to drop CSporkManager, adjusts getblocktemplate to always report superblocks_enabled = true and enforce a synced-node check at superblock heights, updates tests and node setup, and adds release notes documenting the change. Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC as getblocktemplate RPC
participant Node
participant Gov as Governance / MNPayments
participant Chain as ActiveChain
Client->>RPC: request getblocktemplate
RPC->>Node: evaluate request
Node->>Chain: query next block height
Node->>Gov: IsSuperblockTriggered?(next_height)
Gov-->>Node: triggered? and IsValidSuperblock?
alt next block is superblock height and Node not MN-synced
Node-->>RPC: error RPC_CLIENT_IN_INITIAL_DOWNLOAD
RPC-->>Client: return error
else
Node-->>RPC: build template (superblocks_enabled: true)
RPC-->>Client: return template
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean spork removal with correct logic simplification. The main substantive issue is a behavioral regression in getblocktemplate on regtest: SPORK_9's default was OFF, so removing the gate makes the superblock sync check unconditional, breaking regtest mining at superblock heights. Additionally, several dead references to CSporkManager and spork.h remain and should be cleaned up.
Reviewed commit: ee90879
🟡 2 suggestion(s) | 💬 5 nitpick(s)
4 additional findings
🟡 suggestion: Dead member: m_sporkman is no longer used in CMNPaymentsProcessor
src/masternode/payments.h (lines 20-56)
All AreSuperblocksEnabled(m_sporkman) calls were removed from payments.cpp, but four artifacts remain: the forward declaration class CSporkManager; (line 20), the member const CSporkManager& m_sporkman (line 41), the constructor parameter (line 54), and the caller in chainhelper.cpp:27 that passes sporkman. Grep confirms zero references to m_sporkman or sporkman in payments.cpp. This dead code should be removed as part of this cleanup PR.
💬 nitpick: Unused #include after AreSuperblocksEnabled removal
src/governance/governance.cpp (line 14)
The #include <spork.h> was needed for AreSuperblocksEnabled which called CSporkManager::IsSporkActive. Grep confirms no remaining references to IsSporkActive, SporkId, or any SPORK_ constant in governance.cpp. This include is dead.
💬 nitpick: Dead forward declaration: class CSporkManager
src/governance/governance.h (line 43)
The class CSporkManager; forward declaration was only needed for the removed AreSuperblocksEnabled(const CSporkManager&) function declaration. Grep confirms no other reference to CSporkManager in this header.
💬 nitpick: Unused #include
src/rpc/mining.cpp (line 46)
The only governance symbol used in this file after the removal is CSuperblock::IsValidBlockHeight, which is declared in governance/classes.h (already included at line 45). No CGovernanceManager, CGovernanceObject, or other governance.h-specific symbols remain. This include can likely be removed, though compile-time verification is recommended since transitive dependencies may exist.
🤖 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/mining.cpp`:
- [SUGGESTION] lines 734-737: Unconditional superblock sync check changes regtest getblocktemplate behavior
SPORK_9's default value was `4070908800` (OFF). On regtest, `AreSuperblocksEnabled()` previously returned false unless the spork was explicitly activated, so this sync check was skipped. Now it runs unconditionally. On regtest, superblocks start at height 1500 with a 20-block cycle, and `CMasternodeSync::IsSynced()` is false unless the full sync state machine completes. This means `getblocktemplate` will start returning `RPC_CLIENT_IN_INITIAL_DOWNLOAD` every 20 blocks for plain regtest nodes that don't run masternode sync — a silent behavioral regression. Note: `generate()` (used by `BitcoinTestFramework`) is unaffected since it calls `BlockAssembler` directly, so functional tests won't catch this.
In `src/masternode/payments.h`:
- [SUGGESTION] lines 20-56: Dead member: m_sporkman is no longer used in CMNPaymentsProcessor
All `AreSuperblocksEnabled(m_sporkman)` calls were removed from `payments.cpp`, but four artifacts remain: the forward declaration `class CSporkManager;` (line 20), the member `const CSporkManager& m_sporkman` (line 41), the constructor parameter (line 54), and the caller in `chainhelper.cpp:27` that passes `sporkman`. Grep confirms zero references to `m_sporkman` or `sporkman` in `payments.cpp`. This dead code should be removed as part of this cleanup PR.
src/rpc/mining.cpp
Outdated
| if (!node.mn_sync->IsSynced() && CSuperblock::IsValidBlockHeight(active_chain.Height() + 1)) { | ||
| // Next block is a superblock but we need governance info to correctly construct it. | ||
| throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, PACKAGE_NAME " is syncing with network..."); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Unconditional superblock sync check changes regtest getblocktemplate behavior
SPORK_9's default value was 4070908800 (OFF). On regtest, AreSuperblocksEnabled() previously returned false unless the spork was explicitly activated, so this sync check was skipped. Now it runs unconditionally. On regtest, superblocks start at height 1500 with a 20-block cycle, and CMasternodeSync::IsSynced() is false unless the full sync state machine completes. This means getblocktemplate will start returning RPC_CLIENT_IN_INITIAL_DOWNLOAD every 20 blocks for plain regtest nodes that don't run masternode sync — a silent behavioral regression. Note: generate() (used by BitcoinTestFramework) is unaffected since it calls BlockAssembler directly, so functional tests won't catch this.
💡 Suggested change
| if (!node.mn_sync->IsSynced() && CSuperblock::IsValidBlockHeight(active_chain.Height() + 1)) { | |
| // Next block is a superblock but we need governance info to correctly construct it. | |
| throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, PACKAGE_NAME " is syncing with network..."); | |
| } | |
| if (!Params().MineBlocksOnDemand() | |
| && !node.mn_sync->IsSynced() | |
| && CSuperblock::IsValidBlockHeight(active_chain.Height() + 1)) { | |
| // Next block is a superblock but we need governance info to correctly construct it. | |
| throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, PACKAGE_NAME " is syncing with network..."); | |
| } |
source: ['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/mining.cpp`:
- [SUGGESTION] lines 734-737: Unconditional superblock sync check changes regtest getblocktemplate behavior
SPORK_9's default value was `4070908800` (OFF). On regtest, `AreSuperblocksEnabled()` previously returned false unless the spork was explicitly activated, so this sync check was skipped. Now it runs unconditionally. On regtest, superblocks start at height 1500 with a 20-block cycle, and `CMasternodeSync::IsSynced()` is false unless the full sync state machine completes. This means `getblocktemplate` will start returning `RPC_CLIENT_IN_INITIAL_DOWNLOAD` every 20 blocks for plain regtest nodes that don't run masternode sync — a silent behavioral regression. Note: `generate()` (used by `BitcoinTestFramework`) is unaffected since it calls `BlockAssembler` directly, so functional tests won't catch this.
…ter sporks removal
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/rpc/mining.cpp (1)
732-735:⚠️ Potential issue | 🟡 MinorRegtest behavioral change:
getblocktemplatemay fail during superblock heights.The previous
AreSuperblocksEnabled(*node.sporkman)check returnedfalseon regtest (SPORK_9 default was OFF). Now the sync check runs unconditionally, which will throwRPC_CLIENT_IN_INITIAL_DOWNLOADon regtest nodes that haven't completed masternode sync whenCSuperblock::IsValidBlockHeight(active_chain.Height() + 1)is true.Consider adding a regtest bypass:
💡 Suggested fix
- if (!node.mn_sync->IsSynced() && CSuperblock::IsValidBlockHeight(active_chain.Height() + 1)) { + if (!Params().MineBlocksOnDemand() + && !node.mn_sync->IsSynced() + && CSuperblock::IsValidBlockHeight(active_chain.Height() + 1)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/mining.cpp` around lines 732 - 735, The current check in getblocktemplate throws RPC_CLIENT_IN_INITIAL_DOWNLOAD when CSuperblock::IsValidBlockHeight(active_chain.Height() + 1) is true even on regtest, because AreSuperblocksEnabled(*node.sporkman) was removed; update the condition around node.mn_sync->IsSynced()/CSuperblock::IsValidBlockHeight to skip the superblock sync requirement on regtest (or reintroduce the AreSuperblocksEnabled(*node.sporkman) guard) — e.g., ensure the throw only happens when superblocks are enabled and not on regtest by checking AreSuperblocksEnabled(*node.sporkman) and/or Params().IsRegTest() before throwing in the getblocktemplate/mine-related logic so regtest nodes don't hit RPC_CLIENT_IN_INITIAL_DOWNLOAD during superblock heights.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/rpc/mining.cpp`:
- Around line 732-735: The current check in getblocktemplate throws
RPC_CLIENT_IN_INITIAL_DOWNLOAD when
CSuperblock::IsValidBlockHeight(active_chain.Height() + 1) is true even on
regtest, because AreSuperblocksEnabled(*node.sporkman) was removed; update the
condition around node.mn_sync->IsSynced()/CSuperblock::IsValidBlockHeight to
skip the superblock sync requirement on regtest (or reintroduce the
AreSuperblocksEnabled(*node.sporkman) guard) — e.g., ensure the throw only
happens when superblocks are enabled and not on regtest by checking
AreSuperblocksEnabled(*node.sporkman) and/or Params().IsRegTest() before
throwing in the getblocktemplate/mine-related logic so regtest nodes don't hit
RPC_CLIENT_IN_INITIAL_DOWNLOAD during superblock heights.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 23424dbd-a3fe-4b3c-aefb-dc58125e2c00
📒 Files selected for processing (8)
src/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/governance/governance.cppsrc/governance/governance.hsrc/masternode/payments.hsrc/node/chainstate.cppsrc/rpc/mining.cpptest/functional/feature_governance_cl.py
💤 Files with no reviewable changes (2)
- test/functional/feature_governance_cl.py
- src/governance/governance.h
- Update release notes to clarify that SPORK_3 and SPORK_9 were already effectively always enabled on mainnet but continued to gate behavior on test networks. - Add explicit wait_until(IsSynced) in feature_dip3_deterministicmns before the getblocktemplate loop, since -budgetparams=10:10:10 sets superblock cycle to 10 blocks and getblocktemplate now unconditionally checks IsSynced() at superblock heights. - Fix stale comment in DashTestFramework that still referenced "block filtering" after SPORK_3 removal. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean removal of permanently-active sporks SPORK_3 and SPORK_9 with correct cleanup. The only substantive issue is that removing the AreSuperblocksEnabled() guard in getblocktemplate introduces a silent behavioral regression on test chains, where the superblock sync check now fires unconditionally. One stale comment references the removed block filtering spork.
Reviewed commit: 3d9e80e
🟡 1 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/mining.cpp`:
- [SUGGESTION] lines 732-735: Unconditional superblock sync check changes `getblocktemplate` behavior on test chains
Previously, this check was guarded by `AreSuperblocksEnabled(*node.sporkman)`, which checked SPORK_9. On mainnet, SPORK_9 was hardened to active (value `0`), so the check always ran — no behavioral change there. But on regtest/testnet/devnet, SPORK_9 defaulted to `4070908800` (OFF), so this sync check **never executed**. Now it runs unconditionally.
On regtest (`nSuperblockCycle = 20`), any node that hasn't completed masternode sync will get `RPC_CLIENT_IN_INITIAL_DOWNLOAD` from `getblocktemplate` every 20 blocks. The fix in `feature_dip3_deterministicmns.py` (adding `wait_until(... IsSynced)`) confirms the author is aware, but this patches one test caller rather than addressing the API-level change.
The `generate()` RPC bypasses `getblocktemplate` (calls `BlockAssembler` directly), so most functional tests won't surface this, but external tooling or custom test setups using `getblocktemplate` on regtest/devnet will silently break. Consider either:
- Guarding with `!Params().IsTestChain()` to restore previous test-chain behavior, or
- Documenting this as an intentional behavioral change in the release notes
src/rpc/mining.cpp
Outdated
| if (!node.mn_sync->IsSynced() && CSuperblock::IsValidBlockHeight(active_chain.Height() + 1)) { | ||
| // Next block is a superblock but we need governance info to correctly construct it. | ||
| throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, PACKAGE_NAME " is syncing with network..."); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Unconditional superblock sync check changes getblocktemplate behavior on test chains
Previously, this check was guarded by AreSuperblocksEnabled(*node.sporkman), which checked SPORK_9. On mainnet, SPORK_9 was hardened to active (value 0), so the check always ran — no behavioral change there. But on regtest/testnet/devnet, SPORK_9 defaulted to 4070908800 (OFF), so this sync check never executed. Now it runs unconditionally.
On regtest (nSuperblockCycle = 20), any node that hasn't completed masternode sync will get RPC_CLIENT_IN_INITIAL_DOWNLOAD from getblocktemplate every 20 blocks. The fix in feature_dip3_deterministicmns.py (adding wait_until(... IsSynced)) confirms the author is aware, but this patches one test caller rather than addressing the API-level change.
The generate() RPC bypasses getblocktemplate (calls BlockAssembler directly), so most functional tests won't surface this, but external tooling or custom test setups using getblocktemplate on regtest/devnet will silently break. Consider either:
- Guarding with
!Params().IsTestChain()to restore previous test-chain behavior, or - Documenting this as an intentional behavioral change in the release notes
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/mining.cpp`:
- [SUGGESTION] lines 732-735: Unconditional superblock sync check changes `getblocktemplate` behavior on test chains
Previously, this check was guarded by `AreSuperblocksEnabled(*node.sporkman)`, which checked SPORK_9. On mainnet, SPORK_9 was hardened to active (value `0`), so the check always ran — no behavioral change there. But on regtest/testnet/devnet, SPORK_9 defaulted to `4070908800` (OFF), so this sync check **never executed**. Now it runs unconditionally.
On regtest (`nSuperblockCycle = 20`), any node that hasn't completed masternode sync will get `RPC_CLIENT_IN_INITIAL_DOWNLOAD` from `getblocktemplate` every 20 blocks. The fix in `feature_dip3_deterministicmns.py` (adding `wait_until(... IsSynced)`) confirms the author is aware, but this patches one test caller rather than addressing the API-level change.
The `generate()` RPC bypasses `getblocktemplate` (calls `BlockAssembler` directly), so most functional tests won't surface this, but external tooling or custom test setups using `getblocktemplate` on regtest/devnet will silently break. Consider either:
- Guarding with `!Params().IsTestChain()` to restore previous test-chain behavior, or
- Documenting this as an intentional behavioral change in the release notes
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean, well-executed removal of SPORK_3 and SPORK_9. All references are properly cleaned up. Both agents correctly identified that unconditional superblock enablement changes behavior on test chains — Claude identified the getblocktemplate behavioral shift and documentation gap, while Codex identified that generate* RPCs lack the equivalent sync guard. All three findings are valid but low-impact, affecting only test-chain edge cases.
Reviewed commit: b2cde07
🟡 2 suggestion(s) | 💬 1 nitpick(s)
1 additional finding
🟡 suggestion: `generate*` RPCs lack the superblock sync guard added to `getblocktemplate`
src/rpc/mining.cpp (lines 152-160)
The PR adds a sync guard in getblocktemplate() (line 732) that refuses template creation at superblock heights until masternode sync completes. But generateBlocks() calls BlockAssembler::CreateNewBlock() directly with no equivalent check.
When an unsynced node mines at a superblock height via generatetoaddress/generatetodescriptor:
FillBlockPayments(payments.cpp:329) finds no governance trigger (no data) → builds block WITHOUT superblock payments- Local validation passes because
IsBlockValueValid(payments.cpp:235) andIsBlockPayeeValid(payments.cpp:283) both early-returntruewhen!m_mn_sync.IsSynced() - Synced peers with a valid governance trigger will reject this block
generateblock also lacks the guard — while it calls TestBlockValidity, that runs the same validation which is lenient for unsynced nodes.
Before this PR, SPORK_9 being OFF on test chains meant superblocks were disabled entirely, so this divergence was unreachable. Now it's possible.
Practical impact is limited: this requires test chain + unsynced node + superblock height + existing governance trigger + generate* usage. But if a guard is warranted for getblocktemplate, the same logic applies here. Note that a proper fix needs the check inside the while loop (since height changes each iteration), not before it.
🤖 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/mining.cpp`:
- [SUGGESTION] lines 732-735: Unconditional superblock sync check changes `getblocktemplate` behavior on test chains
Before this PR, the sync check at superblock heights was gated by `AreSuperblocksEnabled()` (SPORK_9), which was OFF on test chains — so this check never executed on regtest/testnet/devnet. Now it runs unconditionally.
On regtest (`nSuperblockCycle = 20`), any node that hasn't completed masternode sync will get `RPC_CLIENT_IN_INITIAL_DOWNLOAD` every 20 blocks. The test fix in `feature_dip3_deterministicmns.py` (adding `wait_until(... IsSynced)`) confirms this is a real behavioral change.
Notably, the IBD check at lines 722–730 is already guarded by `!Params().IsTestChain()`, establishing a pattern of leniency on test chains that this new check doesn't follow. If this is fully intentional (as the release notes suggest), no code change is needed — but external test harnesses or mining tooling using `getblocktemplate` on regtest/devnet may silently break.
- [SUGGESTION] lines 152-160: `generate*` RPCs lack the superblock sync guard added to `getblocktemplate`
The PR adds a sync guard in `getblocktemplate()` (line 732) that refuses template creation at superblock heights until masternode sync completes. But `generateBlocks()` calls `BlockAssembler::CreateNewBlock()` directly with no equivalent check.
When an unsynced node mines at a superblock height via `generatetoaddress`/`generatetodescriptor`:
1. `FillBlockPayments` (payments.cpp:329) finds no governance trigger (no data) → builds block WITHOUT superblock payments
2. Local validation passes because `IsBlockValueValid` (payments.cpp:235) and `IsBlockPayeeValid` (payments.cpp:283) both early-return `true` when `!m_mn_sync.IsSynced()`
3. Synced peers with a valid governance trigger will reject this block
`generateblock` also lacks the guard — while it calls `TestBlockValidity`, that runs the same validation which is lenient for unsynced nodes.
Before this PR, SPORK_9 being OFF on test chains meant superblocks were disabled entirely, so this divergence was unreachable. Now it's possible.
Practical impact is limited: this requires test chain + unsynced node + superblock height + existing governance trigger + `generate*` usage. But if a guard is warranted for `getblocktemplate`, the same logic applies here. Note that a proper fix needs the check **inside** the `while` loop (since height changes each iteration), not before it.
src/rpc/mining.cpp
Outdated
| if (!node.mn_sync->IsSynced() && CSuperblock::IsValidBlockHeight(active_chain.Height() + 1)) { | ||
| // Next block is a superblock but we need governance info to correctly construct it. | ||
| throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, PACKAGE_NAME " is syncing with network..."); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Unconditional superblock sync check changes getblocktemplate behavior on test chains
Before this PR, the sync check at superblock heights was gated by AreSuperblocksEnabled() (SPORK_9), which was OFF on test chains — so this check never executed on regtest/testnet/devnet. Now it runs unconditionally.
On regtest (nSuperblockCycle = 20), any node that hasn't completed masternode sync will get RPC_CLIENT_IN_INITIAL_DOWNLOAD every 20 blocks. The test fix in feature_dip3_deterministicmns.py (adding wait_until(... IsSynced)) confirms this is a real behavioral change.
Notably, the IBD check at lines 722–730 is already guarded by !Params().IsTestChain(), establishing a pattern of leniency on test chains that this new check doesn't follow. If this is fully intentional (as the release notes suggest), no code change is needed — but external test harnesses or mining tooling using getblocktemplate on regtest/devnet may silently break.
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/mining.cpp`:
- [SUGGESTION] lines 732-735: Unconditional superblock sync check changes `getblocktemplate` behavior on test chains
Before this PR, the sync check at superblock heights was gated by `AreSuperblocksEnabled()` (SPORK_9), which was OFF on test chains — so this check never executed on regtest/testnet/devnet. Now it runs unconditionally.
On regtest (`nSuperblockCycle = 20`), any node that hasn't completed masternode sync will get `RPC_CLIENT_IN_INITIAL_DOWNLOAD` every 20 blocks. The test fix in `feature_dip3_deterministicmns.py` (adding `wait_until(... IsSynced)`) confirms this is a real behavioral change.
Notably, the IBD check at lines 722–730 is already guarded by `!Params().IsTestChain()`, establishing a pattern of leniency on test chains that this new check doesn't follow. If this is fully intentional (as the release notes suggest), no code change is needed — but external test harnesses or mining tooling using `getblocktemplate` on regtest/devnet may silently break.
| ------------ | ||
|
|
||
| * `getblocktemplate` now always reports `superblocks_enabled` as `true`. The | ||
| field is retained for backwards compatibility. |
There was a problem hiding this comment.
💬 Nitpick: Release notes should mention getblocktemplate sync requirement on test chains
The release notes document that getblocktemplate now always reports superblocks_enabled as true, and that the removed functionality is "permanently enabled across all networks." However, the practical consequence — that getblocktemplate now throws RPC_CLIENT_IN_INITIAL_DOWNLOAD at superblock heights on regtest/testnet/devnet when the node hasn't completed masternode sync — is not called out. This is a subtle but potentially breaking change for test harnesses.
💡 Suggested change
| ------------ | |
| * `getblocktemplate` now always reports `superblocks_enabled` as `true`. The | |
| field is retained for backwards compatibility. | |
| * `getblocktemplate` now always reports `superblocks_enabled` as `true`. The | |
| field is retained for backwards compatibility. On test networks (regtest, | |
| testnet, devnet), `getblocktemplate` now requires masternode sync to be | |
| complete at superblock heights; previously this check was gated by the | |
| now-removed `SPORK_9`. |
source: ['claude']
Move the superblock sync check in getblocktemplate inside the existing !IsTestChain() block, matching the pattern used by the IBD check. Previously this was gated by SPORK_9 which was off on test networks; after spork removal the check would run unconditionally, requiring full masternode sync on test networks at superblock heights. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
One release-note omission remains in the spork-removal stack: it documents the spork and getblocktemplate RPC changes, but not the matching sporkupdate compatibility change for the removed spork names.
Reviewed commit: 18b990b
🟡 1 suggestion(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 `doc/release-notes-7278.md`:
- [SUGGESTION] lines 4-19: Release notes still omit that `sporkupdate` now rejects the removed spork names
This note covers the removed sporks disappearing from `spork` output, but it still misses the other user-visible RPC change from dropping these entries out of `sporkDefs`: `sporkupdate` now resolves `SPORK_3_INSTANTSEND_BLOCK_FILTERING` and `SPORK_9_SUPERBLOCKS_ENABLED` to `SPORK_INVALID` and returns `RPC_INVALID_PARAMETER` (`src/rpc/node.cpp:218-220`). That is a compatibility change for any automation that still tries to toggle those sporks, so it should be called out alongside the existing RPC notes.
| * `SPORK_3_INSTANTSEND_BLOCK_FILTERING` and `SPORK_9_SUPERBLOCKS_ENABLED` have | ||
| been removed. These sporks were already effectively always enabled on mainnet | ||
| but continued to gate behavior on test networks (testnet, regtest, and devnet). | ||
| The associated functionality (InstantSend conflicting-block rejection and | ||
| superblock payments) is now permanently enabled across all networks, and the | ||
| sporks will no longer appear in the `spork` RPC output. | ||
|
|
||
| Updated RPCs | ||
| ------------ | ||
|
|
||
| * `getblocktemplate` now always reports `superblocks_enabled` as `true`. The | ||
| field is retained for backwards compatibility. | ||
|
|
||
| * `getblocktemplate` now requires a fully synced node at superblock heights. | ||
| This check is skipped on test networks. Previously it was gated by | ||
| `SPORK_9_SUPERBLOCKS_ENABLED` which was already always active on mainnet. |
There was a problem hiding this comment.
🟡 Suggestion: Release notes still omit that sporkupdate now rejects the removed spork names
This note covers the removed sporks disappearing from spork output, but it still misses the other user-visible RPC change from dropping these entries out of sporkDefs: sporkupdate now resolves SPORK_3_INSTANTSEND_BLOCK_FILTERING and SPORK_9_SUPERBLOCKS_ENABLED to SPORK_INVALID and returns RPC_INVALID_PARAMETER (src/rpc/node.cpp:218-220). That is a compatibility change for any automation that still tries to toggle those sporks, so it should be called out alongside the existing RPC notes.
source: ['codex-general']
🤖 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 `doc/release-notes-7278.md`:
- [SUGGESTION] lines 4-19: Release notes still omit that `sporkupdate` now rejects the removed spork names
This note covers the removed sporks disappearing from `spork` output, but it still misses the other user-visible RPC change from dropping these entries out of `sporkDefs`: `sporkupdate` now resolves `SPORK_3_INSTANTSEND_BLOCK_FILTERING` and `SPORK_9_SUPERBLOCKS_ENABLED` to `SPORK_INVALID` and returns `RPC_INVALID_PARAMETER` (`src/rpc/node.cpp:218-220`). That is a compatibility change for any automation that still tries to toggle those sporks, so it should be called out alongside the existing RPC notes.
3 participants
Issue being fixed or feature implemented
Both sporks have been effectively permanent fixtures of the network and their gating logic is no longer needed.
What was done?
Drop the enum entries, spork definitions, the
AreSuperblocksEnabled()helper, and all associated branches in governance, InstantSend, masternode payments and mining RPC.getblocktemplatenow always reportssuperblocks_enabledastrue.Functional tests are updated to stop toggling the removed sporks;
feature_multikeysporks.pynow usesSPORK_19_CHAINLOCKS_ENABLEDas its second spork under test.How Has This Been Tested?
Run tests
Breaking Changes
n/a
Checklist: