rpc: Strict type checking for RPC boolean parameters #26213

maflcko commented at 12:46 PM on September 30, 2022: member

No description provided.

maflcko commented at 12:47 PM on September 30, 2022: member

<!-- They are confusing, because: * getBool is different from get_bool, but not documented as being different * isTrue and isFalse return false for types other than VBOOL This is a source of bugs, such as [#26147 (review)](/bitcoin-bitcoin/26147/#discussion_r983741878) For example, this diff passes on master, but fails on the changes here: ```diff diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index ad8ba06824..f59bd81e20 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -190,7 +190,7 @@ class NetTest(BitcoinTestFramework): # Add an IPv6 address to the address manager. ipv6_addr = "1233:3432:2434:2343:3234:2345:6546:4534" - self.nodes[0].addpeeraddress(address=ipv6_addr, port=8333) + self.nodes[0].addpeeraddress(address=ipv6_addr, port=8333, tried={"foo":3}) # Add 10,000 IPv4 addresses to the address manager. Due to the way bucket # and bucket positions are calculated, some of these addresses will collide.

luke-jr commented at 12:52 PM on September 30, 2022: member

Suggest splitting up any fixes from the API change at least, so the fixes can be backported.

maflcko force-pushed on Sep 30, 2022

maflcko commented at 1:04 PM on September 30, 2022: member

There is no API change, but I've split the commit into three (bugfix / refactor)

DrahtBot added the label RPC/REST/ZMQ on Sep 30, 2022

in src/rpc/net.cpp:734 in fa2334c606 outdated

 728 | @@ -729,9 +729,7 @@ static RPCHelpMan setban()
 729 |          if (!request.params[2].isNull())
 730 |              banTime = request.params[2].getInt<int64_t>();
 731 |  
 732 | -        bool absolute = false;
 733 | -        if (request.params[3].isTrue())
 734 | -            absolute = true;
 735 | +        const bool absolute{request.params[3].isNull() ? false : request.params[3].get_bool()};

furszy commented at 1:55 PM on September 30, 2022:

        const bool absolute{!request.params[3].isNull() && request.params[3].get_bool()};

maflcko commented at 2:53 PM on September 30, 2022:

why?

furszy commented at 3:00 PM on September 30, 2022:

it's just a tiny nit, same expression. No need to apply it if you don't like it.

maflcko commented at 3:08 PM on September 30, 2022:

I think it is worse, because it will make it impossible to change the default value without also changing the code logic.

luke-jr commented at 10:20 PM on October 3, 2022:

Agree with Marco, the current version is better

in src/util/system.cpp:482 in 4444376c22 outdated

 491 | @@ -492,7 +492,7 @@ std::vector<std::string> ArgsManager::GetArgs(const std::string& strArg) const
 492 |  {
 493 |      std::vector<std::string> result;
 494 |      for (const util::SettingsValue& value : GetSettingsList(strArg)) {
 495 | -        result.push_back(value.isFalse() ? "0" : value.isTrue() ? "1" : value.get_str());
 496 | +        result.push_back(value.isBool() ? (value.get_bool() ? "1" : "0") : value.get_str());

furszy commented at 2:44 PM on September 30, 2022:

not for this PR, but would be good to place the "1" and "0" strings in constants, so we don't hardcode them when they are needed (here, in SettingToString, SettingToInt, and inside the univalue implementation).

maflcko commented at 2:55 PM on September 30, 2022:

I presume SettingsValue is supposed to be independent of UniValue implementation details. Otherwise we could just return the UniValue internal string representation?

furszy commented at 3:04 PM on September 30, 2022:

ok yeah, agree. leaving out the UniValue implementation, at least all settings methods could share the same constant (SettingToString, ArgsManager::SoftSetBoolArg, ArgsManager::GetArgs).

but.. just a small thing anyway.

maflcko commented at 3:12 PM on September 30, 2022:

Maybe a inline std::string BoolToSetting(bool val) { return val ? "1" : "0"; } helper?

furszy commented at 4:05 PM on September 30, 2022:

sounds good 👍🏼

furszy approved

furszy commented at 2:44 PM on September 30, 2022: member

ACK 4444376c

ryanofsky commented at 3:34 PM on September 30, 2022: contributor

I can definitely understand why the getBool alias for isTrue is a footgun and should be removed. I also think the changes in this PR replacing isTrue with get_bool are appropriate in places where values are type checked, and you want to throw an exception when an unexpected type is encountered.

But in the places where you are replacing isFalse and isTrue with things like isBool && get_bool I think it makes code less clear. getBool really seems like a footgun. But isFalse isTrue don't seem like problems any more than isNull is. They just seem like things that are appropriate to use some places, not appropriate other places.

ryanofsky commented at 3:36 PM on September 30, 2022: contributor

Also agree with luke it would be nice to put the bugfix in a separate minimal PR so it easier to backport and easier to see what the actual bug is.

maflcko commented at 3:39 PM on September 30, 2022: member

But isFalse isTrue don't seem like problems any more than isNull is. They just seem like things that are appropriate to use some places, not appropriate other places.

The UniValue is*() checks are meant for types, with the exception of isTrue/isFalse, which are checking the type and value, which seems confusing to me.

Also agree with luke it would be nice to put the bugfix in a separate minimal PR so it easier to backport and easier to see what the actual bug is.

The commit starts with "bugfix:", so it should be possible to find it.

ryanofsky commented at 4:42 PM on September 30, 2022: contributor

The commit starts with "bugfix:", so it should be possible to find it.

I'm sure it's possible to figure it out, but it would be easier if something said what the bug was: steps to reproduce, expected behavior, actual behavior

DrahtBot commented at 10:42 PM on September 30, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, furszy
Stale ACK	aureleoules

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26129 (wallet, refactor: FundTransaction(): return out-params as util::Result structure by theStack)
#25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko commented at 10:51 AM on October 3, 2022: member

steps to reproduce, expected behavior, actual behavior

This is also there, see #26213 (comment)

DrahtBot added the label Needs rebase on Nov 15, 2022

maflcko force-pushed on Nov 16, 2022

maflcko commented at 3:02 PM on November 16, 2022: member

Rebased. Should be trivial to re-review.

DrahtBot removed the label Needs rebase on Nov 16, 2022

aureleoules approved

aureleoules commented at 6:13 PM on December 1, 2022: member

ACK bbbb41ecd586db4ed51b8bbb217af26e04ff0315

maflcko commented at 7:01 PM on December 1, 2022: member

@ryanofsky Did you want to re-NACK this?

in src/rpc/net.cpp:943 in fa493c7796 outdated

 939 | @@ -942,7 +940,7 @@ static RPCHelpMan addpeeraddress()
 940 |  
 941 |      const std::string& addr_string{request.params[0].get_str()};
 942 |      const auto port{request.params[1].getInt<uint16_t>()};
 943 | -    const bool tried{request.params[2].isTrue()};
 944 | +    const bool tried{request.params[2].isNull() ? false : request.params[2].get_bool()};

ryanofsky commented at 3:18 PM on December 2, 2022:

In commit "bugfix: Add missing type check to isTrue in RPC code" (fa493c77965778c54b1206b3f8ab11c9b4d3beb3)

The code changes in this commit look great and make a lot of sense after you dig into the UniValue::get_bool() implementation.

But the commit description is bad! It's confusing to someone who wants to know what this bugfix does. For example, I consider myself to be pretty familiar with UniValue, but when I looked at this I didn't know what it did at all because I didn't remember that get_bool could throw exceptions, so therefore the new code could throw exceptions when old code wouldn't.

I also thought that specifying RPCArg::Type::BOOL, RPCArg::Default{false} might already enable type checking, so new type checking here might be redundant and also no effect.

Would suggest changing commit description to explicitly describe new behavior:

bugfix: Strict type checking for RPC boolean parameters

Prior to this change setban, addpeeraddress, walletcreatefundedpsbt, methods allowed non-boolean and non-null values to be passed as boolean parameters. Any string, number, array, or object value that was passed would be treated as false. After this change, passing any value except true, false, or null now triggers a JSON value is not of expected type error.

I also think:

For better review, this bugfix commit should be in a separate PR from the third commit which is changing the UniValue API.
It would be nice to add a functional test or unit test triggering at least one of the new exceptions to demonstrate that this change actually does have an effect, and to prevent possible regressions.
It might be nice to have a release note noting RPC type checking is stricter in this release, that some RPC calls that might have worked previously now result in errors.

maflcko commented at 1:58 PM on December 5, 2022:

Nice. I've stolen the description from you to add a release note. I've added a test. I've dropped the third commit for now.

in src/rpc/rawtransaction.cpp:307 in fa671a15d3 outdated

 303 | @@ -304,7 +304,7 @@ static RPCHelpMan createrawtransaction()
 304 |  
 305 |      std::optional<bool> rbf;
 306 |      if (!request.params[3].isNull()) {
 307 | -        rbf = request.params[3].isTrue();
 308 | +        rbf = request.params[3].get_bool();

ryanofsky commented at 3:45 PM on December 2, 2022:

In commit "refactor: Replace isTrue with get_bool" (fa671a15d3ab5f2466ee1066f6388ec67bc313ac)

The code changes in this commit look good, but it would be good to update the commit description to say why this doesn't change behavior. I'd add a sentence like:

in general replacing isTrue with get_bool is not equivalent because get_bool can throw exceptions, but in this case, exceptions won't happen because of RPCTypeCheck() and isNull() checks in the preceding code.

maflcko commented at 1:58 PM on December 5, 2022:

Nice, I've stolen the description

in src/univalue/include/univalue.h:78 in bbbb41ecd5 outdated

  73 |      const UniValue& operator[](size_t index) const;
  74 |      bool exists(const std::string& key) const { size_t i; return findKey(key, i); }
  75 |  
  76 |      bool isNull() const { return (typ == VNULL); }
  77 | -    bool isTrue() const { return (typ == VBOOL) && (val == "1"); }
  78 | -    bool isFalse() const { return (typ == VBOOL) && (val != "1"); }

ryanofsky commented at 4:40 PM on December 2, 2022:

In commit "univalue: Remove confusing getBool/isTrue/isFalse" (bbbb41ecd586db4ed51b8bbb217af26e04ff0315)

I love the part of this commit which is dropping the getBool method. I think that method is useless and dangerous and too easy to confuse with the get_bool method. I also agree the changes in earlier commits removing isTrue isFalse in RPC code were improvements and make that code more robust.

But I don't think removing the isFalse and isTrue calls everywhere is a good idea. Being able to call isFalse and isTrue directly in util code seems clearer and more explicit to me than introducing a new IsNegated method and having to go through that. All of the changes to util code in this commit seem to me like they make it more confusing and less robust, so I'd be happier if they were reverted and this commit only dropped getBool, leaving valid isTrue and isFalse calls alone. Just my opinion though, and I can understand the opposing view.

maflcko commented at 1:59 PM on December 5, 2022:

Removed commit for now

ryanofsky approved

ryanofsky commented at 4:48 PM on December 2, 2022: contributor

I think there are still some issues with this PR, and at a minimum the commit descriptions should be updated to more clearly describe what is changing (suggestions in review comments below). But the code looks correct so I will add my code review ACK bbbb41ecd586db4ed51b8bbb217af26e04ff0315. I'd much more happily ACK this if it:

Had better commit descriptions that made it obvious how behavior was changing or not changing
Moved the third commit changing the UniValue API (bbbb41ecd586db4ed51b8bbb217af26e04ff0315) into a separate PR from the first commit changing RPC behavior (fa493c77965778c54b1206b3f8ab11c9b4d3beb3). The small second cleanup commit (fa671a15d3ab5f2466ee1066f6388ec67bc313ac) could go in either of these two PRs. But trying to do RPC bugfixes and UniValue API changes in the same PR IMO makes the RPC bugfixes more confusing to understand, and the UniValue API change harder to discuss and agree about.

maflcko renamed this:
~~univalue: Remove confusing getBool/isTrue/isFalse~~
rpc: Add missing bool type checks in RPC code
on Dec 5, 2022

maflcko renamed this:
~~rpc: Add missing bool type checks in RPC code~~
rpc: Strict type checking for RPC boolean parameters
on Dec 5, 2022

maflcko force-pushed on Dec 5, 2022

maflcko commented at 1:59 PM on December 5, 2022: member

Force pushed to add a test and docs, thanks!

bugfix: Strict type checking for RPC boolean parameters fa2cc5d1d6

refactor: Replace isTrue with get_bool

This makes the code more robust, see previous commit.

In general replacing isTrue with get_bool is not equivalent because
get_bool can throw exceptions, but in this case, exceptions won't happen
because of RPCTypeCheck() and isNull() checks in the preceding code.

fa0153e609

maflcko force-pushed on Dec 7, 2022

ryanofsky commented at 2:44 PM on December 9, 2022: contributor

Code review ACK fa0153e609caf61a59efb0779e754861edc1684d

Looks great, thanks for taking various suggestions!

ryanofsky referenced this in commit 293849a260 on Dec 9, 2022

furszy approved

furszy commented at 2:49 AM on December 10, 2022: member

Code ACK fa0153e6

fanquake merged this on Dec 10, 2022

fanquake closed this on Dec 10, 2022

fanquake referenced this in commit a28fb36c47 on Dec 10, 2022

maflcko deleted the branch on Dec 10, 2022

sidhujag referenced this in commit b82cb5df21 on Dec 10, 2022

sidhujag referenced this in commit 70af601862 on Dec 10, 2022

janus referenced this in commit ad9af78aff on Jan 20, 2023

bitcoin locked this on Dec 10, 2023