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

[ohcee]

Problem

A staking node on testnet stopped producing blocks entirely. It kept winning stakes and every template it built failed validation, in an endless loop:

CreateCoinStake : kernel found
ERROR: ContextualCheckZerocoinMint: pubcoin 9b72ec46bb was already accumulated in tx 8dfa4a3f3c59632674dfc800e00ef3b7b5e45d74116f0e66d850b7621318cebe
ERROR: ConnectBlock: zerocoin mint failed contextual check in transaction efcccab72b5a90cce73848d42419caa24db79ba0da25055d3a633d9772f04c18
ERROR: CreateNewBlock: TestBlockValidity failed:  (code 16)

The mempool held a transaction minting a pubcoin that a different transaction had already accumulated on chain. Nothing evicted it, every template included it, and the node threw away every stake it won. Because that node was the chain's only active block producer, the chain froze with it.

This is the sibling of #1078. That case was a confirmed transaction stuck in the mempool. This transaction never confirmed at all, so the #1078 guard (own outputs already in the UTXO set) does not reach it.

Description

Zerocoin mints are deterministic from the wallet seed. Two wallets restored from one seed will derive the identical pubcoin and can each broadcast a mint for it in two different transactions, and the same happens when a mint counter rolls back after a restore. One of the two confirms. The other is permanently invalid from that moment, but it does not conflict with the confirmed one on any input, so removeForBlock never touches it.

The guards that should handle this have existed since 2019: AcceptToMemoryPool refuses a mint whose pubcoin is already in the mempool or already in the chain, and a dedup pass inside CreateNewBlock checks every selected transaction's serials and pubcoins against the chain and sweeps offenders out of the mempool. Both guards were blind in exactly the situation that produces the zombie, so the duplicate walked in the front door and then could not be swept.

Root Cause

Two blind spots in the chain membership lookups.

1. IsBlockHashInChain with a reference index excludes the reference's own block (the early return on pindex->nHeight <= nHeight in validation.cpp). That behavior is correct for ConnectBlock, which revalidates blocks whose own writes are already in the zerocoin db and must not trip over them. But AcceptToMemoryPool and the CreateNewBlock dedup pass passed chainActive.Tip() as the reference, so a pubcoin accumulated by the tip block itself was invisible to both. ConnectBlock checks from the new block's index, one height further up, and does see it. So while the accumulating block is the tip, the duplicate is accepted into the mempool, the dedup pass cannot see why it is dead, and TestBlockValidity kills every template. A staker that is the only block producer can never advance the tip past the accumulating block, so the window never closes and the node bricks itself permanently.

2. IsTransactionInChain asks GetTransaction, which prefers the mempool and returns a null block hash on a hit there. Any transaction sitting in the mempool was therefore reported as not in chain, even when it was also confirmed. A confirmed transaction stuck in the mempool is exactly the Stop a confirmed transaction stuck in the mempool from halting block production #1078 zombie, which is why the dedup pass failed to sweep that one too.

Next to these there is a small latent bug in the dedup pass: it passed the BlockAssembler nHeight member as the output parameter of IsSerialInBlockchain, so any hit overwrote the height of the block being assembled with the height of the confirmed spend.

Why broke?

The reference exclusion in IsBlockHashInChain was written for the consensus caller, where checking "strictly below this block on this chain" is the right question. The mempool and miner callers reused it with the tip as the reference without noticing that this silently excludes the newest block, which is precisely where a freshly accumulated duplicate lives. And GetTransaction's mempool preference is inherited behavior that is fine for RPC lookups but wrong as a chain membership oracle, because the one moment the dedup pass most needs the truth (a transaction in both places at once) is the one moment the lookup lies.

Solution

Ask the right question at each call site and make the answer honest.

Mempool and miner callers ask about the whole active chain, tip included, so they pass no reference index and get the plain chainActive.Contains path. The consensus callers keep their reference and their exclusion, unchanged. And when GetTransaction found the transaction in the mempool, IsTransactionInChain now consults the txindex before concluding the transaction is unconfirmed, so mempool residence no longer masks chain membership.

How fix?

src/validation.cpp

• The AcceptToMemoryPool pubcoin guard and the contextual mint check pass no reference index instead of chainActive.Tip(), so a pubcoin accumulated by the tip block is rejected at the door.
• IsTransactionInChain falls through to g_txindex when GetTransaction returns a mempool hit with a null block hash. Nodes without txindex keep the old behavior there.

src/miner.cpp

• The dedup pass pubcoin check passes no reference index, so a duplicate that did get in is skipped and swept instead of poisoning the template.
• The serial check writes its output height into a local instead of the assembler's nHeight member.

src/wallet/wallet.cpp

• IsMintInChain passes no reference index, same reasoning, it asks about the whole active chain.

src/test/miner_tests.cpp

• Two regression tests, described below.

One consensus observation, intentional: the txindex fallthrough makes ConnectBlock's duplicate mint check deterministic where it used to depend on local mempool contents. Today an old node that happens to hold the first minting transaction in its mempool accepts a block that accumulates the same pubcoin a second time, while every other node rejects that block. Validation of the same block already differs from node to node; with this change every node rejects it.

Unit Testing Results

With the fix, the miner suite passes, including the #1078 regression test:

$ ./src/test/test_veil --run_test=miner_tests
Running 3 test cases...
*** No errors detected

Against master with the tests kept and the fix reverted, both new tests fail exactly the way the live node fails, CreateNewBlock returns nothing and the zombie stays in the mempool:

error: in "miner_tests/CreateNewBlock_sweeps_mint_accumulated_in_tip": check pblocktemplate = AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey) has failed
error: in "miner_tests/CreateNewBlock_sweeps_mint_accumulated_in_tip": check !mempool.exists(txidZombie) has failed
error: in "miner_tests/transaction_in_chain_seen_through_mempool": check IsTransactionInChain(txid, nHeightTx, Params().GetConsensus()) has failed
error: in "miner_tests/transaction_in_chain_seen_through_mempool": check nHeightTx == 1 has failed [0 != 1]
*** 4 failures are detected

The adjacent suites (txindex_tests, txvalidation_tests, txvalidationcache_tests, proofofstaketests, mempool_tests) behave identically with and without this change on the build machine. The failures that do show up there (MempoolRemoveTest and a flaky txindex_tests crash) reproduce on unmodified master on the same machine, so they predate this PR.

How to test?

1. ./src/test/test_veil --run_test=miner_tests on this branch: green.
2. Revert the three src files but keep src/test/miner_tests.cpp, rebuild, run again: the two new cases fail, reproducing the incident. CreateNewBlock_sweeps_mint_accumulated_in_tip is the live bug end to end: a real pubcoin recorded as accumulated by the tip block's transaction, a different transaction minting the same pubcoin sitting in the mempool, and CreateNewBlock must produce a template and sweep the duplicate. transaction_in_chain_seen_through_mempool pins the unmasked lookup: a transaction that is in a block and in the mempool at the same time still counts as in chain.
3. On a live node in the bricked state (kernel found followed by TestBlockValidity failed, forever): with this fix the next template logs removing already in chain pubcoin under -debug=blockcreation, produces normally, and the duplicate is gone from getrawmempool. Without the fix the only way out is clearmempool.

[seanPhill]

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.

[seanPhill]

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.

[seanPhill]

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.

[seanPhill]

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.

[seanPhill]

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.

[seanPhill]

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.

[seanPhill]

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.

[seanPhill]

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.

[seanPhill]

Approve with comments. (Please note the in-line comments. Let me know which, if any, of the suggested changes you are prepared to do, otherwise we can go ahead as is.)

---

Root cause is correct: IsBlockHashInChain’s reference exclusion is a ConnectBlock primitive and was being used as a chain-membership oracle with Tip(). Passing nullptr at mempool/miner/wallet sites and leaving the reference on connect is the right split. The assembler nHeight clobber is a real latent bug.

Not blocking: restore pzerocoinDB after the sweep test, add an ATMP case if it is cheap, and soften “every node rejects it” to “default txindex nodes.” Title could take a [Mining] prefix per CONTRIBUTING.md.

[Veil-Project]

The many suggestions will be useful for a follow up PR, but as we would like to roll this into the impending higher priority bug fix, let's merge this and leave the remainder