Besides the build system changes, this is a mostly move-only change for moving the few UniValue-related functions out of kernel files.
UniValue is not required by any of the kernel components and a JSON library should not need to be part of a consensus library.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | theuni, stickies-v, achow101 |
| Concept ACK | fanquake |
| Stale ACK | MarcoFalke, hebasto, jonatack |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
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.
Concept ACK.
Concept ACK
0 | @@ -0,0 +1,47 @@ 1 | +// Copyright (c) 2023 The Bitcoin Core developers 2 | +// Distributed under the MIT software license, see the accompanying 3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php. 4 | +
Missing self-include?
6 | +#define BITCOIN_COMMON_UNIVALUE_HELPERS_H 7 | + 8 | +#include <string> 9 | +#include <vector> 10 | + 11 | +class UniValue;
it may be nice to include univalue.h and then mark it as export for iwyu. This would allow include-sites to drop the univalue.h include, since it should be obvious that there is a univalue dependency with the univalue_helpers.h include already.
Also, this shouldn't increase compile cost, because it would be rare (in this case) for a call-site to include this header, but not need the univalue header.
lgtm. Left two nits, feel free to ignore.
lgtm ACK 03e06e589a24f0bcb7dbd28a0aaa925b18b87b9d 🚆
<details><summary>Show signature</summary>
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 03e06e589a24f0bcb7dbd28a0aaa925b18b87b9d 🚆
2mxQ9s2E83Cpsc3G1cPpXHudEp5PcMmrgHTI+WVd+CI94t4cNzqP53ApM7yvTxGkPfWXNc8sndlQVaImSA5DAg==
</details>
20 | + if (!IsHex(strHex)) 21 | + throw std::runtime_error(strName + " must be hexadecimal string (not '" + strHex + "')"); 22 | + return ParseHex(strHex); 23 | +} 24 | + 25 | +int ParseSighashString(const UniValue& sighash)
I don't really find this fn to be a UniValue helper, pretty much all the logic is sighash specific. While thinking for a better place to sit, I ended up thinking the better solution may be to keep it where it is but just remove the (imo unnecessary) dependency on UV? Offloading the default value and type check to a helper function like ParseSighashString is opaque, and even though my approach here is slightly more verbose in the callsite, I find the clarity of what's happening to be worth it (different callsites may require different behaviour in case of wrong types or sighash_str).
What do you think?
(I think returning a std::optional instead of throwing is probably even better, but orthogonal to this pull)
<details> <summary>git diff on master</summary>
diff --git a/src/core_io.h b/src/core_io.h
index 997f3bfd5b..ea07042b82 100644
--- a/src/core_io.h
+++ b/src/core_io.h
@@ -46,7 +46,7 @@ bool DecodeHexBlockHeader(CBlockHeader&, const std::string& hex_header);
*/
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);
+int ParseSighashString(const std::string& sighash);
// core_write.cpp
UniValue ValueFromAmount(const CAmount amount);
diff --git a/src/core_read.cpp b/src/core_read.cpp
index 84cd559b7f..31f121206c 100644
--- a/src/core_read.cpp
+++ b/src/core_read.cpp
@@ -252,26 +252,20 @@ std::vector<unsigned char> ParseHexUV(const UniValue& v, const std::string& strN
return ParseHex(strHex);
}
-int ParseSighashString(const UniValue& sighash)
+int ParseSighashString(const std::string& sighash_str)
{
- 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)},
+ {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_str)};
+ if (it != map_sighash_values.end()) {
+ return it->second;
}
- return hash_type;
+ throw std::runtime_error(sighash_str + " is not a valid sighash parameter.");
}
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index eb0200ccf5..e0e30b2d9e 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -1964,7 +1964,7 @@ RPCHelpMan descriptorprocesspsbt()
EvalDescriptorStringOrObject(descs[i], provider, /*expand_priv=*/true);
}
- int sighash_type = ParseSighashString(request.params[2]);
+ int sighash_type{request.params[2].isNull() ? SIGHASH_DEFAULT: ParseSighashString(request.params[2].get_str())};
bool bip32derivs = request.params[3].isNull() ? true : request.params[3].get_bool();
bool finalize = request.params[4].isNull() ? true : request.params[4].get_bool();
diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp
index 3a6fa39e4d..7272cca1c5 100644
--- a/src/rpc/rawtransaction_util.cpp
+++ b/src/rpc/rawtransaction_util.cpp
@@ -289,7 +289,7 @@ void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keyst
void SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, const std::map<COutPoint, Coin>& coins, const UniValue& hashType, UniValue& result)
{
- int nHashType = ParseSighashString(hashType);
+ int nHashType{hashType.isNull() ? SIGHASH_DEFAULT : ParseSighashString(hashType.get_str())};
// Script verification errors
std::map<int, bilingual_str> input_errors;
diff --git a/src/test/fuzz/parse_univalue.cpp b/src/test/fuzz/parse_univalue.cpp
index bfa856211d..5cb2c64f26 100644
--- a/src/test/fuzz/parse_univalue.cpp
+++ b/src/test/fuzz/parse_univalue.cpp
@@ -73,10 +73,6 @@ FUZZ_TARGET(parse_univalue, .init = initialize_parse_univalue)
} catch (const UniValue&) {
} catch (const std::runtime_error&) {
}
- try {
- (void)ParseSighashString(univalue);
- } catch (const std::runtime_error&) {
- }
try {
(void)AmountFromValue(univalue);
} catch (const UniValue&) {
diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
index b695d4bed3..3d74b12ef7 100644
--- a/src/wallet/rpc/spend.cpp
+++ b/src/wallet/rpc/spend.cpp
@@ -943,7 +943,7 @@ RPCHelpMan signrawtransactionwithwallet()
// Parse the prevtxs array
ParsePrevouts(request.params[1], nullptr, coins);
- int nHashType = ParseSighashString(request.params[2]);
+ int nHashType{request.params[2].isNull() ? SIGHASH_DEFAULT: ParseSighashString(request.params[2].get_str())};
// Script verification errors
std::map<int, bilingual_str> input_errors;
@@ -1587,7 +1587,7 @@ RPCHelpMan walletprocesspsbt()
}
// Get the sighash type
- int nHashType = ParseSighashString(request.params[2]);
+ int nHashType{request.params[2].isNull() ? SIGHASH_DEFAULT: ParseSighashString(request.params[2].get_str())};
// Fill transaction with our data and also sign
bool sign = request.params[1].isNull() ? true : request.params[1].get_bool();
</details>
Updated 03e06e589a24f0bcb7dbd28a0aaa925b18b87b9d -> 0498ea61f4ce1d73e84f866f420a2afb209fb01c (kernelRmUnivalue_0 -> kernelRmUnivalue_1, compare)
univalue.h includeunivalue_helpers and core_read functions. Changed the return type of the function in core_read to return a util::Result instead of throwing. Since other UniValue calls may throw already in the univalue_helpers function, it seems more appropriate to throw there though.0 | @@ -0,0 +1,35 @@ 1 | +// Copyright (c) 2023 The Bitcoin Core developers 2 | +// Distributed under the MIT software license, see the accompanying 3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php. 4 | + 5 | +#include <common/univalue_helpers.h>
Missing newline after self-include?
Ups, done.
Updated 0498ea61f4ce1d73e84f866f420a2afb209fb01c -> 05477b5566dc6f9c3d5c9609268002c0fbe1e1aa (kernelRmUnivalue_1 -> kernelRmUnivalue_2, compare)
Putting new parsing helpers into the libbitcoin_common library is a bit questionable for me.
I was expecting to see them in the libbitcoin_util. However, it is not possible with the suggested approach as the univalue_helpers.cpp depends on SIGHASH_DEFAULT.
UPD. However, we put to the libbitcoin_common the code that is not used in the kernel.
The ParseHexUV function is used in bitcoin-tx.cpp only. Why not make it static/namespaced in there?
See: https://github.com/hebasto/bitcoin/commit/6d0fcfe3abc8b944d296d2033b9fd2e444d85ce4
The ParseHexUV function is used in bitcoin-tx.cpp only. Why not make it static/namespaced in there?
Sacrificing the fuzz test seems unfortunate. but we already don't fuzz a similar function in bitcoin-tx AmountFromValue. I guess we have poor coverage of bitcoin-tx in general though.
Sacrificing the fuzz test seems unfortunate.
Maybe more fuzzing for ParseHex and IsHex is a better way?
Suggesting an alternative implementation that is based on the following idea:
--- a/src/core_read.cpp
+++ b/src/core_read.cpp
@@ -242,10 +242,10 @@ bool ParseHashStr(const std::string& strHex, uint256& result)
return true;
}
-int ParseSighashString(const UniValue& sighash)
+int ParseSighashString(const std::optional<std::string>& sighash)
{
int hash_type = SIGHASH_DEFAULT;
- if (!sighash.isNull()) {
+ if (sighash.has_value()) {
static std::map<std::string, int> map_sighash_values = {
{std::string("DEFAULT"), int(SIGHASH_DEFAULT)},
{std::string("ALL"), int(SIGHASH_ALL)},
@@ -255,7 +255,7 @@ int ParseSighashString(const UniValue& sighash)
{std::string("SINGLE"), int(SIGHASH_SINGLE)},
{std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY)},
};
- const std::string& strHashType = sighash.get_str();
+ const std::string& strHashType = sighash.value();
const auto& it = map_sighash_values.find(strHashType);
if (it != map_sighash_values.end()) {
hash_type = it->second;
The full branch is here: https://github.com/hebasto/bitcoin/tree/230721-univalue.3.
Diffs are simpler and smaller. A new helper does not depend on symbols from the libbitcoin_consensus library.
Feel free to take it :)
Updated 05477b5566dc6f9c3d5c9609268002c0fbe1e1aa -> 67e172a7c0d82271a13f77f76ce72799faddd05c (kernelRmUnivalue_2 -> kernelRmUnivalue_3, compare)
univalue_helpers file again.rpc/util.bitcoin-tx.cpp, sacrificing the fuzz test.
Updated 67e172a7c0d82271a13f77f76ce72799faddd05c -> b89567f51ade926af8c918e4787046b7ccec8eb0 (kernelRmUnivalue_3 -> kernelRmUnivalue_4, compare)
Approach ACK b89567f51ade926af8c918e4787046b7ccec8eb0. Nice! @TheCharlatan
Do I understand you correctly that your intention is to make the ParseSighash quite generic?
Re #28113#pullrequestreview-1542134637
Do I understand you correctly that your intention is to make the ParseSighash quite generic?
Maybe, but I mostly thought it might be worthwhile to keep this utility around.
260 | + const auto& it = map_sighash_values.find(*sighash); 261 | if (it != map_sighash_values.end()) { 262 | hash_type = it->second; 263 | } else { 264 | - throw std::runtime_error(strHashType + " is not a valid sighash parameter."); 265 | + return util::Error{Untranslated(*sighash + " is not a valid sighash parameter.")};
29e31d8
<details><summary><code>ParseSighash()</code> suggestions</summary><p>
--- a/src/core_io.h
+++ b/src/core_io.h
@@ -47,7 +47,7 @@ bool DecodeHexBlockHeader(CBlockHeader&, const std::string&
-util::Result<int> ParseSighash(const std::optional<std::string>& sighash);
+[[nodiscard]] util::Result<int> ParseSighash(const std::optional<std::string>& sighash);
--- a/src/core_read.cpp
+++ b/src/core_read.cpp
@@ -245,7 +245,7 @@ bool ParseHashStr(const std::string& strHex, uint256& result)
util::Result<int> ParseSighash(const std::optional<std::string>& sighash)
{
int hash_type = SIGHASH_DEFAULT;
- if (sighash) {
+ if (sighash.has_value()) {
static std::map<std::string, int> map_sighash_values = {
@@ -255,11 +255,11 @@ util::Result<int> ParseSighash(const std::optional<std::string>& sighash)
};
- const auto& it = map_sighash_values.find(*sighash);
+ const auto& it = map_sighash_values.find(sighash.value());
if (it != map_sighash_values.end()) {
hash_type = it->second;
} else {
- return util::Error{Untranslated(*sighash + " is not a valid sighash parameter.")};
+ return util::Error{Untranslated(sighash.value() + " is not a valid sighash parameter.")};
}
}
</p></details>
<details><summary>or more straightforward, dropping the <code>hash_type</code> temporary, with less nesting</summary><p>
util::Result<int> ParseSighash(const std::optional<std::string>& sighash)
{
if (!sighash.has_value()) return SIGHASH_DEFAULT;
const 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 auto& it = map_sighash_values.find(sighash.value());
if (it == map_sighash_values.end()) {
return util::Error{Untranslated(sighash.value() + " is not a valid sighash parameter.")};
}
return it->second;
}
</p></details>
5 | @@ -6,7 +6,9 @@ 6 | #define BITCOIN_CORE_IO_H 7 | 8 | #include <consensus/amount.h> 9 | +#include <util/result.h>
https://github.com/bitcoin/bitcoin/commit/29e31d894bd8bedb55c3a5a2d4d2681ba10a9b88 this ought to be added to core_read.cpp too; see full iwyu suggestions at https://cirrus-ci.com/task/4627440203464704?logs=ci#L1786
566 | +{ 567 | + std::string strHex; 568 | + if (v.isStr()) 569 | + strHex = v.getValStr(); 570 | + if (!IsHex(strHex)) 571 | + throw std::runtime_error(strName + " must be hexadecimal string (not '" + strHex + "')");
https://github.com/bitcoin/bitcoin/commit/29e31d894bd8bedb55c3a5a2d4d2681ba10a9b88 (feel free to ignore) I wouldn't find review harder if the missing brackets were added
if (v.isStr()) {
strHex = v.getValStr();
}
if (!IsHex(strHex)) {
throw std::runtime_error(strName + " must be hexadecimal string (not '" + strHex + "')");
}
316 | +{ 317 | + auto parsed_sighash = ParseSighash(sighash.isNull() ? std::nullopt : std::make_optional(sighash.get_str())); 318 | + if (!parsed_sighash) { 319 | + throw JSONRPCError(RPC_INVALID_PARAMETER, util::ErrorString(parsed_sighash).original); 320 | + } 321 | + return parsed_sighash.value();
https://github.com/bitcoin/bitcoin/commit/29e31d894bd8bedb55c3a5a2d4d2681ba10a9b88 When storing a Result type, I think it's clearer to name it result (seems to be becoming a convention). In particular, util::ErrorString(parsed_sighash) is a little confusing.
const auto result{ParseSighash(sighash.isNull() ? std::nullopt : std::make_optional(sighash.get_str()))};
if (!result) {
throw JSONRPCError(RPC_INVALID_PARAMETER, util::ErrorString(result).original);
}
return result.value();
Approach ACK b89567f51ade926af8c918e4787046b7ccec8eb0
Updated b89567f51ade926af8c918e4787046b7ccec8eb0 -> a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240 (kernelRmUnivalue_4 -> kernelRmUnivalue_5, compare)
Did not apply the suggestions that would make the code moves less pure.
ACK a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240
I've restarted the "Win64 native [vs2022]" CI task due to a remote Chocolatey issue.
ACK a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240.
Should throwing RPC_INVALID_PARAMETER be mentioned in the release notes?
99 | @@ -100,6 +100,8 @@ CTxDestination AddAndGetMultisigDestination(const int required, const std::vecto 100 | 101 | UniValue DescribeAddress(const CTxDestination& dest); 102 | 103 | +int ParseSighashString(const UniValue& sighash);
nit: A bit confusing that ParseSighashString parses a UniValue, and ParseSighash parses a string. Naming other way around would be more logical imo? Larger diff but perhaps worth it?
nit: Also, perhaps a docstring to document throwing behaviour and reference ParseSighash? (would be okay with an unstructured one-liner too)
/**
* Parse a sighash string representation
*
* [@throws](/bitcoin-bitcoin/contributor/throws/) RPC_INVALID_PARAMETER if `sighash` is null or does not represent a valid sighash type.
*
* [@see](/bitcoin-bitcoin/contributor/see/) util::Result<int> ParseSighash(const std::optional<std::string>& sighash) for a less
* opinionated alternative
*/
46 | @@ -45,8 +47,7 @@ bool DecodeHexBlockHeader(CBlockHeader&, const std::string& hex_header); 47 | * @see ParseHashV for an RPC-oriented version of this 48 | */ 49 | bool ParseHashStr(const std::string& strHex, uint256& result); 50 | -std::vector<unsigned char> ParseHexUV(const UniValue& v, const std::string& strName); 51 | -int ParseSighashString(const UniValue& sighash); 52 | +util::Result<int> ParseSighash(const std::optional<std::string>& sighash);
Making sighash optional seems quite verbose and unintuitive for just the one callsite that needs the default sighash. It also makes the function behave quite ambiguously (imo), returning the default if no sighash is provided, but throwing if the sighash is not recognized. Would leave that logic to the callsite?
<details> <summary>git diff on a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240</summary>
diff --git a/src/core_io.h b/src/core_io.h
index 551d09e021..b61ce10777 100644
--- a/src/core_io.h
+++ b/src/core_io.h
@@ -8,7 +8,6 @@
#include <consensus/amount.h>
#include <util/result.h>
-#include <optional>
#include <string>
#include <vector>
@@ -47,7 +46,7 @@ bool DecodeHexBlockHeader(CBlockHeader&, const std::string& hex_header);
* [@see](/bitcoin-bitcoin/contributor/see/) ParseHashV for an RPC-oriented version of this
*/
bool ParseHashStr(const std::string& strHex, uint256& result);
-util::Result<int> ParseSighash(const std::optional<std::string>& sighash);
+util::Result<int> ParseSighash(const std::string& sighash);
// core_write.cpp
UniValue ValueFromAmount(const CAmount amount);
diff --git a/src/core_read.cpp b/src/core_read.cpp
index 638cb23bf4..c08af827f9 100644
--- a/src/core_read.cpp
+++ b/src/core_read.cpp
@@ -15,7 +15,6 @@
#include <version.h>
#include <algorithm>
-#include <optional>
#include <string>
namespace {
@@ -243,25 +242,21 @@ bool ParseHashStr(const std::string& strHex, uint256& result)
return true;
}
-util::Result<int> ParseSighash(const std::optional<std::string>& sighash)
+util::Result<int> ParseSighash(const std::string& sighash)
{
- int hash_type = SIGHASH_DEFAULT;
- if (sighash.has_value()) {
- 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 auto& it = map_sighash_values.find(sighash.value());
- if (it != map_sighash_values.end()) {
- hash_type = it->second;
- } else {
- return util::Error{Untranslated(sighash.value() + " is not a valid sighash parameter.")};
- }
+ 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 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.")};
}
- return hash_type;
}
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index d484c4de6d..998e968344 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -10,6 +10,7 @@
#include <outputtype.h>
#include <rpc/util.h>
#include <script/descriptor.h>
+#include <script/interpreter.h>
#include <script/signingprovider.h>
#include <tinyformat.h>
#include <util/check.h>
@@ -314,7 +315,8 @@ UniValue DescribeAddress(const CTxDestination& dest)
int ParseSighashString(const UniValue& sighash)
{
- const auto result{ParseSighash(sighash.isNull() ? std::nullopt : std::make_optional(sighash.get_str()))};
+ if (sighash.isNull()) return SIGHASH_DEFAULT;
+ const auto result{ParseSighash(sighash.get_str())};
if (!result) {
throw JSONRPCError(RPC_INVALID_PARAMETER, util::ErrorString(result).original);
}
</details>
The optional behavior was requested by @hebasto to isolate the sighash-specific logic from the univalue-specific logic. Could just add a docstring instead to the ParseSighash function?
I think that comment was from when we still had the univalue helpers?
Now, we have a string parsing function (in core_read.cpp) and an rpc helper function (in rpc/util.cpp). The string parsing, imo, should do just that, and the rpc helper can inject rpc-specific logic, being that 1) we like using UniValues in the RPC and that 2) for all rpcs, we prefer to interpret missing sighash as SIGHASH_DEFAULT. I don't think it's ideal to make the string parsing opinionated.
Edit: but I'm willing to see past it if you feel like changing this will not be good for the PRs progress. It just feels like a bit of a code smell to me.
Btw if the main concern is with #include <script/interpreter.h> in rpc/util.cpp, can avoid that by just calling ParseSighash("DEFAULT") too.
Btw if the main concern is with
#include <script/interpreter.h>inrpc/util.cpp, can avoid that by just callingParseSighash("DEFAULT")too.
Using string literals scattered across the code seems fragile. In this PR branch @ a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240, we already have two places:
sighashOptions in bitcoin-tx.cpp, andParseSighash in core_read.cppI'd avoid multiplying such cases. We'd better to refactor code (as a follow up) out into a dedicated structure/function.
Agree generally with @stickies-v. The optional param makes no sense because the caller... knows if it has a value :)
But that wonkiness aside, this is also now an outlier compared to the surrounding functions (though the param order is inconsistent among them, sigh):
[[nodiscard]] bool DecodeHexBlk(CBlock&, const std::string& strHexBlk);
bool DecodeHexBlockHeader(CBlockHeader&, const std::string& hex_header);
bool ParseHashStr(const std::string& strHex, uint256& result);
Looking at those, I'd expect to see:
[[nodiscard]] bool ParseSighash(const std::string& sighash, int& result);
And after implementing it in-place, that function has a messy control-flow with returns that are hard to follow. I took a stab at implementing both "cleanly" compared to the rest of the code, as opposed to inheriting the univalue baggage.
Completely untested. Feel free to take it or leave it:
[[nodiscard]] bool ParseSighash(const std::string& sighash, int& result);
bool ParseSighash(const std::string& sighash, int& result)
{
result = SIGHASH_DEFAULT;
if (sighash.empty())
return true;
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 auto& it = map_sighash_values.find(sighash);
if (it != map_sighash_values.end()) {
result = it->second;
return true;
}
return false;
}
int ParseSighashString(const UniValue& sighash)
{
std::string hashstr = {};
if (!sighash.isNull()) {
hashstr = sighash.get_str();
}
int result;
if (!ParseSighash(hashstr, result)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%u is not a valid sighash parameter.", result));
}
return result;
}
@theuni, I did not go with your suggestion, but instead opted to move the weird default handling to the calling function. I feel like that should be the caller's responsibility. Also did not go with the out result type, since the function signatures are not really consistent to begin with, and we don't have an out result for the corresponding SighashToStr function either.
I like this, thanks! It just looks like a more modern version of the other functions. I also like that the weird no-value case is handled at the JSON layer instead. That's indeed cleaner than what I suggested.
Approach ACK a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240
I think the rpc/util approach is much better than before. Would prefer seeing the ParseSighash signature simplified (and renamed), but not necessarily a blocker.
Should throwing RPC_INVALID_PARAMETER be mentioned in the release notes?
The only behaviour change I can see here is the return code changing from -1 (RPC_MISC_ERROR) to -8 (RPC_INVALID_PARAMETER), the latter being strictly more helpful and I don't think this is a backwards incompatible change. The message and other behaviour are the same. I think a release note is not really necessary here, but obviously doesn't hurt.
Concept ACK, but IMO the new helper is quite ugly and awkward :\
I left more specific comments/suggestions.
Updated a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240 -> 42233bea28f6f7c464f0cd6499d675e81b3e8512 (kernelRmUnivalue_5 -> kernelRmUnivalue_6, compare)
rpc/util.cpp.core_read.cpp to SighashFromStr. There is already a function SighashToStr defined in core_io.h, so I thought having a corresponding declaration makes sense.[[nodiscard]] qualifier to SighashFromStr.ParseSighashString.895 | @@ -896,8 +896,8 @@ if BUILD_BITCOIN_KERNEL_LIB 896 | lib_LTLIBRARIES += $(LIBBITCOINKERNEL) 897 | 898 | libbitcoinkernel_la_LDFLAGS = $(AM_LDFLAGS) -no-undefined $(RELDFLAGS) $(PTHREAD_FLAGS) 899 | -libbitcoinkernel_la_LIBADD = $(LIBBITCOIN_CRYPTO) $(LIBUNIVALUE) $(LIBLEVELDB) $(LIBMEMENV) $(LIBSECP256K1) 900 | -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) 901 | +libbitcoinkernel_la_LIBADD = $(LIBBITCOIN_CRYPTO) $(LIBLEVELDB) $(LIBMEMENV) $(LIBSECP256K1)
Woohoo!
ACK 42233bea28f6f7c464f0cd6499d675e81b3e8512
Assuming CI is happy.
Very nice removal :)
5 | @@ -6,7 +6,9 @@ 6 | #define BITCOIN_CORE_IO_H 7 | 8 | #include <consensus/amount.h> 9 | +#include <util/result.h> 10 | 11 | +#include <optional>
Should now be dropped?
14 | +#include <util/result.h> 15 | #include <util/strencodings.h> 16 | #include <version.h> 17 | 18 | #include <algorithm> 19 | +#include <optional>
Should now be dropped?
2 | @@ -3,15 +3,18 @@ 3 | // file COPYING or http://www.opensource.org/licenses/mit-license.php. 4 | 5 | #include <clientversion.h> 6 | +#include <core_io.h> 7 | #include <common/args.h> 8 | #include <consensus/amount.h> 9 | +#include <script/interpreter.h>
Seems a shame with the last change to now include this only to be able to access SIGHASH_DEFAULT = 0. It might be good to extract the signature hash types to their own unit.
I'll leave this for a follow-up.
Perhaps move that check back to the callee until the sighash enum is extracted.
I don't think degrading our interface to save a bit of compilation time is an improvement tbh. I didn't look into it in detail but can see moving the sighash types into a separate header being a worthwhile follow-up, though.
It took a long time to compile bitcoind on my 2-core cpu until I upgraded last year, so given this particular tradeoff of the check in caller/callee, I like faster builds :)
It took a long time to compile bitcoind on my 2-core cpu until I upgraded last year, so given this particular tradeoff of the check in caller/callee, I like faster builds :)
I don't think removing a few lines from a single cpp file is going to change that, unless there are benchmarks available.
Generally, the most expensive headers are the serialize one and the boost ones. And serialize.h will still be included here, because of CScript.
I don't think removing a few lines from a single cpp file is going to change that, unless there are benchmarks available.
script/interpreter.h is ~350 lines ATM, with some template classes, so my point is that if we don't have a strong need to include it, or all of it, seems best not to. (I don't know if we have build benchmarks on, say, a raspberry pi or a laptop from a decade ago, but it's good to keep slower/less powerful machines in mind.)
seems best not to.
Again, I am saying it would be good to have benchmarks. If stuff is split too much, it can easily lead to longer build times, because the compiler needs to be invoked more often. (Maybe not in this case, if the new module is header-only)
Agree. In the absence of reliable benchmarks, I'd consider keeping the check in the callee, then move it to the caller if the new module is done and is as lightweight as it looks it might be.
99 | @@ -100,6 +100,9 @@ CTxDestination AddAndGetMultisigDestination(const int required, const std::vecto 100 | 101 | UniValue DescribeAddress(const CTxDestination& dest); 102 | 103 | +//! Parse a sighash string representation and raise an RPC error if it is invalid.
If you retouch, IIUC per our developer notes the //! Doxygen format is for describing a class member or a variable.
/** ... */ would be for describing a function.
286 | + const auto& it = map_sighash_values.find(sighash); 287 | + if (it != map_sighash_values.end()) { 288 | + return it->second; 289 | + } else { 290 | + return util::Error{Untranslated(sighash + " is not a valid sighash parameter.")}; 291 | }
I like how the code in this function is now quite similar to my suggestion in #28113 (review) 👍
ACK modulo a few comments
Updated 42233bea28f6f7c464f0cd6499d675e81b3e8512 -> c5fac53bca920626843723869a7677cef639df4d (kernelRmUnivalue_6 -> kernelRmUnivalue_7, compare)
67 | @@ -75,6 +68,7 @@ FUZZ_TARGET_INIT(parse_univalue, initialize_parse_univalue) 68 | } 69 | try { 70 | (void)ParseSighashString(univalue); 71 | + } catch (const UniValue&) { 72 | } catch (const std::runtime_error&) {
I think this catch now needs to go?
I'll test this.
Seems like std::runtime_error is still thrown: https://github.com/TheCharlatan/bitcoin/commits/kernelRmUnivalue_8.
Yeah, we can throw a type_error when calling get_str() and type_error inherits from the std::runtime_error.
Edit: sorry, my github didn't refresh, looks like you already answered it yourself.
Ah yes, I see now. const auto result{SighashFromStr(sighash.get_str())}; throws a runtime_error if sighash is not a string-type UniValue. Would suggest to change the ParseSighashString further to also throw RPC_INVALID_PARAMETER if the type is wrong? E.g. something like:
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index 0b2d2b18af..377181dd81 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -318,6 +318,9 @@ 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");
+ }
const auto result{SighashFromStr(sighash.get_str())};
if (!result) {
throw JSONRPCError(RPC_INVALID_PARAMETER, util::ErrorString(result).original);
Mmh, we don't do this for any of the other get_str() calls, so this might be better for a follow up? Would probably also be good to re-use the text from the type error.
Noting that this error doesn't seem to have test coverage, which could be done here before the refactoring to cover the behavior change (or no change), or in a follow-up.
Mmh, we don't do this for any of the other get_str() calls
I think we do? E.g. https://github.com/bitcoin/bitcoin/blob/e35fb7bc48d360585b80d0c7f89ac5087c1d405e/src/rpc/util.cpp#L89-L92 and https://github.com/bitcoin/bitcoin/blob/e35fb7bc48d360585b80d0c7f89ac5087c1d405e/src/rpc/util.cpp#L1199
Ah right, I just looked at the wrong examples :P - will push a patch for this.
I think we do? E.g.
I think all of that code should be removed and the checks be done by RPCHelpMan.
Ah yes, I see now.
const auto result{SighashFromStr(sighash.get_str())};throws a runtime_error if sighash is not a string-type UniValue. Would suggest to change theParseSighashStringfurther to also throwRPC_INVALID_PARAMETERif the type is wrong? E.g. something like:
I'm embarrassed to admit that this looked weird/wrong to me too, but I decided it was probably nothing and didn't mention it here. Ugh. Thanks for the great review @stickies-v !
ACK c5fac53bca920626843723869a7677cef639df4d
with the following notes or follow-ups:
Updated c5fac53bca920626843723869a7677cef639df4d -> d87edf309ffe60ae044a57ce9a0d9cc711edc365 (kernelRmUnivalue_7 -> kernelRmUnivalue_8, compare)
sighashtype header. Can just drop it again, if others find it too controversial.std::runtime_error catch in fuzz test by checking if the input is a string and throwing RPC_INVALID_PARAMETER if it is not.272 | - hash_type = it->second; 273 | - } else { 274 | - throw std::runtime_error(strHashType + " is not a valid sighash parameter."); 275 | - } 276 | + static std::map<std::string, int> map_sighash_values = { 277 | + {std::string("DEFAULT"), int(SIGHASH_DEFAULT)},
nit: missing #include <script/sighashtype.h>?
0 | @@ -0,0 +1,6 @@ 1 | +RPC Wallet 2 | +---------- 3 | + 4 | +- The `signrawtransactionwithkey` and `descriptorprocesspsbt` calls now return
nit: signrawtransactionwithwallet and walletprocesspsbt also affected
Ugh, how did I miss those, will re-push.
Sorry for turning up late to the discussion, but I fail to see how adding the <script/interpreter.h>
include into src/rpc/util.cpp (the discussion in this thread: #28113 (review)) would make any difference to compile times, given it's contents is already contained in the preprocessed output of rpc/libbitcoin_common_a-util.o?
If you compare the preprocessed output of rpc/libbitcoin_common_a-util.o on master (e35fb7bc48d360585b80d0c7f89ac5087c1d405e) and master + <script/interpreter.h> in src/rpc/util.cpp, the difference in the size of the preprocessed output is a single line:
cat rpc/libbitcoin_common_a-util.o_master | wc -l
96263
at rpc/libbitcoin_common_a-util.o_include | wc -l
96264
Looking at the actual diff, that extra line in the include case is a newline:
diffoscope rpc/libbitcoin_common_a-util.o_master rpc/libbitcoin_common_a-util.o_include
--- rpc/libbitcoin_common_a-util.o_master
+++ rpc/libbitcoin_common_a-util.o_include
@@ -94954,14 +94954,15 @@
uint256 DescriptorID(const Descriptor& desc);
# 12 "rpc/util.cpp" 2
+
# 1 "./util/translation.h" 1
# 18 "./util/translation.h"
struct bilingual_str {
and any other difference in the output is line number related, i.e:
@@ -96150,15 +96151,15 @@
std::string res;
for (const auto& i : m_inner) {
res += i.ToString(oneline) + ",";
}
return "[" + res + "...]";
}
}
- throw NonFatalCheckError( "Unreachable code reached (non-fatal)", "rpc/util.cpp", 1151, __func__);
+ throw NonFatalCheckError( "Unreachable code reached (non-fatal)", "rpc/util.cpp", 1152, __func__);
}
static std::pair<int64_t, int64_t> ParseRange(const UniValue& value)
{
if (value.isNum()) {
return {0, value.getInt<int64_t>()};
}
@@ -96238,15 +96239,15 @@
return servicesNames;
}
[[nodiscard]] static UniValue BilingualStringsToUniValue(const std::vector<bilingual_str>& bilingual_strings)
{
- inline_check_non_fatal(!bilingual_strings.empty(), "rpc/util.cpp", 1239, __func__, "!bilingual_strings.empty()");
+ inline_check_non_fatal(!bilingual_strings.empty(), "rpc/util.cpp", 1240, __func__, "!bilingual_strings.empty()");
UniValue result{UniValue::VARR};
for (const auto& s : bilingual_strings) {
result.push_back(s.original);
}
return result;
}
Just for reference, the preprocessed size of the current change is 96391 (bigger than master) and it still contains script/interpreter.h.
Putting all of that aside, I have no issues with the current state of the PR, but in general wanted to mention this here, because I don't think we should be too concerned about adding or removing headers, unless it's very clear that doing so would actually make some significant benefit, i.e dropping Boost or other very heavy includes, clearer code separation etc, and someone has actually bothered to check.
Obviously we should be avoiding making code or interface/design decisions based on whether or not including a header might slightly change compile times for somebody trying to compile Core on a Raspberry Pi. Especially when it turns out adding or removing that header makes 0 actual difference.
ACK d87edf309ffe60ae044a57ce9a0d9cc711edc365
Removes dependency on UniValue in kernel, and imo improves the interface of dealing with sighashtype user input along the way.
No strong view on keeping/reverting the separate script/sighashtype.h header, happy to go either way. Unnecessary code churn is always better to be avoided, but I'd also rather not bike-shed over it. Will quickly re-ACK if consensus is to revert it, though.
This split is done in preparation for the next commit where the
dependency on UniValue in the kernel library is removed.
It is not required by any of the kernel components.
A JSON library should not need to be part of a consensus library.
Updated d87edf309ffe60ae044a57ce9a0d9cc711edc365 -> 6960c81cbfa6208d4098353e53b313e13a21cb49 (kernelRmUnivalue_8 -> kernelRmUnivalue_9, compare)
Re-ACK 6960c81cbfa6208d4098353e53b313e13a21cb49
I think the header split was reasonable btw, and I think it's safe to assume it may be redone in the future, but not for the stated reasons. Breaking something like that out for the sake of modularity and reducing dependencies is fine, but I agree with @fanquake that even if it did speed up compile, that alone would not be a compelling enough reason to reorganize the code.
ACK 6960c81cbfa6208d4098353e53b313e13a21cb49
ACK 6960c81cbfa6208d4098353e53b313e13a21cb49
One follow-up could be test coverage for the error case. @fanquake could you provide more info on the methodology to reproduce your data?
I'm not surprised that the file may have already been included elsewhere, or if the extraction in the push at d87edf3 didn't make a difference, as the include directives in that version were not globally optimized for the change, and it wasn't a blocker and fine to maybe do later. The implementation at https://github.com/jonatack/bitcoin/commits/2023-07-extract-sighashtype-and-scriptverify-enums, for instance, might make more of a difference (or not!) but I'm not sure of the best way to test in order to explore this better.
0 | @@ -0,0 +1,7 @@ 1 | +RPC Wallet 2 | +---------- 3 | + 4 | +- The `signrawtransactionwithkey`, `signrawtransactionwithwallet`, 5 | + `walletprocesspsbt` and `descriptorprocesspsbt` calls now return more 6 | + specific RPC_INVALID_PARAMETER instead of RPC_PARSE_ERROR if their 7 | + sighashtype argument is malformed or not a string.
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)
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.
317 | +{ 318 | + if (sighash.isNull()) { 319 | + return SIGHASH_DEFAULT; 320 | + } 321 | + if (!sighash.isStr()) { 322 | + throw JSONRPCError(RPC_INVALID_PARAMETER, "sighash needs to be null or string");
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)
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.
Could just remove this line again and re-add the runtime_error catch in the test.
yes
@fanquake could you provide more info on the methodology to reproduce your data? @jonatack I'm just running the preprocessor and comparing the to-be-compiled output. You could inject it into the compile command from
V=1. i.e something like/usr/bin/ccache g++ -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config -fmacro-prefix-map=/home/ubuntu/bitcoin=. -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -DHAVE_BUILD_INFO -DPROVIDE_FUZZ_MAIN_FUNCTION -I. -I./minisketch/include -I./secp256k1/include -I./univalue/include -I./leveldb/include -I/usr/include -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE -fdebug-prefix-map=/home/ubuntu/bitcoin=. -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wno-unused-parameter -fno-extended-identifiers -fPIE -g -O2 -MT rpc/libbitcoin_common_a-util.o -MD -MP -MF rpc/.deps/libbitcoin_common_a-util.Tpo -c -o rpc/libbitcoin_common_a-util.o test -f 'rpc/util.cpp' || echo './' rpc/util.cppand add-Eafter g++.
The implementation at https://github.com/jonatack/bitcoin/commits/2023-07-extract-sighashtype-and-scriptverify-enums, for instance, might make more of a difference (or not!) but I'm not sure of the best way to test in order to explore this better.
If the intention was to not "pull in" script/interpreter.h, then it'll make no difference. The relevant include chain is key_io.h (included in rpc/util) -> script/standard.h -> script/interpreter.h.
Outside of that, I'm not sure what sort of (meaningful) change we are actually trying to measure, that would have any significant effects on compile times.