test: Replace satoshi_round with int() or Decimal() #23210

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2110-testRound changing 3 files +8 −12

MarcoFalke commented at 1:18 PM on October 6, 2021: member

satoshi_round will round down. To make the code easier to parse use Decimal() where possible, which does not round. Or use int(), which explicitly rounds down.

test: Replace satoshi_round with int() or Decimal()

satoshi_round will round down. To make the code easier to parse use
Decimal() where possible, which does not round. Or use int(), which
explicitly rounds down.

fa2ac5881e

DrahtBot added the label Tests on Oct 6, 2021
katesalazar commented at 4:14 PM on October 6, 2021: contributor

If the problem is satoshi_round defaulting to rounding down, why not request it to round up using the optional argument? It is there for that.

Decimal is very nice, no surprise satoshi_round is just a Decimal syntactic sugar, but satoshi_round has the advantage of labeling the number with what it is, sats.
katesalazar commented at 4:18 PM on October 6, 2021: contributor

Um sorry I thought satoshi_round had an optional argument and it doesn't (but could be added)
katesalazar commented at 4:36 PM on October 6, 2021: contributor

Decimal() where possible, which does not round

You know this, but it is "Decimal(str) where possible, which does not round, unlike Decimal(float)"

When Decimal(1) is fed a float, machine-specific witchcraft is happening before the decimal module receives the data.
MarcoFalke commented at 5:25 PM on October 6, 2021: member

Um sorry I thought satoshi_round had an optional argument and it doesn't (but could be added)

I don't think the argument should be optional, but required, so that the caller explicitly specifies whether to round down or up. Though, it seems unrelated to the changes here. I am happy to review a follow-up pr.
katesalazar commented at 5:32 PM on October 6, 2021: contributor

Um sorry I thought satoshi_round had an optional argument and it doesn't (but could be added)

I don't think the argument should be optional, but required, so that the caller explicitly specifies whether to round down or up. Though, it seems unrelated to the changes here. I am happy to review a follow-up pr.

If I were to do it right now, I would actually raise TypeError immediately from satoshi_round if data entry is type float, for security reasons. Having it accepting only int or str, no environment-particular quirking. Floating point arithmetic for money is heretic, moreso at tests code.
MarcoFalke commented at 5:34 PM on October 6, 2021: member

I agree, but I think it is unrelated to the changes here.
lsilva01 approved
lsilva01 commented at 7:52 PM on October 6, 2021: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/23210/commits/fa2ac5881edf8d0d3f15c43f089f1831348dfae2 on Ubuntu 20.04.
MarcoFalke merged this on Oct 7, 2021
MarcoFalke closed this on Oct 7, 2021
MarcoFalke deleted the branch on Oct 7, 2021
meshcollider commented at 7:19 AM on October 7, 2021: contributor

I don't think the argument should be optional, but required, so that the caller explicitly specifies whether to round down or up. Though, it seems unrelated to the changes here. I am happy to review a follow-up pr.

I think satoshi_round should only round down, that is its defining behaviour. If you have 1.5*10^-8 BTC, you have 1 satoshi, you never have 2. Perhaps it should just be renamed to satoshi_truncate though.
MarcoFalke commented at 8:05 AM on October 7, 2021: member

I'd say it depends on the value you pass. If it is a fee, you want to round up (See #22949). If it is a value, you want to round down.
sidhujag referenced this in commit eda2ba82c7 on Oct 7, 2021
DrahtBot locked this on Oct 30, 2022

Contributors

Labels