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

hebasto · 2025-01-02T20:50:39Z

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:

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

Running the same test with --tracerpc:

$ python3.12 ./build/test/functional/feature_assumeutxo.py --tracerpc
...
-2269-> createrawtransaction [[{"txid": "7935c5e149f7206122dfc8de0db6e5b2484923650b94ed56b8612609b896e021", "vout": 0, "scriptPubKey": "76a9142b4569203694fc997e13f2c0a1383b9e16c77a0d88ac"}], {"bcrt1pehl52uxx4ndz844j2xkasc3kqesyfmjhfukx42wdte2u5elzxhgqf9nnkt": 24.989999999999998}]
<-- [0.013652] {"jsonrpc":"2.0","error":{"code":-3,"message":"Invalid amount"},"id":2269}

2025-01-03T08:17:42.532000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/test_framework.py", line 135, in main
    self.run_test()
  File "/home/hebasto/dev/bitcoin/./build/test/functional/feature_assumeutxo.py", line 563, in run_test
    raw_tx = n1.createrawtransaction([prevout], {getnewdestination()[2]: 24.99})
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/coverage.py", line 50, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__
    raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Invalid amount (-3)
2025-01-03T08:17:42.588000Z TestFramework (INFO): Stopping nodes
...

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

DrahtBot · 2025-01-02T20:50:42Z

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.

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

DrahtBot · 2025-01-02T20:55:36Z

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

Hints

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 · 2025-01-02T21:39:13Z

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.

hebasto · 2025-01-03T08:20:34Z

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.

l0rinc

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?

l0rinc · 2025-01-03T12:03:33Z

test/functional/feature_assumeutxo.py

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


Is it important this is a Decimal?

Suggested change

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

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

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.

l0rinc · 2025-01-03T12:06:22Z

test/functional/rpc_psbt.py

        self.generate(node, nblocks=1, sync_fun=lambda: self.sync_all(self.nodes[:2]))

        utxos = wallet.listunspent(addresses=[address])
-        psbt = wallet.createpsbt([{"txid": utxos[0]["txid"], "vout": utxos[0]["vout"]}], [{wallet.getnewaddress(): 0.9999}])
+        psbt = wallet.createpsbt([{"txid": utxos[0]["txid"], "vout": utxos[0]["vout"]}], [{wallet.getnewaddress(): Decimal("0.9999")}])


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?

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

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

l0rinc · 2025-01-03T12:09:22Z

test/functional/rpc_psbt.py

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


is this really better than before?

Suggested change

wallet.sendtoaddress(address=address, amount=Decimal("1.0"))

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

or

Suggested change

wallet.sendtoaddress(address=address, amount=Decimal("1.0"))

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

Both seem to work

hebasto · 2025-01-03T15:04:31Z

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 · 2025-01-03T15:54:12Z

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

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.

fanquake · 2025-01-03T18:48:32Z

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

hebasto · 2025-01-04T15:01:46Z

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:

l0rinc · 2025-01-04T15:04:54Z

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

hebasto · 2025-01-04T15:12:50Z

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

I agree with that plus #31595 (comment).

mzumsande · 2025-01-04T19:12:08Z

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 · 2025-01-05T21:28:18Z

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 · 2025-01-06T09:28:37Z

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

hebasto · 2025-01-06T11:32:03Z

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:

bitcoin/test/functional/rpc_psbt.py

Line 683 in a0f0c48

    
           psbt = self.nodes[1].createpsbt([utxo1, utxo2, utxo3], {self.nodes[0].getnewaddress():32.999})

and

bitcoin/test/functional/rpc_psbt.py

Line 706 in a0f0c48

    
           psbt1 = self.nodes[1].createpsbt([utxo1], {self.nodes[0].getnewaddress():Decimal('10.999')})

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 · 2025-01-07T09:58:25Z

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: Fix rounding for decimal

4cf904d

hebasto added the Tests label Jan 2, 2025

qa: Ensure consistent use of decimals instead of floats

4fc1056

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

hebasto force-pushed the 250102-decimal branch from eacc587 to 4fc1056 Compare January 2, 2025 20:55

DrahtBot added the CI failed label Jan 2, 2025

DrahtBot removed the CI failed label Jan 2, 2025

DrahtBot mentioned this pull request Jan 3, 2025

refactor: interfaces, make 'createTransaction' less error-prone bitcoin-core/gui#807

Draft

This was referenced Jan 3, 2025

wallet: Disable creating and loading legacy wallets #31250

Draft

Wallet: "listreceivedby*" fix #30972

Open

l0rinc reviewed Jan 3, 2025

View reviewed changes

DrahtBot mentioned this pull request Jan 3, 2025

Remove the legacy wallet and BDB dependency #28710

Draft

mzumsande reviewed Jan 3, 2025

View reviewed changes

hebasto closed this Jan 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hebasto commented Jan 2, 2025 •

edited

Loading

DrahtBot commented Jan 2, 2025 •

edited

Loading

DrahtBot commented Jan 2, 2025

maflcko commented Jan 2, 2025

hebasto commented Jan 3, 2025

l0rinc left a comment

l0rinc Jan 3, 2025

hebasto Jan 3, 2025

l0rinc Jan 3, 2025

hebasto Jan 3, 2025

maflcko Jan 3, 2025

l0rinc Jan 3, 2025

hebasto commented Jan 3, 2025

fanquake commented Jan 3, 2025

mzumsande left a comment •

edited

Loading

fanquake commented Jan 3, 2025

hebasto commented Jan 4, 2025

l0rinc commented Jan 4, 2025

hebasto commented Jan 4, 2025

mzumsande commented Jan 4, 2025 •

edited

Loading

laanwj commented Jan 5, 2025

laanwj commented Jan 6, 2025 •

edited

Loading

hebasto commented Jan 6, 2025

laanwj commented Jan 7, 2025

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

	wallet.sendtoaddress(address=address, amount=Decimal("1.0"))
	wallet.sendtoaddress(address=address, amount=1)

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

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

Conversation

hebasto commented Jan 2, 2025 • edited Loading

DrahtBot commented Jan 2, 2025 • edited Loading

Code Coverage & Benchmarks

Reviews

Conflicts

DrahtBot commented Jan 2, 2025

maflcko commented Jan 2, 2025

hebasto commented Jan 3, 2025

l0rinc left a comment

Choose a reason for hiding this comment

l0rinc Jan 3, 2025

Choose a reason for hiding this comment

hebasto Jan 3, 2025

Choose a reason for hiding this comment

l0rinc Jan 3, 2025

Choose a reason for hiding this comment

hebasto Jan 3, 2025

Choose a reason for hiding this comment

maflcko Jan 3, 2025

Choose a reason for hiding this comment

l0rinc Jan 3, 2025

Choose a reason for hiding this comment

hebasto commented Jan 3, 2025

fanquake commented Jan 3, 2025

mzumsande left a comment • edited Loading

Choose a reason for hiding this comment

fanquake commented Jan 3, 2025

hebasto commented Jan 4, 2025

l0rinc commented Jan 4, 2025

hebasto commented Jan 4, 2025

mzumsande commented Jan 4, 2025 • edited Loading

laanwj commented Jan 5, 2025

laanwj commented Jan 6, 2025 • edited Loading

hebasto commented Jan 6, 2025

laanwj commented Jan 7, 2025

hebasto commented Jan 2, 2025 •

edited

Loading

DrahtBot commented Jan 2, 2025 •

edited

Loading

mzumsande left a comment •

edited

Loading

mzumsande commented Jan 4, 2025 •

edited

Loading

laanwj commented Jan 6, 2025 •

edited

Loading