nochowderforyou / clams

Clam Project
MIT License
62 stars 58 forks source link

univalue changes stop values being able to begin with a zero then another digit #259

Closed dooglus closed 8 years ago

dooglus commented 8 years ago
$ clamd getblockhash 12345
469fe31985f452d8705ac234457d51de92964ca2b3a983c7e02422b1fe006ec4

$ clamd getblockhash 012345
error: Error parsing JSON:012345

I used to be able to sloppily not care about leading zeroes, but now univalue forbids it.

From the code in src/univalue/lib/univalue_read.cpp:

switch (*raw) {

case 0:
    return JTOK_NONE;
...
case '-':
case '0':
case '1':

ie. there are two case '0' in the same switch. That's likely not right!

tryphe commented 8 years ago

Don't forget the value of '0' is 48, rather than zero. :smiley:

dooglus commented 8 years ago

Right, and possibly more relevantly, the value of 010 is eight, rather than ten. Integers starting with a zero are traditionally interpreted as octal, which may be why we're rejecting them, to avoid the ambiguity.

But they used to be accepted, and so the behavior has unexpectedly changed, breaking existing scripts.

Oh, sorry. I completely misunderstood you. Of course, 0 and '0' are two different cases. Not sure how I missed that, even after you pointed it out! I need some sleep.

dooglus commented 8 years ago

So here's why we can't parse "01":

    if ((*firstDigit == '0') && isdigit(firstDigit[1]))
        return JTOK_ERR;

If it starts with a zero, and is followed by any other digit, it's an error. Duh.

So do we want that behavior or not?

dooglus commented 8 years ago

Commenting those two lines:

$ clamd getblockhash 12345
469fe31985f452d8705ac234457d51de92964ca2b3a983c7e02422b1fe006ec4
$ clamd getblockhash 012345
469fe31985f452d8705ac234457d51de92964ca2b3a983c7e02422b1fe006ec4
$ clamd getblockhash 00012345
469fe31985f452d8705ac234457d51de92964ca2b3a983c7e02422b1fe006ec4
l0rdicon commented 8 years ago

I think its alright to remove those lines. I can't think of any logical reason there needed, its just the convention bitcoin decided to follow.

If its better for your for the client to accept numbers padded with 0's I see no reason not to return to the previous behaviour of the client.

On Fri, Dec 11, 2015 at 1:35 AM, Chris Moore notifications@github.com wrote:

Commenting those two lines:

$ clamd getblockhash 12345 469fe31985f452d8705ac234457d51de92964ca2b3a983c7e02422b1fe006ec4 $ clamd getblockhash 012345 469fe31985f452d8705ac234457d51de92964ca2b3a983c7e02422b1fe006ec4 $ clamd getblockhash 00012345 469fe31985f452d8705ac234457d51de92964ca2b3a983c7e02422b1fe006ec4

— Reply to this email directly or view it on GitHub https://github.com/nochowderforyou/clams/issues/259#issuecomment-163845486 .

dooglus commented 8 years ago

I make a bootstrap file regularly, as well as "partial" bootstrap files containing batched of 10k blocks each.

The script that makes them uses a 3 digit number for the partials. The most recent, bootstrap-075.dat, contains blocks 750000 through 759999. I was taking the 075 file number and appending '0000' and '9999' as parameters to the dumpbootstrap RPC call. It was an easy fix to remove the leading zeroes, so it doesn't affect me (until the next time I ran a different but similarly broken script).

I do think the principal of breaking as little as possible is a good one to follow through.

l0rdicon commented 8 years ago

https://github.com/nochowderforyou/clams/pull/260 merging and closing this issue