kernel: Remove UniValue from kernel library #28113

sedited · 2023-07-20T14:06:16Z

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 · 2023-07-20T14:06:24Z

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.

hebasto · 2023-07-20T14:08:25Z

Concept ACK.

stickies-v · 2023-07-20T14:10:28Z

Concept ACK

maflcko

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

src/common/univalue_helpers.h

stickies-v · 2023-07-20T17:09:58Z

src/common/univalue_helpers.cpp

+    return ParseHex(strHex);
+}
+
+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)

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

sedited · 2023-07-21T14:44:05Z

Updated 03e06e5 -> 0498ea6 (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.

src/common/univalue_helpers.cpp

sedited · 2023-07-21T15:23:48Z

Updated 0498ea6 -> 05477b5 (kernelRmUnivalue_1 -> kernelRmUnivalue_2, compare)

Addressed @MarcoFalke's comment, fixed include spacing.

fanquake · 2023-07-21T15:29:00Z

Concept ACK. cc @theuni

hebasto · 2023-07-21T16:02:35Z

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 · 2023-07-21T20:36:51Z

The ParseHexUV function is used in bitcoin-tx.cpp only. Why not make it static/namespaced in there?

See: hebasto@6d0fcfe

sedited · 2023-07-21T20:43:26Z

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 · 2023-07-21T21:15:08Z

Sacrificing the fuzz test seems unfortunate.

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

hebasto · 2023-07-22T06:03:59Z

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

sedited · 2023-07-22T09:45:33Z

Updated 05477b5 -> 67e172a (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.

sedited · 2023-07-22T18:44:18Z

Updated 67e172a -> b89567f (kernelRmUnivalue_3 -> kernelRmUnivalue_4, compare)

Fixed fuzz test by catching the UniValue exception.

hebasto

Approach ACK b89567f. Nice!

@TheCharlatan

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

sedited · 2023-07-22T20:43:53Z

Re #28113 (review)

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.

jonatack

Approach ACK b89567f

src/core_read.cpp

src/core_io.h

src/bitcoin-tx.cpp

src/rpc/util.cpp

sedited · 2023-07-23T21:21:39Z

Updated b89567f -> a3774d1 (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.

jonatack · 2023-07-23T21:33:12Z

ACK a3774d1

jonatack

ACK modulo a few comments

src/core_io.h

src/core_read.cpp

src/rpc/util.cpp

src/rpc/util.h

src/core_read.cpp

sedited · 2023-07-25T07:04:03Z

Updated 42233be -> c5fac53 (kernelRmUnivalue_6 -> kernelRmUnivalue_7, compare)

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

src/test/fuzz/parse_univalue.cpp

jonatack · 2023-07-25T12:42:01Z

ACK c5fac53

with the following notes or follow-ups:

modulo kernel: Remove UniValue from kernel library #28113 (comment): 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 kernel: Remove UniValue from kernel library #28113 (comment)

sedited · 2023-07-25T14:29:00Z

Updated c5fac53 -> d87edf3 (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.

fanquake · 2023-07-25T15:22:58Z

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 (comment)) 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 (e35fb7b) 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.

stickies-v

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.

src/core_read.cpp

doc/release-notes-28113.md

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.

sedited · 2023-07-25T15:45:55Z

Updated d87edf3 -> 6960c81 (kernelRmUnivalue_8 -> kernelRmUnivalue_9, compare)

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

theuni

Re-ACK 6960c81

theuni · 2023-07-25T17:15:23Z

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

re-ACK 6960c81

achow101 · 2023-07-25T22:02:53Z

ACK 6960c81

jonatack · 2023-07-25T22:19:47Z

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.

maflcko · 2023-07-26T08:32:47Z

doc/release-notes-28113.md

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


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.

maflcko · 2023-07-26T08:33:37Z

src/rpc/util.cpp

+        return SIGHASH_DEFAULT;
+    }
+    if (!sighash.isStr()) {
+        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 · 2023-07-26T10:33:11Z

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

DrahtBot added the Validation label Jul 20, 2023

maflcko approved these changes Jul 20, 2023

View reviewed changes

src/common/univalue_helpers.cpp Outdated Show resolved Hide resolved

src/common/univalue_helpers.h Outdated Show resolved Hide resolved

stickies-v reviewed Jul 20, 2023

View reviewed changes

sedited force-pushed the kernelRmUnivalue branch from 03e06e5 to 0498ea6 Compare July 21, 2023 14:43

maflcko reviewed Jul 21, 2023

View reviewed changes

src/common/univalue_helpers.cpp Outdated Show resolved Hide resolved

sedited force-pushed the kernelRmUnivalue branch from 0498ea6 to 05477b5 Compare July 21, 2023 15:18

sedited force-pushed the kernelRmUnivalue branch 2 times, most recently from 269f254 to 67e172a Compare July 22, 2023 09:41

DrahtBot added the CI failed label Jul 22, 2023

sedited force-pushed the kernelRmUnivalue branch from 67e172a to b89567f Compare July 22, 2023 18:43

hebasto reviewed Jul 22, 2023

View reviewed changes

jonatack reviewed Jul 23, 2023

View reviewed changes

src/core_read.cpp Outdated Show resolved Hide resolved

src/core_io.h Show resolved Hide resolved

src/bitcoin-tx.cpp Show resolved Hide resolved

src/rpc/util.cpp Outdated Show resolved Hide resolved

sedited force-pushed the kernelRmUnivalue branch from b89567f to a3774d1 Compare July 23, 2023 21:21

DrahtBot requested a review from maflcko July 23, 2023 21:33

DrahtBot requested a review from jonatack July 24, 2023 19:51

jonatack reviewed Jul 25, 2023

View reviewed changes

src/core_io.h Outdated Show resolved Hide resolved

src/core_read.cpp Outdated Show resolved Hide resolved

src/rpc/util.cpp Show resolved Hide resolved

src/rpc/util.h Outdated Show resolved Hide resolved

src/core_read.cpp Show resolved Hide resolved

sedited force-pushed the kernelRmUnivalue branch from 42233be to c5fac53 Compare July 25, 2023 07:03

stickies-v reviewed Jul 25, 2023

View reviewed changes

src/test/fuzz/parse_univalue.cpp Outdated Show resolved Hide resolved

DrahtBot requested a review from theuni July 25, 2023 12:42

sedited force-pushed the kernelRmUnivalue branch from c5fac53 to d87edf3 Compare July 25, 2023 14:28

stickies-v approved these changes Jul 25, 2023

View reviewed changes

src/core_read.cpp Show resolved Hide resolved

doc/release-notes-28113.md Outdated Show resolved Hide resolved

DrahtBot requested a review from jonatack July 25, 2023 15:30

sedited added 2 commits July 25, 2023 17:40

kernel: Split ParseSighashString

10eb3a9

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

kernel: Remove Univalue from kernel library

6960c81

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

sedited force-pushed the kernelRmUnivalue branch from d87edf3 to 6960c81 Compare July 25, 2023 15:44

theuni approved these changes Jul 25, 2023

View reviewed changes

DrahtBot requested a review from stickies-v July 25, 2023 17:11

stickies-v approved these changes Jul 25, 2023

View reviewed changes

achow101 merged commit 1ed8a0f into bitcoin:master Jul 25, 2023

maflcko reviewed Jul 26, 2023

View reviewed changes

sedited mentioned this pull request Jul 26, 2023

refactor: Revert additional univalue check in ParseSighashString #28162

Merged

bitcoin locked and limited conversation to collaborators Jul 25, 2024

kernel: Remove UniValue from kernel library #28113

kernel: Remove UniValue from kernel library #28113

Uh oh!

Conversation

sedited commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

hebasto commented Jul 20, 2023

Uh oh!

stickies-v commented Jul 20, 2023

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stickies-v Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sedited commented Jul 21, 2023

Uh oh!

Uh oh!

sedited commented Jul 21, 2023

Uh oh!

fanquake commented Jul 21, 2023

Uh oh!

hebasto commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jul 21, 2023

Uh oh!

sedited commented Jul 21, 2023

Uh oh!

hebasto commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jul 22, 2023

Uh oh!

sedited commented Jul 22, 2023

Uh oh!

sedited commented Jul 22, 2023

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

sedited commented Jul 22, 2023

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sedited commented Jul 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Jul 23, 2023

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sedited commented Jul 25, 2023

Uh oh!

Uh oh!

jonatack commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sedited commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

sedited commented Jul 20, 2023 •

edited

Loading

DrahtBot commented Jul 20, 2023 •

edited

Loading

stickies-v Jul 20, 2023 •

edited

Loading

hebasto commented Jul 21, 2023 •

edited

Loading

hebasto commented Jul 21, 2023 •

edited

Loading

sedited commented Jul 23, 2023 •

edited

Loading

jonatack commented Jul 25, 2023 •

edited

Loading

sedited commented Jul 25, 2023 •

edited

Loading

jonatack commented Jul 25, 2023 •

edited

Loading