Skip to content

refactor: remove Bitcoin's specific workarounds around BIP30/BIP34 #7335

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
knst wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

refactor: remove Bitcoin's specific workarounds around BIP30/BIP34 #7335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bd685b7
test: script to scan pre-bip34 coinbases to find potential issues for…
knst May 25, 2026
88aecec
refactor: remove Bitcoin's specific workarounds around BIP30/BIP34
knst May 23, 2026
cddae75
test: remove unused scan-pre-bip34-coinbases.py once fix is merged
knst May 25, 2026
cb6b058
feat: remove bip30 field from coinstat index
knst May 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/chain.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ enum BlockStatus : uint32_t {
*/
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.
//! Implies all parents are also at least CHAIN.
BLOCK_VALID_CHAIN = 4,

Expand Down
7 changes: 2 additions & 5 deletions src/chainparams.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,7 @@ class CMainParams : public CChainParams {
consensus.nGovernanceMinQuorum = 10;
consensus.nGovernanceFilterElements = 20000;
consensus.nMasternodeMinimumConfirmations = 15;
consensus.BIP34Height = 951;
consensus.BIP34Hash = uint256S("0x000001f35e70f7c5705f64c6c5cc3dea9449e74d5b5c7cf74dad1bcca14a8012");
consensus.BIP34Height = 951; // 000001f35e70f7c5705f64c6c5cc3dea9449e74d5b5c7cf74dad1bcca14a8012
consensus.BIP65Height = 619382; // 00000000000076d8fcea02ec0963de4abfd01e771fec0863f960c2c64fe6f357
consensus.BIP66Height = 245817; // 00000000000b1fa2dfa312863570e13fae9ca7b5566cb27e55422620b469aefa
consensus.BIP147Height = 939456; // 00000000000000117befca4fab5622514772f608852e5edd8df9c55464b6fe37
Expand Down Expand Up @@ -375,8 +374,7 @@ class CTestNetParams : public CChainParams {
consensus.nGovernanceMinQuorum = 1;
consensus.nGovernanceFilterElements = 500;
consensus.nMasternodeMinimumConfirmations = 1;
consensus.BIP34Height = 76;
consensus.BIP34Hash = uint256S("0x000008ebb1db2598e897d17275285767717c6acfeac4c73def49fbea1ddcbcb6");
consensus.BIP34Height = 76; // 000008ebb1db2598e897d17275285767717c6acfeac4c73def49fbea1ddcbcb6
consensus.BIP65Height = 2431; // 0000039cf01242c7f921dcb4806a5994bc003b48c1973ae0c89b67809c2bb2ab
consensus.BIP66Height = 2075; // 0000002acdd29a14583540cb72e1c5cc83783560e38fa7081495d474fe1671f7
consensus.BIP147Height = 4300; // 0000000040c1480d413c9203664253ab18da284130c329bf88fcfc84312bcbe0
Expand Down Expand Up @@ -786,7 +784,6 @@ class CRegTestParams : public CChainParams {
consensus.nGovernanceFilterElements = 100;
consensus.nMasternodeMinimumConfirmations = 1;
consensus.BIP34Height = 1; // Always active unless overridden
consensus.BIP34Hash = uint256();
consensus.BIP65Height = 1; // Always active unless overridden
consensus.BIP66Height = 1; // Always active unless overridden
consensus.BIP147Height = 0; // Always active unless overridden
Expand Down
3 changes: 1 addition & 2 deletions src/consensus/params.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,8 @@ struct Params {
int nGovernanceMinQuorum; // Min absolute vote count to trigger an action
int nGovernanceFilterElements;
int nMasternodeMinimumConfirmations;
/** Block height and hash at which BIP34 becomes active */
/** Block height at which BIP34 becomes active */
int BIP34Height;
uint256 BIP34Hash;
/** Block height at which BIP65 becomes active */
int BIP65Height;
/** Block height at which BIP66 becomes active */
Expand Down
7 changes: 0 additions & 7 deletions src/index/coinstatsindex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,13 +144,6 @@ bool CoinStatsIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
for (size_t i = 0; i < block.vtx.size(); ++i) {
const auto& tx{block.vtx.at(i)};

// Skip duplicate txid coinbase transactions (BIP30).
if (IsBIP30Unspendable(*pindex) && tx->IsCoinBase()) {
m_total_unspendable_amount += block_subsidy;
m_total_unspendables_bip30 += block_subsidy;
continue;
}

for (uint32_t j = 0; j < tx->vout.size(); ++j) {
const CTxOut& out{tx->vout[j]};
Coin coin{out, pindex->nHeight, tx->IsCoinBase()};
Expand Down
1 change: 1 addition & 0 deletions src/index/coinstatsindex.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class CoinStatsIndex final : public BaseIndex
CAmount m_total_new_outputs_ex_coinbase_amount{0};
CAmount m_total_coinbase_amount{0};
CAmount m_total_unspendables_genesis_block{0};
//! There's no unspendable coinbase outputs in dash core. TODO: remove it with a version bump
CAmount m_total_unspendables_bip30{0};
CAmount m_total_unspendables_scripts{0};
CAmount m_total_unspendables_unclaimed_rewards{0};
Expand Down
2 changes: 1 addition & 1 deletion src/kernel/coinstats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ struct CCoinsStats {
CAmount total_coinbase_amount{0};
//! The unspendable coinbase amount from the genesis block
CAmount total_unspendables_genesis_block{0};
//! The two unspendable coinbase outputs total amount caused by BIP30
//! There's no unspendable coinbase outputs in dash core. TODO: remove it with a version bump
CAmount total_unspendables_bip30{0};
//! Total cumulative amount of outputs sent to unspendable scripts (OP_RETURN for example) up to and including this block
CAmount total_unspendables_scripts{0};
Expand Down
6 changes: 2 additions & 4 deletions src/rpc/blockchain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1171,7 +1171,6 @@ static RPCHelpMan gettxoutsetinfo()
{RPCResult::Type::OBJ, "unspendables", "Detailed view of the unspendable categories",
{
{RPCResult::Type::STR_AMOUNT, "genesis_block", "The unspendable amount of the Genesis block subsidy"},
{RPCResult::Type::STR_AMOUNT, "bip30", "Transactions overridden by duplicates (no longer possible with BIP30)"},
{RPCResult::Type::STR_AMOUNT, "scripts", "Amounts sent to scripts that are unspendable (for example OP_RETURN outputs)"},
{RPCResult::Type::STR_AMOUNT, "unclaimed_rewards", "Fee rewards that miners did not claim in their coinbase transaction"},
}}
Expand Down Expand Up @@ -1275,7 +1274,6 @@ static RPCHelpMan gettxoutsetinfo()

UniValue unspendables(UniValue::VOBJ);
unspendables.pushKV("genesis_block", ValueFromAmount(stats.total_unspendables_genesis_block - prev_stats.total_unspendables_genesis_block));
unspendables.pushKV("bip30", ValueFromAmount(stats.total_unspendables_bip30 - prev_stats.total_unspendables_bip30));
unspendables.pushKV("scripts", ValueFromAmount(stats.total_unspendables_scripts - prev_stats.total_unspendables_scripts));
unspendables.pushKV("unclaimed_rewards", ValueFromAmount(stats.total_unspendables_unclaimed_rewards - prev_stats.total_unspendables_unclaimed_rewards));
block_info.pushKV("unspendables", unspendables);
Expand Down Expand Up @@ -2132,9 +2130,9 @@ static RPCHelpMan getblockstats()
size_t out_size = GetSerializeSize(out, PROTOCOL_VERSION) + PER_UTXO_OVERHEAD;
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
// set counts, so they have to be excluded from the statistics
if (pindex.nHeight == 0 || (IsBIP30Repeat(pindex) && tx->IsCoinBase())) continue;
if (pindex.nHeight == 0) continue;
// Skip unspendable outputs since they are not included in the UTXO set
if (out.scriptPubKey.IsUnspendable()) continue;

Expand Down
44 changes: 6 additions & 38 deletions src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2038,21 +2038,11 @@ DisconnectResult CChainState::DisconnectBlock(const CBlock& block, const CBlockI
return DISCONNECT_FAILED;
}

// Ignore blocks that contain transactions which are 'overwritten' by later transactions,
// unless those are already completely spent.
// See https://github.com/bitcoin/bitcoin/issues/22596 for additional information.
// Note: the blocks specified here are different than the ones used in ConnectBlock because DisconnectBlock
// unwinds the blocks in reverse. As a result, the inconsistency is not discovered until the earlier
// blocks with the duplicate coinbase transactions are disconnected.
bool fEnforceBIP30 = !((pindex->nHeight==91722 && pindex->GetBlockHash() == uint256S("0x00000000000271a2dc26e7667f8419f2e15416dc6955e5a6c6cdf3f2574dd08e")) ||
(pindex->nHeight==91812 && pindex->GetBlockHash() == uint256S("0x00000000000af0aed4792b1acee3d966af36cf5def14935db8de83d6f9306f2f")));

// undo transactions in reverse order
for (int i = block.vtx.size() - 1; i >= 0; i--) {
const CTransaction &tx = *(block.vtx[i]);
uint256 hash = tx.GetHash();
bool is_coinbase = tx.IsCoinBase();
bool is_bip30_exception = (is_coinbase && !fEnforceBIP30);

if (fAddressIndex) {
for (unsigned int k = tx.vout.size(); k-- > 0;) {
Expand Down Expand Up @@ -2081,9 +2071,7 @@ DisconnectResult CChainState::DisconnectBlock(const CBlock& block, const CBlockI
Coin coin;
bool is_spent = view.SpendCoin(out, &coin);
if (!is_spent || tx.vout[o] != coin.out || pindex->nHeight != coin.nHeight || is_coinbase != coin.fCoinBase) {
if (!is_bip30_exception) {
fClean = false; // transaction output mismatch
}
fClean = false; // transaction output mismatch
}
}
}
Expand Down Expand Up @@ -2382,24 +2370,16 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state,
// can be duplicated to remove the ability to spend the first instance -- even after
// being sent to another address.
// See BIP30, CVE-2012-1909, and http://r6.ca/blog/20120206T005236Z.html for more information.
// This rule was originally applied to all blocks with a timestamp after March 15, 2012, 0:00 UTC.
// Now that the whole chain is irreversibly beyond that time it is applied to all blocks except the
// two in the chain that violate it. This prevents exploiting the issue against nodes during their
// 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) {
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot May 25, 2026

Choose a reason for hiding this comment

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

P1 Badge Keep BIP30 checks active on non-buried histories

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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

@knst knst May 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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

Comment on lines +2373 to +2382
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 25, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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.py

Repository: 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.

for (const auto& tx : block.vtx) {
for (size_t o = 0; o < tx->vout.size(); o++) {
if (view.HaveCoin(COutPoint(tx->GetHash(), o))) {
Expand Down Expand Up @@ -6157,15 +6137,3 @@ ChainstateManager::~ChainstateManager()
i.clear();
}
}

bool IsBIP30Repeat(const CBlockIndex& block_index)
{
return (block_index.nHeight==91842 && block_index.GetBlockHash() == uint256S("0x00000000000a4d0a398161ffc163c503763b1f4360639393e0e4c8e300e0caec")) ||
(block_index.nHeight==91880 && block_index.GetBlockHash() == uint256S("0x00000000000743f190a18c5577a3c2d2a1f610ae9601ac046a38084ccb7cd721"));
}

bool IsBIP30Unspendable(const CBlockIndex& block_index)
{
return (block_index.nHeight==91722 && block_index.GetBlockHash() == uint256S("0x00000000000271a2dc26e7667f8419f2e15416dc6955e5a6c6cdf3f2574dd08e")) ||
(block_index.nHeight==91812 && block_index.GetBlockHash() == uint256S("0x00000000000af0aed4792b1acee3d966af36cf5def14935db8de83d6f9306f2f"));
}
6 changes: 0 additions & 6 deletions src/validation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1126,10 +1126,4 @@ bool LoadMempool(CTxMemPool& pool, CChainState& active_chainstate, FopenFn mocka
*/
const AssumeutxoData* ExpectedAssumeutxo(const int height, const CChainParams& params);

/** Identifies blocks that overwrote an existing coinbase output in the UTXO set (see BIP30) */
bool IsBIP30Repeat(const CBlockIndex& block_index);

/** Identifies blocks which coinbase output was subsequently overwritten in the UTXO set (see BIP30) */
bool IsBIP30Unspendable(const CBlockIndex& block_index);

#endif // BITCOIN_VALIDATION_H
4 changes: 0 additions & 4 deletions test/functional/feature_coinstatsindex.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ def _test_coin_stats_index(self):
'coinbase': 0,
'unspendables': {
'genesis_block': 50,
'bip30': 0,
'scripts': 0,
'unclaimed_rewards': 0
}
Expand All @@ -152,7 +151,6 @@ def _test_coin_stats_index(self):
'coinbase': Decimal('500.00025500'),
'unspendables': {
'genesis_block': 0,
'bip30': 0,
'scripts': 0,
'unclaimed_rewards': 0,
}
Expand Down Expand Up @@ -189,7 +187,6 @@ def _test_coin_stats_index(self):
'coinbase': Decimal('500.00101000'),
'unspendables': {
'genesis_block': 0,
'bip30': 0,
'scripts': Decimal('20.99900000'),
'unclaimed_rewards': 0,
}
Expand Down Expand Up @@ -220,7 +217,6 @@ def _test_coin_stats_index(self):
'coinbase': 40,
'unspendables': {
'genesis_block': 0,
'bip30': 0,
'scripts': 0,
'unclaimed_rewards': 460
}
Expand Down
Loading