cryptogera/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-03-02 17:56:16 +00:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
8c03318387f6b3d1520a539f426b300bae316fc3 consensus/doc: explain `GetValueOut()` precondition (Lőrinc)
82ef92c8d006b3f5c3baaf00e5f8200d289d85d2 consensus/doc: explain unreachable `bad-txns-fee-outofrange` check (Lőrinc)
232a2bce90a96720f5c8d31413f1d14b4c9d90f2 consensus/test: add out-of-range output unit tests for `CTransaction::GetValueOut` (Lőrinc)
aa87aae14f9eee79e3a0fb9c0f5ff3eaa97433e2 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: 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.

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

Tree-SHA512: 91c65dda99b42d12de99c58b02df0f861203f97d268329a3ecce79bd681fcaf847f508c1d9f2256b2b92a953a94d868cbae647f378def92484681d771722ea27
This commit is contained in:
merge-script 2026-02-09 13:13:19 +01:00
commit acefdce083
No known key found for this signature in database
GPG Key ID: 9B79B45691DB4173
2 changed files with 59 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
src/consensus/tx_verify.cpp
View File

@ -188,6 +188,10 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state,
}
}
// `tx.GetValueOut()` won't throw in validation paths because output-range checks run first
// (`bad-txns-vout-negative`, `bad-txns-vout-toolarge`, `bad-txns-txouttotal-toolarge`):
// * `MemPoolAccept::PreChecks`: `CheckTransaction()` is called before this method;
// * `Chainstate::ConnectBlock`: `CheckTransaction()` is called via `CheckBlock()` before this method.
const CAmount value_out = tx.GetValueOut();
if (nValueIn < value_out) {
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-in-belowout",
@ -197,6 +201,11 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state,
// Tally transaction fees
const CAmount txfee_aux = nValueIn - value_out;
if (!MoneyRange(txfee_aux)) {
// Unreachable, given the following preconditions:
// * `value_out` comes from `tx.GetValueOut()`, which throws unless `MoneyRange(value_out)` and asserts `MoneyRange(nValueOut)` on return.
// * `MoneyRange(nValueIn)` was enforced in the input loop.
// * `nValueIn < value_out` was handled above, so `nValueIn >= value_out` here (and `txfee_aux >= 0`).
// Therefore `0 <= txfee_aux = nValueIn - value_out <= nValueIn <= MAX_MONEY`.
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-fee-outofrange");
}

50
src/test/transaction_tests.cpp
View File

@ -9,6 +9,7 @@
#include <checkqueue.h>
#include <clientversion.h>
#include <consensus/amount.h>
#include <consensus/consensus.h>
#include <consensus/tx_check.h>
#include <consensus/tx_verify.h>
#include <consensus/validation.h>
@ -1113,6 +1114,55 @@ BOOST_AUTO_TEST_CASE(max_standard_legacy_sigops)
BOOST_CHECK(!::AreInputsStandard(CTransaction(tx_max_sigops), coins));
}
BOOST_AUTO_TEST_CASE(checktxinputs_invalid_transactions_test)
{
auto check_invalid{[](CAmount input_value, CAmount output_value, bool coinbase, int spend_height, TxValidationResult expected_result, std::string_view expected_reason) {
CCoinsView coins_dummy;
CCoinsViewCache inputs(&coins_dummy);
const COutPoint prevout{Txid::FromUint256(uint256::ONE), 0};
inputs.AddCoin(prevout, Coin{{input_value, CScript() << OP_TRUE}, /*nHeightIn=*/1, coinbase}, /*possible_overwrite=*/false);
CMutableTransaction mtx;
mtx.vin.emplace_back(prevout);
mtx.vout.emplace_back(output_value, CScript() << OP_TRUE);
TxValidationState state;
CAmount txfee{0};
BOOST_CHECK(!Consensus::CheckTxInputs(CTransaction{mtx}, state, inputs, spend_height, txfee));
BOOST_CHECK(state.IsInvalid());
BOOST_CHECK_EQUAL(state.GetResult(), expected_result);
BOOST_CHECK_EQUAL(state.GetRejectReason(), expected_reason);
}};
check_invalid(/*input_value=*/MAX_MONEY + 1,
/*output_value=*/0,
/*coinbase=*/false,
/*spend_height=*/2,
TxValidationResult::TX_CONSENSUS, /*expected_reason=*/"bad-txns-inputvalues-outofrange");
check_invalid(/*input_value=*/1 * COIN,
/*output_value=*/2 * COIN,
/*coinbase=*/false,
/*spend_height=*/2,
TxValidationResult::TX_CONSENSUS, /*expected_reason=*/"bad-txns-in-belowout");
check_invalid(/*input_value=*/1 * COIN,
/*output_value=*/0,
/*coinbase=*/true,
/*spend_height=*/COINBASE_MATURITY,
TxValidationResult::TX_PREMATURE_SPEND, /*expected_reason=*/"bad-txns-premature-spend-of-coinbase");
}
BOOST_AUTO_TEST_CASE(getvalueout_out_of_range_throws)
{
CMutableTransaction mtx;
mtx.vout.emplace_back(MAX_MONEY + 1, CScript() << OP_TRUE);
const CTransaction tx{mtx};
BOOST_CHECK_EXCEPTION(tx.GetValueOut(), std::runtime_error, HasReason("GetValueOut: value out of range"));
}
/** Sanity check the return value of SpendsNonAnchorWitnessProg for various output types. */
BOOST_AUTO_TEST_CASE(spends_witness_prog)
{