kernel: Remove UniValue from kernel library #28113
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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 |
There was a problem hiding this comment.
lgtm. Left two nits, feel free to ignore.
lgtm ACK 03e06e5 🚆
Show signature
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==
src/common/univalue_helpers.cpp
Outdated
| return ParseHex(strHex); | ||
| } | ||
|
|
||
| int ParseSighashString(const UniValue& sighash) |
There was a problem hiding this comment.
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)
git diff on master
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();
TheCharlatan
force-pushed
the
kernelRmUnivalue
branch
from
03e06e5
to
0498ea6
Compare
|
Updated 03e06e5 -> 0498ea6 (kernelRmUnivalue_0 -> kernelRmUnivalue_1, compare)
|
TheCharlatan
force-pushed
the
kernelRmUnivalue
branch
from
0498ea6
to
05477b5
Compare
|
Updated 0498ea6 -> 05477b5 (kernelRmUnivalue_1 -> kernelRmUnivalue_2, compare)
|
|
Concept ACK. cc @theuni |
|
Putting new parsing helpers into the I was expecting to see them in the UPD. However, we put to the |
|
The See: hebasto@6d0fcfe |
Sacrificing the fuzz test seems unfortunate. but we already don't fuzz a similar function in |
Maybe more fuzzing for |
|
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 Feel free to take it :) |
TheCharlatan
force-pushed
the
kernelRmUnivalue
branch
2 times, most recently
from
269f254
to
67e172a
Compare
|
Updated 05477b5 -> 67e172a (kernelRmUnivalue_2 -> kernelRmUnivalue_3, compare)
|
TheCharlatan
force-pushed
the
kernelRmUnivalue
branch
from
67e172a
to
b89567f
Compare
|
Updated 67e172a -> b89567f (kernelRmUnivalue_3 -> kernelRmUnivalue_4, compare)
|
There was a problem hiding this comment.
Approach ACK b89567f. Nice!
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. |
|
Updated b89567f -> a3774d1 (kernelRmUnivalue_4 -> kernelRmUnivalue_5, compare)
Did not apply the suggestions that would make the code moves less pure. |
TheCharlatan
force-pushed
the
kernelRmUnivalue
branch
from
b89567f
to
a3774d1
Compare
|
ACK a3774d1 |
TheCharlatan
force-pushed
the
kernelRmUnivalue
branch
from
42233be
to
c5fac53
Compare
|
Updated 42233be -> c5fac53 (kernelRmUnivalue_6 -> kernelRmUnivalue_7, compare) |
|
ACK c5fac53 with the following notes or follow-ups:
|
TheCharlatan
force-pushed
the
kernelRmUnivalue
branch
from
c5fac53
to
d87edf3
Compare
|
Updated c5fac53 -> d87edf3 (kernelRmUnivalue_7 -> kernelRmUnivalue_8, compare)
|
|
Sorry for turning up late to the discussion, but I fail to see how adding the If you compare the preprocessed output of cat rpc/libbitcoin_common_a-util.o_master | wc -l
96263
at rpc/libbitcoin_common_a-util.o_include | wc -l
96264Looking 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 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. |
There was a problem hiding this comment.
ACK d87edf3
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.
TheCharlatan
force-pushed
the
kernelRmUnivalue
branch
from
d87edf3
to
6960c81
Compare
|
Updated d87edf3 -> 6960c81 (kernelRmUnivalue_8 -> kernelRmUnivalue_9, compare)
|
|
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 6960c81 |
|
ACK 6960c81 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. |
| - 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. |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
| return SIGHASH_DEFAULT; | ||
| } | ||
| if (!sighash.isStr()) { | ||
| throw JSONRPCError(RPC_INVALID_PARAMETER, "sighash needs to be null or string"); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Could just remove this line again and re-add the runtime_error catch in the test.
yes
@jonatack I'm just running the preprocessor and comparing the to-be-compiled output. You could inject it into the compile command from
If the intention was to not "pull in" 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. |
…ashString 06199a9 refactor: Revert addition of univalue sighash string check (TheCharlatan) 0b47c16 doc: Correct release-notes for sighashtype exceptions (TheCharlatan) Pull request description: This is a follow up for #28113. The string type check is already done by the rpc parser / RPCHelpMan. Re-doing it is adding dead code. Instead, throwing an exception when the assumption does not hold is the already correct behavior. Pointed out in this [comment](https://github.com/bitcoin/bitcoin/pull/28113/files#r1274568557). Also correct the release note for the correct sighashtype exception change. There is no change in the handling of non-string sighashtype arugments. Pointed out in this [comment](https://github.com/bitcoin/bitcoin/pull/28113/files#r1274567555). ACKs for top commit: MarcoFalke: lgtm ACK 06199a9 jonatack: Tested ACK 06199a9 stickies-v: ACK 06199a9 Tree-SHA512: 3faa6b3d2247624c0973df8d79c09fbf1f90ffb99f1be484e359b528f485c31affea45976759bd206e4c81cbb54ebba5ad0ef4127d1deacbfe2a58153fcc94ee
…rseSighashString 06199a9 refactor: Revert addition of univalue sighash string check (TheCharlatan) 0b47c16 doc: Correct release-notes for sighashtype exceptions (TheCharlatan) Pull request description: This is a follow up for bitcoin#28113. The string type check is already done by the rpc parser / RPCHelpMan. Re-doing it is adding dead code. Instead, throwing an exception when the assumption does not hold is the already correct behavior. Pointed out in this [comment](https://github.com/bitcoin/bitcoin/pull/28113/files#r1274568557). Also correct the release note for the correct sighashtype exception change. There is no change in the handling of non-string sighashtype arugments. Pointed out in this [comment](https://github.com/bitcoin/bitcoin/pull/28113/files#r1274567555). ACKs for top commit: MarcoFalke: lgtm ACK 06199a9 jonatack: Tested ACK 06199a9 stickies-v: ACK 06199a9 Tree-SHA512: 3faa6b3d2247624c0973df8d79c09fbf1f90ffb99f1be484e359b528f485c31affea45976759bd206e4c81cbb54ebba5ad0ef4127d1deacbfe2a58153fcc94ee
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.