rpc: Strict type checking for RPC boolean parameters #26213

maflcko · 2022-09-30T12:46:09Z

No description provided.

maflcko · 2022-09-30T12:47:21Z

No description provided.

luke-jr · 2022-09-30T12:52:28Z

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

maflcko · 2022-09-30T13:04:52Z

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

furszy

ACK 4444376

src/rpc/net.cpp

furszy · 2022-09-30T14:44:43Z

src/util/system.cpp

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


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).

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

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.

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

sounds good 👍🏼

ryanofsky · 2022-09-30T15:34:33Z

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 · 2022-09-30T15:36:33Z

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 · 2022-09-30T15:39:41Z

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 · 2022-09-30T16:42:56Z

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 · 2022-09-30T22:42:48Z

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 · 2022-10-03T10:51:03Z

steps to reproduce, expected behavior, actual behavior

This is also there, see #26213 (comment)

maflcko · 2022-11-16T15:02:26Z

Rebased. Should be trivial to re-review.

aureleoules

ACK bbbb41e

maflcko · 2022-12-01T19:01:03Z

@ryanofsky Did you want to re-NACK this?

ryanofsky

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 bbbb41e. 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 (bbbb41e) into a separate PR from the first commit changing RPC behavior (fa493c7). The small second cleanup commit (fa671a1) 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.

src/rpc/net.cpp

src/rpc/rawtransaction.cpp

src/univalue/include/univalue.h

maflcko · 2022-12-05T13:59:49Z

Force pushed to add a test and docs, thanks!

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.

ryanofsky

Code review ACK fa0153e

Looks great, thanks for taking various suggestions!

Drop UniValue::getBool method because it is easy to confuse with the UniValue::get_bool method, and could potentially cause bugs. Unlike get_bool, getBool doesn't ensure that the value is a boolean and returns false for all integer, string, array, and object values instead of throwing an exceptions. The getBool method is also redundant because it is an alias for isTrue. There were only 5 getBool() calls in the codebase, so this commit replaces them with isTrue() or get_bool() calls as appropriate. These changes were originally made by MarcoFalke in bitcoin#26213 but were dropped to limit the scope of that PR. Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

furszy

Code ACK fa0153e

293849a univalue: Remove confusing getBool method (Ryan Ofsky) Pull request description: Drop `UniValue::getBool` method because it is easy to confuse with the `UniValue::get_bool` method, and could potentially cause bugs. Unlike `get_bool`, `getBool` doesn't ensure that the value is a boolean and returns false for all integer, string, array, and object values instead of throwing an exception. The `getBool` method is also redundant because it is an alias for `isTrue`. There were only 5 `getBool()` calls in the codebase, so this commit replaces them with `isTrue()` or `get_bool()` calls as appropriate. These changes were originally made by MarcoFalke in bitcoin/bitcoin#26213 but were dropped to limit the scope of that PR. ACKs for top commit: justinpickering: ACK bitcoin/bitcoin@293849a sipa: utACK 293849a w0xlt: ACK bitcoin/bitcoin@293849a hebasto: ACK 293849a, also verified that the removed `getBool` method is not mentioned in any docs: furszy: ACK 293849a Tree-SHA512: 9fbfe5e2083410f123b18703a0cc0161ecbbb4958f331c9ff808dcfcc6ad499b0e896abd16fb8ea200c53ba29878db9812ce141e59cc5e0fd174741b0bcb192d

…ters fa0153e refactor: Replace isTrue with get_bool (MarcoFalke) fa2cc5d bugfix: Strict type checking for RPC boolean parameters (MarcoFalke) Pull request description: ACKs for top commit: ryanofsky: Code review ACK fa0153e furszy: Code ACK fa0153e Tree-SHA512: b221f823c69d90c94447fd491071ff3659cfd512872b495ebc3e711f50633351974102c9ef7e50fa4a393c4131d349adea8fd41cc9d66f1f31e1f5e7a5f78757

293849a univalue: Remove confusing getBool method (Ryan Ofsky) Pull request description: Drop `UniValue::getBool` method because it is easy to confuse with the `UniValue::get_bool` method, and could potentially cause bugs. Unlike `get_bool`, `getBool` doesn't ensure that the value is a boolean and returns false for all integer, string, array, and object values instead of throwing an exception. The `getBool` method is also redundant because it is an alias for `isTrue`. There were only 5 `getBool()` calls in the codebase, so this commit replaces them with `isTrue()` or `get_bool()` calls as appropriate. These changes were originally made by MarcoFalke in bitcoin#26213 but were dropped to limit the scope of that PR. ACKs for top commit: justinpickering: ACK bitcoin@293849a sipa: utACK 293849a w0xlt: ACK bitcoin@293849a hebasto: ACK 293849a, also verified that the removed `getBool` method is not mentioned in any docs: furszy: ACK 293849a Tree-SHA512: 9fbfe5e2083410f123b18703a0cc0161ecbbb4958f331c9ff808dcfcc6ad499b0e896abd16fb8ea200c53ba29878db9812ce141e59cc5e0fd174741b0bcb192d

Drop UniValue::getBool method because it is easy to confuse with the UniValue::get_bool method, and could potentially cause bugs. Unlike get_bool, getBool doesn't ensure that the value is a boolean and returns false for all integer, string, array, and object values instead of throwing an exceptions. The getBool method is also redundant because it is an alias for isTrue. There were only 5 getBool() calls in the codebase, so this commit replaces them with isTrue() or get_bool() calls as appropriate. These changes were originally made by MarcoFalke in bitcoin/bitcoin#26213 but were dropped to limit the scope of that PR. Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

maflcko mentioned this pull request Sep 30, 2022

refactor: Run type check against RPCArgs (1/2) #26039

Merged

maflcko force-pushed the 2209-uni-bool- branch from fa81eda to 4444376 Compare September 30, 2022 13:03

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

furszy approved these changes Sep 30, 2022

View reviewed changes

DrahtBot mentioned this pull request Oct 1, 2022

wallet, refactor: FundTransaction(): return out-params as util::Result structure #26129

Closed

DrahtBot mentioned this pull request Oct 18, 2022

refactor: Implement missing error checking for ArgsManager flags #16545

Draft

DrahtBot mentioned this pull request Nov 11, 2022

test: Split overly large util_tests.cpp file #26489

Merged

DrahtBot added the Needs rebase label Nov 15, 2022

maflcko force-pushed the 2209-uni-bool- branch from 4444376 to bbbb41e Compare November 16, 2022 14:50

DrahtBot removed the Needs rebase label Nov 16, 2022

aureleoules approved these changes Dec 1, 2022

View reviewed changes

ryanofsky approved these changes Dec 2, 2022

View reviewed changes

src/rpc/net.cpp Show resolved Hide resolved

src/rpc/rawtransaction.cpp Show resolved Hide resolved

src/univalue/include/univalue.h Outdated Show resolved Hide resolved

maflcko changed the title ~~univalue: Remove confusing getBool/isTrue/isFalse~~ rpc: Add missing bool type checks in RPC code Dec 5, 2022

maflcko changed the title ~~rpc: Add missing bool type checks in RPC code~~ rpc: Strict type checking for RPC boolean parameters Dec 5, 2022

maflcko force-pushed the 2209-uni-bool- branch 2 times, most recently from fa20f92 to fa5c5e7 Compare December 5, 2022 13:59

MarcoFalke added 2 commits December 7, 2022 17:55

bugfix: Strict type checking for RPC boolean parameters

fa2cc5d

maflcko force-pushed the 2209-uni-bool- branch from fa5c5e7 to fa0153e Compare December 7, 2022 16:57

ryanofsky reviewed Dec 9, 2022

View reviewed changes

ryanofsky mentioned this pull request Dec 9, 2022

univalue: Remove confusing getBool method #26673

Merged

furszy approved these changes Dec 10, 2022

View reviewed changes

fanquake merged commit 3b5fb6e into bitcoin:master Dec 10, 2022

maflcko deleted the 2209-uni-bool-💉 branch December 10, 2022 12:08

bitcoin locked and limited conversation to collaborators Dec 10, 2023

rpc: Strict type checking for RPC boolean parameters #26213

rpc: Strict type checking for RPC boolean parameters #26213

Uh oh!

Conversation

maflcko commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Sep 30, 2022

Uh oh!

maflcko commented Sep 30, 2022

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

furszy Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

furszy Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Sep 30, 2022

Uh oh!

ryanofsky commented Sep 30, 2022

Uh oh!

maflcko commented Sep 30, 2022

Uh oh!

ryanofsky commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

maflcko commented Oct 3, 2022

Uh oh!

maflcko commented Nov 16, 2022

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 1, 2022

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Dec 5, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Sep 30, 2022 •

edited

Loading

maflcko commented Sep 30, 2022 •

edited

Loading

ryanofsky commented Sep 30, 2022 •

edited

Loading

DrahtBot commented Sep 30, 2022 •

edited

Loading

ryanofsky left a comment •

edited

Loading