From fa41685f438bba00761423d464bbb8dc5ea7ddf6 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 18 Feb 2025 12:55:13 +0100 Subject: [PATCH 1/3] refactor: Remove IsArgSet guard when fallback value is provided Checking for IsArgSet before calling GetArg while providing the args 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. However, ignoring the fallback value is fragile, because it would not be sanitized. Fix all issues by sanitizing the fallback value. --- src/bitcoin-cli.cpp | 2 +- src/init.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index 75911d00874..defe5e4c4ec 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -1078,7 +1078,7 @@ static void ParseGetInfoResult(UniValue& result) } #endif - if (gArgs.IsArgSet("-color")) { + { const std::string color{gArgs.GetArg("-color", DEFAULT_COLOR_SETTING)}; if (color == "always") { should_colorize = true; diff --git a/src/init.cpp b/src/init.cpp index 3cfd301fbab..435c5952b13 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1018,14 +1018,14 @@ bool AppInitParameterInteraction(const ArgsManager& args) } } - if (args.IsArgSet("-blockmaxweight")) { + { const auto max_block_weight = args.GetIntArg("-blockmaxweight", DEFAULT_BLOCK_MAX_WEIGHT); if (max_block_weight > MAX_BLOCK_WEIGHT) { return InitError(strprintf(_("Specified -blockmaxweight (%d) exceeds consensus maximum block weight (%d)"), max_block_weight, MAX_BLOCK_WEIGHT)); } } - if (args.IsArgSet("-blockreservedweight")) { + { const auto block_reserved_weight = args.GetIntArg("-blockreservedweight", DEFAULT_BLOCK_RESERVED_WEIGHT); if (block_reserved_weight > MAX_BLOCK_WEIGHT) { return InitError(strprintf(_("Specified -blockreservedweight (%d) exceeds consensus maximum block weight (%d)"), block_reserved_weight, MAX_BLOCK_WEIGHT)); From fa3fb3c23fae287b161112b9b04cf0937a1051c6 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 18 Feb 2025 15:40:39 +0100 Subject: [PATCH 2/3] 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 435c5952b13..1a0a1f09e94 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1012,9 +1012,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)); } } @@ -1157,11 +1157,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 6810ab906c2..4202f70e8c8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3118,10 +3118,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(" ") + @@ -3131,8 +3131,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)) { @@ -3147,10 +3147,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(" ") + @@ -3162,10 +3162,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(" ") + @@ -3174,10 +3174,10 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri walletInstance->m_discard_rate = CFeeRate{discard_fee.value()}; } - if (args.IsArgSet("-paytxfee")) { - std::optional pay_tx_fee = ParseMoney(args.GetArg("-paytxfee", "")); + if (const auto arg{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(" ") + @@ -3188,15 +3188,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")); @@ -3204,18 +3204,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; } } From fa0cf040baa74b161b4b00ddea95bfb5aac0bb53 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Sun, 16 Mar 2025 14:18:40 +0100 Subject: [PATCH 3/3] doc: Remove outdated and stale todo comment If anything is left to be done, a new discussion issue or pull request can be created. --- src/init.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 1a0a1f09e94..b72ebf7b110 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1010,8 +1010,6 @@ bool AppInitParameterInteraction(const ArgsManager& args) return InitError(Untranslated("peertimeout must be a positive integer.")); } - // Sanity check argument for min fee for including tx in block - // TODO: Harmonize which arguments need sanity checking and where that happens if (const auto arg{args.GetArg("-blockmintxfee")}) { if (!ParseMoney(*arg)) { return InitError(AmountErrMsg("blockmintxfee", *arg));