Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wallet] Move maxtxfee from node to wallet #15778

Merged
merged 1 commit into from Apr 27, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/dummywallet.cpp
Expand Up @@ -24,7 +24,7 @@ class DummyWalletInit : public WalletInitInterface {
void DummyWalletInit::AddWalletOptions() const
{
std::vector<std::string> opts = {"-addresstype", "-changetype", "-disablewallet", "-discardfee=<amt>", "-fallbackfee=<amt>",
"-keypool=<n>", "-mintxfee=<amt>", "-paytxfee=<amt>", "-rescan", "-salvagewallet", "-spendzeroconfchange", "-txconfirmtarget=<n>",
"-keypool=<n>", "-maxtxfee=<amt>", "-mintxfee=<amt>", "-paytxfee=<amt>", "-rescan", "-salvagewallet", "-spendzeroconfchange", "-txconfirmtarget=<n>",
"-upgradewallet", "-wallet=<path>", "-walletbroadcast", "-walletdir=<dir>", "-walletnotify=<cmd>", "-walletrbf", "-zapwallettxes=<mode>",
"-dblogsize=<n>", "-flushwallet", "-privdb", "-walletrejectlongchains"};
gArgs.AddHiddenArgs(opts);
Expand Down
18 changes: 0 additions & 18 deletions src/init.cpp
Expand Up @@ -502,8 +502,6 @@ void SetupServerArgs()
gArgs.AddArg("-mocktime=<n>", "Replace actual time with <n> seconds since epoch (default: 0)", true, OptionsCategory::DEBUG_TEST);
gArgs.AddArg("-maxsigcachesize=<n>", strprintf("Limit sum of signature cache and script execution cache sizes to <n> MiB (default: %u)", DEFAULT_MAX_SIG_CACHE_SIZE), true, OptionsCategory::DEBUG_TEST);
gArgs.AddArg("-maxtipage=<n>", strprintf("Maximum tip age in seconds to consider node in initial block download (default: %u)", DEFAULT_MAX_TIP_AGE), true, OptionsCategory::DEBUG_TEST);
gArgs.AddArg("-maxtxfee=<amt>", strprintf("Maximum total fees (in %s) to use in a single wallet transaction; setting this too low may abort large transactions (default: %s)", // TODO move setting to wallet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another change in behavior that might be worth noting in the commit message or release notes: that specifiying -maxtxfee will now result in an error in non-wallet builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is true. Doesn't defining -maxtxfee in dummywallet.cpp ensure that there's no error?

I can change this so there is an error, which would effectively alert users that the -maxtxfee setting is not used in non-wallet builds.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #15778 (comment)

You're right, I just didn't pay attention to the dummywallet change. I think it probably shouldn't be an error to specify -maxtxfee, based on the way other wallet options are handled, though I could see reasons for doing it either way.

CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MAXFEE)), false, OptionsCategory::DEBUG_TEST);
gArgs.AddArg("-printpriority", strprintf("Log transaction fee per kB when mining blocks (default: %u)", DEFAULT_PRINTPRIORITY), true, OptionsCategory::DEBUG_TEST);
gArgs.AddArg("-printtoconsole", "Send trace/debug info to console (default: 1 when no -daemon. To disable logging to file, set -nodebuglogfile)", false, OptionsCategory::DEBUG_TEST);
gArgs.AddArg("-shrinkdebugfile", "Shrink debug.log file on client startup (default: 1 when no -debug)", false, OptionsCategory::DEBUG_TEST);
Expand Down Expand Up @@ -1123,22 +1121,6 @@ bool AppInitParameterInteraction()
dustRelayFee = CFeeRate(n);
}

// This is required by both the wallet and node
if (gArgs.IsArgSet("-maxtxfee"))
{
CAmount nMaxFee = 0;
if (!ParseMoney(gArgs.GetArg("-maxtxfee", ""), nMaxFee))
return InitError(AmountErrMsg("maxtxfee", gArgs.GetArg("-maxtxfee", "")));
if (nMaxFee > HIGH_MAX_TX_FEE)
InitWarning(_("-maxtxfee is set very high! Fees this large could be paid on a single transaction."));
maxTxFee = nMaxFee;
if (CFeeRate(maxTxFee, 1000) < ::minRelayTxFee)
{
return InitError(strprintf(_("Invalid amount for -maxtxfee=<amount>: '%s' (must be at least the minrelay fee of %s to prevent stuck transactions)"),
gArgs.GetArg("-maxtxfee", ""), ::minRelayTxFee.ToString()));
}
}

fRequireStandard = !gArgs.GetBoolArg("-acceptnonstdtxn", !chainparams.RequireStandard());
if (chainparams.RequireStandard() && !fRequireStandard)
return InitError(strprintf("acceptnonstdtxn is not currently supported for %s chain", chainparams.NetworkIDString()));
Expand Down
1 change: 0 additions & 1 deletion src/interfaces/chain.cpp
Expand Up @@ -339,7 +339,6 @@ class ChainImpl : public Chain
CFeeRate relayMinFee() override { return ::minRelayTxFee; }
CFeeRate relayIncrementalFee() override { return ::incrementalRelayFee; }
CFeeRate relayDustFee() override { return ::dustRelayFee; }
CAmount maxTxFee() override { return ::maxTxFee; }
bool getPruneMode() override { return ::fPruneMode; }
bool p2pEnabled() override { return g_connman != nullptr; }
bool isReadyToBroadcast() override { return !::fImporting && !::fReindex && !IsInitialBlockDownload(); }
Expand Down
6 changes: 0 additions & 6 deletions src/interfaces/chain.h
Expand Up @@ -216,12 +216,6 @@ class Chain
//! Relay dust fee setting (-dustrelayfee), reflecting lowest rate it's economical to spend.
virtual CFeeRate relayDustFee() = 0;

//! Node max tx fee setting (-maxtxfee).
//! This could be replaced by a per-wallet max fee, as proposed at
//! https://github.com/bitcoin/bitcoin/issues/15355
//! But for the time being, wallets call this to access the node setting.
virtual CAmount maxTxFee() = 0;

//! Check if pruning is enabled.
virtual bool getPruneMode() = 0;

Expand Down
1 change: 0 additions & 1 deletion src/interfaces/node.cpp
Expand Up @@ -207,7 +207,6 @@ class NodeImpl : public Node
}
}
bool getNetworkActive() override { return g_connman && g_connman->GetNetworkActive(); }
CAmount getMaxTxFee() override { return ::maxTxFee; }
CFeeRate estimateSmartFee(int num_blocks, bool conservative, int* returned_target = nullptr) override
{
FeeCalculation fee_calc;
Expand Down
3 changes: 0 additions & 3 deletions src/interfaces/node.h
Expand Up @@ -159,9 +159,6 @@ class Node
//! Get network active.
virtual bool getNetworkActive() = 0;

//! Get max tx fee.
virtual CAmount getMaxTxFee() = 0;

//! Estimate smart fee.
virtual CFeeRate estimateSmartFee(int num_blocks, bool conservative, int* returned_target = nullptr) = 0;

Expand Down
1 change: 1 addition & 0 deletions src/interfaces/wallet.cpp
Expand Up @@ -464,6 +464,7 @@ class WalletImpl : public Wallet
bool IsWalletFlagSet(uint64_t flag) override { return m_wallet->IsWalletFlagSet(flag); }
OutputType getDefaultAddressType() override { return m_wallet->m_default_address_type; }
OutputType getDefaultChangeType() override { return m_wallet->m_default_change_type; }
CAmount getDefaultMaxTxFee() override { return m_wallet->m_default_max_tx_fee; }
void remove() override
{
RemoveWallet(m_wallet);
Expand Down
3 changes: 3 additions & 0 deletions src/interfaces/wallet.h
Expand Up @@ -247,6 +247,9 @@ class Wallet
// Get default change type.
virtual OutputType getDefaultChangeType() = 0;

//! Get max tx fee.
virtual CAmount getDefaultMaxTxFee() = 0;

// Remove wallet.
virtual void remove() = 0;

Expand Down
2 changes: 1 addition & 1 deletion src/qt/sendcoinsdialog.cpp
Expand Up @@ -578,7 +578,7 @@ void SendCoinsDialog::processSendCoinsReturn(const WalletModel::SendCoinsReturn
msgParams.second = CClientUIInterface::MSG_ERROR;
break;
case WalletModel::AbsurdFee:
msgParams.first = tr("A fee higher than %1 is considered an absurdly high fee.").arg(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), model->node().getMaxTxFee()));
msgParams.first = tr("A fee higher than %1 is considered an absurdly high fee.").arg(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), model->wallet().getDefaultMaxTxFee()));
break;
case WalletModel::PaymentRequestExpired:
msgParams.first = tr("Payment request expired.");
Expand Down
4 changes: 2 additions & 2 deletions src/qt/walletmodel.cpp
Expand Up @@ -222,9 +222,9 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
}

// reject absurdly high fee. (This can never happen because the
// wallet caps the fee at maxTxFee. This merely serves as a
// wallet caps the fee at m_default_max_tx_fee. This merely serves as a
// belt-and-suspenders check)
if (nFeeRequired > m_node.getMaxTxFee())
if (nFeeRequired > m_wallet->getDefaultMaxTxFee())
return AbsurdFee;
}

Expand Down
1 change: 0 additions & 1 deletion src/validation.cpp
Expand Up @@ -252,7 +252,6 @@ uint256 hashAssumeValid;
arith_uint256 nMinimumChainWork;

CFeeRate minRelayTxFee = CFeeRate(DEFAULT_MIN_RELAY_TX_FEE);
CAmount maxTxFee = DEFAULT_TRANSACTION_MAXFEE;

CBlockPolicyEstimator feeEstimator;
CTxMemPool mempool(&feeEstimator);
Expand Down
8 changes: 0 additions & 8 deletions src/validation.h
Expand Up @@ -53,12 +53,6 @@ static const bool DEFAULT_WHITELISTRELAY = true;
static const bool DEFAULT_WHITELISTFORCERELAY = false;
/** Default for -minrelaytxfee, minimum relay fee for transactions */
static const unsigned int DEFAULT_MIN_RELAY_TX_FEE = 1000;
//! -maxtxfee default
static const CAmount DEFAULT_TRANSACTION_MAXFEE = COIN / 10;
//! Discourage users to set fees higher than this amount (in satoshis) per kB
static const CAmount HIGH_TX_FEE_PER_KB = COIN / 100;
//! -maxtxfee will warn if called with a higher fee than this amount (in satoshis)
static const CAmount HIGH_MAX_TX_FEE = 100 * HIGH_TX_FEE_PER_KB;
/** Default for -limitancestorcount, max number of in-mempool ancestors */
static const unsigned int DEFAULT_ANCESTOR_LIMIT = 25;
/** Default for -limitancestorsize, maximum kilobytes of tx + all in-mempool ancestors */
Expand Down Expand Up @@ -162,8 +156,6 @@ extern bool fCheckpointsEnabled;
extern size_t nCoinCacheUsage;
/** A fee rate smaller than this is considered zero fee (for relaying, mining and transaction creation) */
extern CFeeRate minRelayTxFee;
/** Absolute maximum transaction fee (in satoshis) used by wallet and mempool (rejects high fee in sendrawtransaction) */
extern CAmount maxTxFee;
/** If the tip is older than this (in seconds), the node is considered to be in initial block download. */
extern int64_t nMaxTipAge;
extern bool fEnableReplacement;
Expand Down
4 changes: 2 additions & 2 deletions src/wallet/feebumper.cpp
Expand Up @@ -162,9 +162,9 @@ Result CreateTransaction(const CWallet* wallet, const uint256& txid, const CCoin
}

// Check that in all cases the new fee doesn't violate maxTxFee
const CAmount max_tx_fee = wallet->chain().maxTxFee();
const CAmount max_tx_fee = wallet->m_default_max_tx_fee;
if (new_fee > max_tx_fee) {
errors.push_back(strprintf("Specified or calculated fee %s is too high (cannot be higher than maxTxFee %s)",
errors.push_back(strprintf("Specified or calculated fee %s is too high (cannot be higher than -maxtxfee %s)",
FormatMoney(new_fee), FormatMoney(max_tx_fee)));
return Result::WALLET_ERROR;
}
Expand Down
2 changes: 1 addition & 1 deletion src/wallet/fees.cpp
Expand Up @@ -22,7 +22,7 @@ CAmount GetMinimumFee(const CWallet& wallet, unsigned int nTxBytes, const CCoinC
{
CAmount fee_needed = GetMinimumFeeRate(wallet, coin_control, feeCalc).GetFee(nTxBytes);
// Always obey the maximum
const CAmount max_tx_fee = wallet.chain().maxTxFee();
const CAmount max_tx_fee = wallet.m_default_max_tx_fee;
if (fee_needed > max_tx_fee) {
fee_needed = max_tx_fee;
if (feeCalc) feeCalc->reason = FeeReason::MAXTXFEE;
Expand Down
6 changes: 2 additions & 4 deletions src/wallet/init.cpp
Expand Up @@ -48,6 +48,8 @@ void WalletInit::AddWalletOptions() const
gArgs.AddArg("-fallbackfee=<amt>", strprintf("A fee rate (in %s/kB) that will be used when fee estimation has insufficient data (default: %s)",
CURRENCY_UNIT, FormatMoney(DEFAULT_FALLBACK_FEE)), false, OptionsCategory::WALLET);
gArgs.AddArg("-keypool=<n>", strprintf("Set key pool size to <n> (default: %u)", DEFAULT_KEYPOOL_SIZE), false, OptionsCategory::WALLET);
gArgs.AddArg("-maxtxfee=<amt>", 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)), false, OptionsCategory::DEBUG_TEST);
gArgs.AddArg("-mintxfee=<amt>", strprintf("Fees (in %s/kB) smaller than this are considered zero fee for transaction creation (default: %s)",
CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MINFEE)), false, OptionsCategory::WALLET);
gArgs.AddArg("-paytxfee=<amt>", strprintf("Fee (in %s/kB) to add to transactions you send (default: %s)",
Expand Down Expand Up @@ -124,10 +126,6 @@ bool WalletInit::ParameterInteraction() const
if (gArgs.GetArg("-prune", 0) && gArgs.GetBoolArg("-rescan", false))
return InitError(_("Rescans are not possible in pruned mode. You will need to use -reindex which will download the whole blockchain again."));

if (::minRelayTxFee.GetFeePerK() > HIGH_TX_FEE_PER_KB)
InitWarning(AmountHighWarn("-minrelaytxfee") + " " +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see how it makes sense to disable this particular warning about the -minrelaytxfee setting in non-wallet builds even though it is not a wallet setting, because the warning message is about wallet behavior.

But would it make sense to to keep some warning about really high -minrelaytxfee settings even in non-wallet builds?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning was added as a wallet warning in #8486.

This PR is meant to be a pure refactor (except for the updated log message) so I'm not going to add the suggested new warning for a high -minrelaytxfee to the node, but agree that it could make sense to add it in a separate PR. I'd also suggest that in a follow-up PR we change this warning to only trigger if -minrelayfee is greater than walletInstance->m_default_max_tx_fee.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: these warnings will now show for each wallet at startup rather than just once for the node, so that's also a minor behaviour change when starting with multiple wallets.

_("The wallet will avoid paying less than the minimum relay fee."));

return true;
}

Expand Down
25 changes: 24 additions & 1 deletion src/wallet/wallet.cpp
Expand Up @@ -4204,6 +4204,29 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
return nullptr;
}
}

if (gArgs.IsArgSet("-maxtxfee"))
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

μ-nit: bracket

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't catch this when moving the code from init.cpp. Sorry!

CAmount nMaxFee = 0;
if (!ParseMoney(gArgs.GetArg("-maxtxfee", ""), nMaxFee)) {
chain.initError(AmountErrMsg("maxtxfee", gArgs.GetArg("-maxtxfee", "")));
return nullptr;
}
if (nMaxFee > HIGH_MAX_TX_FEE) {
chain.initWarning(_("-maxtxfee is set very high! Fees this large could be paid on a single transaction."));
}
if (CFeeRate(nMaxFee, 1000) < chain.relayMinFee()) {
chain.initError(strprintf(_("Invalid amount for -maxtxfee=<amount>: '%s' (must be at least the minrelay fee of %s to prevent stuck transactions)"),
gArgs.GetArg("-maxtxfee", ""), chain.relayMinFee().ToString()));
return nullptr;
}
walletInstance->m_default_max_tx_fee = nMaxFee;
}

if (chain.relayMinFee().GetFeePerK() > HIGH_TX_FEE_PER_KB)
chain.initWarning(AmountHighWarn("-minrelaytxfee") + " " +
_("The wallet will avoid paying less than the minimum relay fee."));

walletInstance->m_confirm_target = gArgs.GetArg("-txconfirmtarget", DEFAULT_TX_CONFIRM_TARGET);
walletInstance->m_spend_zero_conf_change = gArgs.GetBoolArg("-spendzeroconfchange", DEFAULT_SPEND_ZEROCONF_CHANGE);
walletInstance->m_signal_rbf = gArgs.GetBoolArg("-walletrbf", DEFAULT_WALLET_RBF);
Expand Down Expand Up @@ -4394,7 +4417,7 @@ bool CWalletTx::AcceptToMemoryPool(interfaces::Chain::Lock& locked_chain, CValid
// user could call sendmoney in a loop and hit spurious out of funds errors
// because we think that this newly generated transaction's change is
// unavailable as we're not yet aware that it is in the mempool.
bool ret = locked_chain.submitToMemoryPool(tx, pwallet->chain().maxTxFee(), state);
bool ret = locked_chain.submitToMemoryPool(tx, pwallet->m_default_max_tx_fee, state);
fInMempool |= ret;
return ret;
}
Expand Down
8 changes: 8 additions & 0 deletions src/wallet/wallet.h
Expand Up @@ -73,6 +73,12 @@ static const unsigned int DEFAULT_TX_CONFIRM_TARGET = 6;
static const bool DEFAULT_WALLET_RBF = false;
static const bool DEFAULT_WALLETBROADCAST = true;
static const bool DEFAULT_DISABLE_WALLET = false;
//! -maxtxfee default
constexpr CAmount DEFAULT_TRANSACTION_MAXFEE{COIN / 10};
//! Discourage users to set fees higher than this amount (in satoshis) per kB
constexpr CAmount HIGH_TX_FEE_PER_KB{COIN / 100};
//! -maxtxfee will warn if called with a higher fee than this amount (in satoshis)
constexpr CAmount HIGH_MAX_TX_FEE{100 * HIGH_TX_FEE_PER_KB};

//! Pre-calculated constants for input size estimation in *virtual size*
static constexpr size_t DUMMY_NESTED_P2WPKH_INPUT_SIZE = 91;
Expand Down Expand Up @@ -983,6 +989,8 @@ class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notific
CFeeRate m_discard_rate{DEFAULT_DISCARD_FEE};
OutputType m_default_address_type{DEFAULT_ADDRESS_TYPE};
OutputType m_default_change_type{DEFAULT_CHANGE_TYPE};
/** Absolute maximum transaction fee (in satoshis) used by default for the wallet */
CAmount m_default_max_tx_fee{DEFAULT_TRANSACTION_MAXFEE};

bool NewKeyPool();
size_t KeypoolCountExternalKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
Expand Down