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.
Using strings and integers will be more predictable than using float,
which is less repeatable.
57 | @@ -58,7 +58,7 @@ def small_txpuzzle_randfee(from_node, conflist, unconflist, amount, min_fee, fee 58 | # It's best to exponentially distribute our random fees 59 | # because the buckets are exponentially spaced. 60 | # Exponentially distributed from 1-128 * fee_increment 61 | - rand_fee = float(fee_increment) * (1.1892 ** random.randint(0, 28)) 62 | + rand_fee = fee_increment * Decimal(1.1892 ** random.randint(0, 28))
Not sure if rejecting a float is the right thing to do when the caller just wraps the float into a Decimal.
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.
I wonder if satoshi_round can be removed if the call sites denote everything in integral satoshi values.
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.
I wonder if
satoshi_roundcan 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.
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.
I wonder if
satoshi_roundcan 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!
Are you still working on this?