The RPC docs for send claim that estimate_mode is case insensitive. This is not the case on master:
This PR fixes that, by converting the estimation mode to lower case before comparing to "unset". Note that the actual parsing of the estimation mode already happens case insensitively, further down in the same function.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ismaelsadeeq |
| Concept NACK | laanwj |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
Concept ACK
Thanks for trying to solve this issue.
This patch does not fix the issue.
Behaviour on master 65c05db660b2ca1d0076b0d8573a6760b3228068
0abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoind -regtest -fallbackfee=0.0000100 -daemon
1Bitcoin Core starting
2abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest loadwallet abubakar-test
3{
4 "name": "abubakar-test"
5}
6abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest send '{"bcrt1qledjqv5a23zjysjvk0204zgu50z2zwch9h88k8": 0.1}' 1 Economical
7{
8 "txid": "fdbfc6e00efef18dcfde0f0f6c428bf7dbacb776cf0f1c710d410eba20dcfcf6",
9 "complete": true
10}
11abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest send '{"bcrt1qledjqv5a23zjysjvk0204zgu50z2zwch9h88k8": 0.1}' 1
12error code: -8
13error message:
14Specify estimate_mode
15abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest send '{"bcrt1qledjqv5a23zjysjvk0204zgu50z2zwch9h88k8": 0.1}' 1 unset
16error code: -8
17error message:
18Specify estimate_mode
19abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest send '{"bcrt1qledjqv5a23zjysjvk0204zgu50z2zwch9h88k8": 0.1}' 1 Unset
20{
21 "txid": "937b0205df5a56414e4751b162510e17955788f09670c76086e825253ac46e23",
22 "complete": true
23}
The last RPC command on should not pass at all, because Unset is an invalid estimatemode
Same behavior on your PR head eed7296bba12dcf0df7c063d335dbf34bf358f9c
0abubakarismail@Abubakars-MacBook-Pro bitcoin % git cherry-pick eed7296bba12dcf0df7c063d335dbf34bf358f9c
1Auto-merging src/wallet/rpc/spend.cpp
2[01-2024-register-validation-interface-fix 3d94c6c3ca0] rpc: validate fee estimation mode case insensitive
3 Author: Torkel Rogstad <torkel@rogstad.io>
4 Date: Thu Jan 4 11:24:07 2024 +0100
5 1 file changed, 1 insertion(+), 1 deletion(-)
6abubakarismail@Abubakars-MacBook-Pro bitcoin % make
7Making all in src
8 CXX wallet/rpc/libbitcoin_wallet_a-spend.o
9 AR libbitcoin_wallet.a
10 GEN obj/build.h
11 CXX libbitcoin_util_a-clientversion.o
12 AR libbitcoin_util.a
13 CXXLD bitcoind
14 CXXLD bitcoin-cli
15 CXXLD bitcoin-tx
16 CXXLD bitcoin-wallet
17 CXXLD bitcoin-util
18 CXXLD test/test_bitcoin
19Making all in doc/man
20make[1]: Nothing to be done for `all'.
21make[1]: Nothing to be done for `all-am'.
22abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest stop
23Bitcoin Core stopping
24abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoind -regtest -fallbackfee=0.0000100 -daemon
25Bitcoin Core starting
26abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest loadwallet abubakar-test
27{
28 "name": "abubakar-test"
29}
30abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest send '{"bcrt1qledjqv5a23zjysjvk0204zgu50z2zwch9h88k8": 0.1}' 1 Unset
31{
32 "txid": "91e9f79d7b653023a522cd0bed715bc482b60eaf1036d35bb7c57d3fd630eecc",
33 "complete": true
34}
35abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest send '{"bcrt1qledjqv5a23zjysjvk0204zgu50z2zwch9h88k8": 0.1}' 1 unset
36error code: -8
37error message:
38Specify estimate_mode
This patch seems to have fix the issue
0diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
1index b26dd864951..172971cd9d2 100644
2--- a/src/wallet/rpc/spend.cpp
3+++ b/src/wallet/rpc/spend.cpp
4@@ -71,7 +71,7 @@ static void InterpretFeeEstimationInstructions(const UniValue& conf_target, cons
5 } else {
6 options.pushKV("fee_rate", fee_rate);
7 }
8- if (!options["conf_target"].isNull() && (options["estimate_mode"].isNull() || (options["estimate_mode"].get_str() == "unset"))) {
9+ if (!options["conf_target"].isNull() && (options["estimate_mode"].isNull() || (ToLower(options["estimate_mode"].get_str()) == "unset"))) {
10 throw JSONRPCError(RPC_INVALID_PARAMETER, "Specify estimate_mode");
11 }
12 }
0abubakarismail@Abubakars-MacBook-Pro bitcoin % make
1Making all in src
2 CXX wallet/rpc/libbitcoin_wallet_a-spend.o
3 AR libbitcoin_wallet.a
4 GEN obj/build.h
5 CXX libbitcoin_util_a-clientversion.o
6 AR libbitcoin_util.a
7 CXXLD bitcoind
8 CXXLD bitcoin-cli
9 CXXLD bitcoin-tx
10 CXXLD bitcoin-wallet
11 CXXLD bitcoin-util
12 CXXLD test/test_bitcoin
13Making all in doc/man
14make[1]: Nothing to be done for `all'.
15make[1]: Nothing to be done for `all-am'.
16abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest stop
17Bitcoin Core stopping
18abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoind -regtest -fallbackfee=0.0000100 -daemon
19Bitcoin Core starting
20abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest loadwallet abubakar-test
21{
22 "name": "abubakar-test"
23}
24abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest send '{"bcrt1qledjqv5a23zjysjvk0204zgu50z2zwch9h88k8": 0.1}' 1 Unset
25error code: -8
26error message:
27Specify estimate_mode
But its worth investigating why we can be able to send with an invalid estimatemode like Unset, or uNset in current master 65c05db660b2ca1d0076b0d8573a6760b3228068.
Please add a functional test that catches this issue
327+ assert res["complete"]
328+
329+ res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1,
330+ arg_estimate_mode=mode, fee_rate=1, add_to_wallet=False
331+ )
332+ assert res["complete"]
This should test for a failed send RPC call because estimatemode is unset.
It is passing because arg_conf_target was not given, currently estimatemode is only validated when a confirmation target is passed.
I omit fee_rate because you should not pass both confirmation target and fee rate.
0 self.test_send(from_wallet=w0, to_wallet=w1, amount=1,
1 arg_estimate_mode=mode, arg_conf_target=1, add_to_wallet=False, expect_error = (-8, 'Specify estimate_mode')
2 )
314+ assert res["complete"]
315+
316+ res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1,
317+ arg_estimate_mode=mode, add_to_wallet=False
318+ )
319+ assert res["complete"]
nit
0 assert_equal(res["complete"], True)
estimatemode is validated when passed with confirmation target
0 res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1,
1 arg_conf_target=1, arg_estimate_mode=mode, add_to_wallet=False
2 )
3 assert_equal(res["complete"], True)
4
70@@ -71,7 +71,7 @@ static void InterpretFeeEstimationInstructions(const UniValue& conf_target, cons
71 } else {
72 options.pushKV("fee_rate", fee_rate);
73 }
74- if (!options["conf_target"].isNull() && (options["estimate_mode"].isNull() || (options["estimate_mode"].get_str() == "unset"))) {
75+ if (!options["conf_target"].isNull() && (options["estimate_mode"].isNull() || (ToLower(options["estimate_mode"].get_str()) == "unset"))) {
76 throw JSONRPCError(RPC_INVALID_PARAMETER, "Specify estimate_mode");
77 }
I think we should also return error when estimatemode is passed without confirmation target and a a test for the case ?
Maybe in its own commit or a follow-up PR.
0 }
1 if (options["conf_target"].isNull() && !options["estimate_mode"].isNull()) {
2 throw JSONRPCError(RPC_INVALID_PARAMETER, "estimate_mode should be passed with conf_target");
3 }
@ismaelsadeeq I fixed the tests per your comments. Note that I pass the arguments through both the arg_ prefixed and non-prefixed versions. Figured we might as well validate that there’s no behavior changes between passing as positional arguments or through options.
Also added a new commits with tests that validates that estimate_mode is passed together with a confirmation target.
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
70@@ -71,9 +71,13 @@ static void InterpretFeeEstimationInstructions(const UniValue& conf_target, cons
71 } else {
nit: iwyu
0#include <util/strencodings.h>
strencodings to manipulate the fee_rate parameter before sending it to options.pushKV?
ToLower() function is declared here.
https://github.com/bitcoin/bitcoin/blob/4ad83ef09b772f3f1e8ea9ea3a8c43dcd9db8458/src/util/strencodings.h#L327
Code review ACK 51fdb4819ebda4d4a8f64c8d36a471af65a033c9
Just a single nit, happy to re ACK when fixed.
I’ve tested this on regtest and it passes as expected.
I believe CI failures are unrelated. Is it possible to force CI to re-run?
It has to do with #29234, If you rebase on master I think the CI will be happy, but IMO there is no need since it’s unrelated
Does it make sense to treat it case insensitive?
It hasn’t been treated that way, so it would be better to fix the docs?
Also, it seems better to be strict when parsing input, as opposed to loosely accepting different values?
Fee estimation modes are currently parsed case-insensitively here:
Which makes it possible to pass arguments case-insensitively to (at least) estimatesmartfee:
0$ bitcoin-cli estimatesmartfee 3 economical
1{
2 "feerate": 0.00022015,
3 "blocks": 3
4}
5
6$ bitcoin-cli estimatesmartfee 3 ECONOMICAL
7{
8 "feerate": 0.00022015,
9 "blocks": 3
10}
Changing this would be a breaking change, and for example break client libraries like the de-facto standard Go-based rpcclient (from the btcd libraries): https://github.com/btcsuite/btcd/blob/ae5533602c46f4ea14a4da725c19cb3db7d5eb88/btcjson/walletsvrcmds.go#L118-L122
Matter of fact, this PR was actually motivated by the fact that I couldn’t get the send RPC to properly work with rpcclient out-of-the-box, without resorting to tweaking the Go code. It therefore seems to me that the best solution here is to parse case-insensitively, due to other, already established practices in other RPCs
Right, there’s something to be said to keep that where it already snuck in, to not break the existing interface. And i guess if the other enumeration items already are parsed case-insensitively, doing that for “UNSET” makes sense too, otherwise it’s silly.
i just think the consensus is that this is not a direction the RPC interface is moving in in general, and i think introducing case-insensitive API was a mistake. But maybe getting rid of this instance has too much breaking impact now, I don’t know.
FeeModeFromString to re-use the existing (case-insensitive) parsing code, than to re-implement the parsing and use hard-coded “unset” strings in all call places.
This is probably more than you wanted to do, but I’d say it would be better to use
FeeModeFromStringto re-use the existing (case-insensitive) parsing code, than to re-implement the parsing and use hard-coded “unset” strings in all call places.
I agree. I think it would make more sense for FeeModeFromString to handle “unset” and to check its return value rather than having these repeated string comparisons. It’s a bit odd that the estimate mode is being validated in 3 places - InterpretFeeEstimationInstructions, SetFeeEstimateMode, and FeeModeFromString. These should be consolidated as much as possible.
