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 #6121

Merged
merged 18 commits into from Jun 4, 2015

Conversation

@jonasschnelli
Copy link
Member

jonasschnelli commented May 11, 2015

Takes up and supersedes #4738.

This PR keeps the JSON Spirit "backward-compatibility" and passes all tests. Therefore there is a new UniValue number type VREAL to distinct between int and double with the current JSON spirit precision of 8 digits after point.
Because after rfc4627 (and UniValue honors that) some basic types/strings like "null" or "0" and "string" are not valid JSON strings, i had to slightly change some unit tests (will comment below directly in the code).

@@ -142,8 +142,8 @@ Object CallRPC(const string& strMethod, const Array& params)
throw runtime_error("no response from server");

// Parse reply
Value valReply;
if (!read_string(strReply, valReply))
Value valReply(UniValue::VSTR);

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli May 11, 2015

Author Member

UniValue required explicit type definition during constructing.
Value should be replaced with UniValue soon.

@jonasschnelli
Copy link
Member Author

jonasschnelli commented May 11, 2015

I did use merge instead of rebase to clearly see my adaptations from #4738.

if (!jVal.read(strVal) || (jVal.isNull() && strVal.size() > 0))
if(!jVal.setNumStr(strVal) || jVal.isNull())
throw runtime_error(string("Error parsing JSON:")+strVal);
}

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli May 11, 2015

Author Member

I had to extend this to gain compatibility between JSON Spirit and UniValue. UniValue is more strict in case of JSON encoding.

Value value;
BOOST_CHECK(read_string(str, value));
UniValue value;
BOOST_CHECK(value.setNumStr(str));

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli May 11, 2015

Author Member

Had to change this to setNumStr because read() only parses valid JSON (things like "0.00000001" are not valid JSON).

EDIT: spelling

@@ -49,7 +49,7 @@ BOOST_AUTO_TEST_CASE(univalue_constructor)

double vd = -7.21;
UniValue v7(vd);
BOOST_CHECK(v7.isNum());
BOOST_CHECK(v7.isReal());

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli May 11, 2015

Author Member

Because now there is a distinction between int and real (to make it JSON Spirit compatible), things like -7.21 are now VREAL and no longer VNUM. I decided to make VREAL a standalone type instead of a VNUM subtype.

Edit: spelling

}

static const char *json1 =
"[1.1,{\"key1\":\"str\",\"key2\":800,\"key3\":{\"name\":\"martian\"}}]";
"[1.10000000,{\"key1\":\"str\",\"key2\":800,\"key3\":{\"name\":\"martian\"}}]";

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli May 11, 2015

Author Member

To keep the JSON/RPC API backward compatible, all reals represented in JSON have a fixed precision to 8 digits after point.

@@ -26,6 +30,9 @@ class UniValue {
UniValue(int64_t val_) {
setInt(val_);
}
UniValue(bool val_) {
setBool(val_);
}

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli May 11, 2015

Author Member

This PR also adds a clean boolean handling to UniValue (necessary for JSONRPC compatibility).

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2015/05/unival branch from 0d57ecc to 95c9b49 May 11, 2015
@jgarzik
Copy link
Contributor

jgarzik commented May 11, 2015

ut ACK - reviewed the whole thing

Will do some testing later this week.

Thanks for moving this forward!

@laanwj
Copy link
Member

laanwj commented May 12, 2015

Thanks for doing this work.

One remark: I see AmountFromValue and ValueFromAmount still make the value pass through a double. My prime gripe with JSON spirit is that monetary values still had to be converted from and to floating point which can cause deviations (see See #3759 and https://bitcoin.stackexchange.com/questions/22716/bitcoind-sendfrom-round-amount-error ).

Ideally a new JSON implementation will avoid this and can interpret and generate Numbers from/to fixed-point or decimal directly. Similar to Python's

postdata = json.dumps(list(rpc_call_list), default=EncodeDecimal)
response = json.loads(responsedata, parse_float=decimal.Decimal)
@jgarzik
Copy link
Contributor

jgarzik commented May 12, 2015

Agree w/ your gripe @laanwj. However, I think of that as a separate logical step:

  1. Convert tree with as much equivalence as possible, to prove everything works as it always did.
  2. Improve the tree beyond json-spirit equivalence.

Separated thusly it is easier to test the conversion before moving on to more ambitious cleanups.

@laanwj laanwj mentioned this pull request May 15, 2015
5 of 5 tasks complete
@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2015/05/unival branch from eee8acb to 2a3126c May 18, 2015
@jonasschnelli
Copy link
Member Author

jonasschnelli commented May 18, 2015

This is now complete.

Just added another commit (2a3126c) which does completely remove JSON Spirit (also the UniValue wrapper) from the sources and the compile process.

Passes all tests.

If the diff size makes this PR to risky, there would also be the chance to pull this in without 2a3126c or / and separate 2a3126c into another PR.

I think it would be good to remove JSON Spirit in this PR while not breaking the RPC API. After this has been done, we could focus on optimizing the monetary values.

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2015/05/unival branch from 2a3126c to 2f292e2 May 18, 2015
@jonasschnelli
Copy link
Member Author

jonasschnelli commented May 18, 2015

Gitian was also happy with this: http://builds.jonasschnelli.ch/pulls/6121/

@jgarzik
Copy link
Contributor

jgarzik commented May 18, 2015

Yes, I agree JSON-spirit should be removed in this PR.

Reviewing & testing right now...

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2015/05/unival branch 2 times, most recently from d8f0bd1 to 8412abd Jun 1, 2015
@jonasschnelli
Copy link
Member Author

jonasschnelli commented Jun 1, 2015

Rebased (puhh).
Thanks in advance for reviewing this (maybe @jgarzik and @theuni) and finally bring this forward.

@laanwj
Copy link
Member

laanwj commented Jun 2, 2015

(apparantly) spurious travis failure with the comparison tool, retriggering

@laanwj
Copy link
Member

laanwj commented Jun 2, 2015

Looks good to me. Tested and reviewed ACK.
Would like to merge this asap now that 0.11 is branched.

@laanwj
Copy link
Member

laanwj commented Jun 2, 2015

This breaks linearize-hashes.py. Likely, this is caused by JSON-RPC batching no longer working:

./linearize-hashes.py  linearize.cfg 
Traceback (most recent call last):
  File "./linearize-hashes.py", line 112, in <module>
    get_block_hashes(settings)
  File "./linearize-hashes.py", line 68, in get_block_hashes
    for x,resp_obj in enumerate(reply):
TypeError: 'NoneType' object is not iterable

(expected output: a list of block hashes)

@jonasschnelli
Copy link
Member Author

jonasschnelli commented Jun 2, 2015

Thanks for testing. Nice catch!
Will have a close look at the batching feature within the next hours.

@jonasschnelli
Copy link
Member Author

jonasschnelli commented Jun 2, 2015

RPC batching issue is fixed.

laanwj added 2 commits Jun 4, 2015
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.
@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2015/05/unival branch from 7f442e7 to c023092 Jun 4, 2015
@laanwj laanwj merged commit 44c7474 into bitcoin:master Jun 4, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Jun 4, 2015
44c7474 univalue: add type check unit tests (Jonas Schnelli)
c023092 univalue: add strict type checking (Wladimir J. van der Laan)
7e98a3c util: Add ParseInt64 and ParseDouble functions (Wladimir J. van der Laan)
043df2b Simplify RPCclient, adapt json_parse_error test (Wladimir J. van der Laan)
519eede fix univalue json parse tests (Jonas Schnelli)
c7fbbc7 fix missing univalue types during constructing (Jonas Schnelli)
8f7e4ab fix rpc batching univalue issue (Jonas Schnelli)
9a8897f Remove JSON Spirit wrapper, remove JSON Spirit leftovers (Jonas Schnelli)
3df0411 remove JSON Spirit UniValue wrapper (Jonas Schnelli)
1f263c8 fix rpc unit test, plain numbers are not JSON compatible object (Jonas Schnelli)
e04d9c2 univalue: correct bool support (Jonas Schnelli)
0c5b2cf univalue: add support for real, fix percision and make it json_spirit compatible (Jonas Schnelli)
21c10de special threatment for null,true,false because they are non valid json (Jonas Schnelli)
6c7bee0 expicit set UniValue type to avoid empty values (Jonas Schnelli)
53b4671 extend conversion to UniValue (Jonas Schnelli)
15982a8 Convert tree to using univalue. Eliminate all json_spirit uses. (Jeff Garzik)
5e3060c UniValue: export NullUniValue global constant (Jeff Garzik)
efc7883 UniValue: prefer .size() to .count(), to harmonize w/ existing tree (Jeff Garzik)
@sdaftuar
Copy link
Member

sdaftuar commented Jun 4, 2015

I'm getting an error now when running qa/rpc-tests/getblocktemplate_proposals.py:
JSONRPC error: Missing data String key for proposal

Looks like it's here:

File "./getblocktemplate_proposals.py", line 86, in assert_template
    rsp = node.getblocktemplate({'data':template_to_hex(tmpl, txlist),'mode':'proposal'})
@jonasschnelli
Copy link
Member Author

jonasschnelli commented Jun 4, 2015

Meh. This bug crept in during multiple transitions from json spirit objects to UniValue.
Fixed with #6234.

Thanks @sdaftuar for reporting!

@jgarzik
Copy link
Contributor

jgarzik commented Jun 4, 2015

Odd that I missed that in my testing. Anyway, re-ACK! Tested.

@theuni
Copy link
Member

theuni commented Jun 5, 2015

@jonasschnelli Sorry for not getting to a final review in time. I'll still look over it again in master for good measure. Great work!

jonasschnelli added a commit to jonasschnelli/bitcoin that referenced this pull request Jun 5, 2015
jonasschnelli added a commit to jonasschnelli/bitcoin that referenced this pull request Jun 6, 2015
gitmarek pushed a commit to gitmarek/libbitcoinrpc that referenced this pull request Mar 3, 2016
gitmarek pushed a commit to gitmarek/libbitcoinrpc that referenced this pull request Mar 3, 2016
zkbot added a commit to zcash/zcash 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 added a commit to zcash/zcash that referenced this pull request Feb 7, 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 added a commit to zcash/zcash that referenced this pull request Feb 10, 2017
…=bitcartel

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 added a commit to zcash/zcash 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 added a commit to zcash/zcash 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.
AllanDoensen referenced this pull request in AllanDoensen/BitcoinUnlimited Apr 23, 2017
was introduced with #6121
reddink added a commit to reddcoin-project/reddcoin that referenced this pull request May 27, 2020
Change `read_string` to fail when not the entire input has been
consumed. This avoids unexpected, even dangerous behavior (fixes bitcoin#6223).

The new JSON parser adapted in bitcoin#6121 also solves this problem so in
master this is a temporary fix, but should be backported to older releases.

Also adds tests for the new behavior.

Github-Pull: bitcoin#6226
Rebased-From: 4e157fc
(cherry picked from commit 181771b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.