Consistently validate txid / blockhash length and encoding in rpc calls #13424

pull Empact wants to merge 1 commits into bitcoin:master from Empact:parse-hash-v changing 14 files +58 −59

Empact commented at 6:49 PM on June 8, 2018: member

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

in src/rpc/mining.cpp:250 in a20130c023 outdated

 246 | @@ -247,7 +247,7 @@ static UniValue prioritisetransaction(const JSONRPCRequest& request)
 247 |  
 248 |      LOCK(cs_main);
 249 |  
 250 | -    uint256 hash = ParseHashStr(request.params[0].get_str(), "txid");
 251 | +    uint256 hash = ParseHashV(request.params[0], "txid");

Empact commented at 6:50 PM on June 8, 2018:

Note this trades ParseHashStr's std::runtime_error-based encoding-only test for ParseHashV's JSONRPCError-based length and encoding tests.

Empact force-pushed on Jun 8, 2018
Empact force-pushed on Jun 8, 2018
fanquake added the label Refactoring on Jun 9, 2018
promag commented at 9:25 PM on June 10, 2018: member

How about getchaintxstats?https://github.com/bitcoin/bitcoin/blob/56f69360dc98bd68704f19646a84d045788d199e/src/rpc/blockchain.cpp#L1575

And there may be other examples.

Concept ACK.
Empact force-pushed on Jun 10, 2018
Empact force-pushed on Jun 10, 2018
Empact force-pushed on Jun 11, 2018
Empact force-pushed on Jun 11, 2018
Empact commented at 2:58 AM on June 11, 2018: member

Thanks @promag, I audited uint256S use and found 4 more cases. Also added more testing wherever the test outcomes changed.
Empact force-pushed on Jun 11, 2018
Empact renamed this:
~~Consistently use ParseHashV to validate hash inputs in rpc~~
Validate txid / blockhash length and encoding separately from validity/presence in rpc calls
on Jun 11, 2018
Empact renamed this:
~~Validate txid / blockhash length and encoding separately from validity/presence in rpc calls~~
Validate txid / blockhash length and encoding separately from validity / presence in rpc calls
on Jun 11, 2018
Empact renamed this:
~~Validate txid / blockhash length and encoding separately from validity / presence in rpc calls~~
Validate txid / blockhash length and encoding separate from validity / presence in rpc calls
on Jun 11, 2018
Empact force-pushed on Jun 11, 2018
Empact commented at 3:30 AM on June 11, 2018: member

Made the get_str call guaranteed, which means non-string values fail with -1, "JSON value is not a string as expected", rather than a length failure.
Empact renamed this:
~~Validate txid / blockhash length and encoding separate from validity / presence in rpc calls~~
Consistently validate txid / blockhash length and encoding in rpc calls
on Jun 11, 2018
DrahtBot commented at 8:27 PM on June 14, 2018: member
Note to reviewers: This pull request conflicts with the following ones:
- #14307 (Consolidate redundant implementations of ParseHashStr by Empact)
- #12153 (Avoid permanent cs_main lock in getblockheader by promag)
- #10973 (Refactor: separate wallet from node by ryanofsky)
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.
DrahtBot commented at 10:35 PM on August 2, 2018: member

The last travis run for this pull request was 52 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
DrahtBot closed this on Aug 2, 2018
DrahtBot reopened this on Aug 2, 2018
kallewoof commented at 4:30 AM on August 7, 2018: member

utACK

Small nit: you're mixing uint256 hash = ParseHashV(...) and uint256 hash(ParseHashV(...)). Would be cool to do only one (latter one looks better IMO but either works).

Consistently use ParseHashV to validate hash inputs in rpc

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

5eb20f81d9

Empact force-pushed on Aug 7, 2018
Empact commented at 4:51 PM on August 7, 2018: member

@kallewoof fair enough since I'm touching those lines anyway. Updated.
kallewoof commented at 5:47 AM on August 8, 2018: member

re-utACK 5eb20f81d9568284dca735e4f770f41a48aa5660
MarcoFalke commented at 12:59 PM on August 8, 2018: member

utACK 5eb20f81d9568284dca735e4f770f41a48aa5660
MarcoFalke merged this on Sep 24, 2018
MarcoFalke closed this on Sep 24, 2018
MarcoFalke referenced this in commit 37612099ec on Sep 24, 2018
deadalnix referenced this in commit 07cc225cf5 on Feb 18, 2020
ftrader referenced this in commit 39392f72df on Apr 16, 2020
pravblockc referenced this in commit 6faf808461 on Jul 23, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#13420 Consolidate hash parsing into core_io.h