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 👍 / 👎.
|
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 (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe change removes BIP30-specific duplicate-transaction handling from block connection and disconnection, deletes related helper functions, and updates coin statistics accounting. RPC responses now report Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
knst
force-pushed
the
bip-30-cleanup
branch
2 times, most recently
from
1ba97e5 to
cb6b058
Compare
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.
|
✅ Review complete (commit 7b3ac94) |
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean, well-scoped removal of Bitcoin-mainnet-specific BIP30 duplicate-coinbase workarounds that never applied to Dash, while correctly preserving the pre-BIP34 BIP30 duplicate-transaction check still needed for Dash's own 951/76 pre-activation blocks. No consensus, behavioral, or build defects found — IsBIP30Repeat/IsBIP30Unspendable are fully dead after removal and the coinstatsindex on-disk format stays compatible. Remaining issues are all documentation drift left behind by the cleanup and permanent-history hygiene on the 4-commit stack (a no-op add/delete of a temporary script, and two commits with empty bodies on consensus/RPC-affecting changes).
Source: reviewers — Sol gpt-5.6-sol (general, dash-core-commit-history); Sonnet claude-sonnet-5 (general, dash-core-commit-history). Verifier — claude-sonnet-5.
Commit-history nitpick: commit 927c607 uses a feat: prefix for a breaking RPC-field removal; rewording it would make the permanent history clearer.
🟡 2 suggestion(s) | 💬 4 nitpick(s)
3 additional finding(s) omitted (not in diff).
🤖 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 `contrib/devtools/scan-pre-bip34-coinbases.py`:
- [SUGGESTION] contrib/devtools/scan-pre-bip34-coinbases.py:1: Temporary verification script is added and then deleted unchanged within the same PR
Commit f753b26e78c adds this 199-line script; commit 7b3ac942708 (HEAD) deletes it with byte-identical content (verified: diff between the added and pre-deletion versions is empty). Its net contribution to the merged tree is zero, but since Dash Core merges without squashing, both commits become permanent, non-bisectable noise in `git log`/`git blame` on `develop` — anyone auditing this file's history sees a script appear and vanish for no lasting reason. The verification methodology and results are already captured in the PR description, so the commits add no unique value once merged. Before merge, either (a) drop both commits from the stack and rely on the PR description as the durable record, or (b) if this script has ongoing value for future pre-BIP34 audits, keep it permanently in contrib/devtools/ (with an entry added to test/util/data/non-backported.txt) instead of deleting it in the same PR.
In `<commit:826640590c8>`:
- [SUGGESTION] <commit:826640590c8>:1: Consensus-critical commits carry no body explaining why the removal is safe for Dash
Commits 826640590c8 (removes ConnectBlock/DisconnectBlock's hardcoded BIP30 exceptions plus IsBIP30Repeat/IsBIP30Unspendable, touching src/validation.cpp, src/chain.h, src/validation.h) and 927c607e419 (removes the gettxoutsetinfo `bip30` field, which the PR description itself calls a breaking RPC change) both have bare one-line subjects with empty bodies. The rationale that makes 826640590c8 safe — Dash's pre-BIP34 window is only 951 mainnet / 76 testnet blocks, and the scan script confirmed no duplicate-coinbase collision exists in that window — currently lives only in the PR description, which isn't attached to `git log -p src/validation.cpp` or `git blame` on this consensus-critical file. Add a short body to each commit summarizing why the change is safe so the reasoning travels with the commit permanently.
| BLOCK_VALID_TRANSACTIONS = 3, | ||
|
|
||
| //! Outputs do not overspend inputs, no double spends, coinbase output ok, no immature coinbase spends, BIP30. | ||
| //! Outputs do not overspend inputs, no double spends, coinbase output ok, no immature coinbase spends. |
There was a problem hiding this comment.
💬 Nitpick: BLOCK_VALID_CHAIN doc comment now omits BIP30, but the check is still performed
This PR (8266405) changed this line from '...no immature coinbase spends, BIP30.' to '...no immature coinbase spends.', dropping the BIP30 mention. But ConnectBlock (src/validation.cpp:2246-2254) still runs the BIP30 duplicate-output check whenever fEnforceBIP30 is true — i.e. for regtest, any chain without the configured BIP34 anchor, and all pre-BIP34 blocks on Dash's own chain (951 mainnet / 76 testnet). Only the Bitcoin-specific hardcoded exceptions for two historical duplicate-coinbase pairs were removed, not BIP30 validation itself. This comment now inaccurately implies BLOCK_VALID_CHAIN no longer covers BIP30, which will mislead anyone using this enum's documentation to reason about what validity level a block has reached.
| //! Outputs do not overspend inputs, no double spends, coinbase output ok, no immature coinbase spends. | |
| //! Outputs do not overspend inputs, no double spends, coinbase output ok, no immature coinbase spends, BIP30. |
source: ['codex-general']
| // 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. |
There was a problem hiding this comment.
💬 Nitpick: Stale reference to '2 existing duplicate coinbase pairs' after removing the code that tracked them
This same commit (8266405) deletes IsBIP30Repeat/IsBIP30Unspendable and the hardcoded height/hash checks for Bitcoin's two known duplicate-coinbase pairs (91722/91812 and 91842/91880) — precisely because the PR's own scan (documented in the PR description) confirms Dash's chain never produced any such collision. Yet this untouched comment still narrates '...other than starting with the 2 existing duplicate coinbase pairs, not possible to create overwriting txs,' which describes Bitcoin mainnet history that doesn't apply to Dash and now has no corresponding code nearby to anchor it. Since the PR's stated goal is specifically to strip Bitcoin-specific BIP30/34 workarounds, this leftover text undercuts that goal by continuing to imply Dash inherited those exact two pairs.
| // 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. | |
| // Dash has no historical duplicate coinbase pairs. Once BIP34 activated, coinbase height | |
| // commitments prevent new duplicate coinbases on the anchored chain. |
source: ['codex-general']
| utxo_size_inc += out_size; | ||
|
|
||
| // The Genesis block and the repeated BIP30 block coinbases don't change the UTXO | ||
| // The Genesis block coinbase don't change the UTXO |
There was a problem hiding this comment.
💬 Nitpick: Grammar error in comment rewritten by this PR
Commit 927c607 rewrote this line from 'The Genesis block and the repeated BIP30 block coinbases don't change...' to 'The Genesis block coinbase don't change...'. The subject was changed from plural ('coinbases') to singular ('coinbase') but the verb was left as the plural contraction 'don't', producing a subject/verb mismatch. Since the line was already touched in this PR, worth a one-word fix.
| // The Genesis block coinbase don't change the UTXO | |
| // The Genesis block coinbase doesn't change the UTXO |
source: ['sonnet5-general']
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: