test: fix ceildiv division by using integers #24239
Conversation
| @@ -226,7 +226,7 @@ def ceildiv(a, b): | |||
| def get_fee(tx_size, feerate_btc_kvb): | |||
| """Calculate the fee in BTC given a feerate is BTC/kvB. Reflects CFeeRate::GetFee""" | |||
| feerate_sat_kvb = int(feerate_btc_kvb * Decimal(1e8)) # Fee in sat/kvb as an int to avoid float precision errors | |||
| target_fee_sat = ceildiv(feerate_sat_kvb * tx_size, 1000) # Round calculated fee up to nearest sat | |||
| target_fee_sat = ceildiv(feerate_sat_kvb * int(tx_size), 1000) # Round calculated fee up to nearest sat | |||
There was a problem hiding this comment.
In commit "test: Call ceildiv helper with integer" (11d4385)
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."
There was a problem hiding this comment.
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.
| @@ -226,7 +226,7 @@ def ceildiv(a, b): | |||
| def get_fee(tx_size, feerate_btc_kvb): | |||
| """Calculate the fee in BTC given a feerate is BTC/kvB. Reflects CFeeRate::GetFee""" | |||
| feerate_sat_kvb = int(feerate_btc_kvb * Decimal(1e8)) # Fee in sat/kvb as an int to avoid float precision errors | |||
| target_fee_sat = ceildiv(feerate_sat_kvb * tx_size, 1000) # Round calculated fee up to nearest sat | |||
| target_fee_sat = ceildiv(feerate_sat_kvb * int(tx_size), 1000) # Round calculated fee up to nearest sat | |||
| return satoshi_round(target_fee_sat / Decimal(1e8)) # Truncate BTC result to nearest sat | |||
There was a problem hiding this comment.
In commit "test: Call ceildiv helper with integer" (11d4385)
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 (comment))
There was a problem hiding this comment.
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.
11d4385 to
26fa206
Compare
It returns an incorrect result when called with a Decimal, for which the "//" operator works differently. Also drop unnecessary call to satoshi_round.
26fa206 to
d1fab9d
Compare
|
Addressed the comments of @ryanofsky - thanks! |
|
ACK d1fab9d |
It returns an incorrect result when called with a Decimal, for which the "//" operator works differently. Also drop unnecessary call to satoshi_round. Github-Pull: bitcoin#24239 Rebased-From: d1fab9d
|
Added to #23276 for backporting. |
It returns an incorrect result when called with a Decimal, for which the "//" operator works differently. Also drop unnecessary call to satoshi_round. Github-Pull: bitcoin#24239 Rebased-From: d1fab9d
269553f test: Call ceildiv helper with integer (Martin Zumsande) 2f60fc6 ci: Replace soon EOL hirsute with jammy (MarcoFalke) 801b0f0 build: patch qt to explicitly define previously implicit header include (Kittywhiskers Van Gogh) c768bfa tests: Calculate fees more similarly to CFeeRate::GetFee (Andrew Chow) f66bc42 tests: Test for assertion when feerate is rounded down (Andrew Chow) bd7e08e fees: Always round up fee calculated from a feerate (Andrew Chow) 227ae65 wallet: fix segfault by avoiding invalid default-ctored `external_spk_managers` entry (Sebastian Falbesoner) 282863a refactor: include a missing <limits> header in fs.cpp (Joan Karadimov) 7febe4f consensus: don't call GetBlockPos in ReadBlockFromDisk without lock (Jon Atack) c671c6f the result of CWallet::IsHDEnabled() was initialized with true. (Saibato) a5a1538 build, qt: Fix typo in QtInputSupport check (Hennadii Stepanov) c95b188 system: skip trying to set the locale on NetBSD (fanquake) c1cdedd guix: Fix powerpc64(le) dynamic linker name (Carl Dong) 92d44ff doc: Add 23061 release notes (MarcoFalke) db76db7 Fix (inverse) meaning of -persistmempool (MarcoFalke) 85c78e0 build: Restrict check for CRC32C intrinsic to aarch64 (W. J. van der Laan) Pull request description: Collecting backports for the 22.1 release. Currently: * #23045 * #23061 * #23148 * #22390 * #22820 * #22781 * #22895 * #23335 * #23333 * #22949 * #23580 * #23504 * #24239 ACKs for top commit: achow101: ACK 269553f Tree-SHA512: b3a57ea241be7a83488eeb032276f4cf82a0987aada906a82f94a20c4acf9f2397708249dcecbe1c7575e70d09c60b835233d4718af4013c7bc58896c618274c
On master,
assert_fee_amount(Decimal("0.00000993"), 217, Decimal("0.00004531"))passesassert_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.pycalls this function with Decimals, and I think this is the reason for the failure reported in the OP of #24151 (wallet_send.py --legacy-walletline 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.