From fa2b529f9269639abdb3ec5e152b66cbed04b890 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 18 Feb 2025 15:43:11 +0100 Subject: [PATCH] refactor: Remove redundant call to IsArgSet Checking for IsArgSet before calling GetArg while providing an arbitrary default value as fallback is both confusing and fragile. It is confusing, because the provided fallback is dead code. So it would be better to just call GetArg without a fallback. Even better would be to provide the true fallback value and sanitize it as if it were user-input, but this can be done in a follow-up. Removing the redundant call to IsArgSet will have to be done either way, so do it now. --- src/init.cpp | 13 ++++++------ src/node/mempool_args.cpp | 18 ++++++++-------- src/qt/intro.cpp | 4 ++-- src/wallet/wallet.cpp | 44 +++++++++++++++++++-------------------- 4 files changed, 39 insertions(+), 40 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 863bedb550a..443742df557 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1038,9 +1038,9 @@ bool AppInitParameterInteraction(const ArgsManager& args) // Sanity check argument for min fee for including tx in block // TODO: Harmonize which arguments need sanity checking and where that happens - if (args.IsArgSet("-blockmintxfee")) { - if (!ParseMoney(args.GetArg("-blockmintxfee", ""))) { - return InitError(AmountErrMsg("blockmintxfee", args.GetArg("-blockmintxfee", ""))); + if (const auto arg{args.GetArg("-blockmintxfee")}) { + if (!ParseMoney(*arg)) { + return InitError(AmountErrMsg("blockmintxfee", *arg)); } } @@ -1183,11 +1183,10 @@ bool CheckHostPortOptions(const ArgsManager& args) { "-port", "-rpcport", }) { - if (args.IsArgSet(port_option)) { - const std::string port = args.GetArg(port_option, ""); + if (const auto port{args.GetArg(port_option)}) { uint16_t n; - if (!ParseUInt16(port, &n) || n == 0) { - return InitError(InvalidPortErrMsg(port_option, port)); + if (!ParseUInt16(*port, &n) || n == 0) { + return InitError(InvalidPortErrMsg(port_option, *port)); } } } diff --git a/src/node/mempool_args.cpp b/src/node/mempool_args.cpp index e3bf5fb57cb..6dbba783815 100644 --- a/src/node/mempool_args.cpp +++ b/src/node/mempool_args.cpp @@ -48,20 +48,20 @@ util::Result ApplyArgsManOptions(const ArgsManager& argsman, const CChainP // incremental relay fee sets the minimum feerate increase necessary for replacement in the mempool // and the amount the mempool min fee increases above the feerate of txs evicted due to mempool limiting. - if (argsman.IsArgSet("-incrementalrelayfee")) { - if (std::optional inc_relay_fee = ParseMoney(argsman.GetArg("-incrementalrelayfee", ""))) { + if (const auto arg{argsman.GetArg("-incrementalrelayfee")}) { + if (std::optional inc_relay_fee = ParseMoney(*arg)) { mempool_opts.incremental_relay_feerate = CFeeRate{inc_relay_fee.value()}; } else { - return util::Error{AmountErrMsg("incrementalrelayfee", argsman.GetArg("-incrementalrelayfee", ""))}; + return util::Error{AmountErrMsg("incrementalrelayfee", *arg)}; } } - if (argsman.IsArgSet("-minrelaytxfee")) { - if (std::optional min_relay_feerate = ParseMoney(argsman.GetArg("-minrelaytxfee", ""))) { + if (const auto arg{argsman.GetArg("-minrelaytxfee")}) { + if (std::optional min_relay_feerate = ParseMoney(*arg)) { // High fee check is done afterward in CWallet::Create() mempool_opts.min_relay_feerate = CFeeRate{min_relay_feerate.value()}; } else { - return util::Error{AmountErrMsg("minrelaytxfee", argsman.GetArg("-minrelaytxfee", ""))}; + return util::Error{AmountErrMsg("minrelaytxfee", *arg)}; } } else if (mempool_opts.incremental_relay_feerate > mempool_opts.min_relay_feerate) { // Allow only setting incremental fee to control both @@ -71,11 +71,11 @@ util::Result ApplyArgsManOptions(const ArgsManager& argsman, const CChainP // Feerate used to define dust. Shouldn't be changed lightly as old // implementations may inadvertently create non-standard transactions - if (argsman.IsArgSet("-dustrelayfee")) { - if (std::optional parsed = ParseMoney(argsman.GetArg("-dustrelayfee", ""))) { + if (const auto arg{argsman.GetArg("-dustrelayfee")}) { + if (std::optional parsed = ParseMoney(*arg)) { mempool_opts.dust_relay_feerate = CFeeRate{parsed.value()}; } else { - return util::Error{AmountErrMsg("dustrelayfee", argsman.GetArg("-dustrelayfee", ""))}; + return util::Error{AmountErrMsg("dustrelayfee", *arg)}; } } diff --git a/src/qt/intro.cpp b/src/qt/intro.cpp index eeee13b2245..81814238554 100644 --- a/src/qt/intro.cpp +++ b/src/qt/intro.cpp @@ -140,9 +140,9 @@ Intro::Intro(QWidget *parent, int64_t blockchain_size_gb, int64_t chain_state_si const int min_prune_target_GB = std::ceil(MIN_DISK_SPACE_FOR_BLOCK_FILES / 1e9); ui->pruneGB->setRange(min_prune_target_GB, std::numeric_limits::max()); - if (gArgs.IsArgSet("-prune")) { + if (const auto arg{gArgs.GetIntArg("-prune")}) { m_prune_checkbox_is_default = false; - ui->prune->setChecked(gArgs.GetIntArg("-prune", 0) >= 1); + ui->prune->setChecked(*arg >= 1); ui->prune->setEnabled(false); } ui->pruneGB->setValue(m_prune_target_gb); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index dad84e38c95..115005c7aa8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3124,10 +3124,10 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri walletInstance->m_default_change_type = parsed.value(); } - if (args.IsArgSet("-mintxfee")) { - std::optional min_tx_fee = ParseMoney(args.GetArg("-mintxfee", "")); + if (const auto arg{args.GetArg("-mintxfee")}) { + std::optional min_tx_fee = ParseMoney(*arg); if (!min_tx_fee) { - error = AmountErrMsg("mintxfee", args.GetArg("-mintxfee", "")); + error = AmountErrMsg("mintxfee", *arg); return nullptr; } else if (min_tx_fee.value() > HIGH_TX_FEE_PER_KB) { warnings.push_back(AmountHighWarn("-mintxfee") + Untranslated(" ") + @@ -3137,8 +3137,8 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri walletInstance->m_min_fee = CFeeRate{min_tx_fee.value()}; } - if (args.IsArgSet("-maxapsfee")) { - const std::string max_aps_fee{args.GetArg("-maxapsfee", "")}; + if (const auto arg{args.GetArg("-maxapsfee")}) { + const std::string& max_aps_fee{*arg}; if (max_aps_fee == "-1") { walletInstance->m_max_aps_fee = -1; } else if (std::optional max_fee = ParseMoney(max_aps_fee)) { @@ -3153,10 +3153,10 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri } } - if (args.IsArgSet("-fallbackfee")) { - std::optional fallback_fee = ParseMoney(args.GetArg("-fallbackfee", "")); + if (const auto arg{args.GetArg("-fallbackfee")}) { + std::optional fallback_fee = ParseMoney(*arg); if (!fallback_fee) { - error = strprintf(_("Invalid amount for %s=: '%s'"), "-fallbackfee", args.GetArg("-fallbackfee", "")); + error = strprintf(_("Invalid amount for %s=: '%s'"), "-fallbackfee", *arg); return nullptr; } else if (fallback_fee.value() > HIGH_TX_FEE_PER_KB) { warnings.push_back(AmountHighWarn("-fallbackfee") + Untranslated(" ") + @@ -3168,10 +3168,10 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri // Disable fallback fee in case value was set to 0, enable if non-null value walletInstance->m_allow_fallback_fee = walletInstance->m_fallback_fee.GetFeePerK() != 0; - if (args.IsArgSet("-discardfee")) { - std::optional discard_fee = ParseMoney(args.GetArg("-discardfee", "")); + if (const auto arg{args.GetArg("-discardfee")}) { + std::optional discard_fee = ParseMoney(*arg); if (!discard_fee) { - error = strprintf(_("Invalid amount for %s=: '%s'"), "-discardfee", args.GetArg("-discardfee", "")); + error = strprintf(_("Invalid amount for %s=: '%s'"), "-discardfee", *arg); return nullptr; } else if (discard_fee.value() > HIGH_TX_FEE_PER_KB) { warnings.push_back(AmountHighWarn("-discardfee") + Untranslated(" ") + @@ -3180,12 +3180,12 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri walletInstance->m_discard_rate = CFeeRate{discard_fee.value()}; } - if (args.IsArgSet("-paytxfee")) { + if (const auto arg{args.GetArg("-paytxfee")}) { warnings.push_back(_("-paytxfee is deprecated and will be fully removed in v31.0.")); - std::optional pay_tx_fee = ParseMoney(args.GetArg("-paytxfee", "")); + std::optional pay_tx_fee = ParseMoney(*arg); if (!pay_tx_fee) { - error = AmountErrMsg("paytxfee", args.GetArg("-paytxfee", "")); + error = AmountErrMsg("paytxfee", *arg); return nullptr; } else if (pay_tx_fee.value() > HIGH_TX_FEE_PER_KB) { warnings.push_back(AmountHighWarn("-paytxfee") + Untranslated(" ") + @@ -3196,15 +3196,15 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri if (chain && walletInstance->m_pay_tx_fee < chain->relayMinFee()) { error = strprintf(_("Invalid amount for %s=: '%s' (must be at least %s)"), - "-paytxfee", args.GetArg("-paytxfee", ""), chain->relayMinFee().ToString()); + "-paytxfee", *arg, chain->relayMinFee().ToString()); return nullptr; } } - if (args.IsArgSet("-maxtxfee")) { - std::optional max_fee = ParseMoney(args.GetArg("-maxtxfee", "")); + if (const auto arg{args.GetArg("-maxtxfee")}) { + std::optional max_fee = ParseMoney(*arg); if (!max_fee) { - error = AmountErrMsg("maxtxfee", args.GetArg("-maxtxfee", "")); + error = AmountErrMsg("maxtxfee", *arg); return nullptr; } else if (max_fee.value() > HIGH_MAX_TX_FEE) { warnings.push_back(strprintf(_("%s is set very high! Fees this large could be paid on a single transaction."), "-maxtxfee")); @@ -3212,18 +3212,18 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri if (chain && CFeeRate{max_fee.value(), 1000} < chain->relayMinFee()) { error = strprintf(_("Invalid amount for %s=: '%s' (must be at least the minrelay fee of %s to prevent stuck transactions)"), - "-maxtxfee", args.GetArg("-maxtxfee", ""), chain->relayMinFee().ToString()); + "-maxtxfee", *arg, chain->relayMinFee().ToString()); return nullptr; } walletInstance->m_default_max_tx_fee = max_fee.value(); } - if (args.IsArgSet("-consolidatefeerate")) { - if (std::optional consolidate_feerate = ParseMoney(args.GetArg("-consolidatefeerate", ""))) { + if (const auto arg{args.GetArg("-consolidatefeerate")}) { + if (std::optional consolidate_feerate = ParseMoney(*arg)) { walletInstance->m_consolidate_feerate = CFeeRate(*consolidate_feerate); } else { - error = AmountErrMsg("consolidatefeerate", args.GetArg("-consolidatefeerate", "")); + error = AmountErrMsg("consolidatefeerate", *arg); return nullptr; } }