kernel: Remove UniValue from kernel library

TheCharlatan commented at 2:06 pm on July 20, 2023: contributor

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.

DrahtBot commented at 2:06 pm on July 20, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28134 (rpc, util: deduplicate AmountFromValue() using util::Result by jonatack)

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 added the label Validation on Jul 20, 2023

hebasto commented at 2:08 pm on July 20, 2023: member

Concept ACK.

stickies-v commented at 2:10 pm on July 20, 2023: contributor

Concept ACK

in src/common/univalue_helpers.cpp:4 in 03e06e589a outdated

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+

MarcoFalke commented at 4:32 pm on July 20, 2023:

Missing self-include?

in src/common/univalue_helpers.h:11 in 03e06e589a outdated

 6+#define BITCOIN_COMMON_UNIVALUE_HELPERS_H
 7+
 8+#include <string>
 9+#include <vector>
10+
11+class UniValue;

MarcoFalke commented at 4:37 pm on July 20, 2023:

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.

MarcoFalke approved

MarcoFalke commented at 4:37 pm on July 20, 2023: member

lgtm. Left two nits, feel free to ignore.

lgtm ACK 03e06e589a24f0bcb7dbd28a0aaa925b18b87b9d 🚆

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK 03e06e589a24f0bcb7dbd28a0aaa925b18b87b9d 🚆
32mxQ9s2E83Cpsc3G1cPpXHudEp5PcMmrgHTI+WVd+CI94t4cNzqP53ApM7yvTxGkPfWXNc8sndlQVaImSA5DAg==

in src/common/univalue_helpers.cpp:26 in 03e06e589a outdated

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)

stickies-v commented at 5:09 pm on July 20, 2023:

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)

  0diff --git a/src/core_io.h b/src/core_io.h
  1index 997f3bfd5b..ea07042b82 100644
  2--- a/src/core_io.h
  3+++ b/src/core_io.h
  4@@ -46,7 +46,7 @@ bool DecodeHexBlockHeader(CBlockHeader&, const std::string& hex_header);
  5  */
  6 bool ParseHashStr(const std::string& strHex, uint256& result);
  7 std::vector<unsigned char> ParseHexUV(const UniValue& v, const std::string& strName);
  8-int ParseSighashString(const UniValue& sighash);
  9+int ParseSighashString(const std::string& sighash);
 10 
 11 // core_write.cpp
 12 UniValue ValueFromAmount(const CAmount amount);
 13diff --git a/src/core_read.cpp b/src/core_read.cpp
 14index 84cd559b7f..31f121206c 100644
 15--- a/src/core_read.cpp
 16+++ b/src/core_read.cpp
 17@@ -252,26 +252,20 @@ std::vector<unsigned char> ParseHexUV(const UniValue& v, const std::string& strN
 18     return ParseHex(strHex);
 19 }
 20 
 21-int ParseSighashString(const UniValue& sighash)
 22+int ParseSighashString(const std::string& sighash_str)
 23 {
 24-    int hash_type = SIGHASH_DEFAULT;
 25-    if (!sighash.isNull()) {
 26-        static std::map<std::string, int> map_sighash_values = {
 27-            {std::string("DEFAULT"), int(SIGHASH_DEFAULT)},
 28-            {std::string("ALL"), int(SIGHASH_ALL)},
 29-            {std::string("ALL|ANYONECANPAY"), int(SIGHASH_ALL|SIGHASH_ANYONECANPAY)},
 30-            {std::string("NONE"), int(SIGHASH_NONE)},
 31-            {std::string("NONE|ANYONECANPAY"), int(SIGHASH_NONE|SIGHASH_ANYONECANPAY)},
 32-            {std::string("SINGLE"), int(SIGHASH_SINGLE)},
 33-            {std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY)},
 34-        };
 35-        const std::string& strHashType = sighash.get_str();
 36-        const auto& it = map_sighash_values.find(strHashType);
 37-        if (it != map_sighash_values.end()) {
 38-            hash_type = it->second;
 39-        } else {
 40-            throw std::runtime_error(strHashType + " is not a valid sighash parameter.");
 41-        }
 42+    static std::map<std::string, int> map_sighash_values = {
 43+        {std::string("DEFAULT"), int(SIGHASH_DEFAULT)},
 44+        {std::string("ALL"), int(SIGHASH_ALL)},
 45+        {std::string("ALL|ANYONECANPAY"), int(SIGHASH_ALL|SIGHASH_ANYONECANPAY)},
 46+        {std::string("NONE"), int(SIGHASH_NONE)},
 47+        {std::string("NONE|ANYONECANPAY"), int(SIGHASH_NONE|SIGHASH_ANYONECANPAY)},
 48+        {std::string("SINGLE"), int(SIGHASH_SINGLE)},
 49+        {std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY)},
 50+    };
 51+    const auto& it{map_sighash_values.find(sighash_str)};
 52+    if (it != map_sighash_values.end()) {
 53+        return it->second;
 54     }
 55-    return hash_type;
 56+    throw std::runtime_error(sighash_str + " is not a valid sighash parameter.");
 57 }
 58diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
 59index eb0200ccf5..e0e30b2d9e 100644
 60--- a/src/rpc/rawtransaction.cpp
 61+++ b/src/rpc/rawtransaction.cpp
 62@@ -1964,7 +1964,7 @@ RPCHelpMan descriptorprocesspsbt()
 63         EvalDescriptorStringOrObject(descs[i], provider, /*expand_priv=*/true);
 64     }
 65 
 66-    int sighash_type = ParseSighashString(request.params[2]);
 67+    int sighash_type{request.params[2].isNull() ? SIGHASH_DEFAULT: ParseSighashString(request.params[2].get_str())};
 68     bool bip32derivs = request.params[3].isNull() ? true : request.params[3].get_bool();
 69     bool finalize = request.params[4].isNull() ? true : request.params[4].get_bool();
 70 
 71diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp
 72index 3a6fa39e4d..7272cca1c5 100644
 73--- a/src/rpc/rawtransaction_util.cpp
 74+++ b/src/rpc/rawtransaction_util.cpp
 75@@ -289,7 +289,7 @@ void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keyst
 76 
 77 void SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, const std::map<COutPoint, Coin>& coins, const UniValue& hashType, UniValue& result)
 78 {
 79-    int nHashType = ParseSighashString(hashType);
 80+    int nHashType{hashType.isNull() ? SIGHASH_DEFAULT : ParseSighashString(hashType.get_str())};
 81 
 82     // Script verification errors
 83     std::map<int, bilingual_str> input_errors;
 84diff --git a/src/test/fuzz/parse_univalue.cpp b/src/test/fuzz/parse_univalue.cpp
 85index bfa856211d..5cb2c64f26 100644
 86--- a/src/test/fuzz/parse_univalue.cpp
 87+++ b/src/test/fuzz/parse_univalue.cpp
 88@@ -73,10 +73,6 @@ FUZZ_TARGET(parse_univalue, .init = initialize_parse_univalue)
 89     } catch (const UniValue&) {
 90     } catch (const std::runtime_error&) {
 91     }
 92-    try {
 93-        (void)ParseSighashString(univalue);
 94-    } catch (const std::runtime_error&) {
 95-    }
 96     try {
 97         (void)AmountFromValue(univalue);
 98     } catch (const UniValue&) {
 99diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
100index b695d4bed3..3d74b12ef7 100644
101--- a/src/wallet/rpc/spend.cpp
102+++ b/src/wallet/rpc/spend.cpp
103@@ -943,7 +943,7 @@ RPCHelpMan signrawtransactionwithwallet()
104     // Parse the prevtxs array
105     ParsePrevouts(request.params[1], nullptr, coins);
106 
107-    int nHashType = ParseSighashString(request.params[2]);
108+    int nHashType{request.params[2].isNull() ? SIGHASH_DEFAULT: ParseSighashString(request.params[2].get_str())};
109 
110     // Script verification errors
111     std::map<int, bilingual_str> input_errors;
112@@ -1587,7 +1587,7 @@ RPCHelpMan walletprocesspsbt()
113     }
114 
115     // Get the sighash type
116-    int nHashType = ParseSighashString(request.params[2]);
117+    int nHashType{request.params[2].isNull() ? SIGHASH_DEFAULT: ParseSighashString(request.params[2].get_str())};
118 
119     // Fill transaction with our data and also sign
120     bool sign = request.params[1].isNull() ? true : request.params[1].get_bool();

TheCharlatan force-pushed on Jul 21, 2023

TheCharlatan commented at 2:44 pm on July 21, 2023: contributor

Updated 03e06e589a24f0bcb7dbd28a0aaa925b18b87b9d -> 0498ea61f4ce1d73e84f866f420a2afb209fb01c (kernelRmUnivalue_0 -> kernelRmUnivalue_1, compare)

Addressed @MarcoFalke’s comment, fixed include
Addressed @MarcoFalke’s comment, exported univalue.h include
Followed up on @stickies-v’s comment, implemented an alternative to the comment by splitting the implementation between univalue_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.

in src/common/univalue_helpers.cpp:5 in 0498ea61f4 outdated

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>

MarcoFalke commented at 3:09 pm on July 21, 2023:

Missing newline after self-include?

TheCharlatan commented at 3:18 pm on July 21, 2023:

Ups, done.

TheCharlatan force-pushed on Jul 21, 2023

TheCharlatan commented at 3:23 pm on July 21, 2023: contributor

Updated 0498ea61f4ce1d73e84f866f420a2afb209fb01c -> 05477b5566dc6f9c3d5c9609268002c0fbe1e1aa (kernelRmUnivalue_1 -> kernelRmUnivalue_2, compare)

Addressed @MarcoFalke’s comment, fixed include spacing.

fanquake commented at 3:29 pm on July 21, 2023: member

Concept ACK. cc @theuni

hebasto commented at 4:02 pm on July 21, 2023: member

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.

hebasto commented at 8:36 pm on July 21, 2023: member

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

TheCharlatan commented at 8:43 pm on July 21, 2023: contributor

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.

hebasto commented at 9:15 pm on July 21, 2023: member

Sacrificing the fuzz test seems unfortunate.

Maybe more fuzzing for ParseHex and IsHex is a better way?

hebasto commented at 6:03 am on July 22, 2023: member

Suggesting an alternative implementation that is based on the following idea:

 0--- a/src/core_read.cpp
 1+++ b/src/core_read.cpp
 2@@ -242,10 +242,10 @@ bool ParseHashStr(const std::string& strHex, uint256& result)
 3     return true;
 4 }
 5 
 6-int ParseSighashString(const UniValue& sighash)
 7+int ParseSighashString(const std::optional<std::string>& sighash)
 8 {
 9     int hash_type = SIGHASH_DEFAULT;
10-    if (!sighash.isNull()) {
11+    if (sighash.has_value()) {
12         static std::map<std::string, int> map_sighash_values = {
13             {std::string("DEFAULT"), int(SIGHASH_DEFAULT)},
14             {std::string("ALL"), int(SIGHASH_ALL)},
15@@ -255,7 +255,7 @@ int ParseSighashString(const UniValue& sighash)
16             {std::string("SINGLE"), int(SIGHASH_SINGLE)},
17             {std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY)},
18         };
19-        const std::string& strHashType = sighash.get_str();
20+        const std::string& strHashType = sighash.value();
21         const auto& it = map_sighash_values.find(strHashType);
22         if (it != map_sighash_values.end()) {
23             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 :)

TheCharlatan force-pushed on Jul 22, 2023

TheCharlatan commented at 9:45 am on July 22, 2023: contributor

Updated 05477b5566dc6f9c3d5c9609268002c0fbe1e1aa -> 67e172a7c0d82271a13f77f76ce72799faddd05c (kernelRmUnivalue_2 -> kernelRmUnivalue_3, compare)

Got rid of the new univalue_helpers file again.
Refactored the first commit to move the UniValue-specific code to rpc/util.
Moved the logic in the second commit to bitcoin-tx.cpp, sacrificing the fuzz test.

DrahtBot added the label CI failed on Jul 22, 2023

TheCharlatan force-pushed on Jul 22, 2023

TheCharlatan commented at 6:44 pm on July 22, 2023: contributor

Updated 67e172a7c0d82271a13f77f76ce72799faddd05c -> b89567f51ade926af8c918e4787046b7ccec8eb0 (kernelRmUnivalue_3 -> kernelRmUnivalue_4, compare)

Fixed fuzz test by catching the UniValue exception.

hebasto commented at 7:14 pm on July 22, 2023: member

Approach ACK b89567f51ade926af8c918e4787046b7ccec8eb0. Nice! @TheCharlatan

Do I understand you correctly that your intention is to make the ParseSighash quite generic?

TheCharlatan commented at 8:43 pm on July 22, 2023: contributor

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.

in src/core_read.cpp:262 in b89567f51a outdated

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.")};

jonatack commented at 5:54 pm on July 23, 2023:

29e31d8

 0--- a/src/core_io.h
 1+++ b/src/core_io.h
 2@@ -47,7 +47,7 @@ bool DecodeHexBlockHeader(CBlockHeader&, const std::string& 
 3-util::Result<int> ParseSighash(const std::optional<std::string>& sighash);
 4+[[nodiscard]] util::Result<int> ParseSighash(const std::optional<std::string>& sighash);
 5
 6--- a/src/core_read.cpp
 7+++ b/src/core_read.cpp
 8@@ -245,7 +245,7 @@ bool ParseHashStr(const std::string& strHex, uint256& result)
 9 util::Result<int> ParseSighash(const std::optional<std::string>& sighash)
10 {
11     int hash_type = SIGHASH_DEFAULT;
12-    if (sighash) {
13+    if (sighash.has_value()) {
14         static std::map<std::string, int> map_sighash_values = {
15@@ -255,11 +255,11 @@ util::Result<int> ParseSighash(const std::optional<std::string>& sighash)
16         };
17-        const auto& it = map_sighash_values.find(*sighash);
18+        const auto& it = map_sighash_values.find(sighash.value());
19         if (it != map_sighash_values.end()) {
20             hash_type = it->second;
21         } else {
22-            return util::Error{Untranslated(*sighash + " is not a valid sighash parameter.")};
23+            return util::Error{Untranslated(sighash.value() + " is not a valid sighash parameter.")};
24         }
25     }

 0util::Result<int> ParseSighash(const std::optional<std::string>& sighash)
 1{
 2    if (!sighash.has_value()) return SIGHASH_DEFAULT;
 3    const std::map<std::string, int> map_sighash_values = {
 4        {std::string("DEFAULT"), int(SIGHASH_DEFAULT)},
 5        {std::string("ALL"), int(SIGHASH_ALL)},
 6        {std::string("ALL|ANYONECANPAY"), int(SIGHASH_ALL|SIGHASH_ANYONECANPAY)},
 7        {std::string("NONE"), int(SIGHASH_NONE)},
 8        {std::string("NONE|ANYONECANPAY"), int(SIGHASH_NONE|SIGHASH_ANYONECANPAY)},
 9        {std::string("SINGLE"), int(SIGHASH_SINGLE)},
10        {std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY)},
11    };
12    const auto& it = map_sighash_values.find(sighash.value());
13    if (it == map_sighash_values.end()) {
14        return util::Error{Untranslated(sighash.value() + " is not a valid sighash parameter.")};
15    }
16    return it->second;
17}

in src/core_io.h:9 in b89567f51a outdated

5@@ -6,7 +6,9 @@
6 #define BITCOIN_CORE_IO_H
7 
8 #include <consensus/amount.h>
9+#include <util/result.h>

jonatack commented at 6:06 pm on July 23, 2023:

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

in src/bitcoin-tx.cpp:571 in b89567f51a outdated

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 + "')");

jonatack commented at 6:24 pm on July 23, 2023:

https://github.com/bitcoin/bitcoin/commit/29e31d894bd8bedb55c3a5a2d4d2681ba10a9b88 (feel free to ignore) I wouldn’t find review harder if the missing brackets were added

0    if (v.isStr()) {
1        strHex = v.getValStr();
2    }
3    if (!IsHex(strHex)) {
4         throw std::runtime_error(strName + " must be hexadecimal string (not '" + strHex + "')");
5    }

in src/rpc/util.cpp:321 in b89567f51a outdated

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();

jonatack commented at 7:00 pm on July 23, 2023:

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.

0    const auto result{ParseSighash(sighash.isNull() ? std::nullopt : std::make_optional(sighash.get_str()))};
1    if (!result) {
2        throw JSONRPCError(RPC_INVALID_PARAMETER, util::ErrorString(result).original);
3    }
4    return result.value();

jonatack commented at 7:08 pm on July 23, 2023: contributor

Approach ACK b89567f51ade926af8c918e4787046b7ccec8eb0

TheCharlatan commented at 9:21 pm on July 23, 2023: contributor

Updated b89567f51ade926af8c918e4787046b7ccec8eb0 -> a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240 (kernelRmUnivalue_4 -> kernelRmUnivalue_5, compare)

Addressed @jonatack’s comment, fixing IWYU.
Addressed @jonatack’s comment, renaming variable holding result type.
Addressed @jonatack’s comment, applying suggestion on optional usage.

Did not apply the suggestions that would make the code moves less pure.

TheCharlatan force-pushed on Jul 23, 2023

jonatack commented at 9:33 pm on July 23, 2023: contributor

ACK a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240

DrahtBot requested review from MarcoFalke on Jul 23, 2023

hebasto commented at 8:19 am on July 24, 2023: member

I’ve restarted the “Win64 native [vs2022]” CI task due to a remote Chocolatey issue.

hebasto commented at 9:15 am on July 24, 2023: member

ACK a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240.

Should throwing RPC_INVALID_PARAMETER be mentioned in the release notes?

DrahtBot removed the label CI failed on Jul 24, 2023

in src/rpc/util.h:104 in a3774d1b2a outdated

 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);

stickies-v commented at 10:44 am on July 24, 2023:

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?

stickies-v commented at 10:52 am on July 24, 2023:

nit: Also, perhaps a docstring to document throwing behaviour and reference ParseSighash? (would be okay with an unstructured one-liner too)

0/**
1 * Parse a sighash string representation
2 *
3 * [@throws](/bitcoin-bitcoin/contributor/throws/) RPC_INVALID_PARAMETER if `sighash` is null or does not represent a valid sighash type.
4 * 
5 * [@see](/bitcoin-bitcoin/contributor/see/) util::Result<int> ParseSighash(const std::optional<std::string>& sighash) for a less
6 *      opinionated alternative
7 */

in src/core_io.h:50 in a3774d1b2a outdated

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);

stickies-v commented at 11:12 am on July 24, 2023:

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?

 0diff --git a/src/core_io.h b/src/core_io.h
 1index 551d09e021..b61ce10777 100644
 2--- a/src/core_io.h
 3+++ b/src/core_io.h
 4@@ -8,7 +8,6 @@
 5 #include <consensus/amount.h>
 6 #include <util/result.h>
 7 
 8-#include <optional>
 9 #include <string>
10 #include <vector>
11 
12@@ -47,7 +46,7 @@ bool DecodeHexBlockHeader(CBlockHeader&, const std::string& hex_header);
13  * [@see](/bitcoin-bitcoin/contributor/see/) ParseHashV for an RPC-oriented version of this
14  */
15 bool ParseHashStr(const std::string& strHex, uint256& result);
16-util::Result<int> ParseSighash(const std::optional<std::string>& sighash);
17+util::Result<int> ParseSighash(const std::string& sighash);
18 
19 // core_write.cpp
20 UniValue ValueFromAmount(const CAmount amount);
21diff --git a/src/core_read.cpp b/src/core_read.cpp
22index 638cb23bf4..c08af827f9 100644
23--- a/src/core_read.cpp
24+++ b/src/core_read.cpp
25@@ -15,7 +15,6 @@
26 #include <version.h>
27 
28 #include <algorithm>
29-#include <optional>
30 #include <string>
31 
32 namespace {
33@@ -243,25 +242,21 @@ bool ParseHashStr(const std::string& strHex, uint256& result)
34     return true;
35 }
36 
37-util::Result<int> ParseSighash(const std::optional<std::string>& sighash)
38+util::Result<int> ParseSighash(const std::string& sighash)
39 {
40-    int hash_type = SIGHASH_DEFAULT;
41-    if (sighash.has_value()) {
42-        static std::map<std::string, int> map_sighash_values = {
43-            {std::string("DEFAULT"), int(SIGHASH_DEFAULT)},
44-            {std::string("ALL"), int(SIGHASH_ALL)},
45-            {std::string("ALL|ANYONECANPAY"), int(SIGHASH_ALL|SIGHASH_ANYONECANPAY)},
46-            {std::string("NONE"), int(SIGHASH_NONE)},
47-            {std::string("NONE|ANYONECANPAY"), int(SIGHASH_NONE|SIGHASH_ANYONECANPAY)},
48-            {std::string("SINGLE"), int(SIGHASH_SINGLE)},
49-            {std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY)},
50-        };
51-        const auto& it = map_sighash_values.find(sighash.value());
52-        if (it != map_sighash_values.end()) {
53-            hash_type = it->second;
54-        } else {
55-            return util::Error{Untranslated(sighash.value() + " is not a valid sighash parameter.")};
56-        }
57+    static std::map<std::string, int> map_sighash_values = {
58+        {std::string("DEFAULT"), int(SIGHASH_DEFAULT)},
59+        {std::string("ALL"), int(SIGHASH_ALL)},
60+        {std::string("ALL|ANYONECANPAY"), int(SIGHASH_ALL|SIGHASH_ANYONECANPAY)},
61+        {std::string("NONE"), int(SIGHASH_NONE)},
62+        {std::string("NONE|ANYONECANPAY"), int(SIGHASH_NONE|SIGHASH_ANYONECANPAY)},
63+        {std::string("SINGLE"), int(SIGHASH_SINGLE)},
64+        {std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY)},
65+    };
66+    const auto& it = map_sighash_values.find(sighash);
67+    if (it != map_sighash_values.end()) {
68+        return it->second;
69+    } else {
70+        return util::Error{Untranslated(sighash + " is not a valid sighash parameter.")};
71     }
72-    return hash_type;
73 }
74diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
75index d484c4de6d..998e968344 100644
76--- a/src/rpc/util.cpp
77+++ b/src/rpc/util.cpp
78@@ -10,6 +10,7 @@
79 #include <outputtype.h>
80 #include <rpc/util.h>
81 #include <script/descriptor.h>
82+#include <script/interpreter.h>
83 #include <script/signingprovider.h>
84 #include <tinyformat.h>
85 #include <util/check.h>
86@@ -314,7 +315,8 @@ UniValue DescribeAddress(const CTxDestination& dest)
87 
88 int ParseSighashString(const UniValue& sighash)
89 {
90-    const auto result{ParseSighash(sighash.isNull() ? std::nullopt : std::make_optional(sighash.get_str()))};
91+    if (sighash.isNull()) return SIGHASH_DEFAULT;
92+    const auto result{ParseSighash(sighash.get_str())};
93     if (!result) {
94         throw JSONRPCError(RPC_INVALID_PARAMETER, util::ErrorString(result).original);
95     }

TheCharlatan commented at 12:38 pm on July 24, 2023:

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?

stickies-v commented at 12:48 pm on July 24, 2023:

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.

stickies-v commented at 1:06 pm on July 24, 2023:

Btw if the main concern is with #include <script/interpreter.h> in rpc/util.cpp, can avoid that by just calling ParseSighash("DEFAULT") too.

hebasto commented at 2:06 pm on July 24, 2023:

Btw if the main concern is with #include <script/interpreter.h> in rpc/util.cpp, can avoid that by just calling ParseSighash("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, and
ParseSighash in core_read.cpp

I’d avoid multiplying such cases. We’d better to refactor code (as a follow up) out into a dedicated structure/function.

theuni commented at 5:20 pm on July 24, 2023:

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):

0[[nodiscard]] bool DecodeHexBlk(CBlock&, const std::string& strHexBlk);
1bool DecodeHexBlockHeader(CBlockHeader&, const std::string& hex_header);
2bool ParseHashStr(const std::string& strHex, uint256& result);

Looking at those, I’d expect to see:

0[[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:

 0[[nodiscard]] bool ParseSighash(const std::string& sighash, int& result);
 1bool ParseSighash(const std::string& sighash, int& result)
 2{
 3    result = SIGHASH_DEFAULT;
 4    if (sighash.empty())
 5        return true;
 6
 7    static std::map<std::string, int> map_sighash_values = {
 8        {std::string("DEFAULT"), int(SIGHASH_DEFAULT)},
 9        {std::string("ALL"), int(SIGHASH_ALL)},
10        {std::string("ALL|ANYONECANPAY"), int(SIGHASH_ALL|SIGHASH_ANYONECANPAY)},
11        {std::string("NONE"), int(SIGHASH_NONE)},
12        {std::string("NONE|ANYONECANPAY"), int(SIGHASH_NONE|SIGHASH_ANYONECANPAY)},
13        {std::string("SINGLE"), int(SIGHASH_SINGLE)},
14        {std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY)},
15    };
16    const auto& it = map_sighash_values.find(sighash);
17    if (it != map_sighash_values.end()) {
18        result = it->second;
19        return true;
20    }
21    return false;
22}

 0int ParseSighashString(const UniValue& sighash)
 1{
 2    std::string hashstr = {};
 3    if (!sighash.isNull()) {
 4        hashstr = sighash.get_str();
 5    }
 6    int result;
 7    if (!ParseSighash(hashstr, result)) {
 8        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%u is not a valid sighash parameter.", result));
 9    }
10    return result;
11}

TheCharlatan commented at 6:48 pm on July 24, 2023:

@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.

theuni commented at 7:39 pm on July 24, 2023:

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.

stickies-v commented at 12:30 pm on July 24, 2023: contributor

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.

theuni commented at 5:23 pm on July 24, 2023: member

Concept ACK, but IMO the new helper is quite ugly and awkward :\

I left more specific comments/suggestions.

TheCharlatan force-pushed on Jul 24, 2023

TheCharlatan commented at 6:45 pm on July 24, 2023: contributor

Updated a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240 -> 42233bea28f6f7c464f0cd6499d675e81b3e8512 (kernelRmUnivalue_5 -> kernelRmUnivalue_6, compare)

Addressed @stickies-v’s comment, pushing the handling of the default back to the function in rpc/util.cpp.
Addressed @stickies-v’s comment, renaming the function in 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.
Added [[nodiscard]] qualifier to SighashFromStr.
Addressed @stickies-v’s comment, but only added a shorter docstring to ParseSighashString.

in src/Makefile.am:899 in 42233bea28 outdated

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)

theuni commented at 7:50 pm on July 24, 2023:

Woohoo!

theuni approved

theuni commented at 7:51 pm on July 24, 2023: member

ACK 42233bea28f6f7c464f0cd6499d675e81b3e8512

Assuming CI is happy.

Very nice removal :)

DrahtBot requested review from hebasto on Jul 24, 2023

DrahtBot requested review from jonatack on Jul 24, 2023

in src/core_io.h:11 in 42233bea28 outdated

 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>

jonatack commented at 1:53 am on July 25, 2023:

Should now be dropped?

in src/core_read.cpp:18 in 42233bea28 outdated

14+#include <util/result.h>
15 #include <util/strencodings.h>
16 #include <version.h>
17 
18 #include <algorithm>
19+#include <optional>

jonatack commented at 1:54 am on July 25, 2023:

Should now be dropped?

in src/rpc/util.cpp:9 in 42233bea28 outdated

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>

jonatack commented at 2:02 am on July 25, 2023:

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.

TheCharlatan commented at 7:05 am on July 25, 2023:

I’ll leave this for a follow-up.

jonatack commented at 12:31 pm on July 25, 2023:

Perhaps move that check back to the callee until the sighash enum is extracted.

stickies-v commented at 12:39 pm on July 25, 2023:

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.

jonatack commented at 12:51 pm on July 25, 2023:

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 :)

MarcoFalke commented at 12:54 pm on July 25, 2023:

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.

jonatack commented at 12:59 pm on July 25, 2023:

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.)

MarcoFalke commented at 1:08 pm on July 25, 2023:

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)

jonatack commented at 1:13 pm on July 25, 2023:

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.

in src/rpc/util.h:103 in 42233bea28 outdated

 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.

jonatack commented at 2:05 am on July 25, 2023:

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.

in src/core_read.cpp:261 in 42233bea28 outdated

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     }

jonatack commented at 2:07 am on July 25, 2023:

I like how the code in this function is now quite similar to my suggestion in #28113 (review) 👍

jonatack commented at 2:08 am on July 25, 2023: contributor

ACK modulo a few comments

TheCharlatan force-pushed on Jul 25, 2023

TheCharlatan commented at 7:04 am on July 25, 2023: contributor

Updated 42233bea28f6f7c464f0cd6499d675e81b3e8512 -> c5fac53bca920626843723869a7677cef639df4d (kernelRmUnivalue_6 -> kernelRmUnivalue_7, compare)

Addressed @jonatack’s comment and comment, removing unused include.
Addressed @jonatack’s comment, correcting docstring.

in src/test/fuzz/parse_univalue.cpp:72 in c5fac53bca outdated

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&) {

stickies-v commented at 9:58 am on July 25, 2023:

I think this catch now needs to go?

TheCharlatan commented at 10:04 am on July 25, 2023:

I’ll test this.

TheCharlatan commented at 11:57 am on July 25, 2023:

Seems like std::runtime_error is still thrown: https://github.com/TheCharlatan/bitcoin/commits/kernelRmUnivalue_8.

TheCharlatan commented at 12:14 pm on July 25, 2023:

Yeah, we can throw a type_error when calling get_str() and type_error inherits from the std::runtime_error.

stickies-v commented at 12:20 pm on July 25, 2023:

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:

 0diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
 1index 0b2d2b18af..377181dd81 100644
 2--- a/src/rpc/util.cpp
 3+++ b/src/rpc/util.cpp
 4@@ -318,6 +318,9 @@ int ParseSighashString(const UniValue& sighash)
 5     if (sighash.isNull()) {
 6         return SIGHASH_DEFAULT;
 7     }
 8+    if (!sighash.isStr()) {
 9+        throw JSONRPCError(RPC_INVALID_PARAMETER, "sighash needs to be null or string");
10+    }
11     const auto result{SighashFromStr(sighash.get_str())};
12     if (!result) {
13         throw JSONRPCError(RPC_INVALID_PARAMETER, util::ErrorString(result).original);

TheCharlatan commented at 12:26 pm on July 25, 2023:

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.

jonatack commented at 12:34 pm on July 25, 2023:

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.

stickies-v commented at 12:36 pm on July 25, 2023:

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

TheCharlatan commented at 12:49 pm on July 25, 2023:

Ah right, I just looked at the wrong examples :P - will push a patch for this.

MarcoFalke commented at 12:52 pm on July 25, 2023:

I think we do? E.g.

I think all of that code should be removed and the checks be done by RPCHelpMan.

theuni commented at 1:28 pm on July 25, 2023:

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:

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 !

jonatack commented at 12:42 pm on July 25, 2023: contributor

ACK c5fac53bca920626843723869a7677cef639df4d

with the following notes or follow-ups:

modulo #28113 (review): move the sighash check back to the callee or extract the sighash types enum to its own unit
test coverage of the error case(s)
small release note if the user-facing error changes
(follow-up) move checks to RPCHelpman per #28113 (review)

DrahtBot requested review from theuni on Jul 25, 2023

TheCharlatan force-pushed on Jul 25, 2023

TheCharlatan commented at 2:29 pm on July 25, 2023: contributor

Updated c5fac53bca920626843723869a7677cef639df4d -> d87edf309ffe60ae044a57ce9a0d9cc711edc365 (kernelRmUnivalue_7 -> kernelRmUnivalue_8, compare)

Addressed @jonatack’s comment, introduced sighashtype header. Can just drop it again, if others find it too controversial.
Addressed @stickies-v’s comment, removed std::runtime_error catch in fuzz test by checking if the input is a string and throwing RPC_INVALID_PARAMETER if it is not.
Added a release note to reflect this change in error return type.

in src/core_read.cpp:248 in d87edf309f outdated

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)},

stickies-v commented at 3:14 pm on July 25, 2023:

nit: missing #include <script/sighashtype.h>?

in doc/release-notes-28113.md:4 in d87edf309f outdated

0@@ -0,0 +1,6 @@
1+RPC Wallet
2+----------
3+
4+- The `signrawtransactionwithkey` and `descriptorprocesspsbt` calls now return

stickies-v commented at 3:18 pm on July 25, 2023:

nit: signrawtransactionwithwallet and walletprocesspsbt also affected

TheCharlatan commented at 3:33 pm on July 25, 2023:

Ugh, how did I miss those, will re-push.

fanquake commented at 3:22 pm on July 25, 2023: member

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:

0cat rpc/libbitcoin_common_a-util.o_master | wc -l
1   96263
2at rpc/libbitcoin_common_a-util.o_include | wc -l
3   96264

Looking at the actual diff, that extra line in the include case is a newline:

 0diffoscope rpc/libbitcoin_common_a-util.o_master rpc/libbitcoin_common_a-util.o_include
 1--- rpc/libbitcoin_common_a-util.o_master
 2+++ rpc/libbitcoin_common_a-util.o_include
 3@@ -94954,14 +94954,15 @@
 4 uint256 DescriptorID(const Descriptor& desc);
 5 # 12 "rpc/util.cpp" 2
 6 
 7 
 8 
 9 
10 
11+
12 # 1 "./util/translation.h" 1
13 # 18 "./util/translation.h"
14 struct bilingual_str {

and any other difference in the output is line number related, i.e:

 0@@ -96150,15 +96151,15 @@
 1         std::string res;
 2         for (const auto& i : m_inner) {
 3             res += i.ToString(oneline) + ",";
 4         }
 5         return "[" + res + "...]";
 6     }
 7     }
 8-    throw NonFatalCheckError( "Unreachable code reached (non-fatal)", "rpc/util.cpp", 1151, __func__);
 9+    throw NonFatalCheckError( "Unreachable code reached (non-fatal)", "rpc/util.cpp", 1152, __func__);
10 }
11 
12 static std::pair<int64_t, int64_t> ParseRange(const UniValue& value)
13 {
14     if (value.isNum()) {
15         return {0, value.getInt<int64_t>()};
16     }
17@@ -96238,15 +96239,15 @@
18 
19     return servicesNames;
20 }
21 
22 
23 [[nodiscard]] static UniValue BilingualStringsToUniValue(const std::vector<bilingual_str>& bilingual_strings)
24 {
25-    inline_check_non_fatal(!bilingual_strings.empty(), "rpc/util.cpp", 1239, __func__, "!bilingual_strings.empty()");
26+    inline_check_non_fatal(!bilingual_strings.empty(), "rpc/util.cpp", 1240, __func__, "!bilingual_strings.empty()");
27     UniValue result{UniValue::VARR};
28     for (const auto& s : bilingual_strings) {
29         result.push_back(s.original);
30     }
31     return result;
32 }

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.

stickies-v approved

stickies-v commented at 3:30 pm on July 25, 2023: contributor

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.

DrahtBot requested review from jonatack on Jul 25, 2023

kernel: Split ParseSighashString

This split is done in preparation for the next commit where the
dependency on UniValue in the kernel library is removed.

10eb3a9faa

kernel: Remove Univalue from kernel library

It is not required by any of the kernel components.
A JSON library should not need to be part of a consensus library.

6960c81cbf

TheCharlatan force-pushed on Jul 25, 2023

TheCharlatan commented at 3:45 pm on July 25, 2023: contributor

Updated d87edf309ffe60ae044a57ce9a0d9cc711edc365 -> 6960c81cbfa6208d4098353e53b313e13a21cb49 (kernelRmUnivalue_8 -> kernelRmUnivalue_9, compare)

Addressed @stickies-v’s comment, fixing release note.
Dropped the first commit splitting the interpreter header again.

theuni approved

theuni commented at 5:11 pm on July 25, 2023: member

Re-ACK 6960c81cbfa6208d4098353e53b313e13a21cb49

DrahtBot requested review from stickies-v on Jul 25, 2023

theuni commented at 5:15 pm on July 25, 2023: member

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.

stickies-v approved

stickies-v commented at 5:36 pm on July 25, 2023: contributor

re-ACK https://github.com/bitcoin/bitcoin/commit/6960c81cbfa6208d4098353e53b313e13a21cb49

achow101 commented at 10:02 pm on July 25, 2023: member

ACK 6960c81cbfa6208d4098353e53b313e13a21cb49

achow101 merged this on Jul 25, 2023

achow101 closed this on Jul 25, 2023

jonatack commented at 10:19 pm on July 25, 2023: contributor

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.

in doc/release-notes-28113.md:7 in 6960c81cbf

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.

MarcoFalke commented at 8:32 am on July 26, 2023:

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

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

TheCharlatan commented at 9:27 am on July 26, 2023:

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.

TheCharlatan commented at 10:10 am on July 26, 2023:

https://github.com/bitcoin/bitcoin/pull/28162

in src/rpc/util.cpp:322 in 6960c81cbf

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");

MarcoFalke commented at 8:33 am on July 26, 2023:

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

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

TheCharlatan commented at 9:28 am on July 26, 2023:

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.

MarcoFalke commented at 9:33 am on July 26, 2023:

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

yes

TheCharlatan commented at 10:10 am on July 26, 2023:

https://github.com/bitcoin/bitcoin/pull/28162

fanquake commented at 10:33 am on July 26, 2023: member

@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.cpp and add -E after 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.

kernel: Remove UniValue from kernel library #28113

Reviews

Conflicts