Consolidate hash parsing into core_io.h #13420

pull Empact wants to merge 3 commits into bitcoin:master from Empact:parse-hash changing 12 files +44 −70

Empact commented at 9:51 AM on June 8, 2018: member
This consolidates separate hash parsing functions from rpc/server.cpp, rest.cpp, and core_read.cpp into a low-level parser ParseHashStr in core_read.cpp, and an rpc-level ParseHash 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 or blockhashes, 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.
- A bunch of calls that did not error meaningfully on bad hashes now do.
Empact force-pushed on Jun 8, 2018
Empact force-pushed on Jun 8, 2018
Empact force-pushed on Jun 8, 2018
Empact force-pushed on Jun 8, 2018

Drop ParseHashUV in favor of calling ParseHashStr

The one existing call in bitcoin-tx already validates the call will
pass via checkObject.

fcadb98d89

Empact force-pushed on Jun 8, 2018
Empact force-pushed on Jun 8, 2018
Empact force-pushed on Jun 8, 2018
Empact force-pushed on Jun 8, 2018
Empact force-pushed on Jun 8, 2018
Empact force-pushed on Jun 8, 2018
Empact force-pushed on Jun 8, 2018
Empact force-pushed on Jun 8, 2018
Empact force-pushed on Jun 8, 2018

Consolidate hash parsing in core_io.h

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.

edbacc6c97

Use ParseHash consistently

These calls were previously unvalidated. As of this change, only
hashes in test and qt are unvalidated.

81d0eb8af4

Empact force-pushed on Jun 8, 2018
Empact commented at 12:37 PM on June 8, 2018: member

TODO: add tests for the new rpc errors on the various calls that now consistently validate input
DrahtBot commented at 5:18 PM on June 8, 2018: member
Note to reviewers: This pull request conflicts with the following ones:
- #13399 (rpc: Add submitblockheader by MarcoFalke)
- #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.
MarcoFalke commented at 5:39 PM on June 8, 2018: member

Concept ACK.
Empact commented at 5:49 PM on June 8, 2018: member

Going to split this up to ease review.
Empact closed this on Jun 8, 2018
MarcoFalke referenced this in commit 2b770080a4 on Jun 15, 2018
Empact deleted the branch on Jul 2, 2018
MarcoFalke referenced this in commit 37612099ec on Sep 24, 2018
PastaPastaPasta referenced this in commit 1c002dd1b6 on Jul 7, 2020
pravblockc referenced this in commit 6faf808461 on Jul 23, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Empact

DrahtBot

MarcoFalke

Linked (view graph)

#13422 Drop ParseHashUV in favor of calling ParseHashStr #13424 Consistently validate txid / blockhash length and encoding in rpc calls