Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: reject float at satoshi_round #23225

Closed
wants to merge 4 commits into from

Conversation

katesalazar
Copy link
Contributor

Using strings and integers will be more predictable than using float,
which is less repeatable.

satoshi_round was used with almost no floats already, I suppose
that because it rounds in a single direction and when floating
point rounding occurs it can go either up or downwards so it
is a risky thing to do. Forbid it explicitly.

katesalazar and others added 4 commits October 7, 2021 20:13
Using strings and integers will be more predictable than using float,
which is less repeatable.
@DrahtBot DrahtBot added the Tests label Oct 7, 2021
@fanquake fanquake changed the title Reject float at satoshi_round test: reject float at satoshi_round Oct 8, 2021
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if satoshi_round can be removed if the call sites denote everything in integral satoshi values.

@@ -58,7 +58,7 @@ def small_txpuzzle_randfee(from_node, conflist, unconflist, amount, min_fee, fee
# It's best to exponentially distribute our random fees
# because the buckets are exponentially spaced.
# Exponentially distributed from 1-128 * fee_increment
rand_fee = float(fee_increment) * (1.1892 ** random.randint(0, 28))
rand_fee = fee_increment * Decimal(1.1892 ** random.randint(0, 28))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if rejecting a float is the right thing to do when the caller just wraps the float into a Decimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • This is an exception: random fee generation. Rounding is not an
    important issue.

  • The corresponding rule: argument will be a well determined monetary
    quantity, for repeatability purposes. Any other case, that is correctly
    already not using float.

@laanwj
Copy link
Member

laanwj commented Oct 8, 2021

Conceptually I agree with this. It would be nice to go to Decimal/Integer only. Floating point numbers shouldn't be used for monetary amounts, not even in tests.

@katesalazar
Copy link
Contributor Author

I wonder if satoshi_round can be removed if the call sites denote everything in integral satoshi values.

If Bitcoin Core won't hard fork increased currency divisibility,
removing it is cleaner.

If Bitcoin Core or other group is going to hard fork increased
currency divisibility, you might prefer to keep this and maybe
use it more than it is used currently.

@meshcollider
Copy link
Contributor

If Bitcoin Core won't hard fork increased currency divisibility,
removing it is cleaner.

It is very safe to modify the test code assuming Bitcoin Core will not hard fork.

@katesalazar
Copy link
Contributor Author

I wonder if satoshi_round can be removed if the call sites denote everything in integral satoshi values.

If Bitcoin Core won't hard fork increased currency divisibility,
removing it is cleaner.

It is very safe to modify the test code assuming Bitcoin Core will not hard fork.

If I had to do this, I'd take all the necessary time to make sure
every rational is a decimal.Decimal instead of a float,
then remove satoshi_round, and then close this PR unmerged.

If possible, I would prefer configuring a "safe mode" at the Python interpreter
to completely forbid floating point at code overseeing and asserting testing,
but I don't know if Python has such feature.

Cheers!

@maflcko
Copy link
Member

maflcko commented Feb 22, 2022

Are you still working on this?

@fanquake fanquake closed this Apr 26, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Apr 26, 2023
@DrahtBot DrahtBot changed the title test: reject float at satoshi_round test: reject float at satoshi_round Mar 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants