From 5821a1d23a89adb3253f252aa3b9f66eb83af494 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Mon, 17 Aug 2020 15:56:49 +1200 Subject: [PATCH] Merge #14582: wallet: always do avoid partial spends if fees are within a specified range MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9 test: test the implicit avoid partial spends functionality (Karl-Johan Alm) b82067bf696c53f22536f9ca2e51949c164f6b06 wallet: try -avoidpartialspends mode and use its result if fees are below threshold (Karl-Johan Alm) Pull request description: The `-avoidpartialspends` feature is normally disabled, as it may affect the optimal fee for payments. This PR introduces a new parameter `-maxapsfee` (max avoid partial spends fee) which acts on the following values: * -1: disable partial spend avoidance completely (do not even try it) * 0: only do partial spend avoidance if fees are the same or better as the regular coin selection * 1..∞: use APS variant if the absolute fee difference is less than or equal to the max APS fee For values other than -1, the code will now try partial spend avoidance once, and if that gives a value within the accepted range, it will use that. Example: -maxapsfee=0.00001000 means the wallet will do regular coin select, APS coin select, and then pick AKS iff the absolute fee difference is <= 1000 satoshi. Edit: updated this to reflect the fact we are now using a max fee. ACKs for top commit: fjahr: tested ACK 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9 achow101: ACK 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9 jonatack: ACK 7f13dfb58, code review, debug build, verified the test fails with `AssertionError: not(2 == 1)` for the number of vouts when `-maxapsfee=0.0001` is changed to 0, and verified the new logging with an added assertion. meshcollider: Code review ACK 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9 Tree-SHA512: 475929df57f6191bb4e36bfbcad5a280a64bb0ecd8767b76cb2e44e2301235d0eb294a3f2fac5bbf15d35d7ecfba47acb2285feadb883c9ce31c08377e3afb3c --- src/dummywallet.cpp | 1 + src/wallet/init.cpp | 2 + src/wallet/wallet.cpp | 59 ++++++++++++++++++++++++++- src/wallet/wallet.h | 13 ++++++ test/functional/wallet_groups.py | 50 ++++++++++++++++++++++- test/functional/wallet_upgradetohd.py | 8 ++-- 6 files changed, 126 insertions(+), 7 deletions(-) diff --git a/src/dummywallet.cpp b/src/dummywallet.cpp index 6bd92a5f25..a9e5ae9486 100644 --- a/src/dummywallet.cpp +++ b/src/dummywallet.cpp @@ -36,6 +36,7 @@ void DummyWalletInit::AddWalletOptions(ArgsManager& argsman) const "-disablewallet", "-instantsendnotify=", "-keypool=", + "-maxapsfee=", "-maxtxfee=", "-rescan=", "-salvagewallet", diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp index 15bd3a1dc5..e81e06fdc9 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -77,6 +77,8 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const CURRENCY_UNIT, FormatMoney(DEFAULT_DISCARD_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET_FEE); argsman.AddArg("-fallbackfee=", strprintf("A fee rate (in %s/kB) that will be used when fee estimation has insufficient data. 0 to entirely disable the fallbackfee feature. (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_FALLBACK_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET_FEE); + argsman.AddArg("-maxapsfee=", 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); + argsman.AddArg("-maxtxfee=", strprintf("Maximum total fees (in %s) to use in a single wallet transaction; setting this too low may abort large transactions (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MAXFEE)), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-mintxfee=", strprintf("Fee rates (in %s/kB) smaller than this are considered zero fee for transaction creation (default: %s)", diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 14e80b3c98..2cb8488c4d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3376,7 +3376,15 @@ bool CWallet::GetBudgetSystemCollateralTX(CTransactionRef& tx, uint256 hash, CAm return true; } -bool CWallet::CreateTransaction(const std::vector& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, bool sign, int nExtraPayloadSize) +bool CWallet::CreateTransactionInternal( + const std::vector& vecSend, + CTransactionRef& tx, + CAmount& nFeeRet, + int& nChangePosInOut, + bilingual_str& error, + const CCoinControl& coin_control, + bool sign, + int nExtraPayloadSize) { CAmount nValue = 0; ReserveDestination reservedest(this); @@ -3744,6 +3752,40 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CTransac return true; } +bool CWallet::CreateTransaction( + const std::vector& vecSend, + CTransactionRef& tx, + CAmount& nFeeRet, + int& nChangePosInOut, + bilingual_str& error, + const CCoinControl& coin_control, + bool sign, + int nExtraPayloadSize) +{ + int nChangePosIn = nChangePosInOut; + CTransactionRef tx2 = tx; + bool res = CreateTransactionInternal(vecSend, tx, nFeeRet, nChangePosInOut, error, coin_control, sign, nExtraPayloadSize); + // try with avoidpartialspends unless it's enabled already + if (res && nFeeRet > 0 /* 0 means non-functional fee rate estimation */ && m_max_aps_fee > -1 && !coin_control.m_avoid_partial_spends) { + CCoinControl tmp_cc = coin_control; + tmp_cc.m_avoid_partial_spends = true; + CAmount nFeeRet2; + int nChangePosInOut2 = nChangePosIn; + bilingual_str error2; // fired and forgotten; if an error occurs, we discard the results + if (CreateTransactionInternal(vecSend, tx2, nFeeRet2, nChangePosInOut2, error2, tmp_cc, sign, nExtraPayloadSize)) { + // 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> orderForm) { LOCK(cs_wallet); @@ -4650,6 +4692,21 @@ std::shared_ptr CWallet::Create(interfaces::Chain& chain, interfaces::C walletInstance->m_min_fee = CFeeRate{min_tx_fee.value()}; } + if (gArgs.IsArgSet("-maxapsfee")) { + CAmount n = 0; + if (gArgs.GetArg("-maxapsfee", "") == "-1") { + n = -1; + } 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")) { std::optional fallback_fee = ParseMoney(gArgs.GetArg("-fallbackfee", "")); if (!fallback_fee) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 59d772c958..4e8f82b800 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -73,6 +73,16 @@ static const CAmount DEFAULT_FALLBACK_FEE = 1000; static const CAmount DEFAULT_DISCARD_FEE = 10000; //! -mintxfee default static const CAmount DEFAULT_TRANSACTION_MINFEE = 1000; +/** + * 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 CAmount DEFAULT_MAX_AVOIDPARTIALSPEND_FEE = 0; +//! discourage APS fee higher than this amount +constexpr CAmount HIGH_APS_FEE{COIN / 10000}; //! minimum recommended increment for BIP 125 replacement txs static const CAmount WALLET_INCREMENTAL_RELAY_FEE = 5000; //! Default for -spendzeroconfchange @@ -806,6 +816,8 @@ private: // ScriptPubKeyMan::GetID. In many cases it will be the hash of an internal structure std::map> m_spk_managers; + bool CreateTransactionInternal(const std::vector& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, bool sign, int nExtraPayloadSize); + public: /* * Main wallet lock. @@ -1125,6 +1137,7 @@ public: */ CFeeRate m_fallback_fee{DEFAULT_FALLBACK_FEE}; CFeeRate m_discard_rate{DEFAULT_DISCARD_FEE}; + CAmount m_max_aps_fee{DEFAULT_MAX_AVOIDPARTIALSPEND_FEE}; //!< note: this is absolute fee, not fee rate /** Absolute maximum transaction fee (in satoshis) used by default for the wallet */ CAmount m_default_max_tx_fee{DEFAULT_TRANSACTION_MAXFEE}; diff --git a/test/functional/wallet_groups.py b/test/functional/wallet_groups.py index 31b701b932..41159fcd0c 100755 --- a/test/functional/wallet_groups.py +++ b/test/functional/wallet_groups.py @@ -15,8 +15,8 @@ from test_framework.util import ( 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 = 480 self.supports_cli = False @@ -65,6 +65,52 @@ class WalletGroupTest(BitcoinTestFramework): 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 = [vout["value"] for vout in tx3["vout"]] + values.sort() + 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) self.sync_all() diff --git a/test/functional/wallet_upgradetohd.py b/test/functional/wallet_upgradetohd.py index 64cf39cf57..6b4cd9869c 100755 --- a/test/functional/wallet_upgradetohd.py +++ b/test/functional/wallet_upgradetohd.py @@ -62,7 +62,7 @@ class WalletUpgradeToHDTest(BitcoinTestFramework): assert_equal(keypath, "m/44'/1'/0'/0/%d" % i) else: keypath = node.getaddressinfo(out['scriptPubKey']['addresses'][0])['hdkeypath'] - assert_equal(keypath, "m/44'/1'/0'/1/%d" % i) + assert_equal(keypath, "m/44'/1'/0'/1/%d" % (i * 2)) self.bump_mocktime(1) node.generate(1) @@ -134,7 +134,7 @@ class WalletUpgradeToHDTest(BitcoinTestFramework): assert node.upgradetohd(mnemonic) assert_equal(mnemonic, node.dumphdinfo()['mnemonic']) assert_equal(chainid, node.getwalletinfo()['hdchainid']) - node.keypoolrefill(5) + node.keypoolrefill(10) assert balance_after != node.getbalance() node.rescanblockchain() assert_equal(balance_after, node.getbalance()) @@ -177,7 +177,7 @@ class WalletUpgradeToHDTest(BitcoinTestFramework): # so we can't compare new balance to balance_non_HD here, # assert_equal(balance_non_HD, node.getbalance()) # won't work assert balance_non_HD != node.getbalance() - node.keypoolrefill(4) + node.keypoolrefill(8) node.rescanblockchain() # All coins should be recovered assert_equal(balance_after, node.getbalance()) @@ -201,7 +201,7 @@ class WalletUpgradeToHDTest(BitcoinTestFramework): # so we can't compare new balance to balance_non_HD here, # assert_equal(balance_non_HD, node.getbalance()) # won't work assert balance_non_HD != node.getbalance() - node.keypoolrefill(4) + node.keypoolrefill(8) node.rescanblockchain() # All coins should be recovered assert_equal(balance_after, node.getbalance())