diff --git a/src/miner.cpp b/src/miner.cpp index 354a310eb..7b594357a 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -342,7 +342,10 @@ std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& sc break; } 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; @@ -364,7 +367,12 @@ std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& sc } else { 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; diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 1f6feac96..773670c94 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -18,6 +18,13 @@ #include #include #include +#include +#include +#include +#include +#include +#include +#include #include @@ -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) +{ + 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 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 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 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) +{ + 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(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() diff --git a/src/validation.cpp b/src/validation.cpp index bb30c7b29..cefb85431 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -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)) { LogPrint(BCLog::NET, "%s: pubcoin already in blockchain. Reject tx %s.\n", __func__, tx.GetHash().GetHex()); return false; } @@ -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)) return state.Invalid(false, REJECT_INVALID, "zcmint-fail-context-check"); } } @@ -1347,6 +1351,14 @@ bool IsTransactionInChain(const uint256& txId, int& nHeightTx, CTransactionRef& if (!GetTransaction(txId, txRef, params, hashBlock, true, nullptr, log)) 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); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0e96eafdc..fd338e796 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -6367,7 +6367,9 @@ bool CWallet::CommitZerocoinSpend(CZerocoinSpendReceipt& receipt, std::vector& listMintsRestored, std::list& listDMintsRestored)