wallet: fix scanning progress calculation for single block range

[theStack]

If the blockchain is rescanned for a single block (i.e. start and stop hashes are equal, and with that also the estimated start/stop verification progress values) the progress calculation could lead to a NaN value caused by a division by zero (0.0/0.0), resulting in an invalid JSON result for the getwalletinfo RPC. This PR fixes this behaviour by setting the progress to zero in that special case. Fixes #20297.

The behaviour can easily be reproduced by continuously running single block rescans in an endless loop, e.g. via

#!/bin/bash
while true
do
    bitcoin-cli rescanblockchain $(bitcoin-cli getblockcount)
done

and at the same time perform some getwalletinfo RPCs.

On the master branch, this leads to frequent invalid responses (tested on mainchain):

$ bitcoin-cli getwalletinfo
error: couldn't parse reply from server
$ curl --user `cat ~/.bitcoin/.cookie` --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getwalletinfo", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
{"result":{"walletname":"","walletversion":169900,"format":"bdb","balance":0.00000000,"unconfirmed_balance":0.00000000,"immature_balance":0.00000000,"txcount":0,"keypoololdest":1603677276,"keypoolsize":1000,"hdseedid":"3196e33ecb47c7130e6ca60f2f895f9259860dca","keypoolsize_hd_internal":1000,"paytxfee":0.00000000,"private_keys_enabled":true,"avoid_reuse":false,"scanning":{"duration":0,"progress":},"descriptors":false},"error":null,"id":"curltest"}

(note that missing value for "progress" in the JSON result).

On the PR branch, the behaviour doesn't occur anymore.

[practicalswift]

ACK 4771bac, but consider getting rid of the UBSan suppression that is no longer needed after this change :)

FWIW I've tried getting rid of this and all other float divide by zeros in our code base before (see PR #17208), but I had to close that PR due to lack of reviewer interest back then :)

Please consider taking on the other cases from #17208 when you're done with this one. I'd be glad to review :)

[kristapsk]

ACK 4771bac

[maflcko]

Please remove the unused suppression:

diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan
index 75257d886..784273952 100644
--- a/test/sanitizer_suppressions/ubsan
+++ b/test/sanitizer_suppressions/ubsan
@@ -1,7 +1,6 @@
 # -fsanitize=undefined suppressions
 # =================================
 float-divide-by-zero:validation.cpp
-float-divide-by-zero:wallet/wallet.cpp
 
 # -fsanitize=integer suppressions
 # ===============================

[hebasto]

Approach ACK 4771bac, tested on Linux Mint 20 (x86_64).

Still addressing of #20344 (comment) is anticipated :)

[theStack]

Force-pushed with the UBSan suppression removal as suggested by practicalswift and MarcoFalke.

Please consider taking on the other cases from #17208 when you're done with this one. I'd be glad to review :)

Thanks for the suggestion and review offer, will definitely take a look into it.

[promag]

Core review ACK 5e14602.

[maflcko]

review ACK 5e14602

[fanquake]

@jonasschnelli reported on IRC that builds have been failing on his CI since this PR was merged. https://bitcoinbuilds.org/index.php?job=ffb7d59f-379f-4f27-a273-a5595b8c5f07:

********* Start testing of WalletTests *********
Config: Using QtTest library 5.9.5, Qt 5.9.5 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 7.4.0)
PASS   : WalletTests::initTestCase()
QDEBUG : WalletTests::walletTests() TransactionTablePriv::refreshWallet
wallet/wallet.cpp:3113:51: runtime error: division by zero
    #0 0x55d52a716807 in CWallet::CreateTransactionInternal(std::vector<CRecipient, std::allocator<CRecipient> > const&, std::shared_ptr<CTransaction const>&, long&, int&, bilingual_str&, CCoinControl const&, FeeCalculation&, bool) /home/ubuntu/src/src/wallet/wallet.cpp:3113:51
    #1 0x55d52a710290 in CWallet::CreateTransaction(std::vector<CRecipient, std::allocator<CRecipient> > const&, std::shared_ptr<CTransaction const>&, long&, int&, bilingual_str&, CCoinControl const&, FeeCalculation&, bool) /home/ubuntu/src/src/wallet/wallet.cpp:3133:16
    #2 0x55d52a470eaa in interfaces::(anonymous namespace)::WalletImpl::createTransaction(std::vector<CRecipient, std::allocator<CRecipient> > const&, CCoinControl const&, bool, int&, long&, bilingual_str&) /home/ubuntu/src/src/interfaces/wallet.cpp:235:24
    #3 0x55d5294ff6b5 in WalletModel::prepareTransaction(WalletModelTransaction&, CCoinControl const&) /home/ubuntu/src/src/qt/walletmodel.cpp:203:27
    #4 0x55d5294351fd in SendCoinsDialog::PrepareSendText(QString&, QString&, QString&) /home/ubuntu/src/src/qt/sendcoinsdialog.cpp:267:28
    #5 0x55d52943a961 in SendCoinsDialog::on_sendButton_clicked() /home/ubuntu/src/src/qt/sendcoinsdialog.cpp:377:10
    #6 0x55d52957527d in SendCoinsDialog::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) /home/ubuntu/src/src/qt/moc_sendcoinsdialog.cpp:203:21
    #7 0x7f6344f9d002 in QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x29b002)
    #8 0x7f6344f9f47c in QMetaObject::invokeMethod(QObject*, char const*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x29d47c)
    #9 0x55d52914357c in QMetaObject::invokeMethod(QObject*, char const*, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs.h:466:16
    #10 0x55d52914357c in (anonymous namespace)::SendCoins(CWallet&, SendCoinsDialog&, boost::variant<CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessUnknown> const&, long, bool) /home/ubuntu/src/src/qt/test/wallettests.cpp:76
    #11 0x55d52913c42f in (anonymous namespace)::TestGUI(interfaces::Node&) /home/ubuntu/src/src/qt/test/wallettests.cpp:187:21
    #12 0x55d529139c98 in WalletTests::walletTests() /home/ubuntu/src/src/qt/test/wallettests.cpp:285:5
    #13 0x55d52916ba22 in WalletTests::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) /home/ubuntu/src/src/qt/test/moc_wallettests.cpp:70:21
    #14 0x7f6344f9d002 in QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x29b002)
    #15 0x7f6344adb799  (/usr/lib/x86_64-linux-gnu/libQt5Test.so.5+0x13799)
    #16 0x7f6344adc4ef  (/usr/lib/x86_64-linux-gnu/libQt5Test.so.5+0x144ef)
    #17 0x7f6344adca60  (/usr/lib/x86_64-linux-gnu/libQt5Test.so.5+0x14a60)
    #18 0x7f6344add02a in QTest::qExec(QObject*, int, char**) (/usr/lib/x86_64-linux-gnu/libQt5Test.so.5+0x1502a)
    #19 0x55d5290e42fc in main /home/ubuntu/src/src/qt/test/test_main.cpp:94:9
    #20 0x7f634172db96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
    #21 0x55d528fecd89 in _start (/home/ubuntu/src/src/qt/test/test_bitcoin-qt+0x2633d89)

SUMMARY: UndefinedBehaviorSanitizer: float-divide-by-zero wallet/wallet.cpp:3113:51 in 
FAIL qt/test/test_bitcoin-qt (exit status: 1)

[jonasschnelli]

This triggers at least a dev-by-0 here (

bitcoin/src/wallet/wallet.cpp

Line 3110 in 9bd1316

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",

)

Maybe at other places?

How could this not trigger a travis error? I guess we run the qt tests there as well?

[maflcko]

Yes, the qt tests did run: https://cirrus-ci.com/task/5112026252967936?command=ci#L3962

[practicalswift]

@fanquake @jonasschnelli Feel free to cherry-pick in 0201cbc from #17208.