mirrors/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-20 14:05:23 +01:00
Code Issues Projects Releases Wiki Activity

Merge bitcoin/bitcoin#29272: wallet: fix coin selection tracing to return -1 when no change pos

Browse source 
d55fdb1a49 Move TRACEx parameters to seperate lines (Richard Myers)
2d58629ee6 wallet: fix coin selection tracing to return -1 when no change pos (Richard Myers)

Pull request description:

  This is a bugfix for from when [optional was introduced](758501b713)  for `change_pos` in the wallet. When optional `change_pos` is unset, we should return -1 and not 0.

  I added two new checks to the `test/functional/interface_usdt_coinselection.py` which adds coverage for the situations when `normal_create_tx_internal` and `aps_create_tx_internal` events occur with no change.

  You can reproduce this bug using the coin-selection-simulation scripts as described in [issue #16](https://github.com/achow101/coin-selection-simulation/issues/16). You can also run the `interface_usdt_coinselection.py` test  without the changes to `wallet/spend.cpp`.

ACKs for top commit:
  0xB10C:
    ACK d55fdb1a49
  achow101:
    ACK d55fdb1a49
  murchandamus:
    ACK d55fdb1a49

Tree-SHA512: 6efac3b756bdf51debbcb759dc3c4b7a4304626bc047b70025cec02f3a04937ace7712e9558ac71e560fd136005a98c518ac5bb4b90c3282d776beccd0de9749
This commit is contained in:
Ava Chow 2024-01-23 14:20:39 -05:00
commit 7cb7759b25
No known key found for this signature in database
GPG key ID: 17565732E08E5E41
2 changed files with 40 additions and 5 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

22
src/wallet/spend.cpp
View file

@ -1127,7 +1127,12 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
return util::Error{err.empty() ?_("Insufficient funds") : err};
}
const SelectionResult& result = *select_coins_res;
TRACE5(coin_selection, selected_coins, wallet.GetName().c_str(), GetAlgorithmName(result.GetAlgo()).c_str(), result.GetTarget(), result.GetWaste(), result.GetSelectedValue());
TRACE5(coin_selection, selected_coins,
wallet.GetName().c_str(),
GetAlgorithmName(result.GetAlgo()).c_str(),
result.GetTarget(),
result.GetWaste(),
result.GetSelectedValue());
const CAmount change_amount = result.GetChange(coin_selection_params.min_viable_change, coin_selection_params.m_change_fee);
if (change_amount > 0) {
@ -1336,8 +1341,11 @@ util::Result<CreatedTransactionResult> CreateTransaction(
LOCK(wallet.cs_wallet);
auto res = CreateTransactionInternal(wallet, vecSend, change_pos, coin_control, sign);
TRACE4(coin_selection, normal_create_tx_internal, wallet.GetName().c_str(), bool(res),
res ? res->fee : 0, res && res->change_pos.has_value() ? *res->change_pos : 0);
TRACE4(coin_selection, normal_create_tx_internal,
wallet.GetName().c_str(),
bool(res),
res ? res->fee : 0,
res && res->change_pos.has_value() ? int32_t(*res->change_pos) : -1);
if (!res) return res;
const auto& txr_ungrouped = *res;
// try with avoidpartialspends unless it's enabled already
@ -1354,8 +1362,12 @@ util::Result<CreatedTransactionResult> CreateTransaction(
auto txr_grouped = CreateTransactionInternal(wallet, vecSend, change_pos, tmp_cc, sign);
// if fee of this alternative one is within the range of the max fee, we use this one
const bool use_aps{txr_grouped.has_value() ? (txr_grouped->fee <= txr_ungrouped.fee + wallet.m_max_aps_fee) : false};
TRACE5(coin_selection, aps_create_tx_internal, wallet.GetName().c_str(), use_aps, txr_grouped.has_value(),
txr_grouped.has_value() ? txr_grouped->fee : 0, txr_grouped.has_value() && txr_grouped->change_pos.has_value() ? *txr_grouped->change_pos : 0);
TRACE5(coin_selection, aps_create_tx_internal,
wallet.GetName().c_str(),
use_aps,
txr_grouped.has_value(),
txr_grouped.has_value() ? txr_grouped->fee : 0,
txr_grouped.has_value() && txr_grouped->change_pos.has_value() ? int32_t(*txr_grouped->change_pos) : -1);
if (txr_grouped) {
wallet.WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n",
txr_ungrouped.fee, txr_grouped->fee, use_aps ? "grouped" : "non-grouped");

23
test/functional/interface_usdt_coinselection.py
View file

@ -204,6 +204,29 @@ class CoinSelectionTracepointTest(BitcoinTestFramework):
assert_equal(success, True)
assert_equal(use_aps, None)
self.log.info("Change position is -1 if no change is created with APS when APS was initially not used")
# We should have 2 tracepoints in the order:
# 1. selected_coins (type 1)
# 2. normal_create_tx_internal (type 2)
# 3. attempting_aps_create_tx (type 3)
# 4. selected_coins (type 1)
# 5. aps_create_tx_internal (type 4)
wallet.sendtoaddress(address=wallet.getnewaddress(), amount=wallet.getbalance(), subtractfeefromamount=True, avoid_reuse=False)
events = self.get_tracepoints([1, 2, 3, 1, 4])
success, use_aps, algo, waste, change_pos = self.determine_selection_from_usdt(events)
assert_equal(success, True)
assert_equal(change_pos, -1)
self.log.info("Change position is -1 if no change is created normally and APS is not used")
# We should have 2 tracepoints in the order:
# 1. selected_coins (type 1)
# 2. normal_create_tx_internal (type 2)
wallet.sendtoaddress(address=wallet.getnewaddress(), amount=wallet.getbalance(), subtractfeefromamount=True)
events = self.get_tracepoints([1, 2])
success, use_aps, algo, waste, change_pos = self.determine_selection_from_usdt(events)
assert_equal(success, True)
assert_equal(change_pos, -1)
self.bpf.cleanup()