From 5b158707f23a290c750a083c8f7d43915bcc48c5 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Thu, 19 Jan 2017 16:17:38 -0500 Subject: Use incrementalRelayFee for BIP 125 replacement --- src/wallet/rpcwallet.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 152e9cff90..07c3688a4d 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2652,8 +2652,8 @@ UniValue bumpfee(const JSONRPCRequest& request) "By default, the new fee will be calculated automatically using estimatefee.\n" "The user can specify a confirmation target for estimatefee.\n" "Alternatively, the user can specify totalFee, or use RPC setpaytxfee to set a higher fee rate.\n" - "At a minimum, the new fee rate must be high enough to pay a new relay fee (relay fee amount returned\n" - "by getnetworkinfo RPC) and to enter the node's mempool.\n" + "At a minimum, the new fee rate must be high enough to pay a new relay fee over the original fee\n" + "to enter the node's mempool.\n" "\nArguments:\n" "1. txid (string, required) The txid to be bumped\n" "2. options (object, optional)\n" @@ -2787,9 +2787,9 @@ UniValue bumpfee(const JSONRPCRequest& request) CFeeRate nNewFeeRate; if (totalFee > 0) { - CAmount minTotalFee = nOldFeeRate.GetFee(maxNewTxSize) + minRelayTxFee.GetFee(maxNewTxSize); + CAmount minTotalFee = nOldFeeRate.GetFee(maxNewTxSize) + ::incrementalRelayFee.GetFee(maxNewTxSize); if (totalFee < minTotalFee) { - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid totalFee, must be at least %s (oldFee %s + relayFee %s)", FormatMoney(minTotalFee), nOldFeeRate.GetFee(maxNewTxSize), minRelayTxFee.GetFee(maxNewTxSize))); + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid totalFee, must be at least %s (oldFee %s + relayFee %s)", FormatMoney(minTotalFee), nOldFeeRate.GetFee(maxNewTxSize), ::incrementalRelayFee.GetFee(maxNewTxSize))); } nNewFee = totalFee; nNewFeeRate = CFeeRate(totalFee, maxNewTxSize); @@ -2804,9 +2804,9 @@ UniValue bumpfee(const JSONRPCRequest& request) nNewFeeRate = CWallet::fallbackFee; } - // new fee rate must be at least old rate + minimum relay rate - if (nNewFeeRate.GetFeePerK() < nOldFeeRate.GetFeePerK() + ::minRelayTxFee.GetFeePerK()) { - nNewFeeRate = CFeeRate(nOldFeeRate.GetFeePerK() + ::minRelayTxFee.GetFeePerK()); + // new fee rate must be at least old rate + minimum incremental relay rate + if (nNewFeeRate.GetFeePerK() < nOldFeeRate.GetFeePerK() + ::incrementalRelayFee.GetFeePerK()) { + nNewFeeRate = CFeeRate(nOldFeeRate.GetFeePerK() + ::incrementalRelayFee.GetFeePerK()); } nNewFee = nNewFeeRate.GetFee(maxNewTxSize); -- cgit v1.2.3 From de6400de5d4b654007f21901d9555ddea0face82 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Thu, 19 Jan 2017 16:18:24 -0500 Subject: Fix missing use of dustRelayFee --- src/wallet/rpcwallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/wallet') diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 07c3688a4d..c0f7bbcb42 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2834,7 +2834,7 @@ UniValue bumpfee(const JSONRPCRequest& request) // If the output would become dust, discard it (converting the dust to fee) poutput->nValue -= nDelta; - if (poutput->nValue <= poutput->GetDustThreshold(::minRelayTxFee)) { + if (poutput->nValue <= poutput->GetDustThreshold(::dustRelayFee)) { LogPrint("rpc", "Bumping fee and discarding dust output\n"); nNewFee += poutput->nValue; tx.vout.erase(tx.vout.begin() + nOutput); -- cgit v1.2.3 From fe8e8efcf91fa92db68aabeb0a1709b032e60dd6 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Thu, 19 Jan 2017 21:18:46 -0500 Subject: [rpc] Add incremental relay fee to getnetworkinfo --- src/wallet/rpcwallet.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index c0f7bbcb42..b62fbeb10e 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2652,8 +2652,8 @@ UniValue bumpfee(const JSONRPCRequest& request) "By default, the new fee will be calculated automatically using estimatefee.\n" "The user can specify a confirmation target for estimatefee.\n" "Alternatively, the user can specify totalFee, or use RPC setpaytxfee to set a higher fee rate.\n" - "At a minimum, the new fee rate must be high enough to pay a new relay fee over the original fee\n" - "to enter the node's mempool.\n" + "At a minimum, the new fee rate must be high enough to pay an additional new relay fee (incrementalfee\n" + "returned by getnetworkinfo) to enter the node's mempool.\n" "\nArguments:\n" "1. txid (string, required) The txid to be bumped\n" "2. options (object, optional)\n" -- cgit v1.2.3 From ae9719ab87a05ca4c3293b2f8675f17b16ed5872 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Wed, 25 Jan 2017 22:20:02 -0500 Subject: Refactor GetMinimumFee to give option of providing targetFee --- src/wallet/wallet.cpp | 9 +++++++-- src/wallet/wallet.h | 5 +++++ 2 files changed, 12 insertions(+), 2 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6251998099..c46d5ed780 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2790,8 +2790,13 @@ CAmount CWallet::GetRequiredFee(unsigned int nTxBytes) CAmount CWallet::GetMinimumFee(unsigned int nTxBytes, unsigned int nConfirmTarget, const CTxMemPool& pool) { - // payTxFee is user-set "I want to pay this much" - CAmount nFeeNeeded = payTxFee.GetFee(nTxBytes); + // payTxFee is the user-set global for desired feerate + return GetMinimumFee(nTxBytes, nConfirmTarget, pool, payTxFee.GetFee(nTxBytes)); +} + +CAmount CWallet::GetMinimumFee(unsigned int nTxBytes, unsigned int nConfirmTarget, const CTxMemPool& pool, CAmount targetFee) +{ + CAmount nFeeNeeded = targetFee; // User didn't set: use -txconfirmtarget to estimate... if (nFeeNeeded == 0) { int estimateFoundTarget = nConfirmTarget; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index ecc63a9a13..f95c0589d6 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -802,6 +802,11 @@ public: * and the required fee */ static CAmount GetMinimumFee(unsigned int nTxBytes, unsigned int nConfirmTarget, const CTxMemPool& pool); + /** + * Estimate the minimum fee considering required fee and targetFee or if 0 + * then fee estimation for nConfirmTarget + */ + static CAmount GetMinimumFee(unsigned int nTxBytes, unsigned int nConfirmTarget, const CTxMemPool& pool, CAmount targetFee); /** * Return the minimum required fee taking into account the * floating relay fee and user set minimum transaction fee -- cgit v1.2.3 From e8021ec9193c7e8137f9716bcafd197a357a624e Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Wed, 25 Jan 2017 22:16:15 -0500 Subject: Use CWallet::GetMinimumFee in bumpfee Use the wallet's fee calculation logic to properly clamp fee against minimums and maximums when calculating the fee for a bumpfee transaction. Unless totalFee is explictly given, in which case, manually check against min, but do nothing to adjust given fee. In all cases do a final check against maxTxFee (after adding any incremental amount). --- src/wallet/rpcwallet.cpp | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index b62fbeb10e..9ae36653b8 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2739,6 +2739,10 @@ UniValue bumpfee(const JSONRPCRequest& request) throw JSONRPCError(RPC_MISC_ERROR, "Transaction does not have a change output"); } + // signature sizes can vary by a byte, so add 1 for each input when calculating the new fee + int64_t txSize = GetVirtualTransactionSize(*(wtx.tx)); + const int64_t maxNewTxSize = txSize + wtx.tx->vin.size(); + // optional parameters bool specifiedConfirmTarget = false; int newConfirmTarget = nTxConfirmTarget; @@ -2764,10 +2768,11 @@ UniValue bumpfee(const JSONRPCRequest& request) } } else if (options.exists("totalFee")) { totalFee = options["totalFee"].get_int64(); - if (totalFee <= 0) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid totalFee (cannot be <= 0)"); - } else if (totalFee > maxTxFee) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid totalFee (cannot be higher than maxTxFee)"); + CAmount requiredFee = CWallet::GetRequiredFee(maxNewTxSize); + if (totalFee < requiredFee ) { + throw JSONRPCError(RPC_INVALID_PARAMETER, + strprintf("Insufficient totalFee (cannot be less than required fee %s)", + FormatMoney(requiredFee))); } } @@ -2776,10 +2781,6 @@ UniValue bumpfee(const JSONRPCRequest& request) } } - // signature sizes can vary by a byte, so add 1 for each input when calculating the new fee - int64_t txSize = GetVirtualTransactionSize(*(wtx.tx)); - const int64_t maxNewTxSize = txSize + wtx.tx->vin.size(); - // calculate the old fee and fee-rate CAmount nOldFee = wtx.GetDebit(ISMINE_SPENDABLE) - wtx.tx->GetValueOut(); CFeeRate nOldFeeRate(nOldFee, txSize); @@ -2794,24 +2795,31 @@ UniValue bumpfee(const JSONRPCRequest& request) nNewFee = totalFee; nNewFeeRate = CFeeRate(totalFee, maxNewTxSize); } else { - // use the user-defined payTxFee if possible, otherwise use smartfee / fallbackfee - if (!specifiedConfirmTarget && payTxFee.GetFeePerK() != 0) { - nNewFeeRate = payTxFee; - } else { - nNewFeeRate = mempool.estimateSmartFee(newConfirmTarget); + // if user specified a confirm target then don't consider any global payTxFee + if (specifiedConfirmTarget) { + nNewFee = CWallet::GetMinimumFee(maxNewTxSize, newConfirmTarget, mempool, CAmount(0)); } - if (nNewFeeRate.GetFeePerK() == 0) { - nNewFeeRate = CWallet::fallbackFee; + // otherwise use the regular wallet logic to select payTxFee or default confirm target + else { + nNewFee = CWallet::GetMinimumFee(maxNewTxSize, newConfirmTarget, mempool); } + nNewFeeRate = CFeeRate(nNewFee, maxNewTxSize); + // new fee rate must be at least old rate + minimum incremental relay rate if (nNewFeeRate.GetFeePerK() < nOldFeeRate.GetFeePerK() + ::incrementalRelayFee.GetFeePerK()) { nNewFeeRate = CFeeRate(nOldFeeRate.GetFeePerK() + ::incrementalRelayFee.GetFeePerK()); + nNewFee = nNewFeeRate.GetFee(maxNewTxSize); } - - nNewFee = nNewFeeRate.GetFee(maxNewTxSize); } + // Check that in all cases the new fee doesn't violate maxTxFee + if (nNewFee > maxTxFee) { + throw JSONRPCError(RPC_MISC_ERROR, + strprintf("Specified or calculated fee %s is too high (cannot be higher than maxTxFee %s)", + FormatMoney(nNewFee), FormatMoney(maxTxFee))); + } + // check that fee rate is higher than mempool's minimum fee // (no point in bumping fee if we know that the new tx won't be accepted to the mempool) // This may occur if the user set TotalFee or paytxfee too low, if fallbackfee is too low, or, perhaps, -- cgit v1.2.3 From 0c0c63f70a5376b87508e8f1c73dcbc59c8a96ed Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Mon, 23 Jan 2017 13:15:24 -0500 Subject: Introduce WALLET_INCREMENTAL_RELAY_FEE Have wallet's default bump value be higher than the default incrementalRelayFee to future proof against changes to incremental relay fee. Only applies when not setting the fee rate directly. --- src/wallet/rpcwallet.cpp | 16 ++++++++++++---- src/wallet/wallet.h | 2 ++ 2 files changed, 14 insertions(+), 4 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 9ae36653b8..6517bae52c 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2786,11 +2786,19 @@ UniValue bumpfee(const JSONRPCRequest& request) CFeeRate nOldFeeRate(nOldFee, txSize); CAmount nNewFee; CFeeRate nNewFeeRate; + // The wallet uses a conservative WALLET_INCREMENTAL_RELAY_FEE value to + // future proof against changes to network wide policy for incremental relay + // fee that our node may not be aware of. + CFeeRate walletIncrementalRelayFee = CFeeRate(WALLET_INCREMENTAL_RELAY_FEE); + if (::incrementalRelayFee > walletIncrementalRelayFee) { + walletIncrementalRelayFee = ::incrementalRelayFee; + } if (totalFee > 0) { CAmount minTotalFee = nOldFeeRate.GetFee(maxNewTxSize) + ::incrementalRelayFee.GetFee(maxNewTxSize); if (totalFee < minTotalFee) { - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid totalFee, must be at least %s (oldFee %s + relayFee %s)", FormatMoney(minTotalFee), nOldFeeRate.GetFee(maxNewTxSize), ::incrementalRelayFee.GetFee(maxNewTxSize))); + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Insufficient totalFee, must be at least %s (oldFee %s + incrementalFee %s)", + FormatMoney(minTotalFee), FormatMoney(nOldFeeRate.GetFee(maxNewTxSize)), FormatMoney(::incrementalRelayFee.GetFee(maxNewTxSize)))); } nNewFee = totalFee; nNewFeeRate = CFeeRate(totalFee, maxNewTxSize); @@ -2806,9 +2814,9 @@ UniValue bumpfee(const JSONRPCRequest& request) nNewFeeRate = CFeeRate(nNewFee, maxNewTxSize); - // new fee rate must be at least old rate + minimum incremental relay rate - if (nNewFeeRate.GetFeePerK() < nOldFeeRate.GetFeePerK() + ::incrementalRelayFee.GetFeePerK()) { - nNewFeeRate = CFeeRate(nOldFeeRate.GetFeePerK() + ::incrementalRelayFee.GetFeePerK()); + // New fee rate must be at least old rate + minimum incremental relay rate + if (nNewFeeRate.GetFeePerK() < nOldFeeRate.GetFeePerK() + walletIncrementalRelayFee.GetFeePerK()) { + nNewFeeRate = CFeeRate(nOldFeeRate.GetFeePerK() + walletIncrementalRelayFee.GetFeePerK()); nNewFee = nNewFeeRate.GetFee(maxNewTxSize); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index f95c0589d6..764a7aa268 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -48,6 +48,8 @@ static const CAmount DEFAULT_TRANSACTION_FEE = 0; static const CAmount DEFAULT_FALLBACK_FEE = 20000; //! -mintxfee default static const CAmount DEFAULT_TRANSACTION_MINFEE = 1000; +//! minimum recommended increment for BIP 125 replacement txs +static const CAmount WALLET_INCREMENTAL_RELAY_FEE = 5000; //! target minimum change amount static const CAmount MIN_CHANGE = CENT; //! final minimum change amount after paying for fees -- cgit v1.2.3 From 4b189c13401bcd350c05cf8194beaeb3d18b3ebc Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Mon, 23 Jan 2017 13:15:48 -0500 Subject: Change bumpfee result value from 'oldfee' to 'origfee'. The result value indicates the actual fee on the transaction that was replaced. But there is an error message which uses the description 'oldfee' to refer to the original fee rate applied to the new transaction's estimated max size. It was confusing that two different uses of 'oldfee' had two different numeric values. --- src/wallet/rpcwallet.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 6517bae52c..2e381630c8 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2674,8 +2674,8 @@ UniValue bumpfee(const JSONRPCRequest& request) "\nResult:\n" "{\n" " \"txid\": \"value\", (string) The id of the new transaction\n" - " \"oldfee\": n, (numeric) Fee of the replaced transaction\n" - " \"fee\": n, (numeric) Fee of the new transaction\n" + " \"origfee\": n, (numeric) Fee of the replaced transaction\n" + " \"fee\": n, (numeric) Fee of the new transaction\n" "}\n" "\nExamples:\n" "\nBump the fee, get the new transaction\'s txid\n" + @@ -2899,7 +2899,7 @@ UniValue bumpfee(const JSONRPCRequest& request) UniValue result(UniValue::VOBJ); result.push_back(Pair("txid", wtxBumped.GetHash().GetHex())); - result.push_back(Pair("oldfee", ValueFromAmount(nOldFee))); + result.push_back(Pair("origfee", ValueFromAmount(nOldFee))); result.push_back(Pair("fee", ValueFromAmount(nNewFee))); return result; -- cgit v1.2.3