consensus/test/doc: cover errors in `CheckTxInputs` with unit tests #34469

l0rinc commented at 8:14 PM on January 31, 2026: contributor

Problem

Coverage reports indicate that a few consensus related validations aren't exercised in unit-, and some not even in the functional-tests: Inspired by the coverage reports:

"bad-txns-premature-spend-of-coinbase": Only covered in functional tests
"bad-txns-inputvalues-outofrange": Unreachable in functional tests [1], uncovered in unit tests
"bad-txns-in-belowout": Only covered in functional tests
"GetValueOut: value out of range" and total coverage report

Replacing them with explicit throws still passes all unit (and sometimes even functional) tests, confirming those branches are not being exercised, see: https://github.com/l0rinc/bitcoin/pull/112

Fixes

Add minimal unit test coverage for Consensus::CheckTxInputs invalid outcomes for bad-txns-premature-spend-of-coinbase, bad-txns-inputvalues-outofrange, bad-txns-in-belowout. Add a unit test covering CTransaction::GetValueOut() throwing for out of range values. After the prerequisits are tested, document why bad-txns-fee-outofrange is unreachable - while keeping the check in place because it is consensus-critical code.

consensus/test: add `MoneyRange` unit tests for `CheckTxInputs`

Add minimal unit tests exercising `Consensus::CheckTxInputs` reject reasons for coinbase maturity (`bad-txns-premature-spend-of-coinbase`), input value range failures (`bad-txns-inputvalues-outofrange`), and for `nValueIn < value_out` (`bad-txns-in-belowout`).

Inspired by b-c-cov coverage reports:
* "bad-txns-premature-spend-of-coinbase" - https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/consensus/tx_verify.cpp.gcov.html#L180
* "bad-txns-inputvalues-outofrange" - https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/consensus/tx_verify.cpp.gcov.html#L187
* "bad-txns-in-belowout" - https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/consensus/tx_verify.cpp.gcov.html#L193

aa87aae14f

consensus/test: add out-of-range output unit tests for `CTransaction::GetValueOut`

Inspired by b-c-cov coverage reports:
* "GetValueOut: value out of range" - https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/primitives/transaction.cpp.gcov.html#L103

232a2bce90

DrahtBot added the label Consensus on Jan 31, 2026

DrahtBot commented at 8:15 PM on January 31, 2026: 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	darosior, maflcko, sedited

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

nValueOut -> value_out [Inconsistent variable name; the comment refers to MoneyRange(nValueOut) but earlier uses value_out, which could confuse readers about which variable is asserted.]

in src/consensus/tx_verify.cpp:200 in 947170381a

 196 | @@ -197,6 +197,11 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state,
 197 |      // Tally transaction fees
 198 |      const CAmount txfee_aux = nValueIn - value_out;
 199 |      if (!MoneyRange(txfee_aux)) {
 200 | +        // Unreachable, given the following invariants:

darosior commented at 4:23 PM on February 2, 2026:

While you are at it, could you also document that GetValueOut() above may throw but never will here because CheckTxInputs() is always called after CheckTransaction()?

l0rinc commented at 6:33 PM on February 2, 2026:

Done in a separate commit with explanation, added you as coauthor: https://github.com/bitcoin/bitcoin/compare/947170381a6317fe3146764383ba13abc8d2bb60..8c03318387f6b3d1520a539f426b300bae316fc3

darosior commented at 3:27 PM on February 4, 2026:

Not a big deal but for the record i don't think i should be co-author of this change, since i didn't author but merely suggested it.

l0rinc commented at 4:56 PM on February 4, 2026:

If you insist, I can remove, but I often add anyone who contributed to the change - and you definitely did. Thanks for the review.

darosior commented at 6:27 PM on February 4, 2026:

No need if you don't have to retouch!

darosior commented at 4:23 PM on February 2, 2026: member

Concept ACK

consensus/doc: explain unreachable `bad-txns-fee-outofrange` check

After the previous conditions were validated, document why `bad-txns-fee-outofrange` is unreachable once `nValueIn` and `value_out` are `MoneyRange` and `nValueIn >= value_out`.

Although unreachable, keep the check itself in place (instead of removing) as it's part of consensus-critical code; the comment serves as a proof for future refactors.

Inspired by b-c-cov coverage reports:
* "bad-txns-fee-outofrange" - https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/consensus/tx_verify.cpp.gcov.html#L200

82ef92c8d0

consensus/doc: explain `GetValueOut()` precondition

`Consensus::CheckTxInputs` calls `tx.GetValueOut()` and assumes output-range checks already ran.
Document the mempool and block validation call paths where this is guaranteed.

Co-authored-by: Antoine Poinsot <darosior@protonmail.com>

8c03318387

l0rinc force-pushed on Feb 2, 2026

l0rinc renamed this:
~~consensus: unit test uncovered `CheckTxInputs` invalid cases~~
consensus/test/doc: cover errors in `CheckTxInputs` with unit tests
on Feb 2, 2026

darosior approved

darosior commented at 3:27 PM on February 4, 2026: member

As far as i can tell the only added test coverage here is for a check that is unreachable in normal conditions. Removing this check would be an awful risk/reward tradeoff. Since we keep the check in place i think it's good to have some basic test coverage for it, which can only happen in unit tests.

Since we are adding coverage for this check, i suppose it does not hurt to also cover the other checks that weren't covered in unit tests even though they are already covered through functional tests.

And i think it's good to explicitly document why a function call that may throw an exception is guaranteed not to in production code paths.

utACK 8c03318387f6b3d1520a539f426b300bae316fc3

in src/consensus/tx_verify.cpp:205 in 8c03318387

 200 | @@ -197,6 +201,11 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state,
 201 |      // Tally transaction fees
 202 |      const CAmount txfee_aux = nValueIn - value_out;
 203 |      if (!MoneyRange(txfee_aux)) {
 204 | +        // Unreachable, given the following preconditions:
 205 | +        // * `value_out` comes from `tx.GetValueOut()`, which throws unless `MoneyRange(value_out)` and asserts `MoneyRange(nValueOut)` on return.

maflcko commented at 8:37 AM on February 5, 2026:

        // * `MoneyRange(value_out)` was enforced by `tx.GetValueOut()`.

nit: I don't think we need to list the full implementation details of GetValueOut. I know it is unlikely to change, but a small comment should be enough, and anyone curious would have to read the actual function anyway.

maflcko commented at 9:06 AM on February 5, 2026:

Also, it would be easier to review by putting the last commit before the second-to-last commit

l0rinc commented at 9:43 AM on February 5, 2026:

I have rephrased this a thousand times, even deleted the details at one point since I felt I was just repeating the obvious. My understanding is that this won't likely change in the future (and when it does we probably want to add extra weight to it). If I need to retouch this, I will consider the simpler version, thank you.

in src/test/transaction_tests.cpp:1 in 232a2bce90 outdated

maflcko commented at 9:09 AM on February 5, 2026:

nit in 232a2bce90a96720f5c8d31413f1d14b4c9d90f2: This is also missing in the "total" coverage report (unit+fun tests). Could refer to that, but up to you. https://maflcko.github.io/b-c-cov/total.coverage/src/primitives/transaction.cpp.gcov.html#L103

maflcko commented at 9:17 AM on February 5, 2026:

nit in aa87aae14f9eee79e3a0fb9c0f5ff3eaa97433e2: Could make sense to just link to the function once for unit tests and once for functional tests:

And then list the coverage:

"bad-txns-premature-spend-of-coinbase": Only covered in functional tests
"bad-txns-inputvalues-outofrange": Unreachable in functional tests [1], uncovered in unit tests
"bad-txns-in-belowout": Only covered in functional tests

[1] Maybe with a database corruption? Would be fun to maybe try to cover this.

l0rinc commented at 9:45 AM on February 5, 2026:

Thanks, added it to the PR description

l0rinc commented at 9:51 AM on February 5, 2026:

list the coverage

Thanks, added it to the PR description.

Could make sense to just link to the function once

I wanted to simplify the job of reviewers and linked to the exact lines - it seemed easier for reviewers to discard information than find it.

Maybe with a database corruption? Would be fun to maybe try to cover this.

That would be a pretty cool functional test, it would likely have to pass crc and deserialization and other checks to even have a chance of getting there. I will consider it in the future, good idea, don't think it's urgent though.

maflcko commented at 9:21 AM on February 5, 2026: member

Left some nits on the commit messages. Feel free to ignore those. Though, I'd be happy to re-ACK a re-order of the commits, which makes review easier and also avoids listing the implementation details of GetValueOut in a comment.

I think the 3rd commit should come last.

Otherwise:

lgtm ACK 8c03318387f6b3d1520a539f426b300bae316fc3 🍴

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 8c03318387f6b3d1520a539f426b300bae316fc3 🍴
pkxdcTbCEeEJRBkVNEIh6qrrYkiFVjoSH+8w6xrGt7rs5tb0EZjGy3FcwBW4Cxmr1hd2AGz9G2pzYoy5aVv6Bw==

</details>

sedited approved

sedited commented at 12:02 PM on February 9, 2026: contributor

ACK 8c03318387f6b3d1520a539f426b300bae316fc3

sedited merged this on Feb 9, 2026

sedited closed this on Feb 9, 2026

l0rinc deleted the branch on Feb 9, 2026