+1, I'd also prefer rejecting unknown values here. The option hasn't been in a release yet, so there's nothing to stay compatible with, but once 32.0 ships, turning the fallback into an error would be a breaking change.
It's also inconsistent with the rest of the RPC: a bad estimate_mode already throws RPC_INVALID_PARAMETER a few lines above, while a typo like {"fee_rate_estimator": "mempool"} silently returns the combined estimate (which may come from block_policy), with nothing telling the caller their input was ignored.
Suggested patch: FeeRateEstimatorTypeFromString() returns std::optional, the RPC throws -8 with the same message format as estimate_mode, the "Unknown values are treated as auto" help sentence is dropped, and the tests are updated. "none" is rejected too, since it was never released.
<details>
<summary>diff</summary>
diff --git a/src/rpc/fees.cpp b/src/rpc/fees.cpp
index 566f9712ea..af177a2a2b 100644
--- a/src/rpc/fees.cpp
+++ b/src/rpc/fees.cpp
@@ -59,8 +59,7 @@ static RPCMethod estimatesmartfee()
"block policy fee rate estimate is returned instead. An error is returned only if the block\n"
"policy fee rate estimate is unavailable.\n"
"\"block_policy\" uses only the block policy fee rate estimator.\n"
- "\"mempool_policy\" uses only the mempool fee rate estimator.\n"
- "Unknown values are treated as \"auto\"."},
+ "\"mempool_policy\" uses only the mempool fee rate estimator."},
{"verbosity", RPCArg::Type::NUM, RPCArg::Default{1},
"1 returns feerate or errors. 2 also returns \"mempool_health_statistics\"."},
},
@@ -112,8 +111,12 @@ static RPCMethod estimatesmartfee()
{"fee_rate_estimator", UniValueType(UniValue::VSTR)},
{"verbosity", UniValueType(UniValue::VNUM)},
}, /*fAllowNull=*/true, /*fStrict=*/true);
- const auto fee_rate_estimator{FeeRateEstimatorTypeFromString(
+ const auto parsed_fee_rate_estimator{FeeRateEstimatorTypeFromString(
options["fee_rate_estimator"].isNull() ? "auto" : options["fee_rate_estimator"].get_str())};
+ if (!parsed_fee_rate_estimator) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid fee_rate_estimator parameter, must be one of: \"auto\", \"block_policy\", \"mempool_policy\"");
+ }
+ const FeeRateEstimatorType fee_rate_estimator{*parsed_fee_rate_estimator};
bool conservative{fee_mode == FeeEstimateMode::CONSERVATIVE};
int verbosity{ParseVerbosity(options["verbosity"], /*default_verbosity=*/1, /*allow_bool=*/false)};
UniValue result(UniValue::VOBJ);
diff --git a/src/test/fees_util_tests.cpp b/src/test/fees_util_tests.cpp
index 70ae23fc48..4c9d37f224 100644
--- a/src/test/fees_util_tests.cpp
+++ b/src/test/fees_util_tests.cpp
@@ -20,9 +20,11 @@ BOOST_AUTO_TEST_CASE(fee_rate_estimator_type_from_string)
BOOST_CHECK(FeeRateEstimatorTypeFromString("auto") == FeeRateEstimatorType::AUTO);
BOOST_CHECK(FeeRateEstimatorTypeFromString("block_policy") == FeeRateEstimatorType::BLOCK_POLICY);
BOOST_CHECK(FeeRateEstimatorTypeFromString("mempool_policy") == FeeRateEstimatorType::MEMPOOL_POLICY);
- BOOST_CHECK(FeeRateEstimatorTypeFromString("unknown") == FeeRateEstimatorType::AUTO);
- // Unknown values, including the previous "none" spelling, are coerced to AUTO.
- BOOST_CHECK(FeeRateEstimatorTypeFromString("none") == FeeRateEstimatorType::AUTO);
+ BOOST_CHECK(FeeRateEstimatorTypeFromString("Block_Policy") == FeeRateEstimatorType::BLOCK_POLICY);
+ BOOST_CHECK(!FeeRateEstimatorTypeFromString("unknown"));
+ BOOST_CHECK(!FeeRateEstimatorTypeFromString(""));
+ // The previous "none" spelling was never released, so it is rejected too.
+ BOOST_CHECK(!FeeRateEstimatorTypeFromString("none"));
}
BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/util/fees.cpp b/src/util/fees.cpp
index 291c92f318..a77689d01b 100644
--- a/src/util/fees.cpp
+++ b/src/util/fees.cpp
@@ -7,6 +7,7 @@
#include <util/strencodings.h>
#include <cassert>
+#include <optional>
#include <string_view>
std::string_view FeeRateEstimatorTypeToString(FeeRateEstimatorType feerate_estimator_type)
@@ -23,10 +24,11 @@ std::string_view FeeRateEstimatorTypeToString(FeeRateEstimatorType feerate_estim
assert(false);
}
-FeeRateEstimatorType FeeRateEstimatorTypeFromString(std::string_view feerate_estimator_type)
+std::optional<FeeRateEstimatorType> FeeRateEstimatorTypeFromString(std::string_view feerate_estimator_type)
{
const auto normalized{ToLower(feerate_estimator_type)};
+ if (normalized == "auto") return FeeRateEstimatorType::AUTO;
if (normalized == "block_policy") return FeeRateEstimatorType::BLOCK_POLICY;
if (normalized == "mempool_policy") return FeeRateEstimatorType::MEMPOOL_POLICY;
- return FeeRateEstimatorType::AUTO;
+ return std::nullopt;
}
diff --git a/src/util/fees.h b/src/util/fees.h
index 43ffa82dcd..ce566b68ec 100644
--- a/src/util/fees.h
+++ b/src/util/fees.h
@@ -9,6 +9,7 @@
#include <util/expected.h>
#include <util/feefrac.h>
+#include <optional>
#include <string>
#include <string_view>
#include <utility>
@@ -90,6 +91,6 @@ inline const FeeRateEstimation& FeeRateEstimationRef(const util::Expected<FeeRat
}
std::string_view FeeRateEstimatorTypeToString(FeeRateEstimatorType feerate_estimator_type);
-FeeRateEstimatorType FeeRateEstimatorTypeFromString(std::string_view feerate_estimator_type);
+std::optional<FeeRateEstimatorType> FeeRateEstimatorTypeFromString(std::string_view feerate_estimator_type);
#endif // BITCOIN_UTIL_FEES_H
diff --git a/test/functional/rpc_estimatefee.py b/test/functional/rpc_estimatefee.py
index abaaf86ad0..de606392a3 100755
--- a/test/functional/rpc_estimatefee.py
+++ b/test/functional/rpc_estimatefee.py
@@ -36,6 +36,9 @@ class EstimateFeeTest(BitcoinTestFramework):
assert_raises_rpc_error(-3, "JSON value of type string is not of expected type number", self.nodes[0].estimaterawfee, 1, 'foo')
assert_raises_rpc_error(-8, 'Invalid estimate_mode parameter, must be one of: "unset", "economical", "conservative"', self.nodes[0].estimatesmartfee, 1, 'foo')
+ for fee_rate_estimator in ["foo", "none", ""]:
+ assert_raises_rpc_error(-8, 'Invalid fee_rate_estimator parameter, must be one of: "auto", "block_policy", "mempool_policy"',
+ self.nodes[0].estimatesmartfee, 1, 'ECONOMICAL', {'fee_rate_estimator': fee_rate_estimator})
assert_raises_rpc_error(-8, "Unknown named parameter fee_rate_estimator", self.nodes[0].estimatesmartfee, 1, fee_rate_estimator=True)
assert_raises_rpc_error(-3, "Unexpected key block_policy_only", self.nodes[0].estimatesmartfee, 1, 'ECONOMICAL', {'block_policy_only': True})
# extra params
@@ -54,9 +57,7 @@ class EstimateFeeTest(BitcoinTestFramework):
self.nodes[0].estimatesmartfee(1, 'conservative')
self.nodes[0].estimatesmartfee(1, 'ECONOMICAL', {"fee_rate_estimator": "block_policy"})
self.nodes[0].estimatesmartfee(1, 'ECONOMICAL', {"fee_rate_estimator": "mempool_policy"})
- self.nodes[0].estimatesmartfee(1, 'ECONOMICAL', {"fee_rate_estimator": "foo"})
self.nodes[0].estimatesmartfee(1, 'ECONOMICAL', {'verbosity': 1, 'fee_rate_estimator': "auto"})
- self.nodes[0].estimatesmartfee(1, 'ECONOMICAL', {'verbosity': 1, 'fee_rate_estimator': "none"})
self.nodes[0].estimaterawfee(1)
self.nodes[0].estimaterawfee(1, None)
</details>
