Skip to content

rest: Do not re-use the function name ParseHashStr (core_io.h) for a function with different behaviour in rest.cpp #14288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
practicalswift wants to merge 1 commit into from
Closed

rest: Do not re-use the function name ParseHashStr (core_io.h) for a function with different behaviour in rest.cpp #14288

practicalswift wants to merge 1 commit into from

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Sep 21, 2018

Do not re-use the function name ParseHashStr (core_io.h) for a function with different behaviour in rest.cpp.

Before:

src/core_io.h:uint256 ParseHashStr(const std::string&, const std::string& strName);
src/core_read.cpp:uint256 ParseHashStr(const std::string& strHex, const std::string& strName)
src/rest.cpp:static bool ParseHashStr(const std::string& strReq, uint256& v)

After:

src/core_io.h:uint256 ParseHashStr(const std::string&, const std::string& strName);
src/core_read.cpp:uint256 ParseHashStr(const std::string& strHex, const std::string& strName)

@practicalswift
rest: Do not re-use the function name ParseHashStr (core_io.h) for a …
6f24de8 
…function with different behaviour in rest.cpp
@maflcko
Copy link
Member

maflcko commented Sep 21, 2018

Reminds me of "Drop ParseHashUV in favor of calling ParseHashStr" #13422 by @Empact

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 21, 2018

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.

@DrahtBot DrahtBot mentioned this pull request Sep 21, 2018
Merged
@l2a5b1
Copy link
Contributor

l2a5b1 commented Sep 24, 2018

I don't mind the change, but also don't mind keeping it as it is. :)
I suspect either way is fine.

@maflcko
Copy link
Member

maflcko commented Sep 24, 2018

Couldn't you remove this function and just call ParseHashStr?

@practicalswift
Copy link
Contributor Author

practicalswift commented Sep 24, 2018 •
edited
Loading

@MarcoFalke Yes, but I don't want to change any behaviour and the functions are not identical. ParseHashStr throws std::runtime_error in case of non-hex input which ParseHexHashStr does not. Also note the length requirement of ParseHexHashStr.

uint256 ParseHashStr(const std::string& strHex, const std::string& strName)
{
    if (!IsHex(strHex)) // Note: IsHex("") is false
        throw std::runtime_error(strName + " must be hexadecimal string (not '" + strHex + "')");

    uint256 result;
    result.SetHex(strHex);
    return result;
}

// Suggested new function name: ParseHexHashStr
static bool ParseHashStr(const std::string& strReq, uint256& v)
{
    if (!IsHex(strReq) || (strReq.size() != 64))
        return false;

    v.SetHex(strReq);
    return true;
}

@Empact Empact mentioned this pull request Sep 24, 2018
Merged
@practicalswift
Copy link
Contributor Author

practicalswift commented Sep 24, 2018

Closing in favour of #14307 which fixes this issue more elegantly :-)

maflcko pushed a commit that referenced this pull request Sep 27, 2018
Merge #14307: Consolidate redundant implementations of ParseHashStr
01211ce 
9c5af58 Consolidate redundant implementations of ParseHashStr (Ben Woosley)

Pull request description:

  This change:
  * adds a length check to all calls to `ParseHashStr`, appropriate given its use to populate
    a 256-bit number from a hex str
  * allows the caller to handle the failure, which allows for the more
    appropriate `JSONRPCError` on failure in `prioritisetransaction` rpc

  Relative to #14288

Tree-SHA512: baa791147e5ceb3c30c70df3981aaf807bf7d4a90a0be3625540b59aa4b9a9d303a452bfef18bf167cbb833ef9591b4ef5948bf4a1ce67b421d804ae8d20ea53
@practicalswift practicalswift deleted the ParseHashStr-duplicate branch April 10, 2021 19:36
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Jul 30, 2021
@pravblockc
Merge bitcoin#14307: Consolidate redundant implementations of ParseH…
a8f037c 
…ashStr

9c5af58 Consolidate redundant implementations of ParseHashStr (Ben Woosley)

Pull request description:

  This change:
  * adds a length check to all calls to `ParseHashStr`, appropriate given its use to populate
    a 256-bit number from a hex str
  * allows the caller to handle the failure, which allows for the more
    appropriate `JSONRPCError` on failure in `prioritisetransaction` rpc

  Relative to bitcoin#14288

Tree-SHA512: baa791147e5ceb3c30c70df3981aaf807bf7d4a90a0be3625540b59aa4b9a9d303a452bfef18bf167cbb833ef9591b4ef5948bf4a1ce67b421d804ae8d20ea53
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Aug 4, 2021
@pravblockc
Merge bitcoin#14307: Consolidate redundant implementations of ParseHa…
59ae418 
…shStr

9c5af58 Consolidate redundant implementations of ParseHashStr (Ben Woosley)

Pull request description:

  This change:
  * adds a length check to all calls to `ParseHashStr`, appropriate given its use to populate
    a 256-bit number from a hex str
  * allows the caller to handle the failure, which allows for the more
    appropriate `JSONRPCError` on failure in `prioritisetransaction` rpc

  Relative to bitcoin#14288

Tree-SHA512: baa791147e5ceb3c30c70df3981aaf807bf7d4a90a0be3625540b59aa4b9a9d303a452bfef18bf167cbb833ef9591b4ef5948bf4a1ce67b421d804ae8d20ea53
gades pushed a commit to cosanta/cosanta-core that referenced this pull request May 4, 2022
@gades
Merge bitcoin#14307: Consolidate redundant implementations of ParseHa…
2bf05ef 
…shStr

9c5af58 Consolidate redundant implementations of ParseHashStr (Ben Woosley)

Pull request description:

  This change:
  * adds a length check to all calls to `ParseHashStr`, appropriate given its use to populate
    a 256-bit number from a hex str
  * allows the caller to handle the failure, which allows for the more
    appropriate `JSONRPCError` on failure in `prioritisetransaction` rpc

  Relative to bitcoin#14288

Tree-SHA512: baa791147e5ceb3c30c70df3981aaf807bf7d4a90a0be3625540b59aa4b9a9d303a452bfef18bf167cbb833ef9591b4ef5948bf4a1ce67b421d804ae8d20ea53
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@practicalswift @maflcko @DrahtBot @l2a5b1