Consolidate hash parsing into core_io.h #13420
Closed
Conversation
This file contains hidden or 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
Empact
force-pushed
the
parse-hash
branch
4 times, most recently
from
4af3e97 to
c1b848f
Compare
The one existing call in bitcoin-tx already validates the call will pass via checkObject.
Empact
force-pushed
the
parse-hash
branch
9 times, most recently
from
40dbbf3 to
5048be5
Compare
This consolidates separate hash parsing functions from rpc/server.cpp, rest.cpp, and core_read.cpp into a low-level parser in core_read.cpp, and an rpc-level parser in rpc/server.cpp. Note the rpc parser calls through to the core_io parser. Behavior changes are: * Hashes are consistently validated to 64 characters, the expected length for 256 bits hexidecimal. Because all newly-tested calls were against txids, this is safe. * prioritisetransaction now throws an RPC_INVALID_PARAMETER rather than std::runtime_error on bad hash. This is in the context of rpc, so seems more appropriate.
These calls were previously unvalidated. As of this change, only hashes in test and qt are unvalidated.
Contributor
Author
|
TODO: add tests for the new rpc errors on the various calls that now consistently validate input |
Contributor
Note to reviewers: This pull request conflicts with the following ones:
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. |
Merged
Member
|
Concept ACK. |
Contributor
Author
|
Going to split this up to ease review. |
This was referenced Jun 8, 2018
maflcko
pushed a commit
that referenced
this pull request
Jun 15, 2018
abd2678 Drop ParseHashUV in favor of calling ParseHashStr (Ben Woosley) Pull request description: The one existing call already validates `get_str` will pass via `checkObject`: https://github.com/bitcoin/bitcoin/pull/13422/files#diff-8fe4d6985ee4acf8bfc1ed8db1e83cb5L586 Split from #13420 Tree-SHA512: 35dfa8c28d0c3ceac7a6de7f4eb4a44d912f4c31f5d21c9438f899566ca2b34851f1a58c3417355e55d0c33abb97385f4a47e034bfc8e3cdbbf5f73813ca0582
maflcko
pushed a commit
that referenced
this pull request
Sep 24, 2018
…ing in rpc calls 5eb20f8 Consistently use ParseHashV to validate hash inputs in rpc (Ben Woosley) Pull request description: ParseHashV validates the length and encoding of the string and throws an informative RPC error on failure, which is as good or better than these alternative calls. Note I switched ParseHashV to check string length first, because IsHex tests that the length is even, and an error like: "must be of length 64 (not 63, for X)" is much more informative than "must be hexadecimal string (not X)" in that case. Split from #13420 Tree-SHA512: f0786b41c0d7793ff76e4b2bb35547873070bbf7561d510029e8edb93f59176277efcd4d183b3185532ea69fc0bbbf3dbe9e19362e8017007ae9d51266cd78ae
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jul 7, 2020
abd2678 Drop ParseHashUV in favor of calling ParseHashStr (Ben Woosley) Pull request description: The one existing call already validates `get_str` will pass via `checkObject`: https://github.com/bitcoin/bitcoin/pull/13422/files#diff-8fe4d6985ee4acf8bfc1ed8db1e83cb5L586 Split from bitcoin#13420 Tree-SHA512: 35dfa8c28d0c3ceac7a6de7f4eb4a44d912f4c31f5d21c9438f899566ca2b34851f1a58c3417355e55d0c33abb97385f4a47e034bfc8e3cdbbf5f73813ca0582
pravblockc
pushed a commit
to pravblockc/dash
that referenced
this pull request
Jul 23, 2021
…nd encoding in rpc calls 5eb20f8 Consistently use ParseHashV to validate hash inputs in rpc (Ben Woosley) Pull request description: ParseHashV validates the length and encoding of the string and throws an informative RPC error on failure, which is as good or better than these alternative calls. Note I switched ParseHashV to check string length first, because IsHex tests that the length is even, and an error like: "must be of length 64 (not 63, for X)" is much more informative than "must be hexadecimal string (not X)" in that case. Split from bitcoin#13420 Tree-SHA512: f0786b41c0d7793ff76e4b2bb35547873070bbf7561d510029e8edb93f59176277efcd4d183b3185532ea69fc0bbbf3dbe9e19362e8017007ae9d51266cd78ae
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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 consolidates separate hash parsing functions from
rpc/server.cpp, rest.cpp, and core_read.cpp into a low-level
parser
ParseHashStrin core_read.cpp, and an rpc-levelParseHashin rpc/server.cpp.Note the rpc parser calls through to the core_io parser.
Behavior changes are:
length for 256 bits hexidecimal. Because all newly-tested calls
were against txids or blockhashes, this is safe.
prioritisetransactionnow throws anRPC_INVALID_PARAMETERratherthan std::runtime_error on bad hash. This is in the context of rpc,
so seems more appropriate.