test: have miner_tests use Mining interface

Sjors commented at 4:34 pm on December 30, 2024: member

Needed for both #31283 and #31564.

By using the Mining interface in miner_tests.cpp we increase its coverage in unit tests.

DrahtBot commented at 4:34 pm on December 30, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31581.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, vasild, tdb3, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31564 (Add checkblock RPC and checkBlock() to Mining interface by Sjors)
#31283 (Add waitNext() to BlockTemplate interface by Sjors)
#30391 (BlockAssembler: return selected packages virtual size and fee by ismaelsadeeq)
#30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Tests on Dec 30, 2024

Sjors renamed this:
~~test: have miner_tests use Mining interface and fewer cs_main locks~~
test: have miner_tests use Mining interface
on Dec 30, 2024

Sjors force-pushed on Dec 30, 2024

Sjors commented at 7:19 pm on December 30, 2024: member

I dropped the second commit, as I was confused. This PR should now be very easy to review. Also rebased on #31563.

Sjors force-pushed on Dec 30, 2024

in src/test/miner_tests.cpp:142 in e4e76f27d7 outdated

143-    BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 4U);
144-    BOOST_CHECK(pblocktemplate->block.vtx[1]->GetHash() == hashParentTx);
145-    BOOST_CHECK(pblocktemplate->block.vtx[2]->GetHash() == hashHighFeeTx);
146-    BOOST_CHECK(pblocktemplate->block.vtx[3]->GetHash() == hashMediumFeeTx);
147+    std::unique_ptr<BlockTemplate> block_template = miner->createNewBlock(options);
148+    CBlock block{block_template->getBlock()};

ryanofsky commented at 2:18 pm on January 2, 2025:

In commit “test: use Mining interface in miner_tests” (e4e76f27d73646d887ed8b660c67da4b4b785d8e)

Other tests seem to add BOOST_REQUIRE(block_template); before this.

Sjors commented at 10:48 am on January 3, 2025:

A failing BOOST_REQUIRE produces an easier to debug failure than dereferencing nullptr, so it’s always a good one to add.

I updated the test to do this after every miner->createNewBlock() call.

Similarly BOOST_REQUIRE stops the test immedidately, so I replaced a few BOOST_CHECK calls with it.

in src/test/miner_tests.cpp:619 in e4e76f27d7 outdated

620-    BOOST_CHECK(pblocktemplate->block.vtx[3]->GetHash() == hashParentTx);
621-    BOOST_CHECK(pblocktemplate->block.vtx[4]->GetHash() == hashPrioritsedChild);
622-    BOOST_CHECK(pblocktemplate->block.vtx[5]->GetHash() == hashFreeChild);
623-    for (size_t i=0; i<pblocktemplate->block.vtx.size(); ++i) {
624+    auto block_template = miner->createNewBlock(options);
625+    CBlock block{block_template->getBlock()};

ryanofsky commented at 2:19 pm on January 2, 2025:

In commit “test: use Mining interface in miner_tests” (e4e76f27d73646d887ed8b660c67da4b4b785d8e)

Other tests seem to add BOOST_REQUIRE(block_template); before this.

in src/test/miner_tests.cpp:107 in e4e76f27d7 outdated

103@@ -106,6 +104,10 @@ static std::unique_ptr<CBlockIndex> CreateBlockIndex(int nHeight, CBlockIndex* a
104 void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const std::vector<CTransactionRef>& txFirst)
105 {
106     CTxMemPool& tx_mempool{MakeMempool()};
107+    auto miner{MakeMining()};

ryanofsky commented at 2:32 pm on January 2, 2025:

In commit “test: use Mining interface in miner_tests” (e4e76f27d73646d887ed8b660c67da4b4b785d8e)

In other tests below this is followed by BOOST_REQUIRE(miner);. Seems fine to keep or drop, but would be good to be consistent.

Also I think it might be a little clearer if variable was called mining instead of miner since it’s not a miner and can’t mine blocks, it’s just a pointer to the interface used for mining. But feel free to ignore this if you prefer the current name.

Sjors commented at 10:48 am on January 3, 2025:

Renamed.

in src/test/miner_tests.cpp:676 in e4e76f27d7 outdated

697+            block.hashMerkleRoot = BlockMerkleRoot(block);
698+            block.nNonce = bi.nonce;
699+        }
700+        std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(block);
701+        // Alternate calls between Chainman's ProcessNewBlock and submitSolution
702+        // via the Mining interface.

ryanofsky commented at 3:01 pm on January 2, 2025:

In commit “test: use Mining interface in miner_tests” (e4e76f27d73646d887ed8b660c67da4b4b785d8e)

Can this comment be updated to say the reason for alternating between these? (e.g. to provide test coverage for net_processing code)

I think it would also be clearer to drop the comments about this in the PR and commit descriptions:

“We mainly call the createNewBlock() method, but also getTip()->height and submitSolution(). Calls to the latter are alternated with direct calls to Chainman’s ProcessNewBlock, because e.g. net_processing.cpp uses that.”
“Use createNewBlock via the interface instead of using the BlockAssembler directly. The latter is always called via the interface in production code.”

Those comments were confusing to me without seeing context here. They also seem unrelated to all the other changes in this PR (unless I am not seeing a bigger connection). If those comments are just supposed to describe this if statement, it would be better to move them here.

Sjors commented at 10:48 am on January 3, 2025:

Ok, I dropped them for the commit and PR description and updated the code comment.

in src/test/miner_tests.cpp:683 in e4e76f27d7 outdated

707         }
708-        std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(*pblock);
709-        BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlock(shared_pblock, true, true, nullptr));
710-        pblock->hashPrevBlock = pblock->GetHash();
711+        // ProcessNewBlock and submitSolution do not check if the new block is
712+        // valid. If it is, they will wait for the tip to update. Therefore the

ryanofsky commented at 3:10 pm on January 2, 2025:

In commit “test: use Mining interface in miner_tests” (e4e76f27d73646d887ed8b660c67da4b4b785d8e)

Unless I’m misunderstanding something it doesn’t seem accurate to say that these don’t check if the new block is valid. Would seem more accurate to say they return true even if the block is invalid. Also it would seem be more accurate to say that they update the tip, rather than that they wait for the tip to update.

Sjors commented at 10:48 am on January 3, 2025:

I dropped this comment.

in src/test/miner_tests.cpp:686 in e4e76f27d7 outdated

710-        pblock->hashPrevBlock = pblock->GetHash();
711+        // ProcessNewBlock and submitSolution do not check if the new block is
712+        // valid. If it is, they will wait for the tip to update. Therefore the
713+        // following check will either return immediately, or be stuck indefinately.
714+        // It's mainly there to increase test coverage.
715+        miner->waitTipChanged(block.hashPrevBlock);

ryanofsky commented at 3:19 pm on January 2, 2025:

In commit “test: use Mining interface in miner_tests” (e4e76f27d73646d887ed8b660c67da4b4b785d8e)

Seems fine to call waitTipChange if idea is to add more test coverage for it, but wouldn’t it be more direct to just check that the new tip matches the block hash if the idea is to check that the block was connected? This way if the block was not connected the test would fail instead of hanging indefinitely.

Sjors commented at 10:48 am on January 3, 2025:

That makes sense, added an explicit check above the waitTipChanged() call.

ryanofsky approved

ryanofsky commented at 3:30 pm on January 2, 2025: contributor

Code review ACK e4e76f27d73646d887ed8b660c67da4b4b785d8e. Pretty straightforward test updates that add some more coverage. I left some comments below but none are very important.

test: use Mining interface in miner_tests 04249682e3

Sjors force-pushed on Jan 3, 2025

Sjors commented at 10:49 am on January 3, 2025: member

Rebased and addressed feedback.

ryanofsky approved

ryanofsky commented at 3:56 pm on January 3, 2025: contributor

Code review ACK 04249682e381f976de6ba56bb4fb2996dfa194ab, just minor suggested changes (renames, comments, BOOST_REQUIREs) since last review and some more extra clarifications and checks added to the CreateNewBlock_validity test. The CreateNewBlock_validity changes seem clear and easy to understand now.

in src/test/miner_tests.cpp:683 in 04249682e3

704+        // via the Mining interface. The former is used by net_processing as well
705+        // as the submitblock RPC.
706+        if (current_height % 2 == 0) {
707+            BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(shared_pblock, /*force_processing=*/true, /*min_pow_checked=*/true, nullptr));
708+        } else {
709+            BOOST_REQUIRE(block_template->submitSolution(block.nVersion, block.nTime, block.nNonce, MakeTransactionRef(txCoinbase)));

vasild commented at 6:41 pm on January 3, 2025:

Feel free to ignore: could use block.vtx[0] here instead of MakeTransactionRef(txCoinbase):

0            BOOST_REQUIRE(block_template->submitSolution(block.nVersion, block.nTime, block.nNonce, block.vtx[0]));

and reduce the scope of txCoinbase to only inside the above { } block.

Sjors commented at 7:42 pm on January 3, 2025:

Thanks, will consider if I need to retouch. Though I think txCoinbase is more clear than block.vtx[0] and limiting the scope isn’t that important.

vasild approved

vasild commented at 6:45 pm on January 3, 2025: contributor

ACK 04249682e381f976de6ba56bb4fb2996dfa194ab

Not in this PR, just an observation: given that we have two concepts: “block” and “block template”, I find the below pattern a bit confusing:

0block_template = mining->createNewBlock()
1block = block_template->getBlock()

because I would expect createNewBlock() to create a new block, but apparently it creates a new block template.

Sjors commented at 7:45 pm on January 3, 2025: member

because I would expect createNewBlock() to create a new block, but apparently it creates a new block template.

Indeed, I could rename the interface method (different PR) to e.g. newBlockTemplate(). Though I’d rather avoid the code churn in my pile of PRs :-)

in src/test/miner_tests.cpp:165 in 04249682e3

159@@ -158,11 +160,13 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const
160     tx.vout[0].nValue = 5000000000LL - 1000 - 50000 - feeToUse;
161     Txid hashLowFeeTx = tx.GetHash();
162     AddToMempool(tx_mempool, entry.Fee(feeToUse).FromTx(tx));
163-    pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock();
164+    block_template = mining->createNewBlock(options);
165+    BOOST_REQUIRE(block_template);
166+    block = block_template->getBlock();

tdb3 commented at 9:17 pm on January 3, 2025:

This pattern is repeated throughout the file. Unless I’m missing something, seems like a good opportunity to de-duplicate with something like:

0CBlock CreateAndGetBlock(const node::BlockCreateOptions& options)
1{
2    auto mining{MakeMining()};
3    BOOST_REQUIRE(mining);
4    std::unique_ptr<BlockTemplate> block_template = mining->createNewBlock(options);
5    BOOST_REQUIRE(block_template);
6    return block_template->getBlock();
7}

Sjors commented at 1:07 pm on January 4, 2025:

That makes sense. Will do if I need to retouch.

tdb3 approved

tdb3 commented at 9:21 pm on January 3, 2025: contributor

ACK 04249682e381f976de6ba56bb4fb2996dfa194ab

Nice updates. Left a minor non-blocking comment.

achow101 commented at 9:42 pm on January 6, 2025: member

ACK 04249682e381f976de6ba56bb4fb2996dfa194ab

achow101 merged this on Jan 6, 2025

achow101 closed this on Jan 6, 2025

in src/test/miner_tests.cpp:688 in 04249682e3

709+            BOOST_REQUIRE(block_template->submitSolution(block.nVersion, block.nTime, block.nNonce, MakeTransactionRef(txCoinbase)));
710+        }
711+        {
712+            LOCK(cs_main);
713+            // The above calls don't guarantee the tip is actually updated, so
714+            // we explictly check this.

ismaelsadeeq commented at 4:20 pm on January 7, 2025:

nit:

0src/test/miner_tests.cpp:704: explictly ==> explicitly

Sjors deleted the branch on Jan 8, 2025

Sjors commented at 10:18 am on January 8, 2025: member

I made a note in #31283 to look into both suggestions.

test: have miner_tests use Mining interface #31581

Code Coverage & Benchmarks

Reviews

Conflicts