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

[Qt] Add option to pause/resume block downloads #9502

Closed

Conversation

@jonasschnelli
Copy link
Member

commented Jan 10, 2017

This, almost UI only change, will add a Pause/Resume button to the modal overlay to pause/resume block downloads during IBD.

This is an effective way to pause/resume IBD during a time when the computers resources are required somewhere else.

bildschirmfoto 2017-01-10 um 18 34 09

@jonasschnelli jonasschnelli added the GUI label Jan 10, 2017

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2017/01/autodownload branch Jan 10, 2017

@paveljanik

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2017

Isn't network toggle button usable in this case? If it is not, let's fix it instead...

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2017

I extracted this from my SPV branch. Especially there, it is very useful to pause IBD and continue with SPV during a time where you don't want to use all available resources on verification.

But also without SPV, I think this can be useful (pause IBD and not loose broadcast capabilities, fetch headers but not the blocks)...

@luke-jr

This comment has been minimized.

Copy link
Member

commented Jan 10, 2017

IMO it would be too confusing to be worth it without "SPV" mode, but probably should go in after the latter is.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 10, 2017

@luke-jr

This comment has been minimized.

Copy link
Member

commented Jan 10, 2017

The "Pause" button won't be visible in normal cases.

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2017

Isn't network toggle button usable in this case? If it is not, let's fix it instead...

Imo those are different features, but I agree that the GUI should not
"diverge" in regard to presenting features. The toggle network
functionality should be removed from the network icon and a proper
button should be put beside the new "Pause" button?

Yes. These are internally two completely different features. Expose to the users, these have similar effects.

The "Pause" button won't be visible in normal cases.

Yes. The modal overlay is currently only accessible during IBD. Though, we could extend it to support a state where the chain is in-sync and show it when someone click on the network statusbar icon.

@kallewoof
Copy link
Member

left a comment

utACK abf0941

src/net_processing.h Outdated
@@ -14,6 +14,10 @@ void RegisterNodeSignals(CNodeSignals& nodeSignals);
/** Unregister a network node */
void UnregisterNodeSignals(CNodeSignals& nodeSignals);

/** if disabled, blocks will not be requested automatically, usefull for low-resources-available mode */

This comment has been minimized.

Copy link
@kallewoof

kallewoof Feb 28, 2017

Member

Typo: usefull (one L)

src/qt/clientmodel.h Outdated
@@ -81,6 +81,10 @@ class ClientModel : public QObject
QString formatClientStartupTime() const;
QString dataDir() const;

// get/set state about autorequesting-blocks during IBD
bool isAutorequestBlocks() const;

This comment has been minimized.

Copy link
@kallewoof

kallewoof Feb 28, 2017

Member

Nit: isAutorequestingBlocks (add ing). The setAutorequestBlocks below is fine as is, though.

@laanwj laanwj added this to the 0.15.0 milestone Mar 14, 2017

@laanwj

This comment has been minimized.

Copy link
Member

commented Mar 14, 2017

Added 0.15 milestone

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2017

Rebased.

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2017/01/autodownload branch 2 times, most recently to e2e86ec Mar 17, 2017

@@ -23,6 +23,10 @@ void RegisterNodeSignals(CNodeSignals& nodeSignals);
/** Unregister a network node */
void UnregisterNodeSignals(CNodeSignals& nodeSignals);

/** if disabled, blocks will not be requested automatically, usefull for low-resources-available mode */
static const bool DEFAULT_AUTOMATIC_BLOCK_REQUESTS = true;
extern std::atomic<bool> fAutoRequestBlocks;

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 19, 2017

Member

Instead of exporting this variable (whose definition is an implementation detail) from net_processing.h I'd prefer a setter/getter, e.g. SetAutoRequestBlocks(bool) GetAutoRequestBlocks()

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Apr 19, 2017

Author Member

Agree. Will change...

@@ -233,6 +234,22 @@ QString ClientModel::dataDir() const
return GUIUtil::boostPathToQString(GetDataDir());
}

bool ClientModel::isAutorequestBlocks() const

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 19, 2017

Member

isAutorequestBlocks is a bit of a strange name; isAutoRequestingBlocks maybe?

@laanwj

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

When re-enabling AutoRequestBlocks, what 'kicks off' the block requesting process again? Setting the flag will make it request blocks the next time FindNextBlocksToDownload is called; is that good enough? I suppose it is, because SendMessages is called periodically (every 100 ms?).

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2017/01/autodownload branch from e2e86ec Apr 20, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2017

Overhauled the PR and addresses @laanwj points.

Also, I added the info "Blocks requested from peers" (blocks in flight). This may be important because pause will not result in disconnecting peers. Already requested blocks will be downloaded (and verified) in the "pause" state.

If blocks are in flight and the pause has been triggered, there is now a special info label Wait to finish current downloads.

@paveljanik

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2017

Well, I have to change my previous opinion. I think this can be useful.

Concept ACK

Will test soon.

@paveljanik

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

Needs rebase.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented May 1, 2017

ACK e952b67d294043d9758b37bf2be57a982b41c051

Code looks good. On the UI feature, two minor comments:

  • Pausing the download throws off "Estimated time left till synced." It would be better pausing didn't affect estimated time.
  • Pause/Resume button really sticks out where it's currently placed. Maybe it would make more sense next to the progress bar or near the hide button.
@sipa

This comment has been minimized.

Copy link
Member

commented May 1, 2017

Needs rebase.

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2017/01/autodownload branch May 11, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

Rebased.

@paveljanik

This comment has been minimized.

Copy link
Contributor

commented May 15, 2017

Testing this again.

ACK fc84323

I think this could be even more usable if it can be called once fully in sync with the network. But I can't display the overlay then...

@laanwj

This comment has been minimized.

Copy link
Member

commented May 24, 2017

I think this could be even more usable if it can be called once fully in sync with the network. But I can't display the overlay then...

Hm I vaguely remember I added that functionality once, you should be able to bring up the overlay by the secret trick of clicking on the sync icon.

@paveljanik

This comment has been minimized.

Copy link
Contributor

commented May 24, 2017

@laanwj But the sync icon (you mean the triangle with an exclamation mark inside?) is not displayed when you are "in sync" with the network.

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

commented May 24, 2017

I think this could be even more usable if it can be called once fully in sync with the network. But I can't display the overlay then...

There are four ways how the modal-overlay can be opened:
-> auto-opens when in IBD/sync
-> Click on the warning icons next to the balance
-> Click on the progress bar during IBD/sync
-> Click on the sync icon in the status bar

Though I agree with you, there is no option how to open it once you are in sync... which could be useful, but independent to this PR.

@laanwj

This comment has been minimized.

Copy link
Member

commented May 24, 2017

@paveljanik Oh, though it also worked with the checkmark that takes its place.

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2018

Rebased.
Thanks for re-reviewing.

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2017/01/autodownload branch Apr 18, 2018

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2018

Rebased.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2018

The last travis run for this pull request was 95 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

@DrahtBot DrahtBot closed this Jul 22, 2018

@DrahtBot DrahtBot reopened this Jul 22, 2018

src/qt/bitcoingui.cpp Outdated
@@ -493,6 +493,10 @@ void BitcoinGUI::setClientModel(ClientModel *_clientModel)
connect(_clientModel, SIGNAL(networkActiveChanged(bool)), this, SLOT(setNetworkActive(bool)));

modalOverlay->setKnownBestHeight(_clientModel->getHeaderTipHeight(), QDateTime::fromTime_t(_clientModel->getHeaderTipTime()));
modalOverlay->setPauseResumeState(!_clientModel->isAutoRequestingBlocks());
connect(modalOverlay, SIGNAL(requestVerificationPauseOrResume()), _clientModel, SLOT(toggleAutoRequestBlocks()));
connect(_clientModel, SIGNAL(verificationProgressPauseStateHasChanged(bool)), modalOverlay, SLOT(setPauseResumeState(bool)));

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 22, 2018

Member

could use the new connect syntax, so it wouldn't have to be changed again in the future?

@laanwj laanwj modified the milestones: 0.17.0, 0.18.0 Jul 23, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2018

Needs rebase

jonasschnelli added some commits Jan 10, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

Rebased.

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2017/01/autodownload branch Oct 17, 2018

jonasschnelli added some commits Apr 19, 2017

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2017/01/autodownload branch to 718ceef Oct 17, 2018


// disable pause button when we can fetch directly
// avoid using the core-layer's existing CanFetchDirectly()
bool canFetchDirectly = (blockDate.toTime_t() > GetAdjustedTime() - Params().GetConsensus().nPowTargetSpacing * 20);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 19, 2018

Member

nit: Redundant parentheses :-)

@laanwj laanwj removed this from the 0.18.0 milestone Feb 6, 2019

@fanquake

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Discussion has dragged on and died out here. Going to label "Up for grabs" and close for now.

@fanquake fanquake closed this Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.