Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert entire source tree from json_spirit to UniValue #1990

Merged
merged 26 commits into from Feb 10, 2017

Conversation

@str4d
Copy link
Contributor

commented Jan 6, 2017

This PR cherry-picks bitcoin/bitcoin#6121 and then migrates the Zcash-specific code to UniValue.

Also cherry-picks:

Closes #1985.

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2017

There is currently a single compile bug, unrelated to any UniValue refactoring, caused by 12eb903 from upstream in combination with our use of -Werror:

test/util_tests.cpp:353:1: error: integer constant is so large that it is unsigned [-Werror]
     BOOST_CHECK(ParseInt64("-9223372036854775808", &n) && n == 9223372036854775808LL);
 ^
cc1plus: all warnings being treated as errors
@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2017

Somewhat strangely, it isn't possible to use std::vector<UniValue>::iterator. Maybe this has been fixed in another PR upstream, but for now I've adjusted the tests to manually iterate.

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jan 9, 2017

☔️ The latest upstream changes (presumably #1949) made this pull request unmergeable. Please resolve the merge conflicts.

@str4d str4d force-pushed the str4d:1985-replace-json-spirit-with-univalue branch from 0fc18be to 45a3301 Jan 10, 2017
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jan 16, 2017

☔️ The latest upstream changes (presumably #2021) made this pull request unmergeable. Please resolve the merge conflicts.

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2017

@ebfull To keep on schedule with the three week release cycle, let's consider merging in the other PRs first and if we have time, review and merge this, otherwise make this the first to review and merge for 1.0.6.

@bitcartel bitcartel requested a review from daira Jan 16, 2017
@str4d str4d force-pushed the str4d:1985-replace-json-spirit-with-univalue branch from 45a3301 to a2ac5a6 Jan 24, 2017
@str4d str4d added this to the 1.0.6 milestone Feb 1, 2017
@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2017

Adding to 1.0.6 per @bitcartel's comment above; release manager to confirm.

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2017

Confirmed that the compile error in the test code (#1990 (comment)) does not occur without -Werror. So we probably need to rewrite that test...

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2017

This was a bug in the original PR that happened to work with Bitcoin's compiler at the time. Detected when clang was complaining, and fixed in bitcoin/bitcoin#6241.

@str4d str4d added the review needed label Feb 1, 2017
Array witness_ser_tests = read_json(std::string(json_tests::merkle_witness_serialization, json_tests::merkle_witness_serialization + sizeof(json_tests::merkle_witness_serialization)));
Array path_tests = read_json(std::string(json_tests::merkle_path, json_tests::merkle_path + sizeof(json_tests::merkle_path)));
Array commitment_tests = read_json(std::string(json_tests::merkle_commitments, json_tests::merkle_commitments + sizeof(json_tests::merkle_commitments)));
UniValue root_tests = read_json(std::string(json_tests::merkle_roots, json_tests::merkle_roots + sizeof(json_tests::merkle_roots)));

This comment has been minimized.

Copy link
@daira

daira Feb 2, 2017

Contributor

There should be some macro

#define MAKE_STRING(x) std::string((x), (x)+sizeof(x))

[does not block]

HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
OTHER DEALINGS IN THE SOFTWARE.

This comment has been minimized.

Copy link
@daira

daira Feb 2, 2017

Contributor

Please also remove the contrib/debian/copyright entry here.

@@ -11,8 +11,10 @@
#include <set>
#include <stdint.h>

#include <boost/algorithm/string/case_conv.hpp> // for to_lower()

This comment has been minimized.

Copy link
@daira

daira Feb 2, 2017

Contributor

That function does a locale-dependent conversion, based on std::locale() by default. Is that really what we want? (For example, the lowercase of 'I' in Turkish is 'ı', not 'i'.)

This comment has been minimized.

Copy link
@daira

daira Feb 2, 2017

Contributor

Actually I don't see where to_lower is used in this file.

This comment has been minimized.

Copy link
@str4d

str4d Feb 7, 2017

Author Contributor

This is still present in upstream master, also with no apparent usage in the file.

This comment has been minimized.

Copy link
@daira

daira Feb 8, 2017

Contributor

Let's delete it.

Array txids = params[0].get_array();
BOOST_FOREACH(Value& txid, txids) {
UniValue txids = params[0].get_array();
for (unsigned int idx = 0; idx < txids.size(); idx++) {

This comment has been minimized.

Copy link
@daira

daira Feb 2, 2017

Contributor

UniValue::size() returns size_t.


CMutableTransaction rawTx;

BOOST_FOREACH(const Value& input, inputs) {
const Object& o = input.get_obj();
for (unsigned int idx = 0; idx < inputs.size(); idx++) {

This comment has been minimized.

Copy link
@daira

daira Feb 2, 2017

Contributor

size_t

Array keys = params[2].get_array();
BOOST_FOREACH(Value k, keys) {
UniValue keys = params[2].get_array();
for (unsigned int idx = 0; idx < keys.size(); idx++) {

This comment has been minimized.

Copy link
@daira

daira Feb 2, 2017

Contributor

size_t

totalAmount += nAmount;

bool fSubtractFeeFromAmount = false;
BOOST_FOREACH(const Value& addr, subtractFeeFromAmount)
if (addr.get_str() == s.name_)
for (unsigned int idx = 0; idx < subtractFeeFromAmount.size(); idx++) {

This comment has been minimized.

Copy link
@daira

daira Feb 2, 2017

Contributor

size_t

{
if (output.type() != obj_type)
UniValue outputs = params[1].get_array();
for (unsigned int idx = 0; idx < outputs.size(); idx++) {

This comment has been minimized.

Copy link
@daira

daira Feb 2, 2017

Contributor

size_t

Array inputs = params[2].get_array();
BOOST_FOREACH(Value& input, inputs) {
UniValue inputs = params[2].get_array();
for (unsigned int idx = 0; idx < inputs.size(); idx++) {

This comment has been minimized.

Copy link
@daira

daira Feb 2, 2017

Contributor

size_t

@@ -347,7 +347,7 @@ static void MutateTxSign(CMutableTransaction& tx, const string& flagStr)
UniValue keysObj = registers["privatekeys"];
fGivenKeys = true;

for (unsigned int kidx = 0; kidx < keysObj.count(); kidx++) {
for (unsigned int kidx = 0; kidx < keysObj.size(); kidx++) {

This comment has been minimized.

Copy link
@daira

daira Feb 2, 2017

Contributor

size_t

@@ -364,7 +364,7 @@ static void MutateTxSign(CMutableTransaction& tx, const string& flagStr)
throw runtime_error("prevtxs register variable must be set.");
UniValue prevtxsObj = registers["prevtxs"];
{
for (unsigned int previdx = 0; previdx < prevtxsObj.count(); previdx++) {
for (unsigned int previdx = 0; previdx < prevtxsObj.size(); previdx++) {

This comment has been minimized.

Copy link
@daira

daira Feb 2, 2017

Contributor

size_t

@@ -175,21 +182,21 @@ bool UniValue::checkObject(const std::map<std::string,UniValue::VType>& t)
const UniValue& UniValue::operator[](const std::string& key) const
{
if (typ != VOBJ)
return nullValue;
return NullUniValue;

int index = findKey(key);

This comment has been minimized.

Copy link
@daira

daira Feb 2, 2017

Contributor

Ugh, maybe change the return type of findKey to ssize_t? But then we need to prevent the length of values from exceeding std::numeric_limits<ssize_t>::max(). (Well, that won't happen in practice because it's pretty difficult to add 263 items in feasible time. So maybe add an assert(values.size() <= std::numeric_limits<ssize_t>::max()); in findKey?)

This comment has been minimized.

Copy link
@str4d

str4d Feb 7, 2017

Author Contributor

This is still present in latest upstream, so should be turned into a PR there.


return values[index];
}

const UniValue& UniValue::operator[](unsigned int index) const
{
if (typ != VOBJ && typ != VARR)
return nullValue;
return NullUniValue;

This comment has been minimized.

if (index >= values.size())
return nullValue;
return NullUniValue;

This comment has been minimized.

Copy link
@daira

daira Feb 2, 2017

Contributor

... or this.

This comment has been minimized.

Copy link
@daira

daira Feb 2, 2017

Contributor

I guess it will throw an exception if you call any of the get_* methods on a null result. Failing faster would be preferable though (to preserve more information about the context in which the error occurred). It's unfortunate that the current code may depend on this behaviour, so we probably can't change it :-(

This comment has been minimized.

Copy link
@str4d

str4d Feb 7, 2017

Author Contributor

I wouldn't change it in this PR; instead, we'd want to open a PR upstream, and we'd also probably have to provide a PR for Bitcoin in order for them to consider making the change.

{
if (typ != VBOOL)
throw std::runtime_error("JSON value is not a boolean as expected");
return getBool();

This comment has been minimized.

Copy link
@daira

daira Feb 2, 2017

Contributor

This is the only use of getBool(). Perhaps getBool should not have been public.

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2017

@daira sorry for not mentioning this before your review: there is another upstream PR (bitcoin/bitcoin#6637) that updates the UniValue code to a newer version (using a git subtree). I will be making a PR for that after this merges, as that is the same ordering as upstream (and therefore made this PR less-conflicted); reviews of src/univalue should probably happen after that (or then), and feedback be pushed upstream.

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2017

@str4d So the focus for the review of this PR should be that all tests pass?

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2017

Before @str4d makes the PR above, @daira @nathan-at-least using git subtree for the updated version of Univalue would be inconsistent with other dependent packages. However there are no official tarballs of Univalue where we can check hash digests. What's the best course of action?

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2017

Compiles fine locally. Let's run the tests.
@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 5, 2017

⌛️ Trying commit 913a31b with merge a2e69fa...

zkbot added a commit that referenced this pull request Feb 5, 2017
…=<try>

Convert entire source tree from json_spirit to UniValue

This PR cherry-picks bitcoin/bitcoin#6121 and then migrates the Zcash-specific code to UniValue.

Also cherry-picks:
- bitcoin/bitcoin#6241
- bitcoin/bitcoin#6234

Closes #1985.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 5, 2017

💔 Test failed - zcash

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2017

@str4d See test error above. Looks like the PR needs to be rebased and tweaked for recent logging commit which used a json_spirit call (to output object on one line):

wallet/asyncrpcoperation_sendmany.cpp: In constructor ‘AsyncRPCOperation_sendmany::AsyncRPCOperation_sendmany(std::string, std::vector<std::tuple<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, long int, std::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, std::vector<std::tuple<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, long int, std::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, int, CAmount, UniValue)’:
wallet/asyncrpcoperation_sendmany.cpp:97:85: error: ‘json_spirit’ has not been declared
         LogPrint("zrpcunsafe", "%s: z_sendmany initialized (params=%s)\n", getId(), json_spirit::write_string( contextInfo, false));
                                                                                     ^
Makefile:4603: recipe for target 'wallet/libbitcoin_wallet_a-asyncrpcoperation_sendmany.o' failed
make[2]: *** [wallet/libbitcoin_wallet_a-asyncrpcoperation_sendmany.o] Error 1
make[2]: *** Waiting for unfinished jobs....
jonasschnelli and others added 14 commits Jun 2, 2015
# Conflicts:
#	src/test/rpc_tests.cpp
Strict parsing functions for other numeric types.

- ParseInt64 analogous to ParseInt32, but for 64-bit values.
- ParseDouble for doubles.
- Make all three Parse* functions more strict (e.g. reject whitespace on
  the inside)

Also add tests.
was introduced with #6121
@str4d str4d force-pushed the str4d:1985-replace-json-spirit-with-univalue branch from 0976020 to 02972a1 Feb 10, 2017
@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2017

Rebased on master to change a single Array from another PR to use UniValue.

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 10, 2017

📌 Commit 02972a1 has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 10, 2017

⌛️ Testing commit 02972a1 with merge a7b8d93...

zkbot added a commit that referenced this pull request Feb 10, 2017
…=str4d

Convert entire source tree from json_spirit to UniValue

This PR cherry-picks bitcoin/bitcoin#6121 and then migrates the Zcash-specific code to UniValue.

Also cherry-picks:
- bitcoin/bitcoin#6241
- bitcoin/bitcoin#6234

Closes #1985.
@str4d str4d referenced this pull request Feb 10, 2017
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 10, 2017

💔 Test failed - zcash

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2017

@bitcartel looks like the failure was caused by a slow JoinSplit during RPC testing: https://ci.z.cash/builders/zcash/builds/315/steps/rpc-tests.sh/logs/stdio

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2017

@str4d Yep. This seems to happen now and again. I will retry later.

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2017

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 10, 2017

⌛️ Testing commit 02972a1 with merge e51bd1b...

zkbot added a commit that referenced this pull request Feb 10, 2017
…=str4d

Convert entire source tree from json_spirit to UniValue

This PR cherry-picks bitcoin/bitcoin#6121 and then migrates the Zcash-specific code to UniValue.

Also cherry-picks:
- bitcoin/bitcoin#6241
- bitcoin/bitcoin#6234

Closes #1985.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 10, 2017

☀️ Test successful - zcash

@zkbot zkbot merged commit 02972a1 into zcash:master Feb 10, 2017
1 check passed
1 check passed
homu Test successful
Details
@str4d str4d referenced this pull request Feb 13, 2017
196 of 452 tasks complete
@str4d str4d referenced this pull request Mar 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.