Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Oct 16, 2020

Conversation

jonasschnelli
Copy link
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.

@Sjors
Copy link
Member

Sjors commented Sep 18, 2020

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
Copy link
Contributor Author

@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
Copy link
Member

Sjors commented Sep 18, 2020

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

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 0d9d2a1.

@@ -134,7 +136,7 @@ void SendCoinsDialog::setClientModel(ClientModel *_clientModel)
this->clientModel = _clientModel;

if (_clientModel) {
connect(_clientModel, &ClientModel::numBlocksChanged, this, &SendCoinsDialog::updateSmartFeeLabel);
connect(_clientModel, &ClientModel::numBlocksChanged, this, &SendCoinsDialog::updateNumberOfBlocks);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
    }
});

@hebasto
Copy link
Member

hebasto commented Oct 5, 2020

Concept ACK.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -28,6 +28,8 @@
#include <wallet/fees.h>
#include <wallet/wallet.h>

#include <validation.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (

@maflcko maflcko merged commit 9e8d2bd into bitcoin-core:master Oct 16, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 16, 2020
…allets)

0d9d2a1 Only update the updateSmartFeeLabel once in sync (Jonas Schnelli)

Pull request description:

  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`.

ACKs for top commit:
  ryanofsky:
    Code review ACK 0d9d2a1. Clever fix. Didn't test but I remember I could reproduce the startup issue easily before by putting a sleep in estimateSmartFee.
  promag:
    Code review ACK 0d9d2a1.
  hebasto:
    ACK 0d9d2a1, tested on Linux Mint 20 (x86_64) with `QT_FATAL_WARNINGS=1` and `-debug=qt`.

Tree-SHA512: 85ec2266f06ddd7b523e24d2a462f10ed965d5b4d479005263056f81b7fe49996e1568dafb84658af406e9202ed3bfa846d59c10bb951e0f97cee230e30fafd5
apoelstra added a commit to apoelstra/elements that referenced this pull request Dec 3, 2020
stevenroose pushed a commit to ElementsProject/elements that referenced this pull request Mar 22, 2021
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 25, 2021
Summary:
> 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.

This is a backport of [[bitcoin-core/gui#97 | core-gui#97]]

Test Plan: `ninja && src/qt/bitcoin-qt`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10516
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants