highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-06-10 14:48:46 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#35456: test: Perform full reset of CoinsResult in order to avoid passing 21M BTC

Browse Source 
d0b76c7f3e rpc+bitcoin-tx: Specify correct type for ParseFixedPoint() (Hodlinator)
43ca54ca00 refactor(test): Make CAmount arg explicit for BuildCreditingTransaction() (Hodlinator)
b5e91e946c wallet: Remove CoinsResult::Clear() (Hodlinator)

Pull request description:

  The *knapsack_solver_test* in *coinselector_tests.cpp* was accumulating satoshi amounts beyond 21M BTC. This was uncovered while experimenting with adding checks to `CAmount`. Fix that by fully resetting the `CoinsResult` object accumulating those amounts, inspired by https://github.com/bitcoin/bitcoin/issues/35449#issuecomment-4613968627.

  Also, while we're at it, add 2 commits which correct some `int64_t`/`CAmount` confusion.

  Fixes https://github.com/bitcoin/bitcoin/issues/35449

ACKs for top commit:
  sedited:
    ACK d0b76c7f3e
  furszy:
    utACK d0b76c7f3e
  brunoerg:
    code review ACK d0b76c7f3e

Tree-SHA512: 6d989ded6f6327dc657f437dc256d4adf42a34a1252621421ee38d7851c6cdc97a462f033a4728e3aa7d5514deee4db6e83646105633f9cf7ed6e7e90406b67d
This commit is contained in:
merge-script
2026-06-08 21:59:58 +02:00
parent 1d3bc816c3 d0b76c7f3e
commit e36f5d5d0b
7 changed files with 15 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
src/bitcoin-tx.cpp
View File

@@ -556,7 +556,7 @@ static CAmount AmountFromValue(const UniValue& value)
{
if (!value.isNum() && !value.isStr())
throw std::runtime_error("Amount is not a number or string");
CAmount amount;
int64_t amount;
if (!ParseFixedPoint(value.getValStr(), 8, &amount))
throw std::runtime_error("Invalid amount");
if (!MoneyRange(amount))

2
src/rpc/util.cpp
View File

@@ -99,7 +99,7 @@ CAmount AmountFromValue(const UniValue& value, int decimals)
{
if (!value.isNum() && !value.isStr())
throw JSONRPCError(RPC_TYPE_ERROR, "Amount is not a number or string");
CAmount amount;
int64_t amount;
if (!ParseFixedPoint(value.getValStr(), decimals, &amount))
throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount");
if (!MoneyRange(amount))

2
src/test/util/transaction_utils.cpp
View File

@@ -7,7 +7,7 @@
#include <script/signingprovider.h>
#include <test/util/transaction_utils.h>
CMutableTransaction BuildCreditingTransaction(const CScript& scriptPubKey, int nValue)
CMutableTransaction BuildCreditingTransaction(const CScript& scriptPubKey, CAmount nValue)
{
CMutableTransaction txCredit;
txCredit.version = 1;

2
src/test/util/transaction_utils.h
View File

@@ -15,7 +15,7 @@ class CCoinsViewCache;
// create crediting transaction
// [1 coinbase input => 1 output with given scriptPubkey and value]
CMutableTransaction BuildCreditingTransaction(const CScript& scriptPubKey, int nValue = 0);
CMutableTransaction BuildCreditingTransaction(const CScript& scriptPubKey, CAmount nValue = 0);
// create spending transaction
// [1 input with referenced transaction outpoint, scriptSig, scriptWitness =>

4
src/wallet/spend.cpp
View File

@@ -210,10 +210,6 @@ std::vector<COutput> CoinsResult::All() const
return all;
}
void CoinsResult::Clear() {
coins.clear();
}
void CoinsResult::Erase(const std::unordered_set<COutPoint, SaltedOutpointHasher>& coins_to_remove)
{
for (auto& [type, vec] : coins) {

3
src/wallet/spend.h
View File

@@ -40,7 +40,7 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* walle
* This struct is really just a wrapper around OutputType vectors with a convenient
* method for concatenating and returning all COutputs as one vector.
*
* Size(), Clear(), Erase(), Shuffle(), and Add() methods are implemented to
* Size(), Erase(), Shuffle(), and Add() methods are implemented to
* allow easy interaction with the struct.
*/
struct CoinsResult {
@@ -54,7 +54,6 @@ struct CoinsResult {
size_t Size() const;
/** Return how many different output types this struct stores */
size_t TypesCount() const { return coins.size(); }
void Clear();
void Erase(const std::unordered_set<COutPoint, SaltedOutpointHasher>& coins_to_remove);
void Shuffle(FastRandomContext& rng_fast);
void Add(OutputType type, const COutput& out);

20
src/wallet/test/coinselector_tests.cpp
View File

@@ -203,7 +203,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
BOOST_CHECK(!SelectCoinsBnB(GroupCoins(available_coins.All()), 1 * CENT, coin_selection_params_bnb.m_cost_of_change));
// Test fees subtracted from output:
available_coins.Clear();
available_coins = {};
add_coin(available_coins, *wallet, 1 * CENT, coin_selection_params_bnb.m_effective_feerate);
available_coins.All().at(0).input_bytes = 40;
const auto result9 = SelectCoinsBnB(GroupCoins(available_coins.All()), 1 * CENT, coin_selection_params_bnb.m_cost_of_change);
@@ -352,7 +352,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
// test multiple times to allow for differences in the shuffle order
for (int i = 0; i < RUN_TESTS; i++)
{
available_coins.Clear();
available_coins = {};
// with an empty wallet we can't even pay one cent
BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 1 * CENT, CENT));
@@ -421,7 +421,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
BOOST_CHECK_EQUAL(result8->GetInputSet().size(), 1U);
// now clear out the wallet and start again to test choosing between subsets of smaller coins and the next biggest coin
available_coins.Clear();
available_coins = {};
add_coin(available_coins, *wallet, 6*CENT);
add_coin(available_coins, *wallet, 7*CENT);
@@ -479,7 +479,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
// empty the wallet and start again, now with fractions of a cent, to test small change avoidance
available_coins.Clear();
available_coins = {};
add_coin(available_coins, *wallet, CENT * 1 / 10);
add_coin(available_coins, *wallet, CENT * 2 / 10);
add_coin(available_coins, *wallet, CENT * 3 / 10);
@@ -511,7 +511,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
// run the 'mtgox' test (see https://blockexplorer.com/tx/29a3efd3ef04f9153d47a990bd7b048a4b2d213daaa5fb8ed670fb85f13bdbcf)
// they tried to consolidate 10 50k coins into one 500k coin, and ended up with 50k in change
available_coins.Clear();
available_coins = {};
for (int j = 0; j < 20; j++)
add_coin(available_coins, *wallet, 50000 * COIN);
@@ -524,7 +524,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
// we need to try finding an exact subset anyway
// sometimes it will fail, and so we use the next biggest coin:
available_coins.Clear();
available_coins = {};
add_coin(available_coins, *wallet, CENT * 5 / 10);
add_coin(available_coins, *wallet, CENT * 6 / 10);
add_coin(available_coins, *wallet, CENT * 7 / 10);
@@ -535,7 +535,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
BOOST_CHECK_EQUAL(result20->GetInputSet().size(), 1U);
// but sometimes it's possible, and we use an exact subset (0.4 + 0.6 = 1.0)
available_coins.Clear();
available_coins = {};
add_coin(available_coins, *wallet, CENT * 4 / 10);
add_coin(available_coins, *wallet, CENT * 6 / 10);
add_coin(available_coins, *wallet, CENT * 8 / 10);
@@ -546,7 +546,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
BOOST_CHECK_EQUAL(result21->GetInputSet().size(), 2U); // in two coins 0.4+0.6
// test avoiding small change
available_coins.Clear();
available_coins = {};
add_coin(available_coins, *wallet, CENT * 5 / 100);
add_coin(available_coins, *wallet, CENT * 1);
add_coin(available_coins, *wallet, CENT * 100);
@@ -566,7 +566,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
// test with many inputs
for (CAmount amt=1500; amt < COIN; amt*=10) {
available_coins.Clear();
available_coins = {};
// Create 676 inputs (= (old MAX_STANDARD_TX_SIZE == 100000) / 148 bytes per input)
for (uint16_t j = 0; j < 676; j++)
add_coin(available_coins, *wallet, amt);
@@ -592,7 +592,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
// test randomness
{
available_coins.Clear();
available_coins = {};
for (int i2 = 0; i2 < 100; i2++)
add_coin(available_coins, *wallet, COIN);