wallet: fix potential division by 0 in WalletLogPrintf #20378

pull jonasschnelli wants to merge 1 commits into bitcoin:master from jonasschnelli:2020/11/fix_devnull_wallet changing 1 files +2 −2
  1. jonasschnelli commented at 3:39 PM on November 12, 2020: contributor

    #20344 removed the divide-by-zero sanitizer suppression in wallet/wallet.cpp but kept a potential devision by zero in wallet.cpp's fee logging.

    Detected here https://bitcoinbuilds.org/index.php?job=ffb7d59f-379f-4f27-a273-a5595b8c5f07

  2. in src/wallet/wallet.cpp:3113 in 2fcc6212ed outdated 
    3109 | @@ -3110,10 +3110,10 @@ bool CWallet::CreateTransactionInternal(
3110 |      WalletLogPrintf("Fee Calculation: Fee:%d Bytes:%u Needed:%d Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n",
3111 |                nFeeRet, nBytes, nFeeNeeded, feeCalc.returnedTarget, feeCalc.desiredTarget, StringForFeeReason(feeCalc.reason), feeCalc.est.decay,
3112 |                feeCalc.est.pass.start, feeCalc.est.pass.end,
3113 | -              100 * feeCalc.est.pass.withinTarget / (feeCalc.est.pass.totalConfirmed + feeCalc.est.pass.inMempool + feeCalc.est.pass.leftMempool),
3114 | +              (feeCalc.est.pass.totalConfirmed + feeCalc.est.pass.inMempool + feeCalc.est.pass.leftMempool) == 0.0 ? 0.0 : 100 * feeCalc.est.pass.withinTarget / (feeCalc.est.pass.totalConfirmed + feeCalc.est.pass.inMempool + feeCalc.est.pass.leftMempool),
    hebasto commented at 3:43 PM on November 12, 2020:

    Could a local variable with feeCalc.est.pass.totalConfirmed + feeCalc.est.pass.inMempool + feeCalc.est.pass.leftMempool value improve readability?

    jonasschnelli commented at 3:46 PM on November 12, 2020:

    I did that initially but changed it to the inline-approach because it's just a log print... and I think it's still readable.

    practicalswift commented at 4:00 PM on November 12, 2020:

    Nit: Could re-phrase as (a + b + c) > 0.0 ? 100 * … / (a + b + c) : 0.0 to avoid comparing floating point with == :)

    jonasschnelli commented at 4:11 PM on November 12, 2020:

    Good point. Fixed.

  3. hebasto approved
  4. hebasto commented at 3:51 PM on November 12, 2020: member

    ACK 2fcc6212ed874c5afb42498ac3c6775b1962d9e9, I have reviewed the code and it looks OK, I agree it can be merged.

  5. practicalswift commented at 3:51 PM on November 12, 2020: contributor

    Concept ACK

    Thanks for fixing this. See #17208 for some historical context.

    Somewhat related: I think we have a float divide-by-zero in src/validation.cpp in the case of nBlocksTotal == 0. gmaxwell suggested initializing nBlocksTotal to 1 instead of 0 as a simple way to avoid the float divide-by-zero. I think that makes sense.

  6. in src/wallet/wallet.cpp:3116 in 2fcc6212ed outdated 
    3109 | @@ -3110,10 +3110,10 @@ bool CWallet::CreateTransactionInternal(
3110 |      WalletLogPrintf("Fee Calculation: Fee:%d Bytes:%u Needed:%d Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n",
3111 |                nFeeRet, nBytes, nFeeNeeded, feeCalc.returnedTarget, feeCalc.desiredTarget, StringForFeeReason(feeCalc.reason), feeCalc.est.decay,
3112 |                feeCalc.est.pass.start, feeCalc.est.pass.end,
3113 | -              100 * feeCalc.est.pass.withinTarget / (feeCalc.est.pass.totalConfirmed + feeCalc.est.pass.inMempool + feeCalc.est.pass.leftMempool),
3114 | +              (feeCalc.est.pass.totalConfirmed + feeCalc.est.pass.inMempool + feeCalc.est.pass.leftMempool) == 0.0 ? 0.0 : 100 * feeCalc.est.pass.withinTarget / (feeCalc.est.pass.totalConfirmed + feeCalc.est.pass.inMempool + feeCalc.est.pass.leftMempool),
3115 |                feeCalc.est.pass.withinTarget, feeCalc.est.pass.totalConfirmed, feeCalc.est.pass.inMempool, feeCalc.est.pass.leftMempool,
3116 |                feeCalc.est.fail.start, feeCalc.est.fail.end,
3117 | -              100 * feeCalc.est.fail.withinTarget / (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool),
3118 | +              (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) == 0.0 ? 0.0 : 100 * feeCalc.est.fail.withinTarget / (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool),
    practicalswift commented at 4:00 PM on November 12, 2020:

    Same nit applies here.

  7. jonasschnelli commented at 4:09 PM on November 12, 2020: contributor

    Confirmed that this fixes the broken master builds (https://bitcoinbuilds.org/index.php?job=3b3274e1-c511-4799-aad2-38595f9c81fe).

  8. fix potential devision by 0 440f8d3abe
  9. jonasschnelli force-pushed on Nov 12, 2020
  10. practicalswift commented at 4:13 PM on November 12, 2020: contributor

    ACK 440f8d3abe97b96f434dad5216d417a08fc10253

  11. hebasto approved
  12. hebasto commented at 4:13 PM on November 12, 2020: member

    re-ACK 440f8d3abe97b96f434dad5216d417a08fc10253

  13. practicalswift commented at 4:22 PM on November 12, 2020: contributor

    Somewhat related: I think we have a float divide-by-zero in src/validation.cpp in the case of nBlocksTotal == 0. gmaxwell suggested initializing nBlocksTotal to 1 instead of 0 as a simple way to avoid the float divide-by-zero. I think that makes sense.

    It seems like this case got fixed by @instagibbs in ec30a79f1c430cc7fbda37e5d747b0b31b262fa5 (#15283). Nice!

    If so test/sanitizer_suppressions/ubsan:float-divide-by-zero:validation.cpp can be dropped :)

  14. MarcoFalke renamed this:
    fix potential devision by 0
    wallet: fix potential devision by 0
    on Nov 12, 2020
  15. MarcoFalke renamed this:
    wallet: fix potential devision by 0
    wallet: fix potential devision by 0 in WalletLogPrintf
    on Nov 12, 2020
  16. MarcoFalke added the label Utils/log/libs on Nov 12, 2020
  17. MarcoFalke added the label Wallet on Nov 12, 2020
  18. MarcoFalke added this to the milestone 0.21.0 on Nov 12, 2020
  19. in src/wallet/wallet.cpp:3117 in 440f8d3abe 
    3114 | +              (feeCalc.est.pass.totalConfirmed + feeCalc.est.pass.inMempool + feeCalc.est.pass.leftMempool) > 0.0 ? 100 * feeCalc.est.pass.withinTarget / (feeCalc.est.pass.totalConfirmed + feeCalc.est.pass.inMempool + feeCalc.est.pass.leftMempool) : 0.0,
3115 |                feeCalc.est.pass.withinTarget, feeCalc.est.pass.totalConfirmed, feeCalc.est.pass.inMempool, feeCalc.est.pass.leftMempool,
3116 |                feeCalc.est.fail.start, feeCalc.est.fail.end,
3117 | -              100 * feeCalc.est.fail.withinTarget / (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool),
3118 | +              (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) > 0.0 ? 100 * feeCalc.est.fail.withinTarget / (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) : 0.0,
3119 |                feeCalc.est.fail.withinTarget, feeCalc.est.fail.totalConfirmed, feeCalc.est.fail.inMempool, feeCalc.est.fail.leftMempool);
    LarryRuane commented at 7:56 PM on November 12, 2020:

    Suggestion; I haven't tested this yet, but it compiles. (UPDATE: tested)

        auto sum = [](const EstimatorBucket& b) { return b.totalConfirmed + b.inMempool + b.leftMempool; };
    WalletLogPrintf("Fee Calculation: Fee:%d Bytes:%u Needed:%d Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n",
              nFeeRet, nBytes, nFeeNeeded, feeCalc.returnedTarget, feeCalc.desiredTarget, StringForFeeReason(feeCalc.reason), feeCalc.est.decay,
              feeCalc.est.pass.start, feeCalc.est.pass.end,
              sum(feeCalc.est.pass) > 0.0 ? 100 * feeCalc.est.pass.withinTarget / sum(feeCalc.est.pass) : 0.0,
              feeCalc.est.pass.withinTarget, feeCalc.est.pass.totalConfirmed, feeCalc.est.pass.inMempool, feeCalc.est.pass.leftMempool,
              feeCalc.est.fail.start, feeCalc.est.fail.end,
              sum(feeCalc.est.fail) > 0.0 ? 100 * feeCalc.est.fail.withinTarget / sum(feeCalc.est.fail) : 0.0,
              feeCalc.est.fail.withinTarget, feeCalc.est.fail.totalConfirmed, feeCalc.est.fail.inMempool, feeCalc.est.fail.leftMempool);
    laanwj commented at 10:15 PM on November 12, 2020:

    Yes, making that a function is a bit more elegant, it makes it clearer that the same value is used for 0-comparison as for division.

    ajtowns commented at 4:23 AM on November 13, 2020:

    Or move the whole thing into a lambda?

        auto bucket_pct = [](const EstimatorBucket& b) {
        double sum = b.totalConfirmed + b.inMempool + b.leftMempool;
        return sum > 0.0 ? (100.0 * b.withinTaget / sum) : 0.0;
    };
  20. laanwj commented at 10:11 PM on November 12, 2020: member

    Code review ACK 440f8d3abe97b96f434dad5216d417a08fc10253

  21. fanquake renamed this:
    wallet: fix potential devision by 0 in WalletLogPrintf
    wallet: fix potential division by 0 in WalletLogPrintf
    on Nov 13, 2020
  22. jonasschnelli merged this on Nov 13, 2020
  23. jonasschnelli closed this on Nov 13, 2020
  24. sidhujag referenced this in commit 91c2657380 on Nov 13, 2020
  25. PastaPastaPasta referenced this in commit 0b48bfa01e on Jun 27, 2021
  26. PastaPastaPasta referenced this in commit da38a0c378 on Jun 28, 2021
  27. PastaPastaPasta referenced this in commit abc732e280 on Jun 29, 2021
  28. PastaPastaPasta referenced this in commit 11f6aae2ba on Jul 1, 2021
  29. PastaPastaPasta referenced this in commit 04e5e42713 on Jul 1, 2021
  30. PastaPastaPasta referenced this in commit 81a867cbe0 on Jul 15, 2021
  31. PastaPastaPasta referenced this in commit 4e0744fb2b on Jul 15, 2021
  32. PastaPastaPasta referenced this in commit e8a9c28c82 on Jul 16, 2021
  33. gabriel-bjg referenced this in commit a1858222f0 on Jul 16, 2021
  34. DrahtBot locked this on Feb 15, 2022
Contributors
jonasschnellihebastopracticalswiftLarryRuanelaanwjajtowns
Labels
WalletUtils/log/libs

Milestone
0.21.0

Linked (view graph)
#20344 wallet: fix scanning progress calculation for single block range
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me