test: fix ceildiv division by using integers #24239

1. 
   mzumsande commented at 5:10 pm on February 2, 2022: contributor

   On master,

   assert_fee_amount(Decimal("0.00000993"), 217, Decimal("0.00004531")) passes assert_fee_amount(Decimal("0.00000993"), Decimal("217"), Decimal("0.00004531")) fails.

   the reason is that the // operator in ceildiv(a,b) = -(-a//b) has a different behavior for Decimals, see doc.

   wallet_send.py calls this function with Decimals, and I think this is the reason for the failure reported in the OP of #24151 (wallet_send.py --legacy-wallet line 332, the numbers used in the example above are from there). However, the other failures reported there cannot be explained by this, so this is just a partial fix.

2. maflcko added this to the milestone 23.0 on Feb 2, 2022
3. maflcko requested review from ryanofsky on Feb 2, 2022
4. maflcko requested review from achow101 on Feb 2, 2022
5. maflcko added the label Needs backport (22.x) on Feb 2, 2022
6. maflcko added the label Tests on Feb 2, 2022
7. in test/functional/test_framework/util.py:229 in 11d4385819 outdated

   225@@ -226,7 +226,7 @@ def ceildiv(a, b):
   226 def get_fee(tx_size, feerate_btc_kvb):
   227     """Calculate the fee in BTC given a feerate is BTC/kvB. Reflects CFeeRate::GetFee"""
   228     feerate_sat_kvb = int(feerate_btc_kvb * Decimal(1e8)) # Fee in sat/kvb as an int to avoid float precision errors
   229-    target_fee_sat = ceildiv(feerate_sat_kvb * tx_size, 1000) # Round calculated fee up to nearest sat
   230+    target_fee_sat = ceildiv(feerate_sat_kvb * int(tx_size), 1000) # Round calculated fee up to nearest sat
   

   ---

   ---

   

   ryanofsky commented at 7:39 pm on February 4, 2022:

   In commit “test: Call ceildiv helper with integer” (11d4385819154e637eb34879bf3e4040f85ef194)

   Nice find!

   This seems like a working fix but fragile. It seems bad if callers are passing transaction sizes as decimals that will get silently rounded, instead of just passing integer values. I think it would be better to drop the rounding cast, and write assert isinstance(tx_size, int), then fix the bad callers which are passing fractional transaction sizes.

   I think it would also be good to add assert isinstance(a, int) and assert isinstance(b, int) to the ceildiv function with a comment like “# Implementation requires python integers, which have a // operator that does floor division. Other types like decimal.Decimal whose // operator truncates towards 0 will not work.”

   ---

   

   mzumsande commented at 2:39 pm on February 7, 2022:

   Thanks, that makes sense. I added the asserts as suggested and also your explanation. I changed the call sites to use the count_bytes helper for the tx size, so that the function is called with integers now.
8. in test/functional/test_framework/util.py:230 in 11d4385819 outdated

   225@@ -226,7 +226,7 @@ def ceildiv(a, b):
   226 def get_fee(tx_size, feerate_btc_kvb):
   227     """Calculate the fee in BTC given a feerate is BTC/kvB. Reflects CFeeRate::GetFee"""
   228     feerate_sat_kvb = int(feerate_btc_kvb * Decimal(1e8)) # Fee in sat/kvb as an int to avoid float precision errors
   229-    target_fee_sat = ceildiv(feerate_sat_kvb * tx_size, 1000) # Round calculated fee up to nearest sat
   230+    target_fee_sat = ceildiv(feerate_sat_kvb * int(tx_size), 1000) # Round calculated fee up to nearest sat
   231     return satoshi_round(target_fee_sat / Decimal(1e8)) # Truncate BTC result to nearest sat
   

   ---

   ---

   

   ryanofsky commented at 7:41 pm on February 4, 2022:

   In commit “test: Call ceildiv helper with integer” (11d4385819154e637eb34879bf3e4040f85ef194)

   Not directly related to this PR, but it seems like this satoshi_round call does nothing and should be dropped. (I suggested this previously #22949 (review))

   ---

   

   mzumsande commented at 2:54 pm on February 7, 2022:

   Yes, I agree it does nothing since we pass an argument that needs no rounding because target_fee_sat is calculated in sat. I removed the call to satoshi_round here.

   The only difference in behavior I could see is that if we’d call get_fee with absurdly high feerates such as get_fee(1000, Decimal("1.00000001")), we’d get an decimal.InvalidOperation assert from satoshi_round before because the specified Decimal precision of 8 would not be enough.

9. ryanofsky approved
10. 
    ryanofsky commented at 7:55 pm on February 4, 2022: contributor

    Code review ACK 11d4385819154e637eb34879bf3e4040f85ef194
11. mzumsande force-pushed on Feb 7, 2022
12. test: Call ceildiv helper with integer

    It returns an incorrect result when called with a Decimal,
    for which the "//" operator works differently.
    Also drop unnecessary call to satoshi_round.

    d1fab9d5d2
13. mzumsande force-pushed on Feb 7, 2022
14. 
    mzumsande commented at 2:55 pm on February 7, 2022: contributor

    Addressed the comments of @ryanofsky - thanks!
15. ryanofsky approved
16. 
    ryanofsky commented at 3:58 pm on February 7, 2022: contributor

    Code review ACK d1fab9d5d27a2db2546db0f610e0f6929ec4864e. Tracking down this problem was a good find, and code seems safer and easier to understand now
17. maflcko merged this on Feb 7, 2022
18. maflcko closed this on Feb 7, 2022

    ---

19. sidhujag referenced this in commit 9d51fbc6e9 on Feb 7, 2022
20. 
    achow101 commented at 4:46 pm on February 7, 2022: member

    ACK d1fab9d5d27a2db2546db0f610e0f6929ec4864e
21. Vic23M approved
22. Vic23M approved
23. fanquake referenced this in commit a90dfbc13c on Feb 14, 2022
24. fanquake removed the label Needs backport (22.x) on Feb 14, 2022
25. 
    fanquake commented at 1:43 pm on February 14, 2022: member

    Added to #23276 for backporting.
26. fanquake referenced this in commit 269553fe73 on Feb 15, 2022
27. fanquake referenced this in commit 9b5f674abb on Mar 1, 2022
28. bitcoin locked this on Feb 14, 2023
29. mzumsande deleted the branch on Oct 13, 2023

Contributors
mzumsande ryanofsky achow101 Vic23M fanquake

Review Requested
achow101

Labels
Tests

Milestone
23.0