test: added coverage to mining_basic.py #27603

pull kevkevinpal wants to merge 1 commits into bitcoin:master from kevkevinpal:test/mining/emptyBlockVtx changing 1 files +1 −0

kevkevinpal commented at 1:53 AM on May 9, 2023: contributor

Included a test that checks if we call submitblock with block.vtx.empty() then it throws an rpc deserialization error, currently we only test if !block.vtx->IsCoinBase() throws an rpc deserialization error

I've tested to make sure this actually doing what I intended by breaking up this if block into two if blocks with different error messages and running the functional test https://github.com/bitcoin/bitcoin/blob/322ec63b01499c1ec52d3912ee382ebd59f2366b/src/rpc/mining.cpp#L963

This change should increase the test coverage for the submitblock() rpc in ./src/rpc/mining.cpp
DrahtBot commented at 1:54 AM on May 9, 2023: contributor



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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK theStack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Tests on May 9, 2023
kevkevinpal force-pushed on May 9, 2023
kevkevinpal force-pushed on May 9, 2023
fanquake requested review from theStack on May 10, 2023

Type	Reviewers
ACK	theStack

in test/functional/mining_basic.py:134 in 75ecfc3ae6 outdated

 126 | @@ -127,6 +127,12 @@ def assert_submitblock(block, result_str_1, result_str_2=None):
 127 |          block.nTime = tmpl["curtime"]
 128 |          block.nBits = int(tmpl["bits"], 16)
 129 |          block.nNonce = 0
 130 | +
 131 | +        bad_block_empty_vtx = copy.deepcopy(block)
 132 | +
 133 | +        self.log.info("submitblock: Test block decode failure with empty vtx")
 134 | +        assert_raises_rpc_error(-22, "Block does not start with a coinbase", node.submitblock, bad_block_empty_vtx.serialize().hex())

theStack commented at 12:31 PM on May 10, 2023:

nit: could just directly serialize a newly created temporary CBlock() instance, instead of creating a new variable

        self.log.info("submitblock: Test block decode failure with empty vtx")
        assert_raises_rpc_error(-22, "Block does not start with a coinbase", node.submitblock, CBlock().serialize().hex())

Would also suggest to move this down to the test checking the other condition for the same error message ("submitblock: Test invalid coinbase transaction").

kevkevinpal commented at 12:52 PM on May 10, 2023:

FIxed in a7b46a1

Thanks for the suggestion!

theStack commented at 12:32 PM on May 10, 2023: contributor

Concept ACK

test: added coverage to mining_basic.py

Included a test that checks if we call submitblock with
block.vtx.empty() then it throws an rpc deserialization error, currently
we only test if !block.vtx->IsCoinBase() throws an rpc deserialization
error

a7b46a1fea

kevkevinpal force-pushed on May 10, 2023
theStack approved
theStack commented at 1:13 PM on May 10, 2023: contributor

ACK a7b46a1feae60e38fe4bdcacf5034f44cae49222
glozow merged this on Jun 2, 2023
glozow closed this on Jun 2, 2023
sidhujag referenced this in commit d4acec82ca on Jun 2, 2023
bitcoin deleted a comment on Jun 4, 2023
bitcoin locked this on Jun 3, 2024

Contributors

Labels

test: added coverage to mining_basic.py #27603

Reviews