Relax GUI freezes during IBD (when using wallets) #97

jonasschnelli commented at 9:14 AM on September 18, 2020: contributor

Calling updateSmartFeeLabel and therefore estimateSmartFee is pointless during IBD.

GUI freezes appear because estimateSmartFee competes with processBlock for the m_cs_fee_estimator lock leading to multiple seconds of blocking the GUI thread in updateSmartFeeLabel.

Only update the updateSmartFeeLabel once in sync 0d9d2a1f7c

Sjors commented at 9:53 AM on September 18, 2020: member

I can't reproduce GUI freezes in the Send dialog during IBD (macOS 10.15.6, fresh data dir and fresh empty wallet on mainnet), but the change looks reasonable to me.

jonasschnelli commented at 10:48 AM on September 18, 2020: contributor

@Sjors: I think you need to be at a height where blocks require more CPU cycles to verify. I can 100% reproduce the freezes when syncing up a mainnet node after – say – a week of offlinetime and thus falling out of sync.

Sjors commented at 10:49 AM on September 18, 2020: member

I see, in that case invalidateblock and reconsiderblock should be handy for testing.

ryanofsky approved

ryanofsky commented at 3:40 PM on September 18, 2020: contributor

Code review ACK 0d9d2a1f7c26dc9c7b233ea8c3182fe1f8936bca. Clever fix. Didn't test but I remember I could reproduce the startup issue easily before by putting a sleep in estimateSmartFee.

in src/qt/sendcoinsdialog.cpp:139 in 0d9d2a1f7c

 135 | @@ -134,7 +136,7 @@ void SendCoinsDialog::setClientModel(ClientModel *_clientModel)
 136 |      this->clientModel = _clientModel;
 137 |  
 138 |      if (_clientModel) {
 139 | -        connect(_clientModel, &ClientModel::numBlocksChanged, this, &SendCoinsDialog::updateSmartFeeLabel);
 140 | +        connect(_clientModel, &ClientModel::numBlocksChanged, this, &SendCoinsDialog::updateNumberOfBlocks);

promag commented at 5:38 PM on September 20, 2020:

nit, could connect to lambda to avoid changes in header file:

connect(_clientModel, &ClientModel::numBlocksChanged, [this](int, const QDateTime&, double, bool, SynchronizationState sync_state) {
    if (sync_state == SynchronizationState::POST_INIT) {
        updateSmartFeeLabel();
    }
});

promag commented at 5:39 PM on September 20, 2020: contributor

Code review ACK 0d9d2a1f7c26dc9c7b233ea8c3182fe1f8936bca.

hebasto commented at 3:17 PM on October 5, 2020: member

Concept ACK.

in src/qt/sendcoinsdialog.cpp:31 in 0d9d2a1f7c

  27 | @@ -28,6 +28,8 @@
  28 |  #include <wallet/fees.h>
  29 |  #include <wallet/wallet.h>
  30 |  
  31 | +#include <validation.h>

hebasto commented at 3:41 PM on October 5, 2020:

nit: No reasons to create a separate group of headers.

in src/qt/sendcoinsdialog.h:102 in 0d9d2a1f7c

  98 | @@ -98,6 +99,7 @@ private Q_SLOTS:
  99 |      void coinControlClipboardLowOutput();
 100 |      void coinControlClipboardChange();
 101 |      void updateFeeSectionControls();
 102 | +    void updateNumberOfBlocks(int count, const QDateTime& blockDate, double nVerificationProgress, bool headers, SynchronizationState sync_state);

hebasto commented at 3:45 PM on October 5, 2020:

nit: updateNumberOfBlocks slot name for an object that knows nothing about "number of blocks" looks a bit weird. But I cannot suggest a better name (

hebasto approved

hebasto commented at 3:47 PM on October 5, 2020: member

ACK 0d9d2a1f7c26dc9c7b233ea8c3182fe1f8936bca, tested on Linux Mint 20 (x86_64) with QT_FATAL_WARNINGS=1 and -debug=qt.

MarcoFalke merged this on Oct 16, 2020

MarcoFalke closed this on Oct 16, 2020

sidhujag referenced this in commit 81bf919ddd on Oct 16, 2020

apoelstra referenced this in commit 15ad824e15 on Dec 3, 2020

stevenroose referenced this in commit ebadde11cd on Mar 22, 2021

Fabcien referenced this in commit 4a41495d0e on Nov 25, 2021

bitcoin-core locked this on Feb 15, 2022