test: adds coverage to src/validation for invalid tx coinbase

kevkevinpal commented at 11:43 pm on April 11, 2025: contributor

Summary

This PR adds coverage to src/validation in the functional test suite when a transaction that is the coinbase is sent as a tx

This adds another tx to invalid_txs.py

How to check for coverage

This is only covered currently in mempool_accept.py

you can test this by modifying the coinbase message and then running the test suite and observing that only mempool_accept.py fails

You can also check that it is not covered by doing the following grep -nri "coinbase" ./test/functional/data/invalid_txs.py grep -nri "assert_raises.*coinbase" ./test/functional grep -nr "\"coinbase" ./test/functional

test: adds coverage to src/validation for invalid tx coinbase 5a2e00d616

DrahtBot commented at 11:43 pm on April 11, 2025: 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/32253.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

DrahtBot added the label Tests on Apr 11, 2025

maflcko commented at 6:59 am on April 12, 2025: member

According to https://maflcko.github.io/b-c-cov/total.coverage/src/validation.cpp.gcov.html it is covered

maflcko commented at 7:00 am on April 12, 2025: member

You can check that it is not covered by doing the following (I’m not 100% certain as there are a lot of instances of coinbase in the code) grep -nri "coinbase" ./test/functional/data/invalid_txs.py grep -nri "assert_raises.*coinbase" ./test/functional grep -nr "\"coinbase" ./test/functional

A better way to check that a piece of code is covered is simply adding an assert(false);, then run the tests. This will then also surface which exact test is covering it.

kevkevinpal commented at 2:39 pm on April 12, 2025: contributor

I tried adding assert(false); but it seemed to break most tests but when modifying the coinbase message I saw errors in

mempool_accept.py

but that seems like the only place, not sure if it makes sense to add this coinbase tx to invalid_txs.py or if it is preferable to have it only tested in mempool_accept.py

maflcko commented at 3:12 pm on April 12, 2025: member

I tried adding assert(false); but it seemed to break most tests but when modifying the coinbase message I saw errors in

That seems odd. According to the coverage report it is covered twice, so at most two tests could fail. What was your diff?

arejula27 commented at 9:01 am on April 14, 2025: none

I’ll try to run it and check the difference in the coverage, but as @maflcko pointed out, the code already appears to be fully covered in the report

I would like to know where your new IsCoinbase template is used, I am unfamiliar with this file.

kevkevinpal commented at 11:47 pm on April 14, 2025: contributor

I tried adding assert(false); but it seemed to break most tests but when modifying the coinbase message I saw errors in

That seems odd. According to the coverage report it is covered twice, so at most two tests could fail. What was your diff?

This is the diff

 0(master) bitcoin $ git diff
 1diff --git a/src/validation.cpp b/src/validation.cpp
 2index aa1effb736..c74217ef68 100644
 3--- a/src/validation.cpp
 4+++ b/src/validation.cpp
 5@@ -786,7 +786,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
 6
 7     // Coinbase is only valid in a block, not as a loose transaction
 8     if (tx.IsCoinBase())
 9-        return state.Invalid(TxValidationResult::TX_CONSENSUS, "coinbase");
10+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "test123");
11
12     // Rather not work on nonstandard transactions (unless -testnet/-regtest)
13     std::string reason;

and this is the only test that fails

 0223/318 - mempool_accept.py failed, Duration: 4 s
 1
 2stdout:
 32025-04-14T23:44:58.304000Z TestFramework (INFO): PRNG seed is: 6232609879919670956
 42025-04-14T23:44:58.304000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20250414_194238/mempool_accept_101
 52025-04-14T23:44:58.606000Z TestFramework (INFO): Start with empty mempool, and 200 blocks
 62025-04-14T23:44:58.609000Z TestFramework (INFO): Should not accept garbage to testmempoolaccept
 72025-04-14T23:44:58.615000Z TestFramework (INFO): A transaction already in the blockchain
 82025-04-14T23:44:58.629000Z TestFramework (INFO): A transaction not in the mempool
 92025-04-14T23:44:58.633000Z TestFramework (INFO): A final transaction not in the mempool
102025-04-14T23:44:58.638000Z TestFramework (INFO): A transaction in the mempool
112025-04-14T23:44:58.641000Z TestFramework (INFO): A transaction that replaces a mempool transaction
122025-04-14T23:44:58.646000Z TestFramework (INFO): A transaction with missing inputs, that never existed
132025-04-14T23:44:58.649000Z TestFramework (INFO): A transaction with missing inputs, that existed once in the past
142025-04-14T23:44:58.668000Z TestFramework (INFO): Create a "reference" tx for later use
152025-04-14T23:44:58.672000Z TestFramework (INFO): A transaction with no outputs
162025-04-14T23:44:58.674000Z TestFramework (INFO): A really large transaction
172025-04-14T23:45:01.913000Z TestFramework (INFO): A transaction with negative output value
182025-04-14T23:45:01.915000Z TestFramework (INFO): A transaction with too large output value
192025-04-14T23:45:01.916000Z TestFramework (INFO): A transaction with too large sum of output values
202025-04-14T23:45:01.916000Z TestFramework (INFO): A transaction with duplicate inputs
212025-04-14T23:45:01.917000Z TestFramework (INFO): A non-coinbase transaction with coinbase-like outpoint
222025-04-14T23:45:01.917000Z TestFramework (INFO): A coinbase transaction
232025-04-14T23:45:01.918000Z TestFramework (ERROR): Assertion failed
24Traceback (most recent call last):
25  File "/mnt/shared_drive/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 182, in main
26    self.run_test()
27  File "/mnt/shared_drive/DEVDIR/bitcoin/build_dir/test/functional/mempool_accept.py", line 277, in run_test
28    self.check_mempool_result(
29  File "/mnt/shared_drive/DEVDIR/bitcoin/build_dir/test/functional/mempool_accept.py", line 73, in check_mempool_result
30    assert_equal(result_expected, result_test)
31  File "/mnt/shared_drive/DEVDIR/bitcoin/test/functional/test_framework/util.py", line 77, in assert_equal
32    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
33AssertionError: not([{'txid': 'ff07cc811d2826c6f5a6386dce61cdd3adfcdbacad4de26f3024d68d6dc16d13', 'allowed': False, 'reject-reason': 'coinbase'}] == [{'txid': 'ff07cc811d2826c6f5a6386dce61cdd3adfcdbacad4de26f3024d68d6dc16d13', 'allowed': False, 'reject-reason': 'test123'}])
342025-04-14T23:45:01.972000Z TestFramework (INFO): Not stopping nodes as test failed. The dangling processes will be cleaned up later.
352025-04-14T23:45:01.972000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_₿_🏃_20250414_194238/mempool_accept_101
362025-04-14T23:45:01.972000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_₿_🏃_20250414_194238/mempool_accept_101/test_framework.log
372025-04-14T23:45:01.972000Z TestFramework (ERROR):
382025-04-14T23:45:01.974000Z TestFramework (ERROR): Hint: Call /mnt/shared_drive/DEVDIR/bitcoin/test/functional/combine_logs.py '/tmp/test_runner_₿_🏃_20250414_194238/mempool_accept_101' to consolidate all logs
392025-04-14T23:45:01.974000Z TestFramework (ERROR):
402025-04-14T23:45:01.974000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
412025-04-14T23:45:01.974000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
422025-04-14T23:45:01.974000Z TestFramework (ERROR):
43
44
45stderr:
46[node 0] Cleaning up leftover process

kevkevinpal commented at 11:51 pm on April 14, 2025: contributor

I’ll try to run it and check the difference in the coverage, but as @maflcko pointed out, the code already appears to be fully covered in the report

I would like to know where your new IsCoinbase template is used, I am unfamiliar with this file.

Thank you for the review!

The IsCoinbase template is defined in ./test/functional/data/invalid_txs.py and that file is used in ./test/functional/p2p_invalid_tx.py.

The way p2p_invalid_tx.py uses the templates is that it loops through an asserts the reject-reason is correct in the logs

kevkevinpal commented at 11:57 pm on April 14, 2025: contributor

I do have some tests that are being skipped due to –legacy-wallet being used let me see if any of those maybe are also covering

maflcko commented at 6:43 am on April 15, 2025: member

I tried adding assert(false); but it seemed to break most tests

The line is covered twice, and at least for me it breaks exactly only two tests (one unit and one functional test).

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index aa1effb736..39744c0516 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -785,8 +785,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
 5     }
 6 
 7     // Coinbase is only valid in a block, not as a loose transaction
 8-    if (tx.IsCoinBase())
 9+    if (tx.IsCoinBase())     {assert(false);
10         return state.Invalid(TxValidationResult::TX_CONSENSUS, "coinbase");
11+        }
12 
13     // Rather not work on nonstandard transactions (unless -testnet/-regtest)
14     std::string reason;

test: adds coverage to src/validation for invalid tx coinbase #32253

Summary

How to check for coverage

Code Coverage & Benchmarks

Reviews