test: Convert amounts from float to decimal #20039

ghost commented at 6:06 PM on September 29, 2020: none

decimal is preferred in accounting applications

https://docs.python.org/3.8/library/decimal.html

Decimal type saves an exact value so better than using float.

~~3 variables declared with type as 'Decimal' in test/functional/mempool_accept.py: fee, fee_expected, output_amount~~

~~Not required to convert to string anymore for using the above variables as decimal~~

fee, fee_expected, output_amount ~~+ 8 decimal places~~
Using value of coin['amount'] as decimal and removed 'int'
Removed unnecessary parentheses
Remove str() and use quotes

Fixes https://github.com/bitcoin/bitcoin/issues/20011

in test/functional/mempool_accept.py:86 in 51b8b2e139 outdated

  82 | @@ -83,7 +83,7 @@ def run_test(self):
  83 |          )
  84 |  
  85 |          self.log.info('A transaction not in the mempool')
  86 | -        fee = 0.00000700
  87 | +        fee: Decimal = 0.00000700

luke-jr commented at 6:23 PM on September 29, 2020:

Where is this syntax defined? It seems very ugly/weird for Python, and in my Python 3.7.8 console, it also doesn't result in a Decimal type...

sipa commented at 6:30 PM on September 29, 2020:

It's a type annotation. It has no operational semantics (so execution will be identical to not having them at all). All it does is fail type checking if that's enabled.

I assume OP is incorrectly assuming that it actually determines the runtime type.

unknown commented at 6:40 PM on September 29, 2020:

I found it here: https://docs.python.org/3/library/typing.html

laanwj commented at 11:26 AM on September 30, 2020:

I don't think this is correct. Python never interprets inline numbers as Decimal, and the type annotation doesn't change that. I think what you want is:

fee: Decimal = Decimal('0.00000700')

unknown commented at 4:43 PM on September 30, 2020:

As sipa mentioned, I assumed python has introduced something like few other languages which I missed. I should have read the details and tested few things. @laanwj your suggestion is correct however I am not sure if we should use the type annotations if they don't change anything. Planning to make a new commit with changes like 'test4' mentioned in the below pic after trying few things on my system and doing more research:

in test/functional/mempool_accept.py:95 in 51b8b2e139 outdated

  91 | @@ -92,22 +92,22 @@ def run_test(self):
  92 |          tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_0)))
  93 |          txid_0 = tx.rehash()
  94 |          self.check_mempool_result(
  95 | -            result_expected=[{'txid': txid_0, 'allowed': True, 'vsize': tx.get_vsize(), 'fees': {'base': Decimal(str(fee))}}],
  96 | +            result_expected=[{'txid': txid_0, 'allowed': True, 'vsize': tx.get_vsize(), 'fees': {'base': (fee)}}],

luke-jr commented at 6:23 PM on September 29, 2020:

Prefer to drop the unnecessary parenthesis here.

DrahtBot added the label Tests on Sep 29, 2020

ghost commented at 12:26 PM on October 1, 2020: none

Made the changes in https://github.com/bitcoin/bitcoin/pull/20039/commits/ba15b848d9d9bb05f5e1b655ab417ac9654df32e

Was even thinking about adding getcontext().prec = 8 so that all operations in this file on decimals eventually result in 8 decimal places but not sure if it's the best approach to be used because it counts from most significant non-zero digit

unknown marked this as ready for review on Oct 3, 2020

in test/functional/mempool_accept.py:193 in 3f56b32c89 outdated

 189 | @@ -190,7 +190,7 @@ def run_test(self):
 190 |          tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_reference)))
 191 |          # Reference tx should be valid on itself
 192 |          self.check_mempool_result(
 193 | -            result_expected=[{'txid': tx.rehash(), 'allowed': True, 'vsize': tx.get_vsize(), 'fees': { 'base': Decimal(str(0.1 - 0.05))}}],
 194 | +            result_expected=[{'txid': tx.rehash(), 'allowed': True, 'vsize': tx.get_vsize(), 'fees': { 'base': Decimal(str(0.10000000 - 0.05000000))}}],

guggero commented at 10:28 AM on October 4, 2020:

These are the values from L170 and L188. I think we should extract those to a proper variable that is also of type Decimal. Or at the very least this line should read ... { 'base': Decimal('0.10000000') - Decimal('0.05000000')} IMO.

unknown commented at 2:32 PM on October 5, 2020:

Made the changes to remove str() in 7be315edd7bb5dbc4c27052b3f38d1498b51ce8b

guggero commented at 10:29 AM on October 4, 2020: contributor

Concept ACK 3f56b32c.

decryp2kanon commented at 4:53 PM on October 4, 2020: contributor

Concept ACK

kristapsk commented at 11:25 PM on October 7, 2020: contributor

Concept ACK, but should squash the commits, I think.

in test/functional/mempool_accept.py:89 in c7c3831ce5 outdated

  82 | @@ -83,31 +83,31 @@ def run_test(self):
  83 |          )
  84 |  
  85 |          self.log.info('A transaction not in the mempool')
  86 | -        fee = 0.00000700
  87 | +        fee = Decimal('0.00000700')
  88 |          raw_tx_0 = node.signrawtransactionwithwallet(node.createrawtransaction(
  89 |              inputs=[{"txid": txid_in_block, "vout": 0, "sequence": BIP125_SEQUENCE_NUMBER}],  # RBF is used later
  90 | -            outputs=[{node.getnewaddress(): 0.3 - fee}],
  91 | +            outputs=[{node.getnewaddress(): Decimal('0.3000000') - fee}],

guggero commented at 12:35 PM on October 21, 2020:

This is missing a zero, only 7 decimal places are shown.

unknown commented at 12:44 PM on October 21, 2020:

Fixed. Now has 8 decimal places.

guggero commented at 12:54 PM on October 21, 2020: contributor

ACK 3afdc9bd - assuming commits are being squashed on merge.

in test/functional/mempool_accept.py:89 in 3afdc9bd5e outdated

  82 | @@ -83,31 +83,31 @@ def run_test(self):
  83 |          )
  84 |  
  85 |          self.log.info('A transaction not in the mempool')
  86 | -        fee = 0.00000700
  87 | +        fee = Decimal('0.00000700')
  88 |          raw_tx_0 = node.signrawtransactionwithwallet(node.createrawtransaction(
  89 |              inputs=[{"txid": txid_in_block, "vout": 0, "sequence": BIP125_SEQUENCE_NUMBER}],  # RBF is used later
  90 | -            outputs=[{node.getnewaddress(): 0.3 - fee}],
  91 | +            outputs=[{node.getnewaddress(): Decimal('0.30000000') - fee}],

MarcoFalke commented at 12:58 PM on October 21, 2020:

Trailing zeros are ignored, so you can leave them out (same for the changes below)

unknown commented at 1:09 PM on October 21, 2020:

@MarcoFalke Should I remove trailing zeros for all or keep the things as they are right now after last commit in this PR?

MarcoFalke commented at 2:30 PM on October 21, 2020:

I'd prefer to remove them, unless there is a reason to add them

MarcoFalke commented at 12:59 PM on October 21, 2020: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

Convert amounts from float to decimal

+ fee, fee_expected, output_amount
+ Using value of coin['amount'] as decimal and removed 'int'
+ Removed unnecessary parentheses
+ Remove str() and use quotes

5aadd4be18

guggero commented at 8:02 AM on October 22, 2020: contributor

ACK 5aadd4be1883386a04bef6a04e9a1142601ef7a7

MarcoFalke merged this on Oct 22, 2020

MarcoFalke closed this on Oct 22, 2020

sidhujag referenced this in commit 1288257bea on Oct 22, 2020

deadalnix referenced this in commit 9c9c2e26f7 on Dec 6, 2021

DrahtBot locked this on Feb 15, 2022