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

1. 
   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.

2. 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
3. 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
4. DrahtBot added the label Consensus on Jan 31, 2026
5. 
   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 <!–meta-tag:bot-skip–> 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.]

   2026-02-02 18:32:56

6. 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!
7. 
   darosior commented at 4:23 pm on February 2, 2026: member

   Concept ACK
8. 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
9. 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
10. l0rinc force-pushed on Feb 2, 2026
11. l0rinc renamed this:
    consensus: unit test uncovered `CheckTxInputs` invalid cases
    consensus/test/doc: cover errors in `CheckTxInputs` with unit tests
    on Feb 2, 2026
12. darosior approved
13. 
    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

14. 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:

    0        // * `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.
15. 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:

    • https://maflcko.github.io/b-c-cov/total.coverage/src/consensus/tx_verify.cpp.gcov.html#L164
    • https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/consensus/tx_verify.cpp.gcov.html#L164

    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.

16. 
    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 🍴

    Signature:

    0untrusted 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}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: lgtm ACK 8c03318387f6b3d1520a539f426b300bae316fc3 🍴
    3pkxdcTbCEeEJRBkVNEIh6qrrYkiFVjoSH+8w6xrGt7rs5tb0EZjGy3FcwBW4Cxmr1hd2AGz9G2pzYoy5aVv6Bw==
    

17. sedited approved
18. 
    sedited commented at 12:02 pm on February 9, 2026: contributor

    ACK 8c03318387f6b3d1520a539f426b300bae316fc3
19. sedited merged this on Feb 9, 2026
20. sedited closed this on Feb 9, 2026

    ---

21. l0rinc deleted the branch on Feb 9, 2026

Contributors
l0rinc DrahtBot darosior maflcko sedited

Labels
Consensus