refactor: Revert additional univalue check in ParseSighashString #28162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
stickies-v
reviewed
Jul 26, 2023
jonatack
reviewed
Jul 26, 2023
TheCharlatan
force-pushed
the
kernelRmUnivalueFollowup
branch
from
cc266f9
to
5f38408
Compare
|
Updated cc266f9 -> 06199a9 (kernelRmUnivalueFollowup_0 -> kernelRmUnivalueFollowup_1, compare)
|
stickies-v
reviewed
Jul 27, 2023
TheCharlatan
force-pushed
the
kernelRmUnivalueFollowup
branch
from
5f38408
to
31f1f1a
Compare
This check is already done by the rpc parser. Re-doing it is adding dead code. Instead, throwing an exception when the assumption does not hold is the already correct behavior. To make the fuzz test more accurate and not swallow all runtime errors, add a check that the passed in UniValue sighash argument is either a string or null. Co-authored-by: stickies-v <stickies-v@protonmail.com>
TheCharlatan
force-pushed
the
kernelRmUnivalueFollowup
branch
from
31f1f1a
to
06199a9
Compare
|
lgtm ACK 06199a9 |
stickies-v
reviewed
Jul 27, 2023
| @@ -67,7 +67,7 @@ FUZZ_TARGET(parse_univalue, .init = initialize_parse_univalue) | |||
| } catch (const std::runtime_error&) { | |||
| } | |||
| try { | |||
| (void)ParseSighashString(univalue); | |||
| if (univalue.isNull() || univalue.isStr()) (void)ParseSighashString(univalue); | |||
There was a problem hiding this comment.
Ooh actually, would this be better? Equally clear (I think), but we still feed it unexpected types.
diff --git a/src/test/fuzz/parse_univalue.cpp b/src/test/fuzz/parse_univalue.cpp
index a3d6ab6375..df3410eab8 100644
--- a/src/test/fuzz/parse_univalue.cpp
+++ b/src/test/fuzz/parse_univalue.cpp
@@ -67,8 +67,10 @@ FUZZ_TARGET(parse_univalue, .init = initialize_parse_univalue)
} catch (const std::runtime_error&) {
}
try {
- if (univalue.isNull() || univalue.isStr()) (void)ParseSighashString(univalue);
+ (void)ParseSighashString(univalue);
} catch (const UniValue&) {
+ } catch (std::runtime_error&) {
+ if(univalue.isNull() || univalue.isStr()) throw; //a non-null, non-string univalue is expected to throw on .get_str()
}
try {
(void)AmountFromValue(univalue);
There was a problem hiding this comment.
Might also want to catch it NOT throwing an error when it ought to.
|
Tested ACK 06199a9 Verified relevant tests still pass after cherry-picking 647d95a from #28166 to this branch. I don't mind re-ACKing if you like the suggestion in #28162 (comment) (mind the spaces on the last line of its diff or run clang-format on it). |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 9, 2023
…rseSighashString 06199a9 refactor: Revert addition of univalue sighash string check (TheCharlatan) 0b47c16 doc: Correct release-notes for sighashtype exceptions (TheCharlatan) Pull request description: This is a follow up for bitcoin#28113. The string type check is already done by the rpc parser / RPCHelpMan. Re-doing it is adding dead code. Instead, throwing an exception when the assumption does not hold is the already correct behavior. Pointed out in this [comment](https://github.com/bitcoin/bitcoin/pull/28113/files#r1274568557). Also correct the release note for the correct sighashtype exception change. There is no change in the handling of non-string sighashtype arugments. Pointed out in this [comment](https://github.com/bitcoin/bitcoin/pull/28113/files#r1274567555). ACKs for top commit: MarcoFalke: lgtm ACK 06199a9 jonatack: Tested ACK 06199a9 stickies-v: ACK 06199a9 Tree-SHA512: 3faa6b3d2247624c0973df8d79c09fbf1f90ffb99f1be484e359b528f485c31affea45976759bd206e4c81cbb54ebba5ad0ef4127d1deacbfe2a58153fcc94ee
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow up for #28113.
The string type check is already done by the rpc parser / RPCHelpMan. Re-doing it is adding dead code. Instead, throwing an exception when the assumption does not hold is the already correct behavior. Pointed out in this comment.
Also correct the release note for the correct sighashtype exception change. There is no change in the handling of non-string sighashtype arugments. Pointed out in this comment.