Skip to content
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

kernel: Remove UniValue from kernel library #28113

Merged
merged 2 commits into from
Jul 25, 2023
Merged

kernel: Remove UniValue from kernel library #28113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
10eb3a9
kernel: Split ParseSighashString
TheCharlatan Jul 21, 2023
6960c81
kernel: Remove Univalue from kernel library
TheCharlatan Jul 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions doc/release-notes-28113.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
RPC Wallet
----------

- The `signrawtransactionwithkey`, `signrawtransactionwithwallet`,
`walletprocesspsbt` and `descriptorprocesspsbt` calls now return more
specific RPC_INVALID_PARAMETER instead of RPC_PARSE_ERROR if their
sighashtype argument is malformed or not a string.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this true? Before on master this will already throw:

test_framework.authproxy.JSONRPCException: Wrong type passed:
{
    "Position 3 (sighashtype)": "JSON value of type bool is not of expected type string"
} (-3)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is very wrong :(

Before a RPC_PARSE_ERROR was raised if the argument is the wrong type, and a RPC_MISC_ERROR if the sighash type could not be parsed.

Now a RPC_PARSE_ERROR is still raised if the argument is the wrong type, and a RPC_INVALID_PARAMETER if the sighash type could not be parsed.

Will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#28162

4 changes: 2 additions & 2 deletions src/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -896,8 +896,8 @@ if BUILD_BITCOIN_KERNEL_LIB
lib_LTLIBRARIES += $(LIBBITCOINKERNEL)

libbitcoinkernel_la_LDFLAGS = $(AM_LDFLAGS) -no-undefined $(RELDFLAGS) $(PTHREAD_FLAGS)
libbitcoinkernel_la_LIBADD = $(LIBBITCOIN_CRYPTO) $(LIBUNIVALUE) $(LIBLEVELDB) $(LIBMEMENV) $(LIBSECP256K1)
libbitcoinkernel_la_CPPFLAGS = $(AM_CPPFLAGS) -I$(builddir)/obj -I$(srcdir)/secp256k1/include -DBUILD_BITCOIN_INTERNAL $(BOOST_CPPFLAGS) $(LEVELDB_CPPFLAGS) -I$(srcdir)/$(UNIVALUE_INCLUDE_DIR_INT)
libbitcoinkernel_la_LIBADD = $(LIBBITCOIN_CRYPTO) $(LIBLEVELDB) $(LIBMEMENV) $(LIBSECP256K1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woohoo!

libbitcoinkernel_la_CPPFLAGS = $(AM_CPPFLAGS) -I$(builddir)/obj -I$(srcdir)/secp256k1/include -DBUILD_BITCOIN_INTERNAL $(BOOST_CPPFLAGS) $(LEVELDB_CPPFLAGS)

# libbitcoinkernel requires default symbol visibility, explicitly specify that
# here so that things still work even when user configures with
Expand Down
10 changes: 10 additions & 0 deletions src/bitcoin-tx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,16 @@ static CAmount AmountFromValue(const UniValue& value)
return amount;
}

static std::vector<unsigned char> ParseHexUV(const UniValue& v, const std::string& strName)
{
std::string strHex;
if (v.isStr())
strHex = v.getValStr();
if (!IsHex(strHex))
throw std::runtime_error(strName + " must be hexadecimal string (not '" + strHex + "')");
TheCharlatan marked this conversation as resolved.
Show resolved Hide resolved
return ParseHex(strHex);
}

static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)
{
int nHashType = SIGHASH_ALL;
Expand Down
4 changes: 2 additions & 2 deletions src/core_io.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#define BITCOIN_CORE_IO_H

#include <consensus/amount.h>
#include <util/result.h>
TheCharlatan marked this conversation as resolved.
Show resolved Hide resolved

#include <string>
#include <vector>
Expand Down Expand Up @@ -45,8 +46,7 @@ bool DecodeHexBlockHeader(CBlockHeader&, const std::string& hex_header);
* @see ParseHashV for an RPC-oriented version of this
*/
bool ParseHashStr(const std::string& strHex, uint256& result);
std::vector<unsigned char> ParseHexUV(const UniValue& v, const std::string& strName);
int ParseSighashString(const UniValue& sighash);
[[nodiscard]] util::Result<int> SighashFromStr(const std::string& sighash);

// core_write.cpp
UniValue ValueFromAmount(const CAmount amount);
Expand Down
47 changes: 16 additions & 31 deletions src/core_read.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#include <script/sign.h>
#include <serialize.h>
#include <streams.h>
#include <univalue.h>
#include <util/result.h>
#include <util/strencodings.h>
#include <version.h>

Expand Down Expand Up @@ -242,36 +242,21 @@ bool ParseHashStr(const std::string& strHex, uint256& result)
return true;
}

std::vector<unsigned char> ParseHexUV(const UniValue& v, const std::string& strName)
util::Result<int> SighashFromStr(const std::string& sighash)
{
std::string strHex;
if (v.isStr())
strHex = v.getValStr();
if (!IsHex(strHex))
throw std::runtime_error(strName + " must be hexadecimal string (not '" + strHex + "')");
return ParseHex(strHex);
}

int ParseSighashString(const UniValue& sighash)
{
int hash_type = SIGHASH_DEFAULT;
if (!sighash.isNull()) {
static std::map<std::string, int> map_sighash_values = {
{std::string("DEFAULT"), int(SIGHASH_DEFAULT)},
{std::string("ALL"), int(SIGHASH_ALL)},
{std::string("ALL|ANYONECANPAY"), int(SIGHASH_ALL|SIGHASH_ANYONECANPAY)},
{std::string("NONE"), int(SIGHASH_NONE)},
{std::string("NONE|ANYONECANPAY"), int(SIGHASH_NONE|SIGHASH_ANYONECANPAY)},
{std::string("SINGLE"), int(SIGHASH_SINGLE)},
{std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY)},
};
const std::string& strHashType = sighash.get_str();
const auto& it = map_sighash_values.find(strHashType);
if (it != map_sighash_values.end()) {
hash_type = it->second;
} else {
throw std::runtime_error(strHashType + " is not a valid sighash parameter.");
}
static std::map<std::string, int> map_sighash_values = {
{std::string("DEFAULT"), int(SIGHASH_DEFAULT)},
TheCharlatan marked this conversation as resolved.
Show resolved Hide resolved
{std::string("ALL"), int(SIGHASH_ALL)},
{std::string("ALL|ANYONECANPAY"), int(SIGHASH_ALL|SIGHASH_ANYONECANPAY)},
{std::string("NONE"), int(SIGHASH_NONE)},
{std::string("NONE|ANYONECANPAY"), int(SIGHASH_NONE|SIGHASH_ANYONECANPAY)},
{std::string("SINGLE"), int(SIGHASH_SINGLE)},
{std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY)},
};
const auto& it = map_sighash_values.find(sighash);
if (it != map_sighash_values.end()) {
return it->second;
} else {
return util::Error{Untranslated(sighash + " is not a valid sighash parameter.")};
}
TheCharlatan marked this conversation as resolved.
Show resolved Hide resolved
return hash_type;
}
18 changes: 18 additions & 0 deletions src/rpc/util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,18 @@
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <clientversion.h>
#include <core_io.h>
#include <common/args.h>
#include <consensus/amount.h>
#include <script/interpreter.h>
TheCharlatan marked this conversation as resolved.
Show resolved Hide resolved
#include <key_io.h>
#include <outputtype.h>
#include <rpc/util.h>
#include <script/descriptor.h>
#include <script/signingprovider.h>
#include <tinyformat.h>
#include <util/check.h>
#include <util/result.h>
#include <util/strencodings.h>
#include <util/string.h>
#include <util/translation.h>
Expand Down Expand Up @@ -310,6 +313,21 @@ UniValue DescribeAddress(const CTxDestination& dest)
return std::visit(DescribeAddressVisitor(), dest);
}

int ParseSighashString(const UniValue& sighash)
{
if (sighash.isNull()) {
return SIGHASH_DEFAULT;
}
if (!sighash.isStr()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "sighash needs to be null or string");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about adding dead code. Before on master this will already throw and never reach this part of the code:

test_framework.authproxy.JSONRPCException: Wrong type passed:
{
    "Position 3 (sighashtype)": "JSON value of type bool is not of expected type string"
} (-3)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a suggestion for how to handle the fuzz test then? Could just remove this line again and re-add the runtime_error catch in the test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could just remove this line again and re-add the runtime_error catch in the test.

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#28162

}
const auto result{SighashFromStr(sighash.get_str())};
if (!result) {
throw JSONRPCError(RPC_INVALID_PARAMETER, util::ErrorString(result).original);
}
return result.value();
}

unsigned int ParseConfirmTarget(const UniValue& value, unsigned int max_target)
{
const int target{value.getInt<int>()};
Expand Down
3 changes: 3 additions & 0 deletions src/rpc/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ CTxDestination AddAndGetMultisigDestination(const int required, const std::vecto

UniValue DescribeAddress(const CTxDestination& dest);

/** Parse a sighash string representation and raise an RPC error if it is invalid. */
int ParseSighashString(const UniValue& sighash);
TheCharlatan marked this conversation as resolved.
Show resolved Hide resolved
TheCharlatan marked this conversation as resolved.
Show resolved Hide resolved

//! Parse a confirm target option and raise an RPC error if it is invalid.
unsigned int ParseConfirmTarget(const UniValue& value, unsigned int max_target);

Expand Down
9 changes: 1 addition & 8 deletions src/test/fuzz/parse_univalue.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <chainparams.h>
#include <core_io.h>
#include <rpc/client.h>
#include <rpc/util.h>
#include <test/fuzz/fuzz.h>
Expand Down Expand Up @@ -57,12 +56,6 @@ FUZZ_TARGET_INIT(parse_univalue, initialize_parse_univalue)
(void)ParseHexO(univalue, random_string);
} catch (const UniValue&) {
}
try {
(void)ParseHexUV(univalue, "A");
(void)ParseHexUV(univalue, random_string);
} catch (const UniValue&) {
} catch (const std::runtime_error&) {
}
try {
(void)ParseHexV(univalue, "A");
} catch (const UniValue&) {
Expand All @@ -75,7 +68,7 @@ FUZZ_TARGET_INIT(parse_univalue, initialize_parse_univalue)
}
try {
(void)ParseSighashString(univalue);
} catch (const std::runtime_error&) {
} catch (const UniValue&) {
}
try {
(void)AmountFromValue(univalue);
Expand Down