Skip to content

Conversation

@alecalve
Copy link
Contributor

In python2.*, rounding a Decimal returns a float, however, in python 3, rounding a Decimal returns a Decimal. This leads to infinite loops in EncodeDecimal when decoding a response containing a float in python3 (tested with python 3.4.0).

File "/home/antoine/…/virtualenv/lib/python3.4/site-packages/bitcoinrpc/authproxy.py", line 64, in EncodeDecimal
return round(o, 8)
RuntimeError: maximum recursion depth exceeded while calling a Python object

@robertsdotpm
Copy link

I can confirm the same error still exists in the master branch. I just opened an identical pull before I saw this. Lesson learned: check open pulls first.

@luke-jr
Copy link
Contributor

luke-jr commented Apr 3, 2015

Duplicate of #27, neither of them actually fixing the problem.

@arnuschky
Copy link
Contributor

Patch works for me, python 3.4.0. Can you be more specific in which way this patch doesn't fix the problem, @luke-jr?

@luke-jr
Copy link
Contributor

luke-jr commented Apr 20, 2015

See my comment on #27 #27 (comment)

@arnuschky
Copy link
Contributor

Reposting @luke-jr's comment (misposted?):

Hmm, this doesn't seem to actually fix the issue, just avoids the infinite loop. In particular, by casting to a float you're not ensuring the Decimal value gets encoded correctly (with 8 decimal places). I think fixing this properly will require subclassing JSONEncoder - no need to round the value, though, just get it included precisely.

I had a look at it and subclassing JSONEncoder seems to be rather complex. A new Decimal serializer would need to handle all special cases as well (NaN etc). As the float serializer is called in several places, we would need to rewrite large parts of the encoder. Rounding and casting to float seems to be much more feasible and does the job just as well.

Another option would be to switch to simplejson, which handles decimals explicitly. Additionally, it would resolve #27, I think.

@luke-jr
Copy link
Contributor

luke-jr commented Apr 20, 2015

http://stackoverflow.com/a/1960649/2927053 doesn't look that complicated. The whole point of using Decimal is to avoid the float rounding - which casting to float doesn't do.

@arnuschky
Copy link
Contributor

That patch is ~6 years old and doesn't apply to today's structure. I only have a cursory look at it and thought that it won't be that simple to do it similarly. Glad to be proven wrong, though. :)

Regarding rounding issues: I still don't understand what you mean with included precisely. If I understand correctly, there are two issues: 1) rounding is required rather than truncating to prevent issues as described in https://en.bitcoin.it/wiki/Proper_Money_Handling_%28JSON-RPC%29, and 2) float should not be used as the base type of operations as rounding errors accumulate.

The usage as described in this PR appears to be fine to me.

@ghost
Copy link

ghost commented Oct 14, 2015

Without this the library does not work on Python 3. I understand the ideology of @luke-jr, however this PR simply allows the library to continue with the same behavior as it always has. It simply takes into account an incompatible change made in Python. I vote for merging this and if we wanted to rework the entire thing it could be a separate issue.

I have been testing @alecalve's branch on Python 3.4.2 and have had no problems.

jgarzik pushed a commit that referenced this pull request Nov 19, 2015
Explicit casting of Decimal to float for python3.3+
@jgarzik jgarzik merged commit 9191d77 into jgarzik:master Nov 19, 2015
@jgarzik
Copy link
Owner

jgarzik commented Nov 19, 2015

This is the best fix for right now, until a better fix is written

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants