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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoind -regtest -fallbackfee=0.0000100 -daemon
Bitcoin Core starting
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest loadwallet abubakar-test
{
"name": "abubakar-test"
}
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest send '{"bcrt1qledjqv5a23zjysjvk0204zgu50z2zwch9h88k8": 0.1}' 1 Economical
{
"txid": "fdbfc6e00efef18dcfde0f0f6c428bf7dbacb776cf0f1c710d410eba20dcfcf6",
"complete": true
}
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest send '{"bcrt1qledjqv5a23zjysjvk0204zgu50z2zwch9h88k8": 0.1}' 1
error code: -8
error message:
Specify estimate_mode
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest send '{"bcrt1qledjqv5a23zjysjvk0204zgu50z2zwch9h88k8": 0.1}' 1 unset
error code: -8
error message:
Specify estimate_mode
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest send '{"bcrt1qledjqv5a23zjysjvk0204zgu50z2zwch9h88k8": 0.1}' 1 Unset
{
"txid": "937b0205df5a56414e4751b162510e17955788f09670c76086e825253ac46e23",
"complete": true
}
The last RPC command on should not pass at all, because Unset is an invalid estimatemode
Same behavior on your PR head eed7296bba12dcf0df7c063d335dbf34bf358f9c
abubakarismail@Abubakars-MacBook-Pro bitcoin % git cherry-pick eed7296bba12dcf0df7c063d335dbf34bf358f9c
Auto-merging src/wallet/rpc/spend.cpp
[01-2024-register-validation-interface-fix 3d94c6c3ca0] rpc: validate fee estimation mode case insensitive
Author: Torkel Rogstad <torkel@rogstad.io>
Date: Thu Jan 4 11:24:07 2024 +0100
1 file changed, 1 insertion(+), 1 deletion(-)
abubakarismail@Abubakars-MacBook-Pro bitcoin % make
Making all in src
CXX wallet/rpc/libbitcoin_wallet_a-spend.o
AR libbitcoin_wallet.a
GEN obj/build.h
CXX libbitcoin_util_a-clientversion.o
AR libbitcoin_util.a
CXXLD bitcoind
CXXLD bitcoin-cli
CXXLD bitcoin-tx
CXXLD bitcoin-wallet
CXXLD bitcoin-util
CXXLD test/test_bitcoin
Making all in doc/man
make[1]: Nothing to be done for `all'.
make[1]: Nothing to be done for `all-am'.
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest stop
Bitcoin Core stopping
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoind -regtest -fallbackfee=0.0000100 -daemon
Bitcoin Core starting
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest loadwallet abubakar-test
{
"name": "abubakar-test"
}
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest send '{"bcrt1qledjqv5a23zjysjvk0204zgu50z2zwch9h88k8": 0.1}' 1 Unset
{
"txid": "91e9f79d7b653023a522cd0bed715bc482b60eaf1036d35bb7c57d3fd630eecc",
"complete": true
}
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest send '{"bcrt1qledjqv5a23zjysjvk0204zgu50z2zwch9h88k8": 0.1}' 1 unset
error code: -8
error message:
Specify estimate_mode
This patch seems to have fix the issue
diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
index b26dd864951..172971cd9d2 100644
--- a/src/wallet/rpc/spend.cpp
+++ b/src/wallet/rpc/spend.cpp
@@ -71,7 +71,7 @@ static void InterpretFeeEstimationInstructions(const UniValue& conf_target, cons
} else {
options.pushKV("fee_rate", fee_rate);
}
- if (!options["conf_target"].isNull() && (options["estimate_mode"].isNull() || (options["estimate_mode"].get_str() == "unset"))) {
+ if (!options["conf_target"].isNull() && (options["estimate_mode"].isNull() || (ToLower(options["estimate_mode"].get_str()) == "unset"))) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Specify estimate_mode");
}
}
abubakarismail@Abubakars-MacBook-Pro bitcoin % make
Making all in src
CXX wallet/rpc/libbitcoin_wallet_a-spend.o
AR libbitcoin_wallet.a
GEN obj/build.h
CXX libbitcoin_util_a-clientversion.o
AR libbitcoin_util.a
CXXLD bitcoind
CXXLD bitcoin-cli
CXXLD bitcoin-tx
CXXLD bitcoin-wallet
CXXLD bitcoin-util
CXXLD test/test_bitcoin
Making all in doc/man
make[1]: Nothing to be done for `all'.
make[1]: Nothing to be done for `all-am'.
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest stop
Bitcoin Core stopping
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoind -regtest -fallbackfee=0.0000100 -daemon
Bitcoin Core starting
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest loadwallet abubakar-test
{
"name": "abubakar-test"
}
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest send '{"bcrt1qledjqv5a23zjysjvk0204zgu50z2zwch9h88k8": 0.1}' 1 Unset
error code: -8
error message:
Specify 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
Thanks for the excellent review @ismaelsadeeq , very much appreciated. You're correct in your findings! I've updated the logic, and added functional tests for this
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.
self.test_send(from_wallet=w0, to_wallet=w1, amount=1,
arg_estimate_mode=mode, arg_conf_target=1, add_to_wallet=False, expect_error = (-8, 'Specify estimate_mode')
)
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
assert_equal(res["complete"], True)
estimatemode is validated when passed with confirmation target
res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1,
arg_conf_target=1, arg_estimate_mode=mode, add_to_wallet=False
)
assert_equal(res["complete"], True)
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.
}
if (options["conf_target"].isNull() && !options["estimate_mode"].isNull()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "estimate_mode should be passed with conf_target");
}
CI failure is unrelated?
@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.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 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.
<sub>Debug: https://github.com/bitcoin/bitcoin/runs/20482987264</sub>
I believe CI failures are unrelated. Is it possible to force CI to re-run?
70 | @@ -71,9 +71,13 @@ static void InterpretFeeEstimationInstructions(const UniValue& conf_target, cons
71 | } else {
nit: iwyu
#include <util/strencodings.h>
Sorry, I don't understand what you mean. Do you want me to use one of the functions in strencodings to manipulate the fee_rate parameter before sending it to options.pushKV?
I meant Import what you use.
ToLower() function is declared here.
https://github.com/bitcoin/bitcoin/blob/4ad83ef09b772f3f1e8ea9ea3a8c43dcd9db8458/src/util/strencodings.h#L327
Ah, thanks. I'm very new to C++, so I didn't realize I could even use things that weren't imported. Didn't realize I should also include a header statement, since it compiled just fine! Added the include.
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
Rebased, let's see if that solves the CI failure.
Re-ACK be8ae64b82e2c5b003a8703668ce1751442288e4
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?
Concept NACK. i think the solution here is to update the documentation. AFAIK, none of our RPC APIs have historically been case insensitive, and i don't think there's a good reason to start here. Ideally, imo, commands and field names should be considered "just blobs", not subject to unicode casing rules.
Fee estimation modes are currently parsed case-insensitively here:
Which makes it possible to pass arguments case-insensitively to (at least) estimatesmartfee:
$ bitcoin-cli estimatesmartfee 3 economical
{
"feerate": 0.00022015,
"blocks": 3
}
$ bitcoin-cli estimatesmartfee 3 ECONOMICAL
{
"feerate": 0.00022015,
"blocks": 3
}
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.
It's a fair point to not want to introduce new instances of undesirable behavior. But I definitely don't think we should change existing behavior. That would break ALL users that use Go for interfacing with Bitcoin Core, and maybe other languages as well
This is probably more than you wanted to do, but I'd say it would be better to use 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.
@torkelrogstad, are you still working on this?
No, not really. Feel free to pick it up and get it in a mergeable state.