Skip to content

Commit

Permalink
wallet: always do avoid partial spends if fees are within a specified…
Browse files Browse the repository at this point in the history 
… range

Summary:
 * wallet: try -avoidpartialspends mode and use its result if fees are below threshold

The threshold is defined by a new max avoid partial spends fee flag, which defaults to 0 (i.e. if fees are unchanged, use the grouped option).

 * test: test the implicit avoid partial spends functionality

Co-authored-by: Fabian Jahr <fjahr@protonmail.com>

This is a backport of Core [[bitcoin/bitcoin#14582 | PR14582]]

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Subscribers: majcosta

Differential Revision: https://reviews.bitcoinabc.org/D7807
  • Loading branch information
@kallewoof @deadalnix
kallewoof authored and deadalnix committed Oct 8, 2020
1 parent 8ef3395 commit 81fe776
Show file tree
Hide file tree
Showing 5 changed files with 156 additions and 21 deletions.
8 changes: 4 additions & 4 deletions src/dummywallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@ class DummyWalletInit : public WalletInitInterface {
void DummyWalletInit::AddWalletOptions() const {
std::vector<std::string> opts = {
"-avoidpartialspends", "-disablewallet", "-fallbackfee=<amt>",
"-keypool=<n>", "-maxtxfee=<amt>", "-mintxfee=<amt>", "-paytxfee=<amt>",
"-rescan", "-salvagewallet", "-spendzeroconfchange", "-upgradewallet",
"-wallet=<path>", "-walletbroadcast", "-walletdir=<dir>",
"-walletnotify=<cmd>", "-zapwallettxes=<mode>",
"-keypool=<n>", "-maxapsfee=<n>", "-maxtxfee=<amt>", "-mintxfee=<amt>",
"-paytxfee=<amt>", "-rescan", "-salvagewallet", "-spendzeroconfchange",
"-upgradewallet", "-wallet=<path>", "-walletbroadcast",
"-walletdir=<dir>", "-walletnotify=<cmd>", "-zapwallettxes=<mode>",
// Wallet debug options
"-dblogsize=<n>", "-flushwallet", "-privdb", "-walletrejectlongchains"};
gArgs.AddHiddenArgs(opts);
Expand Down
7 changes: 7 additions & 0 deletions src/wallet/init.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,13 @@ void WalletInit::AddWalletOptions() const {
strprintf("Set key pool size to <n> (default: %u)",
DEFAULT_KEYPOOL_SIZE),
ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
gArgs.AddArg(
"-maxapsfee=<n>",
strprintf(
"Spend up to this amount in additional (absolute) fees (in %s) if "
"it allows the use of partial spend avoidance (default: %s)",
CURRENCY_UNIT, FormatMoney(DEFAULT_MAX_AVOIDPARTIALSPEND_FEE)),
ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
gArgs.AddArg(
"-maxtxfee=<amt>",
strprintf("Maximum total fees (in %s) to use in a single wallet "
Expand Down
89 changes: 74 additions & 15 deletions src/wallet/wallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2880,15 +2880,17 @@ CWallet::TransactionChangeType(OutputType change_type,
return m_default_address_type;
}

bool CWallet::CreateTransaction(interfaces::Chain::Lock &locked_chainIn,
const std::vector<CRecipient> &vecSend,
CTransactionRef &tx, Amount &nFeeRet,
int &nChangePosInOut, bilingual_str &error,
const CCoinControl &coinControl, bool sign) {
bool CWallet::CreateTransactionInternal(interfaces::Chain::Lock &locked_chainIn,
const std::vector<CRecipient> &vecSend,
CTransactionRef &tx, Amount &nFeeRet,
int &nChangePosInOut,
bilingual_str &error,
const CCoinControl &coin_control,
bool sign) {
Amount nValue = Amount::zero();
const OutputType change_type = TransactionChangeType(
coinControl.m_change_type ? *coinControl.m_change_type
: m_default_change_type,
coin_control.m_change_type ? *coin_control.m_change_type
: m_default_change_type,
vecSend);
ReserveDestination reservedest(this, change_type);
int nChangePosRequest = nChangePosInOut;
Expand Down Expand Up @@ -2921,7 +2923,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock &locked_chainIn,
LOCK(cs_wallet);

std::vector<COutput> vAvailableCoins;
AvailableCoins(*locked_chain, vAvailableCoins, true, &coinControl);
AvailableCoins(*locked_chain, vAvailableCoins, true, &coin_control);
// Parameters for coin selection, init with dummy
CoinSelectionParams coin_selection_params;

Expand All @@ -2931,8 +2933,8 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock &locked_chainIn,
CScript scriptChange;

// coin control: send change to custom address
if (!boost::get<CNoDestination>(&coinControl.destChange)) {
scriptChange = GetScriptForDestination(coinControl.destChange);
if (!boost::get<CNoDestination>(&coin_control.destChange)) {
scriptChange = GetScriptForDestination(coin_control.destChange);

// no coin control: send change to newly generated address
} else {
Expand Down Expand Up @@ -2965,7 +2967,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock &locked_chainIn,
GetSerializeSize(change_prototype_txout);

// Get the fee rate to use effective values in coin selection
CFeeRate nFeeRateNeeded = GetMinimumFeeRate(*this, coinControl);
CFeeRate nFeeRateNeeded = GetMinimumFeeRate(*this, coin_control);

nFeeRet = Amount::zero();
bool pick_new_inputs = true;
Expand Down Expand Up @@ -3042,7 +3044,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock &locked_chainIn,
this);
coin_selection_params.effective_fee = nFeeRateNeeded;
if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins,
nValueIn, coinControl, coin_selection_params,
nValueIn, coin_control, coin_selection_params,
bnb_used)) {
// If BnB was used, it was the first pass. No longer the
// first pass and continue loop with knapsack.
Expand Down Expand Up @@ -3095,13 +3097,13 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock &locked_chainIn,

CTransaction txNewConst(txNew);
int nBytes = CalculateMaximumSignedTxSize(
txNewConst, this, coinControl.fAllowWatchOnly);
txNewConst, this, coin_control.fAllowWatchOnly);
if (nBytes < 0) {
error = _("Signing transaction failed");
return false;
}

Amount nFeeNeeded = GetMinimumFee(*this, nBytes, coinControl);
Amount nFeeNeeded = GetMinimumFee(*this, nBytes, coin_control);

if (nFeeRet >= nFeeNeeded) {
// Reduce fee to only the needed amount if possible. This
Expand All @@ -3121,7 +3123,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock &locked_chainIn,
unsigned int tx_size_with_change =
nBytes + coin_selection_params.change_output_size + 2;
Amount fee_needed_with_change =
GetMinimumFee(*this, tx_size_with_change, coinControl);
GetMinimumFee(*this, tx_size_with_change, coin_control);
Amount minimum_value_for_change = GetDustThreshold(
change_prototype_txout, chain().relayDustFee());
if (nFeeRet >=
Expand Down Expand Up @@ -3253,6 +3255,46 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock &locked_chainIn,
return true;
}

bool CWallet::CreateTransaction(interfaces::Chain::Lock &locked_chain,
const std::vector<CRecipient> &vecSend,
CTransactionRef &tx, Amount &nFeeRet,
int &nChangePosInOut, bilingual_str &error,
const CCoinControl &coin_control, bool sign) {
int nChangePosIn = nChangePosInOut;
CTransactionRef tx2 = tx;
bool res =
CreateTransactionInternal(locked_chain, vecSend, tx, nFeeRet,
nChangePosInOut, error, coin_control, sign);
// try with avoidpartialspends unless it's enabled already
if (res &&
nFeeRet >
Amount::zero() /* 0 means non-functional fee rate estimation */
&& m_max_aps_fee > (-1 * SATOSHI) &&
!coin_control.m_avoid_partial_spends) {
CCoinControl tmp_cc = coin_control;
tmp_cc.m_avoid_partial_spends = true;
Amount nFeeRet2;
int nChangePosInOut2 = nChangePosIn;
// fired and forgotten; if an error occurs, we discard the results
bilingual_str error2;
if (CreateTransactionInternal(locked_chain, vecSend, tx2, nFeeRet2,
nChangePosInOut2, error2, 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 = nFeeRet2 <= nFeeRet + m_max_aps_fee;
WalletLogPrintf(
"Fee non-grouped = %lld, grouped = %lld, using %s\n", nFeeRet,
nFeeRet2, use_aps ? "grouped" : "non-grouped");
if (use_aps) {
tx = tx2;
nFeeRet = nFeeRet2;
nChangePosInOut = nChangePosInOut2;
}
}
}
return res;
}

void CWallet::CommitTransaction(
CTransactionRef tx, mapValue_t mapValue,
std::vector<std::pair<std::string, std::string>> orderForm) {
Expand Down Expand Up @@ -4172,6 +4214,23 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(
walletInstance->m_min_fee = CFeeRate(n);
}

if (gArgs.IsArgSet("-maxapsfee")) {
Amount n = Amount::zero();
if (gArgs.GetArg("-maxapsfee", "") == "-1") {
n = -1 * SATOSHI;
} else if (!ParseMoney(gArgs.GetArg("-maxapsfee", ""), n)) {
error = AmountErrMsg("maxapsfee", gArgs.GetArg("-maxapsfee", ""));
return nullptr;
}
if (n > HIGH_APS_FEE) {
warnings.push_back(
AmountHighWarn("-maxapsfee") + Untranslated(" ") +
_("This is the maximum transaction fee you pay to prioritize "
"partial spend avoidance over regular coin selection."));
}
walletInstance->m_max_aps_fee = n;
}

if (gArgs.IsArgSet("-fallbackfee")) {
Amount nFeePerK = Amount::zero();
if (!ParseMoney(gArgs.GetArg("-fallbackfee", ""), nFeePerK)) {
Expand Down
22 changes: 22 additions & 0 deletions src/wallet/wallet.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include <ui_interface.h>
#include <util/strencodings.h>
#include <util/system.h>
#include <util/translation.h>
#include <validationinterface.h>
#include <wallet/coinselection.h>
#include <wallet/crypter.h>
Expand Down Expand Up @@ -76,6 +77,19 @@ constexpr Amount DEFAULT_PAY_TX_FEE = Amount::zero();
static const Amount DEFAULT_FALLBACK_FEE = Amount::zero();
//! -mintxfee default
static const Amount DEFAULT_TRANSACTION_MINFEE_PER_KB = 1000 * SATOSHI;
/**
* maximum fee increase allowed to do partial spend avoidance, even for nodes
* with this feature disabled by default
*
* A value of -1 disables this feature completely.
* A value of 0 (current default) means to attempt to do partial spend
* avoidance, and use its results if the fees remain *unchanged* A value > 0
* means to do partial spend avoidance if the fee difference against a regular
* coin selection instance is in the range [0..value].
*/
static const Amount DEFAULT_MAX_AVOIDPARTIALSPEND_FEE = Amount::zero();
//! discourage APS fee higher than this amount
constexpr Amount HIGH_APS_FEE{COIN / 10000};
//! minimum recommended increment for BIP 125 replacement txs
static const Amount WALLET_INCREMENTAL_RELAY_FEE(5000 * SATOSHI);
//! Default for -spendzeroconfchange
Expand Down Expand Up @@ -770,6 +784,12 @@ class CWallet final : public WalletStorage,
*/
int m_last_block_processed_height GUARDED_BY(cs_wallet) = -1;

bool CreateTransactionInternal(interfaces::Chain::Lock &locked_chain,
const std::vector<CRecipient> &vecSend,
CTransactionRef &tx, Amount &nFeeRet,
int &nChangePosInOut, bilingual_str &error,
const CCoinControl &coin_control, bool sign);

public:
const CChainParams &chainParams;
/*
Expand Down Expand Up @@ -1123,6 +1143,8 @@ class CWallet final : public WalletStorage,
* -fallbackfee
*/
CFeeRate m_fallback_fee{DEFAULT_FALLBACK_FEE};
//! note: this is absolute fee, not fee rate
Amount m_max_aps_fee{DEFAULT_MAX_AVOIDPARTIALSPEND_FEE};
OutputType m_default_address_type{DEFAULT_ADDRESS_TYPE};
OutputType m_default_change_type{DEFAULT_CHANGE_TYPE};
/**
Expand Down
51 changes: 49 additions & 2 deletions test/functional/wallet_groups.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@
class WalletGroupTest(BitcoinTestFramework):
def set_test_params(self):
self.setup_clean_chain = True
self.num_nodes = 3
self.extra_args = [[], [], ['-avoidpartialspends']]
self.num_nodes = 4
self.extra_args = [
[], [], ['-avoidpartialspends'], ["-maxapsfee=0.0001"]]
self.rpc_timeout = 120

def skip_test_if_missing_module(self):
Expand Down Expand Up @@ -62,6 +63,52 @@ def run_test(self):
assert_approx(v[0], 0.2)
assert_approx(v[1], 1.3, 0.0001)

# Test 'avoid partial if warranted, even if disabled'
self.sync_all()
self.nodes[0].generate(1)
# Nodes 1-2 now have confirmed UTXOs (letters denote destinations):
# Node #1: Node #2:
# - A 1.0 - D0 1.0
# - B0 1.0 - D1 0.5
# - B1 0.5 - E0 1.0
# - C0 1.0 - E1 0.5
# - C1 0.5 - F ~1.3
# - D ~0.3
assert_approx(self.nodes[1].getbalance(), 4.3, 0.0001)
assert_approx(self.nodes[2].getbalance(), 4.3, 0.0001)
# Sending 1.4 btc should pick one 1.0 + one more. For node #1,
# this could be (A / B0 / C0) + (B1 / C1 / D). We ensure that it is
# B0 + B1 or C0 + C1, because this avoids partial spends while not being
# detrimental to transaction cost
txid3 = self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), 1.4)
tx3 = self.nodes[1].getrawtransaction(txid3, True)
# tx3 should have 2 inputs and 2 outputs
assert_equal(2, len(tx3["vin"]))
assert_equal(2, len(tx3["vout"]))
# the accumulated value should be 1.5, so the outputs should be
# ~0.1 and 1.4 and should come from the same destination
values = sorted([vout["value"] for vout in tx3["vout"]])
assert_approx(values[0], 0.1, 0.0001)
assert_approx(values[1], 1.4)

input_txids = [vin["txid"] for vin in tx3["vin"]]
input_addrs = [self.nodes[1].gettransaction(
txid)['details'][0]['address'] for txid in input_txids]
assert_equal(input_addrs[0], input_addrs[1])
# Node 2 enforces avoidpartialspends so needs no checking here

# Test wallet option maxapsfee with Node 3
addr_aps = self.nodes[3].getnewaddress()
self.nodes[0].sendtoaddress(addr_aps, 1.0)
self.nodes[0].sendtoaddress(addr_aps, 1.0)
self.nodes[0].generate(1)
txid4 = self.nodes[3].sendtoaddress(self.nodes[0].getnewaddress(), 0.1)
tx4 = self.nodes[3].getrawtransaction(txid4, True)
# tx4 should have 2 inputs and 2 outputs although one output would
# have been enough and the transaction caused higher fees
assert_equal(2, len(tx4["vin"]))
assert_equal(2, len(tx4["vout"]))

# Empty out node2's wallet
self.nodes[2].sendtoaddress(address=self.nodes[0].getnewaddress(
), amount=self.nodes[2].getbalance(), subtractfeefromamount=True)
Expand Down

0 comments on commit 81fe776

Please sign in to comment.