From 9af14f25683af866b348ccd9d6638ccfe2369190 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 24 Dec 2024 13:15:55 +0100 Subject: [PATCH] test: use Mining interface in miner_tests Alternate calls between Chainman's ProcessNewBlock and submitSolution. The Chainman code path is used in production, so it's important to keep test coverage for it. Use createNewBlock via the interface instead of using the BlockAssembler directly. The latter is always called via the interface in production code. --- src/test/miner_tests.cpp | 188 +++++++++++++++++++++++---------------- 1 file changed, 111 insertions(+), 77 deletions(-) diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 8a264f3fd390d5..9ec2cd0751072b 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -28,8 +29,9 @@ #include using namespace util::hex_literals; +using interfaces::BlockTemplate; +using interfaces::Mining; using node::BlockAssembler; -using node::CBlockTemplate; namespace miner_tests { struct MinerTestingSetup : public TestingSetup { @@ -54,7 +56,10 @@ struct MinerTestingSetup : public TestingSetup { Assert(error.empty()); return *m_node.mempool; } - BlockAssembler AssemblerForTest(CTxMemPool& tx_mempool, BlockAssembler::Options options); + std::unique_ptr MakeMining() + { + return interfaces::MakeMining(m_node); + } }; } // namespace miner_tests @@ -62,13 +67,6 @@ BOOST_FIXTURE_TEST_SUITE(miner_tests, MinerTestingSetup) static CFeeRate blockMinFeeRate = CFeeRate(DEFAULT_BLOCK_MIN_TX_FEE); -BlockAssembler MinerTestingSetup::AssemblerForTest(CTxMemPool& tx_mempool, BlockAssembler::Options options) -{ - options.nBlockMaxWeight = MAX_BLOCK_WEIGHT; - options.blockMinFeeRate = blockMinFeeRate; - return BlockAssembler{m_node.chainman->ActiveChainstate(), &tx_mempool, options}; -} - constexpr static struct { unsigned char extranonce; unsigned int nonce; @@ -106,6 +104,10 @@ static std::unique_ptr CreateBlockIndex(int nHeight, CBlockIndex* a void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const std::vector& txFirst) { CTxMemPool& tx_mempool{MakeMempool()}; + auto miner{MakeMining()}; + BlockAssembler::Options options; + options.coinbase_output_script = scriptPubKey; + LOCK(tx_mempool.cs); // Test the ancestor feerate transaction selection. TestMemPoolEntryHelper entry; @@ -135,13 +137,12 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const Txid hashHighFeeTx = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(50000).Time(Now()).SpendsCoinbase(false).FromTx(tx)); - BlockAssembler::Options options; - options.coinbase_output_script = scriptPubKey; - std::unique_ptr pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 4U); - BOOST_CHECK(pblocktemplate->block.vtx[1]->GetHash() == hashParentTx); - BOOST_CHECK(pblocktemplate->block.vtx[2]->GetHash() == hashHighFeeTx); - BOOST_CHECK(pblocktemplate->block.vtx[3]->GetHash() == hashMediumFeeTx); + std::unique_ptr block_template = miner->createNewBlock(options); + CBlock block{block_template->getBlock()}; + BOOST_REQUIRE_EQUAL(block.vtx.size(), 4U); + BOOST_CHECK(block.vtx[1]->GetHash() == hashParentTx); + BOOST_CHECK(block.vtx[2]->GetHash() == hashHighFeeTx); + BOOST_CHECK(block.vtx[3]->GetHash() == hashMediumFeeTx); // Test that a package below the block min tx fee doesn't get included tx.vin[0].prevout.hash = hashHighFeeTx; @@ -158,11 +159,13 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vout[0].nValue = 5000000000LL - 1000 - 50000 - feeToUse; Txid hashLowFeeTx = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(feeToUse).FromTx(tx)); - pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); + block_template = miner->createNewBlock(options); + BOOST_REQUIRE(block_template); + block = block_template->getBlock(); // Verify that the free tx and the low fee tx didn't get selected - for (size_t i=0; iblock.vtx.size(); ++i) { - BOOST_CHECK(pblocktemplate->block.vtx[i]->GetHash() != hashFreeTx); - BOOST_CHECK(pblocktemplate->block.vtx[i]->GetHash() != hashLowFeeTx); + for (size_t i=0; iGetHash() != hashFreeTx); + BOOST_CHECK(block.vtx[i]->GetHash() != hashLowFeeTx); } // Test that packages above the min relay fee do get included, even if one @@ -172,10 +175,12 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vout[0].nValue -= 2; // Now we should be just over the min relay fee hashLowFeeTx = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(feeToUse + 2).FromTx(tx)); - pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 6U); - BOOST_CHECK(pblocktemplate->block.vtx[4]->GetHash() == hashFreeTx); - BOOST_CHECK(pblocktemplate->block.vtx[5]->GetHash() == hashLowFeeTx); + block_template = miner->createNewBlock(options); + BOOST_REQUIRE(block_template); + block = block_template->getBlock(); + BOOST_REQUIRE_EQUAL(block.vtx.size(), 6U); + BOOST_CHECK(block.vtx[4]->GetHash() == hashFreeTx); + BOOST_CHECK(block.vtx[5]->GetHash() == hashLowFeeTx); // Test that transaction selection properly updates ancestor fee // calculations as ancestor transactions get included in a block. @@ -194,12 +199,14 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vout[0].nValue = 5000000000LL - 100000000 - feeToUse; Txid hashLowFeeTx2 = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(feeToUse).SpendsCoinbase(false).FromTx(tx)); - pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); + block_template = miner->createNewBlock(options); + BOOST_REQUIRE(block_template); + block = block_template->getBlock(); // Verify that this tx isn't selected. - for (size_t i=0; iblock.vtx.size(); ++i) { - BOOST_CHECK(pblocktemplate->block.vtx[i]->GetHash() != hashFreeTx2); - BOOST_CHECK(pblocktemplate->block.vtx[i]->GetHash() != hashLowFeeTx2); + for (size_t i=0; iGetHash() != hashFreeTx2); + BOOST_CHECK(block.vtx[i]->GetHash() != hashLowFeeTx2); } // This tx will be mineable, and should cause hashLowFeeTx2 to be selected @@ -207,9 +214,11 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vin[0].prevout.n = 1; tx.vout[0].nValue = 100000000 - 10000; // 10k satoshi fee AddToMempool(tx_mempool, entry.Fee(10000).FromTx(tx)); - pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 9U); - BOOST_CHECK(pblocktemplate->block.vtx[8]->GetHash() == hashLowFeeTx2); + block_template = miner->createNewBlock(options); + BOOST_REQUIRE(block_template); + block = block_template->getBlock(); + BOOST_REQUIRE_EQUAL(block.vtx.size(), 9U); + BOOST_CHECK(block.vtx[8]->GetHash() == hashLowFeeTx2); } void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std::vector& txFirst, int baseheight) @@ -225,6 +234,9 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: const CAmount HIGHFEE = COIN; const CAmount HIGHERFEE = 4 * COIN; + auto miner{MakeMining()}; + BOOST_REQUIRE(miner); + BlockAssembler::Options options; options.coinbase_output_script = scriptPubKey; @@ -233,8 +245,9 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: LOCK(tx_mempool.cs); // Just to make sure we can still make simple blocks - auto pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_CHECK(pblocktemplate); + auto block_template{miner->createNewBlock(options)}; + BOOST_REQUIRE(block_template); + CBlock block{block_template->getBlock()}; // block sigops > limit: 1000 CHECKMULTISIG + 1 tx.vin.resize(1); @@ -253,7 +266,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: tx.vin[0].prevout.hash = hash; } - BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool, options).CreateNewBlock(), std::runtime_error, HasReason("bad-blk-sigops")); + BOOST_CHECK_EXCEPTION(miner->createNewBlock(options), std::runtime_error, HasReason("bad-blk-sigops")); } { @@ -270,7 +283,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now()).SpendsCoinbase(spendsCoinbase).SigOpsCost(80).FromTx(tx)); tx.vin[0].prevout.hash = hash; } - BOOST_CHECK(AssemblerForTest(tx_mempool, options).CreateNewBlock()); + BOOST_CHECK(miner->createNewBlock(options)); } { @@ -294,7 +307,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now()).SpendsCoinbase(spendsCoinbase).FromTx(tx)); tx.vin[0].prevout.hash = hash; } - BOOST_CHECK(AssemblerForTest(tx_mempool, options).CreateNewBlock()); + BOOST_CHECK(miner->createNewBlock(options)); } { @@ -304,7 +317,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: // orphan in tx_mempool, template creation fails hash = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now()).FromTx(tx)); - BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool, options).CreateNewBlock(), std::runtime_error, HasReason("bad-txns-inputs-missingorspent")); + BOOST_CHECK_EXCEPTION(miner->createNewBlock(options), std::runtime_error, HasReason("bad-txns-inputs-missingorspent")); } { @@ -325,7 +338,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: tx.vout[0].nValue = tx.vout[0].nValue + BLOCKSUBSIDY - HIGHERFEE; // First txn output + fresh coinbase - new txn fee hash = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(HIGHERFEE).Time(Now()).SpendsCoinbase(true).FromTx(tx)); - BOOST_CHECK(AssemblerForTest(tx_mempool, options).CreateNewBlock()); + BOOST_CHECK(miner->createNewBlock(options)); } { @@ -341,7 +354,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: // give it a fee so it'll get mined AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now()).SpendsCoinbase(false).FromTx(tx)); // Should throw bad-cb-multiple - BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool, options).CreateNewBlock(), std::runtime_error, HasReason("bad-cb-multiple")); + BOOST_CHECK_EXCEPTION(miner->createNewBlock(options), std::runtime_error, HasReason("bad-cb-multiple")); } { @@ -358,7 +371,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: tx.vout[0].scriptPubKey = CScript() << OP_2; hash = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(HIGHFEE).Time(Now()).SpendsCoinbase(true).FromTx(tx)); - BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool, options).CreateNewBlock(), std::runtime_error, HasReason("bad-txns-inputs-missingorspent")); + BOOST_CHECK_EXCEPTION(miner->createNewBlock(options), std::runtime_error, HasReason("bad-txns-inputs-missingorspent")); } { @@ -378,7 +391,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: next->BuildSkip(); m_node.chainman->ActiveChain().SetTip(*next); } - BOOST_CHECK(AssemblerForTest(tx_mempool, options).CreateNewBlock()); + BOOST_CHECK(miner->createNewBlock(options)); // Extend to a 210000-long block chain. while (m_node.chainman->ActiveChain().Tip()->nHeight < 210000) { CBlockIndex* prev = m_node.chainman->ActiveChain().Tip(); @@ -390,7 +403,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: next->BuildSkip(); m_node.chainman->ActiveChain().SetTip(*next); } - BOOST_CHECK(AssemblerForTest(tx_mempool, options).CreateNewBlock()); + BOOST_CHECK(miner->createNewBlock(options)); // invalid p2sh txn in tx_mempool, template creation fails tx.vin[0].prevout.hash = txFirst[0]->GetHash(); @@ -406,7 +419,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: tx.vout[0].nValue -= LOWFEE; hash = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now()).SpendsCoinbase(false).FromTx(tx)); - BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool, options).CreateNewBlock(), std::runtime_error, HasReason("mandatory-script-verify-flag-failed")); + BOOST_CHECK_EXCEPTION(miner->createNewBlock(options), std::runtime_error, HasReason("mandatory-script-verify-flag-failed")); // Delete the dummy blocks again. while (m_node.chainman->ActiveChain().Tip()->nHeight > nHeight) { @@ -508,14 +521,15 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: tx.vin[0].nSequence = CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG | 1; BOOST_CHECK(!TestSequenceLocks(CTransaction{tx}, tx_mempool)); // Sequence locks fail - auto pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_CHECK(pblocktemplate); + auto block_template = miner->createNewBlock(options); + BOOST_CHECK(block_template); // None of the of the absolute height/time locked tx should have made // it into the template because we still check IsFinalTx in CreateNewBlock, // but relative locked txs will if inconsistently added to mempool. // For now these will still generate a valid template until BIP68 soft fork - BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 3U); + CBlock block{block_template->getBlock()}; + BOOST_CHECK_EQUAL(block.vtx.size(), 3U); // However if we advance height by 1 and time by SEQUENCE_LOCK_TIME, all of them should be mined for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i) { CBlockIndex* ancestor{Assert(m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i))}; @@ -524,12 +538,17 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: m_node.chainman->ActiveChain().Tip()->nHeight++; SetMockTime(m_node.chainman->ActiveChain().Tip()->GetMedianTimePast() + 1); - BOOST_CHECK(pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock()); - BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 5U); + block_template = miner->createNewBlock(options); + BOOST_REQUIRE(block_template); + block = block_template->getBlock(); + BOOST_CHECK_EQUAL(block.vtx.size(), 5U); } void MinerTestingSetup::TestPrioritisedMining(const CScript& scriptPubKey, const std::vector& txFirst) { + auto miner{MakeMining()}; + BOOST_REQUIRE(miner); + BlockAssembler::Options options; options.coinbase_output_script = scriptPubKey; @@ -594,34 +613,33 @@ void MinerTestingSetup::TestPrioritisedMining(const CScript& scriptPubKey, const Txid hashFreeGrandchild = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(0).SpendsCoinbase(false).FromTx(tx)); - auto pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 6U); - BOOST_CHECK(pblocktemplate->block.vtx[1]->GetHash() == hashFreeParent); - BOOST_CHECK(pblocktemplate->block.vtx[2]->GetHash() == hashFreePrioritisedTx); - BOOST_CHECK(pblocktemplate->block.vtx[3]->GetHash() == hashParentTx); - BOOST_CHECK(pblocktemplate->block.vtx[4]->GetHash() == hashPrioritsedChild); - BOOST_CHECK(pblocktemplate->block.vtx[5]->GetHash() == hashFreeChild); - for (size_t i=0; iblock.vtx.size(); ++i) { + auto block_template = miner->createNewBlock(options); + CBlock block{block_template->getBlock()}; + BOOST_REQUIRE_EQUAL(block.vtx.size(), 6U); + BOOST_CHECK(block.vtx[1]->GetHash() == hashFreeParent); + BOOST_CHECK(block.vtx[2]->GetHash() == hashFreePrioritisedTx); + BOOST_CHECK(block.vtx[3]->GetHash() == hashParentTx); + BOOST_CHECK(block.vtx[4]->GetHash() == hashPrioritsedChild); + BOOST_CHECK(block.vtx[5]->GetHash() == hashFreeChild); + for (size_t i=0; iblock.vtx[i]->GetHash() != hashFreeGrandchild); + BOOST_CHECK(block.vtx[i]->GetHash() != hashFreeGrandchild); // De-prioritised transaction should not be included. - BOOST_CHECK(pblocktemplate->block.vtx[i]->GetHash() != hashMediumFeeTx); + BOOST_CHECK(block.vtx[i]->GetHash() != hashMediumFeeTx); } } // NOTE: These tests rely on CreateNewBlock doing its own self-validation! BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) { + auto miner{MakeMining()}; + BOOST_REQUIRE(miner); + // Note that by default, these tests run with size accounting enabled. CScript scriptPubKey = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex << OP_CHECKSIG; - std::unique_ptr pblocktemplate; - BlockAssembler::Options options; options.coinbase_output_script = scriptPubKey; - - CTxMemPool& tx_mempool{*m_node.mempool}; - // Simple block creation, nothing special yet: - BOOST_CHECK(pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock()); + std::unique_ptr block_template; // We can't make transactions until we have inputs // Therefore, load 110 blocks :) @@ -629,27 +647,43 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) int baseheight = 0; std::vector txFirst; for (const auto& bi : BLOCKINFO) { - CBlock *pblock = &pblocktemplate->block; // pointer for convenience + const int current_height{miner->getTip()->height}; + + // Simple block creation, nothing special yet: + block_template = miner->createNewBlock(options); + BOOST_REQUIRE(block_template); + + CBlock block{block_template->getBlock()}; + CMutableTransaction txCoinbase(*block.vtx[0]); { LOCK(cs_main); - pblock->nVersion = VERSIONBITS_TOP_BITS; - pblock->nTime = m_node.chainman->ActiveChain().Tip()->GetMedianTimePast()+1; - CMutableTransaction txCoinbase(*pblock->vtx[0]); + block.nVersion = VERSIONBITS_TOP_BITS; + block.nTime = Assert(m_node.chainman)->ActiveChain().Tip()->GetMedianTimePast()+1; txCoinbase.version = 1; - txCoinbase.vin[0].scriptSig = CScript{} << (m_node.chainman->ActiveChain().Height() + 1) << bi.extranonce; + txCoinbase.vin[0].scriptSig = CScript{} << (current_height + 1) << bi.extranonce; txCoinbase.vout.resize(1); // Ignore the (optional) segwit commitment added by CreateNewBlock (as the hardcoded nonces don't account for this) txCoinbase.vout[0].scriptPubKey = CScript(); - pblock->vtx[0] = MakeTransactionRef(std::move(txCoinbase)); + block.vtx[0] = MakeTransactionRef(txCoinbase); if (txFirst.size() == 0) - baseheight = m_node.chainman->ActiveChain().Height(); + baseheight = current_height; if (txFirst.size() < 4) - txFirst.push_back(pblock->vtx[0]); - pblock->hashMerkleRoot = BlockMerkleRoot(*pblock); - pblock->nNonce = bi.nonce; + txFirst.push_back(block.vtx[0]); + block.hashMerkleRoot = BlockMerkleRoot(block); + block.nNonce = bi.nonce; + } + std::shared_ptr shared_pblock = std::make_shared(block); + // Alternate calls between Chainman's ProcessNewBlock and submitSolution + // via the Mining interface. + if (current_height % 2 == 0) { + BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(shared_pblock, /*force_processing=*/true, /*min_pow_checked=*/true, nullptr)); + } else { + BOOST_REQUIRE(block_template->submitSolution(block.nVersion, block.nTime, block.nNonce, MakeTransactionRef(txCoinbase))); } - std::shared_ptr shared_pblock = std::make_shared(*pblock); - BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlock(shared_pblock, true, true, nullptr)); - pblock->hashPrevBlock = pblock->GetHash(); + // ProcessNewBlock and submitSolution do not check if the new block is + // valid. If it is, they will wait for the tip to update. Therefore the + // following check will either return immediately, or be stuck indefinately. + // It's mainly there to increase test coverage. + miner->waitTipChanged(block.hashPrevBlock); } LOCK(cs_main);