refactor: remove Bitcoin's specific workarounds around BIP30/BIP34#7335
refactor: remove Bitcoin's specific workarounds around BIP30/BIP34#7335knst wants to merge 4 commits into
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bac78340c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| if (fEnforceBIP30) { | ||
| //Only continue to enforce if we're below BIP34 activation height | ||
| if (fEnforceBIP30 || pindex->nHeight <= m_params.GetConsensus().BIP34Height) { |
There was a problem hiding this comment.
fEnforceBIP30 is hardcoded to true and then combined with || in the guard, so the BIP30 duplicate-output scan now runs for every block rather than only around pre-BIP34 history as the comment describes. This reintroduces unnecessary HaveCoin lookups during normal sync/validation and can noticeably degrade IBD performance on production nodes.
Useful? React with 👍 / 👎.
WalkthroughThis PR removes BIP30 duplicate-transaction-ID consensus logic from the Dash codebase. The changes eliminate the sequenceDiagram
participant Block as Block
participant Connect as CChainState::ConnectBlock
participant CoinsDB as UTXO_DB
participant CoinStats as CoinStatsIndex::WriteBlock
participant RPC as gettxoutsetinfo/getblockstats
Block->>Connect: apply block (validation & overwrite checks)
Connect->>CoinsDB: update UTXO set (apply txins/txouts)
Connect->>CoinStats: notify/WriteBlock (index updates per-vout only)
CoinStats->>CoinsDB: read coin changes (compute unspendables)
RPC->>CoinStats: query block_info.unspendables (returns genesis_block only)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/validation.cpp (1)
2373-2382: ⚡ Quick winRemove the dead
fEnforceBIP30branch.Line 2375 hardcodes
fEnforceBIP30totrue, so Line 2382 is unconditional and the BIP34-height clause never affects behavior. The surrounding comments now describe logic the code no longer has, which is easy to misread in a consensus path.Proposed cleanup
- // This prevents exploiting the issue against nodes during their - // initial block download. - bool fEnforceBIP30 = true; - - // Once BIP34 activated it was not possible to create new duplicate coinbases and thus other than starting - // with the 2 existing duplicate coinbase pairs, not possible to create overwriting txs. - // If we're on the known chain at height greater than where BIP34 activated, we can save the db accesses needed for the BIP30 check. - assert(pindex->pprev); - //Only continue to enforce if we're below BIP34 activation height - if (fEnforceBIP30 || pindex->nHeight <= m_params.GetConsensus().BIP34Height) { + // Dash enforces the duplicate-txid overwrite check unconditionally. + assert(pindex->pprev); + { for (const auto& tx : block.vtx) { for (size_t o = 0; o < tx->vout.size(); o++) { if (view.HaveCoin(COutPoint(tx->GetHash(), o))) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/validation.cpp` around lines 2373 - 2382, Remove the dead fEnforceBIP30 branch: delete the hardcoded declaration of fEnforceBIP30 and replace the combined condition if (fEnforceBIP30 || pindex->nHeight <= m_params.GetConsensus().BIP34Height) with a single check that uses only the BIP34 height clause (e.g. if (pindex->nHeight <= m_params.GetConsensus().BIP34Height) { ... }). Also tidy up the surrounding comments that reference the removed flag so the code and comments reflect the actual consensus check; keep existing references to pindex and m_params.GetConsensus().BIP34Height.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/validation.cpp`:
- Around line 2373-2382: Remove the dead fEnforceBIP30 branch: delete the
hardcoded declaration of fEnforceBIP30 and replace the combined condition if
(fEnforceBIP30 || pindex->nHeight <= m_params.GetConsensus().BIP34Height) with a
single check that uses only the BIP34 height clause (e.g. if (pindex->nHeight <=
m_params.GetConsensus().BIP34Height) { ... }). Also tidy up the surrounding
comments that reference the removed flag so the code and comments reflect the
actual consensus check; keep existing references to pindex and
m_params.GetConsensus().BIP34Height.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4c9dee68-e33a-4409-9710-6e55d6188f2e
📒 Files selected for processing (9)
src/chain.hsrc/chainparams.cppsrc/consensus/params.hsrc/index/coinstatsindex.cppsrc/index/coinstatsindex.hsrc/kernel/coinstats.hsrc/rpc/blockchain.cppsrc/validation.cppsrc/validation.h
💤 Files with no reviewable changes (2)
- src/validation.h
- src/index/coinstatsindex.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The branch is not merge-ready at 1bac78340ccba0da66c8c41b0cf14a1ef7e961b1. The gettxoutsetinfo() RPC no longer returns unspendables.bip30, but the functional test suite still asserts exact dictionaries containing that key, and the earlier BIP30 refactor in this branch also removed the post-BIP34 fast path so ConnectBlock() now does duplicate-coin checks on every block indefinitely.
🔴 1 blocking | 🟡 1 suggestion(s)
2 additional finding(s)
blocking: `feature_coinstatsindex` still expects the removed `unspendables.bip30` field
test/functional/feature_coinstatsindex.py (line 131)
This test compares block_info with exact dictionaries that still include 'bip30': 0, but gettxoutsetinfo() no longer emits that key in src/rpc/blockchain.cpp. The first failure is in the genesis-block assertion here, and the same stale expectation is repeated for the block-102, block-108, and block-109 checks later in the file. As written, the functional suite will fail as soon as it exercises the indexed verbose path.
suggestion: The BIP30 duplicate-output scan now stays enabled for every block
src/validation.cpp (line 2375)
fEnforceBIP30 is hard-coded to true, so if (fEnforceBIP30 || pindex->nHeight <= m_params.GetConsensus().BIP34Height) is always true and the view.HaveCoin() scan runs for every output in every block. The surrounding comment still says the check can be skipped once validation is past BIP34 activation, and the parent version of this code used a known-chain guard to do exactly that. The new branch state therefore removes an existing hot-path optimization and adds permanent extra UTXO lookups during IBD and steady-state block validation.
🤖 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.
- [BLOCKING] In `test/functional/feature_coinstatsindex.py`:131-141: `feature_coinstatsindex` still expects the removed `unspendables.bip30` field
This test compares `block_info` with exact dictionaries that still include `'bip30': 0`, but `gettxoutsetinfo()` no longer emits that key in `src/rpc/blockchain.cpp`. The first failure is in the genesis-block assertion here, and the same stale expectation is repeated for the block-102, block-108, and block-109 checks later in the file. As written, the functional suite will fail as soon as it exercises the indexed verbose path.
- [SUGGESTION] In `src/validation.cpp`:2375-2382: The BIP30 duplicate-output scan now stays enabled for every block
`fEnforceBIP30` is hard-coded to `true`, so `if (fEnforceBIP30 || pindex->nHeight <= m_params.GetConsensus().BIP34Height)` is always true and the `view.HaveCoin()` scan runs for every output in every block. The surrounding comment still says the check can be skipped once validation is past BIP34 activation, and the parent version of this code used a known-chain guard to do exactly that. The new branch state therefore removes an existing hot-path optimization and adds permanent extra UTXO lookups during IBD and steady-state block validation.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
Force-pushed twice to fix |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb6b058891
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| if (fEnforceBIP30) { | ||
| //Only continue to enforce if we're below BIP34 activation height | ||
| if (fEnforceBIP30 && pindex->nHeight <= m_params.GetConsensus().BIP34Height) { |
There was a problem hiding this comment.
The new height gate in ConnectBlock disables BIP30 for every block above BIP34Height, which changes consensus behavior on regtest/dev/custom-activation chains and on any non-buried alternative history: duplicate coinbase txids after activation are no longer rejected with bad-txns-BIP30 and can overwrite live UTXOs. This contradicts the intended regtest behavior (see test/functional/feature_block.py, where duplicate coinbases are expected to be rejected even with -testactivationheight=bip34@2) and re-opens CVE-2012-1909-style acceptance in those environments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
duplicate coinbase txids after activation are no longer rejected with bad-txns-BIP30 and can overwrite live UTXOs.
there's no any in Dash Core, see testing section in PR description
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/validation.cpp`:
- Around line 2373-2382: The BIP30 enforcement in validation.cpp currently gates
on pindex->nHeight <= m_params.GetConsensus().BIP34Height which conflicts with
regtest's expectation (CRegTestParams sets consensus.BIP34Height=1 but tests
expect BIP30 always enforced); either make the BIP30 check unconditional when
m_params.NetworkIDString() == "regtest" (i.e., bypass the BIP34Height comparison
for regtest) or update the test/comment to reflect the height-window
behavior—locate the check around pindex->nHeight,
m_params.GetConsensus().BIP34Height in function(s) performing the BIP30 check
and implement the chosen behavior so regtest always enforces BIP30 or tests/docs
match the conditional enforcement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 97d489ff-bbe5-4222-9771-918d368d9288
📒 Files selected for processing (10)
src/chain.hsrc/chainparams.cppsrc/consensus/params.hsrc/index/coinstatsindex.cppsrc/index/coinstatsindex.hsrc/kernel/coinstats.hsrc/rpc/blockchain.cppsrc/validation.cppsrc/validation.htest/functional/feature_coinstatsindex.py
💤 Files with no reviewable changes (3)
- src/validation.h
- src/index/coinstatsindex.cpp
- test/functional/feature_coinstatsindex.py
✅ Files skipped from review due to trivial changes (3)
- src/index/coinstatsindex.h
- src/chain.h
- src/kernel/coinstats.h
| // This prevents exploiting the issue against nodes during their | ||
| // initial block download. | ||
| bool fEnforceBIP30 = !IsBIP30Repeat(*pindex); | ||
| bool fEnforceBIP30 = true; | ||
|
|
||
| // Once BIP34 activated it was not possible to create new duplicate coinbases and thus other than starting | ||
| // with the 2 existing duplicate coinbase pairs, not possible to create overwriting txs. But by the | ||
| // time BIP34 activated, in each of the existing pairs the duplicate coinbase had overwritten the first | ||
| // before the first had been spent. Since those coinbases are sufficiently buried it's no longer possible to create further | ||
| // duplicate transactions descending from the known pairs either. | ||
| // with the 2 existing duplicate coinbase pairs, not possible to create overwriting txs. | ||
| // If we're on the known chain at height greater than where BIP34 activated, we can save the db accesses needed for the BIP30 check. | ||
| assert(pindex->pprev); | ||
| CBlockIndex* pindexBIP34height = pindex->pprev->GetAncestor(m_params.GetConsensus().BIP34Height); | ||
| //Only continue to enforce if we're below BIP34 activation height or the block hash at that height doesn't correspond. | ||
| fEnforceBIP30 = fEnforceBIP30 && (!pindexBIP34height || !(pindexBIP34height->GetBlockHash() == m_params.GetConsensus().BIP34Hash)); | ||
|
|
||
| if (fEnforceBIP30) { | ||
| //Only continue to enforce if we're below BIP34 activation height | ||
| if (fEnforceBIP30 && pindex->nHeight <= m_params.GetConsensus().BIP34Height) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Regtest BIP34Height / chainparams ==="
rg -n -C3 'CRegTestParams|BIP34Height|regtest' src/chainparams.cpp
echo
echo "=== BIP30 functional-test expectation ==="
rg -n -C3 'BIP30 is always checked on regtest|bad-txns-BIP30' test/functional/feature_block.pyRepository: dashpay/dash
Length of output: 10822
Regtest BIP30 enforcement still matches the functional-test expectation
src/validation.cpp gates the duplicate-coinbase (BIP30) check on pindex->nHeight <= m_params.GetConsensus().BIP34Height, but regtest’s CRegTestParams sets consensus.BIP34Height to 1 by default and allows overriding via -testactivationheight=bip34@<height>, which means the functional test’s expectation is only valid for the configured BIP34 activation height window.
Update: test/functional/feature_block.py explicitly expects bad-txns-BIP30 on regtest and documents “BIP30 is always checked on regtest, regardless of the BIP34 activation height”, so this comment should be reconciled with the actual consensus gating on BIP34Height (either by making the code truly unconditional for regtest or by adjusting the test/comment to reflect the height window).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/validation.cpp` around lines 2373 - 2382, The BIP30 enforcement in
validation.cpp currently gates on pindex->nHeight <=
m_params.GetConsensus().BIP34Height which conflicts with regtest's expectation
(CRegTestParams sets consensus.BIP34Height=1 but tests expect BIP30 always
enforced); either make the BIP30 check unconditional when
m_params.NetworkIDString() == "regtest" (i.e., bypass the BIP34Height comparison
for regtest) or update the test/comment to reflect the height-window
behavior—locate the check around pindex->nHeight,
m_params.GetConsensus().BIP34Height in function(s) performing the BIP30 check
and implement the chosen behavior so regtest always enforces BIP30 or tests/docs
match the conditional enforcement.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review of 1ba97e51c95448039dbf6ec173b4004e961cd5fd: the latest delta fixes the prior blocking feature_coinstatsindex failure by removing all stale unspendables.bip30 expectations from the exact dictionaries. The remaining carried-forward issue is the validation hot-path regression: the BIP30 duplicate-output scan is still enabled for every block, so this run has one non-blocking suggestion and no new findings from the latest delta.
Reviewed commit: 1ba97e5
🟡 1 suggestion(s)
Prior findings reconciliation
- FIXED:
feature_coinstatsindexno longer expects the removedunspendables.bip30field. - STILL VALID: The BIP30 duplicate-output scan now stays enabled for every block.
Verified findings
🟡 suggestion: The BIP30 duplicate-output scan now stays enabled for every block
src/validation.cpp lines 2375-2382
This is a carried-forward prior finding that is still valid at 1ba97e51. fEnforceBIP30 is hard-coded to true, and the following condition is if (fEnforceBIP30 || pindex->nHeight <= m_params.GetConsensus().BIP34Height), so the view.HaveCoin() duplicate-output scan runs for every output in every connected block indefinitely. The surrounding comment still says the check can be skipped once validation is past BIP34 activation, and the parent code used that guard to avoid these UTXO lookups on the known chain. Since this PR's premise is that the Bitcoin-specific historical BIP30 exceptions are irrelevant for Dash, keeping permanent duplicate-output scans does not add the intended exception handling; it only adds extra work on the IBD and steady-state validation hot path.
source: codex
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest delta fixes the carried-forward prior-1 issue in src/validation.cpp: the BIP30 duplicate-output scan is no longer executed for every block after BIP34 activation. After re-reading the current validation path and the existing functional coverage, I did not confirm any remaining correctness, Dash-specific integration, or test-coverage issue in this checkout. The only agent finding in this round is a false positive because test/functional/feature_block.py already provides automated BIP30 rejection coverage, so the PR is clean at SHA cb6b05889154f78445fe12745dce833c2f86ae9f.
2 participants
Issue being fixed or feature implemented
Bitcoin Core has workarounds for BIP30/BIP34 historical blocks; this workaround is irrelevant for Dash Core.
What was done?
Removed Bitcoin Core exceptions from validation for Dash Core.
Removed bip30 counter from RPC output
gettxoutsetinfowhich is always 0 for dash core.Though, we can't fully remove checks from validation.cpp same as it is done in PIVX (PIVX-Project#1775) because we had 951 blocks at mainnet and 76 blocks on testnet before BIP34 has been activated and this check is still used for validation of old blocks.
How Has This Been Tested?
Run
scan-pre-bip34-coinbases.pyto find a blocks that violates bip34 safety measures before bip34 is activated for both main and testnet.Extra double check is call of
gettxoutsetinfo noneto be sure thatbip30is 0 for main and testnets:Breaking Changes
Removed rpc field
bip30from RPCgettxoutsetinfoChecklist: