Skip to content

consensus: poison bad-txns-inputvalues-outofrange and bad-txns-fee-outofrange#112

Closed
l0rinc wants to merge 1 commit intomasterfrom
detached478
Closed

consensus: poison bad-txns-inputvalues-outofrange and bad-txns-fee-outofrange#112
l0rinc wants to merge 1 commit intomasterfrom
detached478

Conversation

@l0rinc
Copy link
Owner

@l0rinc l0rinc commented Jan 31, 2026

To prove that they need unit tests, see: bitcoin#34469

@l0rinc l0rinc changed the title test: add case where TOTAL_TRIES is exceeded yet solution remains consensus: poison bad-txns-inputvalues-outofrange and bad-txns-fee-outofrange Jan 31, 2026
@l0rinc l0rinc closed this Jan 31, 2026
@l0rinc l0rinc reopened this Jan 31, 2026
@l0rinc l0rinc marked this pull request as ready for review February 1, 2026 11:35
sedited added a commit to bitcoin/bitcoin that referenced this pull request Feb 9, 2026
…h unit tests

8c03318 consensus/doc: explain `GetValueOut()` precondition (Lőrinc)
82ef92c consensus/doc: explain unreachable `bad-txns-fee-outofrange` check (Lőrinc)
232a2bc consensus/test: add out-of-range output unit tests for `CTransaction::GetValueOut` (Lőrinc)
aa87aae consensus/test: add `MoneyRange` unit tests for `CheckTxInputs` (Lőrinc)

Pull request description:

  ### 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"](https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/consensus/tx_verify.cpp.gcov.html#L180): Only covered in functional tests
  * ["bad-txns-inputvalues-outofrange"](https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/consensus/tx_verify.cpp.gcov.html#L187): Unreachable in functional tests [1], uncovered in unit tests
  * ["bad-txns-in-belowout"](https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/consensus/tx_verify.cpp.gcov.html#L193): Only covered in functional tests
  * ["GetValueOut: value out of range"](https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/primitives/transaction.cpp.gcov.html#L103) and [total coverage report](https://maflcko.github.io/b-c-cov/total.coverage/src/primitives/transaction.cpp.gcov.html#L103)

  Replacing them with explicit throws still passes all unit (and sometimes even functional) tests, confirming those branches are not being exercised, see: l0rinc#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.

ACKs for top commit:
  maflcko:
     lgtm ACK 8c03318 🍴
  darosior:
    utACK 8c03318
  sedited:
    ACK 8c03318

Tree-SHA512: 91c65dda99b42d12de99c58b02df0f861203f97d268329a3ecce79bd681fcaf847f508c1d9f2256b2b92a953a94d868cbae647f378def92484681d771722ea27
@l0rinc l0rinc closed this Feb 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant