Convert entire source tree from json_spirit to UniValue #6121

pull jonasschnelli wants to merge 18 commits into bitcoin:master from jonasschnelli:2015/05/unival changing 44 files +1129 −2742
  1. jonasschnelli commented at 8:19 AM on May 11, 2015: contributor

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

  2. in src/bitcoin-cli.cpp:None in 0d57ecc1fe outdated 
     141 | @@ -142,8 +142,8 @@ Object CallRPC(const string& strMethod, const Array& params)
 142 |          throw runtime_error("no response from server");
 143 |  
 144 |      // Parse reply
 145 | -    Value valReply;
 146 | -    if (!read_string(strReply, valReply))
 147 | +    Value valReply(UniValue::VSTR);
    jonasschnelli commented at 8:20 AM on May 11, 2015:

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

  3. jonasschnelli commented at 8:21 AM on May 11, 2015: contributor

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

  4. in src/rpcclient.cpp:None in 0d57ecc1fe outdated 
     149 | +            else
 150 | +            {
 151 | +                if (!jVal.read(strVal) || (jVal.isNull() && strVal.size() > 0))
 152 | +                    if(!jVal.setNumStr(strVal) || jVal.isNull())
 153 | +                        throw runtime_error(string("Error parsing JSON:")+strVal);
 154 | +            }
    jonasschnelli commented at 8:25 AM on May 11, 2015:

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

  5. in src/test/rpc_tests.cpp:None in 0d57ecc1fe outdated 
     132 |  static Value ValueFromString(const std::string &str)
 133 |  {
 134 | -    Value value;
 135 | -    BOOST_CHECK(read_string(str, value));
 136 | +    UniValue value;
 137 | +    BOOST_CHECK(value.setNumStr(str));
    jonasschnelli commented at 8:28 AM on May 11, 2015:

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

    EDIT: spelling

  6. in src/test/univalue_tests.cpp:None in 0d57ecc1fe outdated 
      48 | @@ -49,7 +49,7 @@ BOOST_AUTO_TEST_CASE(univalue_constructor)
  49 |  
  50 |      double vd = -7.21;
  51 |      UniValue v7(vd);
  52 | -    BOOST_CHECK(v7.isNum());
  53 | +    BOOST_CHECK(v7.isReal());
    jonasschnelli commented at 8:30 AM on May 11, 2015:

    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

  7. in src/test/univalue_tests.cpp:None in 0d57ecc1fe outdated 
     244 | +    BOOST_CHECK_EQUAL(obj.size(), 0);
 245 |  }
 246 |  
 247 |  static const char *json1 =
 248 | -"[1.1,{\"key1\":\"str\",\"key2\":800,\"key3\":{\"name\":\"martian\"}}]";
 249 | +"[1.10000000,{\"key1\":\"str\",\"key2\":800,\"key3\":{\"name\":\"martian\"}}]";
    jonasschnelli commented at 8:31 AM on May 11, 2015:

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

  8. in src/univalue/univalue.h:None in 0d57ecc1fe outdated 
      29 | @@ -26,6 +30,9 @@ class UniValue {
  30 |      UniValue(int64_t val_) {
  31 |          setInt(val_);
  32 |      }
  33 | +    UniValue(bool val_) {
  34 | +        setBool(val_);
  35 | +    }
    jonasschnelli commented at 8:33 AM on May 11, 2015:

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

  9. jonasschnelli force-pushed on May 11, 2015
  10. jgarzik commented at 3:15 PM on May 11, 2015: contributor

    ut ACK - reviewed the whole thing

    Will do some testing later this week.

    Thanks for moving this forward!

  11. laanwj added the label RPC on May 12, 2015
  12. laanwj added the label REST on May 12, 2015
  13. laanwj commented at 12:48 PM on May 12, 2015: member

    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)
  14. jgarzik commented at 2:09 PM on May 12, 2015: contributor

    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.

  15. jonasschnelli force-pushed on May 18, 2015
  16. jonasschnelli commented at 12:08 PM on May 18, 2015: contributor

    This is now complete.

    Just added another commit (https://github.com/bitcoin/bitcoin/commit/2a3126c336fc0cc2b19f9b89bd48ed3e489a0817) 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 https://github.com/bitcoin/bitcoin/commit/2a3126c336fc0cc2b19f9b89bd48ed3e489a0817 or / and separate https://github.com/bitcoin/bitcoin/commit/2a3126c336fc0cc2b19f9b89bd48ed3e489a0817 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.

  17. jonasschnelli force-pushed on May 18, 2015
  18. jonasschnelli commented at 4:21 PM on May 18, 2015: contributor

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

  19. jgarzik commented at 4:51 PM on May 18, 2015: contributor

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

    Reviewing & testing right now...

  20. jonasschnelli force-pushed on Jun 1, 2015
  21. jonasschnelli force-pushed on Jun 1, 2015
  22. jonasschnelli commented at 1:36 PM on June 1, 2015: contributor

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

  23. laanwj commented at 7:34 AM on June 2, 2015: member

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

  24. laanwj commented at 8:10 AM on June 2, 2015: member

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

  25. laanwj commented at 8:43 AM on June 2, 2015: member

    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)

  26. jonasschnelli commented at 9:20 AM on June 2, 2015: contributor

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

  27. jonasschnelli commented at 9:41 AM on June 2, 2015: contributor

    RPC batching issue is fixed.

  28. laanwj commented at 10:18 AM on June 2, 2015: member

    Can confirm that it is fixed. Damnit, another random travis fail.

  29. jonasschnelli commented at 10:30 AM on June 2, 2015: contributor

    added another commit that fixes a univalue type issue within getrawmempool and getmempoolinfo

  30. jtimon commented at 11:00 AM on June 2, 2015: contributor

    Concept ACK. Is this getting into 0.11? This kind of change tends to be painful to maintain...

  31. jonasschnelli commented at 11:36 AM on June 2, 2015: contributor

    @jtimon It's not for 0.11. It's meant for current master which aims for 0.12.

  32. laanwj referenced this in commit a4993f7515 on Jun 3, 2015
  33. laanwj referenced this in commit 46f92fc9d8 on Jun 3, 2015
  34. laanwj referenced this in commit 4e157fc60d on Jun 3, 2015
  35. laanwj referenced this in commit a7dcb7eb04 on Jun 3, 2015
  36. laanwj referenced this in commit 5901596548 on Jun 3, 2015
  37. laanwj referenced this in commit 181771b712 on Jun 3, 2015
  38. laanwj commented at 6:48 AM on June 4, 2015: member

    Needs rebase after #6226, will merge after that

  39. UniValue: prefer .size() to .count(), to harmonize w/ existing tree efc7883772
  40. UniValue: export NullUniValue global constant 5e3060c0d1
  41. Convert tree to using univalue. Eliminate all json_spirit uses. 15982a8b69
  42. extend conversion to UniValue 53b4671a9d
  43. expicit set UniValue type to avoid empty values 6c7bee0624
  44. special threatment for null,true,false because they are non valid json 21c10de8c2
  45. univalue: add support for real, fix percision and make it json_spirit compatible
    - avoid breaking the API because of different number/percision handling
    0c5b2cf69a
  46. univalue: correct bool support e04d9c25cf
  47. fix rpc unit test, plain numbers are not JSON compatible object
    UniValues read() does only read valid json.
    1f263c899e
  48. remove JSON Spirit UniValue wrapper 3df0411ad9
  49. Remove JSON Spirit wrapper, remove JSON Spirit leftovers
    - implement find_value() function for UniValue
- replace all Array/Value/Object types with UniValues, remove JSON Spirit to UniValue wrapper
- remove JSON Spirit sources
    9a8897f4ac
  50. fix rpc batching univalue issue 8f7e4abbe6
  51. fix missing univalue types during constructing c7fbbc7e1d
  52. fix univalue json parse tests 519eedeba7
  53. jonasschnelli force-pushed on Jun 4, 2015
  54. Simplify RPCclient, adapt json_parse_error test
    # Conflicts:
#	src/test/rpc_tests.cpp
    043df2b568
  55. util: Add ParseInt64 and ParseDouble functions
    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.
    7e98a3c642
  56. univalue: add strict type checking c02309204b
  57. jonasschnelli force-pushed on Jun 4, 2015
  58. univalue: add type check unit tests 44c7474446
  59. laanwj merged this on Jun 4, 2015
  60. laanwj closed this on Jun 4, 2015
  61. laanwj referenced this in commit 466f0ea0e6 on Jun 4, 2015
  62. sdaftuar commented at 7:01 PM on June 4, 2015: member

    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'})
  63. jonasschnelli commented at 7:45 PM on June 4, 2015: contributor

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

    Thanks @sdaftuar for reporting!

  64. jgarzik commented at 8:10 PM on June 4, 2015: contributor

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

  65. theuni commented at 12:08 AM on June 5, 2015: member

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

  66. jonasschnelli referenced this in commit 0fd01949ed on Jun 5, 2015
  67. jonasschnelli referenced this in commit d0656d0001 on Jun 5, 2015
  68. jonasschnelli referenced this in commit 40c7bd627c on Jun 5, 2015
  69. jonasschnelli referenced this in commit c946ebed5e on Jun 6, 2015
  70. gitmarek referenced this in commit 2b4ef1951a on Mar 3, 2016
  71. gitmarek referenced this in commit cb5227e06e on Mar 3, 2016
  72. zkbot referenced this in commit a2e69fac97 on Feb 5, 2017
  73. zkbot referenced this in commit 50807c5dc4 on Feb 7, 2017
  74. zkbot referenced this in commit 9a6ebaba36 on Feb 10, 2017
  75. zkbot referenced this in commit a7b8d93656 on Feb 10, 2017
  76. zkbot referenced this in commit e51bd1b556 on Feb 10, 2017
  77. AllanDoensen referenced this in commit 2b81a4acb2 on Apr 23, 2017
  78. reddink referenced this in commit 9bfca8c140 on May 27, 2020
  79. DrahtBot locked this on Aug 16, 2022
Contributors
jonasschnellijgarziklaanwjjtimonsdaftuartheuni
Labels
RPC/REST/ZMQ
Linked (view graph)
#4738 Convert entire source tree from json_spirit to UniValue.#6226 json: fail read_string if string contains trailing garbage#6234 fix rpcmining/getblocktemplate univalue transition logic error#6241 fix util_tests.cpp clang warnings
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-03 00:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me