Skip to content

Stop a duplicate zerocoin mint in the mempool from halting block production #1086

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

Merged
Veil-Project merged 1 commit into from
Aug 30, 2026
+178 −5
Merged

Stop a duplicate zerocoin mint in the mempool from halting block production #1086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 10 additions & 2 deletions src/miner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,10 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
break;

@seanPhill seanPhill Aug 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good catch. nHeight here is BlockAssembler’s height-of-the-block-being-built. Passing it as IsSerialInBlockchain’s out-param meant any confirmed serial hit silently rewrote the assembler height to the spend height. Local nHeightTx is the right fix; the value is unused after the predicate, which is fine.

} else {
uint256 txid;
if (IsSerialInBlockchain(hashSerial, nHeight, txid)) {
// Out param must not be the nHeight member: a hit would overwrite the
// height of the block being assembled with the confirmed spend's height.
int nHeightTx = 0;
if (IsSerialInBlockchain(hashSerial, nHeightTx, txid)) {
setDuplicate.emplace(ptx->GetHash());
LogPrint(BCLog::BLOCKCREATION, "%s: removing serial that is already in chain, tx=%s\n", __func__, ptx->GetHash().GetHex());
fRemove = true;
Expand All @@ -364,7 +367,12 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
} else {

@seanPhill seanPhill Aug 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is the sweep that unbricks a node that already has the zombie. Same Tip()-exclusion bug as ATMP.

Leaving the serial check on the no-reference overload is right — that overload already asks IsTransactionInChain without a pindex. The only serial bug in this function was the nHeight clobber above.

uint256 txid;
int nHeightTx = 0;
if (IsPubcoinInBlockchain(hashPubcoin, nHeightTx, txid, chainActive.Tip())) {
// No reference index here: a reference excludes its own block, so passing
// the tip made a pubcoin accumulated in the tip block invisible. The
// template then carried the duplicate mint, TestBlockValidity failed on
// it, and a staker that was the only block producer could never advance
// the tip to clear it, bricking block production entirely.
if (IsPubcoinInBlockchain(hashPubcoin, nHeightTx, txid, nullptr)) {
setDuplicate.emplace(ptx->GetHash());
LogPrint(BCLog::BLOCKCREATION, "%s: removing already in chain pubcoin : tx %s\n", __func__, ptx->GetHash().GetHex());
fRemove = true;
Expand Down
151 changes: 151 additions & 0 deletions src/test/miner_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@
#include <util/system.h>
#include <util/strencodings.h>
#include <pow.h>
#include <index/txindex.h>
#include <libzerocoin/Coin.h>
#include <primitives/zerocoin.h>
#include <txdb.h>
#include <util/memory.h>
#include <util/time.h>
#include <veil/zerocoin/zchain.h>

#include <test/test_veil.h>

Expand Down Expand Up @@ -586,4 +593,148 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_skips_already_confirmed)
BOOST_CHECK(!mempool.exists(txid)); // and removed from the mempool
}

// The zerocoin sibling of the case above: the mempool transaction itself never
// confirmed, but its pubcoin was already accumulated by a DIFFERENT transaction, the
// state deterministic mints produce when two wallets derive from one seed. The dedup
// pass in CreateNewBlock has always checked for this, but it asked "is the accumulating
// tx in the chain" with the tip as the reference index, and a reference index excludes
// its own block, so a pubcoin accumulated by the tip block was invisible. ConnectBlock
// checks from the new block's index and does see it, so TestBlockValidity failed and no
// block was produced. A staker that is the chain's only block producer can never
// advance the tip past the accumulating block that way, and bricks permanently.
BOOST_FIXTURE_TEST_CASE(CreateNewBlock_sweeps_mint_accumulated_in_tip, TestChain100Setup)

@seanPhill seanPhill Aug 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is the test I wanted: it spells out the IsBlockHashInChain(tip, tip) trap, binds a real pubcoin to the tip coinbase, stuffs a different mint into the mempool, and asserts both “template is non-null” and “zombie is gone.” That is the live stall.

Please restore the previous pzerocoinDB instead of reset() to null at the end. The suite happens to end after these cases today; a later zerocoin miner test, or a change in case order, will inherit a null global. Something like:

auto prevZerocoinDB = std::move(pzerocoinDB);
pzerocoinDB.reset(new CZerocoinDB(1 << 23, true));
// ...
pzerocoinDB = std::move(prevZerocoinDB);

Also consider a sibling ATMP test so the acceptance-path change is not only covered indirectly.

{
const auto chainParams = CreateChainParams(CBaseChainParams::REGTEST);
const CChainParams& chainparams = *chainParams;
CScript scriptPubKey = CScript() << OP_TRUE;
TestMemPoolEntryHelper entry;

// TestingSetup does not create a zerocoin db; use an in-memory one.
pzerocoinDB.reset(new CZerocoinDB(1 << 23, true /* fMemory */));

// A real pubcoin, recorded as accumulated by the coinbase of the TIP block.
libzerocoin::PrivateCoin priv(Params().Zerocoin_Params(), libzerocoin::CoinDenomination::ZQ_TEN, true);
const libzerocoin::PublicCoin pub = priv.getPublicCoin();
const uint256 txidAccumulated = m_coinbase_txns.back()->GetHash();
{
std::map<libzerocoin::PublicCoin, uint256> mintInfo;
mintInfo.emplace(pub, txidAccumulated);
BOOST_CHECK(pzerocoinDB->WriteCoinMintBatch(mintInfo));
}

// The trap itself, spelled out: a reference index cannot see its own block, only
// the plain active-chain lookup can. This is why the mempool side must never pass
// the tip as a reference.
{
LOCK(cs_main);
int nHeightDummy = 0;
const uint256 hashTip = chainActive.Tip()->GetBlockHash();
BOOST_CHECK(!IsBlockHashInChain(hashTip, nHeightDummy, chainActive.Tip()));
BOOST_CHECK(IsBlockHashInChain(hashTip, nHeightDummy, nullptr));
}

// A different transaction minting the same pubcoin, stuck in the mempool. Real
// input coin so nothing else disqualifies it from selection.
CMutableTransaction tx;
tx.vin.resize(1);
tx.vin[0].prevout = COutPoint(uint256S("0000000000000000000000000000000000000000000000000000000000000002"), 0);
tx.vin[0].scriptSig = CScript() << OP_11;
CScript scriptMint = CScript() << OP_ZEROCOINMINT << pub.getValue().getvch().size() << pub.getValue().getvch();
tx.vpout.resize(1);
tx.vpout[0] = CTxOut(libzerocoin::ZerocoinDenominationToAmount(pub.getDenomination()), scriptMint).GetSharedPtr();
const uint256 txidZombie = tx.GetHash();

// The crafted mint output parses back to the exact pubcoin hash the db record was
// written under, so the dedup pass is looking at the right key.
{
const CTransaction ctx(tx);
std::set<uint256> setHashes;
BOOST_CHECK(TxToPubcoinHashSet(&ctx, setHashes));
BOOST_CHECK(setHashes.count(GetPubCoinHash(pub.getValue())));
}

{
LOCK(cs_main);
CTxOut in(libzerocoin::ZerocoinDenominationToAmount(pub.getDenomination()) + 100000, CScript() << OP_11 << OP_EQUAL);
pcoinsTip->AddCoin(tx.vin[0].prevout, Coin(std::move(in), 1, false), false);
BOOST_CHECK(pcoinsTip->HaveCoin(tx.vin[0].prevout));
}
{
LOCK(cs_main);
LOCK(mempool.cs);
mempool.addUnchecked(txidZombie, entry.Fee(10000).FromTx(tx));
BOOST_CHECK(mempool.exists(txidZombie));
}

// Before the fix the template carried the duplicate mint and TestBlockValidity
// failed on it, so this returned null on every attempt. Now the dedup pass sees
// the tip-block accumulation, skips the transaction and sweeps it out.
std::unique_ptr<CBlockTemplate> pblocktemplate;
BOOST_CHECK(pblocktemplate = AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey));
if (pblocktemplate) {
for (const auto& btx : pblocktemplate->block.vtx)
BOOST_CHECK(btx->GetHash() != txidZombie);
}
BOOST_CHECK(!mempool.exists(txidZombie));

pzerocoinDB.reset();
}

// IsTransactionInChain answered "not in chain" for any transaction sitting in the
// mempool, even one that was also confirmed: GetTransaction prefers the mempool and
// returns a null block hash on a hit there. A confirmed transaction stuck in the
// mempool is exactly the zombie state above, so the one moment the dedup pass most
// needed the truth was the one moment the lookup lied. With a txindex available the
// lookup must see through the mempool.
BOOST_FIXTURE_TEST_CASE(transaction_in_chain_seen_through_mempool, TestChain100Setup)

@seanPhill seanPhill Aug 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Right test for the GetTransaction lie, including the negative case (mempool-only stays unconfirmed). Stop-then-reset of g_txindex is correct.

This is the flake candidate: it boots a real TxIndex and waits up to 10s, and the PR already notes txindex_tests is crashy on the same machine. Fine to land, but if CI starts failing here I would rather stub FindTx than keep a live index in miner_tests.

{
TestMemPoolEntryHelper entry;

// Put a long-confirmed transaction (block 1's coinbase) back into the mempool.
// addUnchecked bypasses acceptance on purpose, mirroring the race that creates
// the stuck state in production.
const CTransactionRef txConfirmed = m_coinbase_txns.front();
const uint256 txid = txConfirmed->GetHash();
{
LOCK(cs_main);
LOCK(mempool.cs);
mempool.addUnchecked(txid, entry.Fee(10000).FromTx(CMutableTransaction(*txConfirmed)));
BOOST_CHECK(mempool.exists(txid));
}

// Bring up the global txindex the fixed lookup consults when the mempool masks
// the answer.
g_txindex = MakeUnique<TxIndex>(1 << 20, true);
g_txindex->Start();
constexpr int64_t timeout_ms = 10 * 1000;
const int64_t time_start = GetTimeMillis();
while (!g_txindex->BlockUntilSyncedToCurrentChain()) {
BOOST_REQUIRE(time_start + timeout_ms > GetTimeMillis());
UninterruptibleSleep(std::chrono::milliseconds{100});
}

int nHeightTx = 0;
// In the chain and in the mempool at once: still in the chain.
BOOST_CHECK(IsTransactionInChain(txid, nHeightTx, Params().GetConsensus()));
BOOST_CHECK_EQUAL(nHeightTx, 1);

// A transaction that only exists in the mempool must still count as unconfirmed.
CMutableTransaction txLoose;
txLoose.vin.resize(1);
txLoose.vin[0].prevout = COutPoint(uint256S("0000000000000000000000000000000000000000000000000000000000000003"), 0);
txLoose.vin[0].scriptSig = CScript() << OP_11;
txLoose.vpout.resize(1);
txLoose.vpout[0] = CTxOut(1 * COIN, CScript() << OP_11 << OP_EQUAL).GetSharedPtr();
const uint256 txidLoose = txLoose.GetHash();
{
LOCK(cs_main);
LOCK(mempool.cs);
mempool.addUnchecked(txidLoose, entry.Fee(10000).FromTx(txLoose));
}
BOOST_CHECK(!IsTransactionInChain(txidLoose, nHeightTx, Params().GetConsensus()));

g_txindex->Stop(); // Stop thread before calling destructor
g_txindex.reset();
}

BOOST_AUTO_TEST_SUITE_END()
16 changes: 14 additions & 2 deletions src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -888,7 +888,10 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool

int nHeightTx;
uint256 txid;
if (IsPubcoinInBlockchain(hashPubcoin, nHeightTx, txid, chainActive.Tip())) {
// No reference index here: a reference excludes its own block, so passing the
// tip made a pubcoin accumulated in the tip block invisible and let a
// duplicate mint into the mempool, where it poisoned every block template.
if (IsPubcoinInBlockchain(hashPubcoin, nHeightTx, txid, nullptr)) {

@seanPhill seanPhill Aug 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The nullptr here is the actual door that should have stayed shut. IsBlockHashInChain(ref) excludes the reference block, so Tip() made a pubcoin accumulated by the tip look absent and let the zombie into the mempool.

Two small notes, neither blocking:

  • This branch still return false without state.Invalid(...). The ContextualCheck a few lines down does set zcmint-fail-context-check. After this change both fire for a tip-accumulated pubcoin; the first one still gives the caller a bare false. Pre-existing, but this is a reasonable place to give it a reject reason.
  • The new miner test injects with addUnchecked, so it never exercises this path. An ATMP case with a pubcoin already written against the tip would cover the half of the incident this hunk is supposed to stop.

LogPrint(BCLog::NET, "%s: pubcoin already in blockchain. Reject tx %s.\n", __func__, tx.GetHash().GetHex());
return false;
}
Expand All @@ -903,7 +906,8 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
libzerocoin::PublicCoin pubcoin(Params().Zerocoin_Params());
if (!OutputToPublicCoin(pout.get(), pubcoin))
return state.Invalid(false, REJECT_INVALID, "zcmint-malformed");
if (!ContextualCheckZerocoinMint(tx, pubcoin, chainActive.Tip()))
// Same as above: no reference index, the whole active chain counts here.
if (!ContextualCheckZerocoinMint(tx, pubcoin, nullptr))

@seanPhill seanPhill Aug 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Agreed. Mempool policy is “already on the active chain, tip included?” ConnectBlock below still passes pindex so a block cannot fail ContextualCheckZerocoinMint because of its own zerocoin-DB writes. That split is the whole fix — please keep the connect-path reference as-is.

return state.Invalid(false, REJECT_INVALID, "zcmint-fail-context-check");
}
}
Expand Down Expand Up @@ -1347,6 +1351,14 @@ bool IsTransactionInChain(const uint256& txId, int& nHeightTx, CTransactionRef&
if (!GetTransaction(txId, txRef, params, hashBlock, true, nullptr, log))

@seanPhill seanPhill Aug 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Correct reading of GetTransaction: a mempool hit returns true and leaves hashBlock null, so “in mempool” was treated as “not confirmed.” That is exactly the #1078 mask.

One accuracy nit on the PR body, not on this hunk: this only makes the connect-path duplicate-mint check deterministic on nodes that have g_txindex. Veil defaults txindex on, so normal operators get it; -txindex=0 / prune still take the old answer. Worth saying “default txindex nodes” instead of “every node.”

Also fine, but worth knowing: GetTransaction drops cs_main before we get here, so FindTx is not under cs_main. That matches how TxIndex is used elsewhere.

return false;

// GetTransaction prefers the mempool and leaves hashBlock null on a hit there, but a
// transaction can be in the mempool and confirmed at the same time (accepted in the
// same moment its block connected, the state #1078 guards against). Being in the
// mempool says nothing about the chain, so ask the txindex before concluding the
// transaction is unconfirmed.
if (hashBlock.IsNull() && g_txindex)
g_txindex->FindTx(txId, hashBlock, txRef, log);

return IsBlockHashInChain(hashBlock, nHeightTx, pindex);
}

Expand Down
4 changes: 3 additions & 1 deletion src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6367,7 +6367,9 @@ bool CWallet::CommitZerocoinSpend(CZerocoinSpendReceipt& receipt, std::vector<Co

bool IsMintInChain(const uint256& hashPubcoin, uint256& txid, int& nHeight)

@seanPhill seanPhill Aug 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same question as mempool/miner: ReconsiderZerocoins wants “is this mint on the active chain, including the tip?” Tip() as reference would miss a mint accumulated in the current tip after a restore. nullptr is correct.

No test coverage for this helper, which is acceptable — it is the same lookup, not a new policy.

{
return IsPubcoinInBlockchain(hashPubcoin, nHeight, txid, chainActive.Tip());
// No reference index: a reference excludes its own block, and this asks about the
// whole active chain, tip included.
return IsPubcoinInBlockchain(hashPubcoin, nHeight, txid, nullptr);
}

void CWallet::ReconsiderZerocoins(std::list<CZerocoinMint>& listMintsRestored, std::list<CDeterministicMint>& listDMintsRestored)
Expand Down