qa: Ensure consistent use of decimals instead of floats

hebasto commented at 8:50 pm on January 2, 2025: member

On NetBSD, with newer Python versions 3.12.8 and 3.13.1, many functional tests fail due to float numbers internal representation.

A typical error looks as follows:

 0$ python3.12 ./build/test/functional/feature_assumeutxo.py
 1...
 22025-01-02T20:43:01.865000Z TestFramework (INFO): Submit a spending transaction for a snapshot chainstate coin to the mempool
 32025-01-02T20:43:01.889000Z TestFramework (ERROR): JSONRPC error
 4Traceback (most recent call last):
 5  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/test_framework.py", line 135, in main
 6    self.run_test()
 7  File "/home/hebasto/dev/bitcoin/./build/test/functional/feature_assumeutxo.py", line 563, in run_test
 8    raw_tx = n1.createrawtransaction([prevout], {getnewdestination()[2]: 24.99})
 9             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
10  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/coverage.py", line 50, in __call__
11    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
12                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
13  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__
14    raise JSONRPCException(response['error'], status)
15test_framework.authproxy.JSONRPCException: Invalid amount (-3)
162025-01-02T20:43:01.954000Z TestFramework (INFO): Stopping nodes
17...

Running the same test with --tracerpc:

 0$ python3.12 ./build/test/functional/feature_assumeutxo.py --tracerpc
 1...
 2-2269-> createrawtransaction [[{"txid": "7935c5e149f7206122dfc8de0db6e5b2484923650b94ed56b8612609b896e021", "vout": 0, "scriptPubKey": "76a9142b4569203694fc997e13f2c0a1383b9e16c77a0d88ac"}], {"bcrt1pehl52uxx4ndz844j2xkasc3kqesyfmjhfukx42wdte2u5elzxhgqf9nnkt": 24.989999999999998}]
 3<-- [0.013652] {"jsonrpc":"2.0","error":{"code":-3,"message":"Invalid amount"},"id":2269}
 4
 52025-01-03T08:17:42.532000Z TestFramework (ERROR): JSONRPC error
 6Traceback (most recent call last):
 7  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/test_framework.py", line 135, in main
 8    self.run_test()
 9  File "/home/hebasto/dev/bitcoin/./build/test/functional/feature_assumeutxo.py", line 563, in run_test
10    raw_tx = n1.createrawtransaction([prevout], {getnewdestination()[2]: 24.99})
11             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
12  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/coverage.py", line 50, in __call__
13    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
14                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
15  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__
16    raise JSONRPCException(response['error'], status)
17test_framework.authproxy.JSONRPCException: Invalid amount (-3)
182025-01-03T08:17:42.588000Z TestFramework (INFO): Stopping nodes
19...

This PR fixes this issue by consistent use of Decimal numbers.

qa: Fix rounding for decimal 4cf904d7f0

hebasto added the label Tests on Jan 2, 2025

DrahtBot commented at 8:50 pm on January 2, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31595.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/807 (refactor: interfaces, make ‘createTransaction’ less error-prone by furszy)
#31250 (wallet: Disable creating and loading legacy wallets by achow101)
#30972 (Wallet: “listreceivedby*” fix by BrandonOdiwuor)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#25269 (wallet: re-activate “AmountWithFeeExceedsBalance” error by furszy)
#21283 (Implement BIP 370 PSBTv2 by achow101)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#10102 (Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

qa: Ensure consistent use of decimals instead of floats

This change fixes tests for Python 3.12.8 and 3.13.1 on NetBSD.

4fc10565f8

hebasto force-pushed on Jan 2, 2025

DrahtBot added the label CI failed on Jan 2, 2025

DrahtBot commented at 8:55 pm on January 2, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/35086805437

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

maflcko commented at 9:39 pm on January 2, 2025: member

many functional tests fail due to float numbers internal representation.

A typical error looks as follows:

It would be good to explain this a bit better and provide a --tracerpc log, or similar.

DrahtBot removed the label CI failed on Jan 2, 2025

hebasto commented at 8:20 am on January 3, 2025: member

many functional tests fail due to float numbers internal representation. A typical error looks as follows:

It would be good to explain this a bit better and provide a --tracerpc log, or similar.

Sure! I’ve updated the PR description with the --tracerpc log.

in test/functional/feature_assumeutxo.py:564 in 4fc10565f8

560@@ -560,7 +561,7 @@ def check_tx_counts(final: bool) -> None:
561         prev_tx = n0.getblock(spend_coin_blockhash, 3)['tx'][0]
562         prevout = {"txid": prev_tx['txid'], "vout": 0, "scriptPubKey": prev_tx['vout'][0]['scriptPubKey']['hex']}
563         privkey = n0.get_deterministic_priv_key().key
564-        raw_tx = n1.createrawtransaction([prevout], {getnewdestination()[2]: 24.99})
565+        raw_tx = n1.createrawtransaction([prevout], {getnewdestination()[2]: Decimal("24.99")})

l0rinc commented at 12:03 pm on January 3, 2025:

Is it important this is a Decimal?

0        raw_tx = n1.createrawtransaction([prevout], {getnewdestination()[2]: "24.99"})

Seems to pass as well - floating point values weren’t defined to be this precise anyway

hebasto commented at 7:06 pm on January 3, 2025:

Is it important this is a Decimal?

I confirm that your patch works.

… floating point values weren’t defined to be this precise anyway

See @mzumsande’s comment.

in test/functional/rpc_psbt.py:85 in 4fc10565f8

82+        wallet.sendtoaddress(address=address, amount=Decimal("1.0"))
83         self.generate(node, nblocks=1, sync_fun=lambda: self.sync_all(self.nodes[:2]))
84 
85         utxos = wallet.listunspent(addresses=[address])
86-        psbt = wallet.createpsbt([{"txid": utxos[0]["txid"], "vout": utxos[0]["vout"]}], [{wallet.getnewaddress(): 0.9999}])
87+        psbt = wallet.createpsbt([{"txid": utxos[0]["txid"], "vout": utxos[0]["vout"]}], [{wallet.getnewaddress(): Decimal("0.9999")}])

l0rinc commented at 12:06 pm on January 3, 2025:

we have many other instances where we’ve kept the floating point format - can we set up a linter if that’s important instead of fixing only a subset?

hebasto commented at 3:06 pm on January 3, 2025:

I agree, but I’m not familiar with the Python linter capabilities.

maflcko commented at 3:30 pm on January 3, 2025:

It should be trivial to write a python function that accepts json (value/dict/array) and fails if any value is float. The function could be run in authproxy

in test/functional/rpc_psbt.py:81 in 4fc10565f8

77@@ -78,17 +78,17 @@ def test_psbt_incomplete_after_invalid_modification(self):
78         node = self.nodes[2]
79         wallet = node.get_wallet_rpc(self.default_wallet_name)
80         address = wallet.getnewaddress()
81-        wallet.sendtoaddress(address=address, amount=1.0)
82+        wallet.sendtoaddress(address=address, amount=Decimal("1.0"))

l0rinc commented at 12:09 pm on January 3, 2025:

is this really better than before?

0        wallet.sendtoaddress(address=address, amount=1)

or

0        wallet.sendtoaddress(address=address, amount="1")

Both seem to work

l0rinc commented at 12:12 pm on January 3, 2025: contributor

I’m not sure about this, aren’t we just masking a bigger problem (i.e. treating floats as if they were real numbers). Also, can we update any one of the CI tasks to use the latest python to make sure we don’t have to fix these manually all the time?

hebasto commented at 3:04 pm on January 3, 2025: member

Also, can we update any one of the CI tasks to use the latest python to make sure we don’t have to fix these manually all the time?

That’s the plan for the nightly builds. The working branch is 250102-decimal-demo.

fanquake commented at 3:54 pm on January 3, 2025: member

It’d be good if you could explain what changed in Python 3.12+, and why this is only relevant on NetBSD. (a nightly build isn’t strictly needed given many devs already use the latest version of Python locally).

mzumsande commented at 6:08 pm on January 3, 2025: contributor

See https://github.com/python/cpython/issues/128005 (long discussion, skip to this post to save time) for more background on this. This is NetBSD-specific, and apparently caused by a pkgsrc patch which inadvertently caused sys.float_repr_style to be “legacy”. So the question is if this qualifies as a bug in NetBSD or not and whether we need to accomodate it by moving to Decimals.

fanquake commented at 6:48 pm on January 3, 2025: member

@mzumsande thanks for providing the actual context. Given the above, this doesn’t seem like a great change.

hebasto commented at 3:01 pm on January 4, 2025: member

See python/cpython#128005 (long discussion, skip to this post to save time) for more background on this. This is NetBSD-specific, and apparently caused by a pkgsrc patch which inadvertently caused sys.float_repr_style to be “legacy”. So the question is if this qualifies as a bug in NetBSD or not and whether we need to accomodate it by moving to Decimals.

The NetBSD’s bug in the Python build system has been fixed:

hebasto closed this on Jan 4, 2025

l0rinc commented at 3:04 pm on January 4, 2025: contributor

I could see this going in a different way: not storing RPC parameters as python floats, and changing all such usages to strings instead (as shown in #31595 (review)).

hebasto commented at 3:12 pm on January 4, 2025: member

I could see this going in a different way: not storing RPC parameters as python floats…

I agree with that plus #31595 (review).

mzumsande commented at 7:12 pm on January 4, 2025: contributor

Why? While we are currently relying on the short python repr() algorithm for floats, this has been stable and reliable since it’s been introduced in python 3.1 as far as I know - so I don’t really see the benefit of changing dozens of functional tests / adding a new linter - unless there have been other bugs related to this in the past?

laanwj commented at 9:28 pm on January 5, 2025: member

Concept ACK. All in all we should be using decimals or strings for amounts and not floats. Using floats for monetary amounts is dangerous, although arguably it’s not critical in tests it’s good to give the right example.

laanwj commented at 9:28 am on January 6, 2025: member

See also #31420 which tried to achieve a similar thing by adding conversion functions with typed parameters.

hebasto commented at 11:32 am on January 6, 2025: member

Why? While we are currently relying on the short python repr() algorithm for floats, this has been stable and reliable since it’s been introduced in python 3.1 as far as I know - so I don’t really see the benefit of changing dozens of functional tests / adding a new linter - unless there have been other bugs related to this in the past?

Consider two lines from test/functional/rpc_psbt.py:https://github.com/bitcoin/bitcoin/blob/a0f0c48ae20ef19c70118419ac7ec6bfd611abc8/test/functional/rpc_psbt.py#L683 and https://github.com/bitcoin/bitcoin/blob/a0f0c48ae20ef19c70118419ac7ec6bfd611abc8/test/functional/rpc_psbt.py#L706

For a code reader without deep knowledge of our test framework, it is quite confusing why two different approaches are used in identical cases.

laanwj commented at 9:58 am on January 7, 2025: member

Why? While we are currently relying on the short python repr() algorithm for floats, this has been stable and reliable since it’s been introduced in python 3.1 as far as I know

Yes, the thing is, repr(float) depends on implementation details and settings of the FPU hardware, isn’t guaranteed to be stable across operating systems, C libraries, and CPU architectures. As we see here.

qa: Ensure consistent use of decimals instead of floats #31595

Code Coverage & Benchmarks

Reviews

Conflicts