[Qt] Improve progress display during headers-sync and peer-finding #9461
Conversation
jonasschnelli
force-pushed
the
2017/01/qt_sync
branch
from
fbf8d9b to
8757896
Compare
|
utACK |
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
Concept ACK 8757896cdb93db6e111835e345e615e0121093eb
src/qt/bitcoingui.cpp
Outdated
There was a problem hiding this comment.
I think this is dead code, because whenever getNumConnections()==0, you display Connecting .... And whenever getNumConnections()>0, you are in the case of BLOCK_SOURCE_NETWORK.
|
Nit: If you feel like it, you can also fix the documentation for getBlockSource, which wrongly states a boolean is returned. https://dev.visucore.com/bitcoin/doxygen/class_client_model.html#acd447f61bfd57267a31a0aa88c176ee6 |
jonasschnelli
force-pushed
the
2017/01/qt_sync
branch
from
8757896 to
862c6f1
Compare
|
Fixed @MarcoFalke findings... |
src/qt/bitcoingui.cpp
Outdated
There was a problem hiding this comment.
Please don't hardcode the interblock time 600, but use Consensus::Params::nPowTargetTimespan.
There was a problem hiding this comment.
I dont believe this was fixed?
src/qt/bitcoingui.cpp
Outdated
There was a problem hiding this comment.
Please use spaces around operators.
src/qt/bitcoingui.cpp
Outdated
There was a problem hiding this comment.
What does the boolean return value indicate? It seems unused.
There was a problem hiding this comment.
The boolean indicates whether the label was updated or not. This reduces redundant calls in bitcoingui.cpp. I'll document it a bit more.
There was a problem hiding this comment.
Nevermind. It's no longer in use! I'll fix that.
src/qt/bitcoingui.cpp
Outdated
There was a problem hiding this comment.
Where does the value 24 come from? Can you provide a named constant for it?
There was a problem hiding this comment.
Comes from #8821 over modaloverlay.cpp.
I'll try to move this into a constant.
|
This is currently tagged for 0.14, but seems not yet ready. (I blame myself for not taking a second look). Given that it is mostly only a minor gui change, I'd prefer to defer this and assign the 0.15 milestone? |
jonasschnelli
force-pushed
the
2017/01/qt_sync
branch
from
862c6f1 to
d9d029e
Compare
|
Thanks @sipa for the review. Fixed all points. @MarcoFalke: We could postpone this for 0.15. Let's try to get one or two tested ACK's (or NACKs) today, and, if not, we untag it tomorrow. |
src/qt/bitcoingui.cpp
Outdated
There was a problem hiding this comment.
I believe this is dead code - getBlockSource will always return at least BLOCK_SOURCE_NETWORK if getNumConnections is > 0.
jonasschnelli
force-pushed
the
2017/01/qt_sync
branch
from
d9d029e to
40ec7c7
Compare
|
Thanks for the review @TheBlueMatt. Updated and remove the dead-code part. |
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
I was trying to reproduce the scenario mentioned in the OP, but somehow it shows just right already in current master...
| { | ||
| int64_t headersTipTime = clientModel->getHeaderTipTime(); | ||
| int headersTipHeight = clientModel->getHeaderTipHeight(); | ||
| int estHeadersLeft = (GetTime() - headersTipTime)/600; |
There was a problem hiding this comment.
Nit: I know I initially put it there, but please change it to Consensus::Params::nPowTargetSpacing, as sipa requested.
|
utACK 40ec7c7, would be nice to not use the 600 constant but it doesnt matter too much for now. |
|
Tested ACK 40ec7c7 |
…er-finding 40ec7c7 [Qt] Improve progress display during headers-sync and peer-finding (Jonas Schnelli)
|
Posthumous concept ACK, nice. |
…tions.py If both numeric format specifiers and "others" are used, assume we're dealing with a Qt-formatted message. In the case of Qt formatting (see https://doc.qt.io/qt-5/qstring.html#arg) only numeric formats are replaced at all. This means "(percentage: %1%)" is valid (which was introduced in bitcoin#9461), without needing any kind of escaping that would be necessary for strprintf. Without this, this function would wrongly detect '%)' as a printf format specifier.
… and peer-finding 40ec7c7 [Qt] Improve progress display during headers-sync and peer-finding (Jonas Schnelli)
* Merge bitcoin#8996: Network activity toggle 19f46f1 Qt: New network_disabled icon (Luke Dashjr) 54cf997 RPC/Net: Use boolean consistently for networkactive, and remove from getinfo (Luke Dashjr) b2b33d9 Overhaul network activity toggle (Jonas Schnelli) 32efa79 Qt: Add GUI feedback and control of network activity state. (Jon Lund Steffensen) e38993b RPC: Add "togglenetwork" method to toggle network activity temporarily (Jon Lund Steffensen) 7c9a98a Allow network activity to be temporarily suspended. (Jon Lund Steffensen) * Revert on-click behavior of network status icon to showing peers list Stay with the way Dash handled clicking on the status icon * Add theme support for network disabled icon * Merge bitcoin#8874: Multiple Selection for peer and ban tables 1077577 Fix auto-deselection of peers (Andrew Chow) addfdeb Multiple Selection for peer and ban tables (Andrew Chow) * Merge bitcoin#9190: qt: Plug many memory leaks ed998ea qt: Avoid OpenSSL certstore-related memory leak (Wladimir J. van der Laan) 5204598 qt: Avoid shutdownwindow-related memory leak (Wladimir J. van der Laan) e4f126a qt: Avoid splash-screen related memory leak (Wladimir J. van der Laan) 693384e qt: Prevent thread/memory leak on exiting RPCConsole (Wladimir J. van der Laan) 47db075 qt: Plug many memory leaks (Wladimir J. van der Laan) * Merge bitcoin#9218: qt: Show progress overlay when clicking spinner icon 042f9fa qt: Show progress overlay when clicking spinner icon (Wladimir J. van der Laan) 827d9a3 qt: Replace NetworkToggleStatusBarControl with generic ClickableLabel (Wladimir J. van der Laan) * Merge bitcoin#9266: Bugfix: Qt/RPCConsole: Put column enum in the right places df17fe0 Bugfix: Qt/RPCConsole: Put column enum in the right places (Luke Dashjr) * Merge bitcoin#9255: qt: layoutAboutToChange signal is called layoutAboutToBeChanged f36349e qt: Remove on_toggleNetworkActiveButton_clicked from RPCConsole (Wladimir J. van der Laan) 297cc20 qt: layoutAboutToChange signal is called layoutAboutToBeChanged (Wladimir J. van der Laan) * Use UniValue until bitcoin PR bitcoin#8788 is backported Network active toggle was already based on "[RPC] Give RPC commands more information about the RPC request" We need to use the old UniValue style until that one is backported * Merge bitcoin#8906: [qt] sync-overlay: Don't show progress twice fafeec3 [qt] sync-overlay: Don't show progress twice (MarcoFalke) * Merge bitcoin#8985: Use pindexBestHeader instead of setBlockIndexCandidates for NotifyHeaderTip() 3154d6e [Qt] use NotifyHeaderTip's height and date for the progress update (Jonas Schnelli) 0a261b6 Use pindexBestHeader instead of setBlockIndexCandidates for NotifyHeaderTip() (Jonas Schnelli) * Merge bitcoin#9280: [Qt] Show ModalOverlay by pressing the progress bar, allow hiding 89a3723 [Qt] Show ModalOverlay by pressing the progress bar, disabled show() in sync mode (Jonas Schnelli) * Merge bitcoin#9461: [Qt] Improve progress display during headers-sync and peer-finding 40ec7c7 [Qt] Improve progress display during headers-sync and peer-finding (Jonas Schnelli) * Merge bitcoin#9588: qt: Use nPowTargetSpacing constant fa4d478 qt: Use nPowTargetSpacing constant (MarcoFalke) * Hide modal overlay forever when syncing has catched up Don't allow to open it again by clicking on the progress bar and spinner icon. Currently the overlay does not show meaningful information about masternode sync and it gives the impression of being stuck after the block chain sync is done. * Don't include chainparams.h in sendcoinsdialog.cpp This was just a remainder of a backported PR which meant to change some calculation in this file which does not apply to Dash. * Also check for fNetworkActive in ConnectNode * Merge bitcoin#9528: [qt] Rename formateNiceTimeOffset(qint64) to formatNiceTimeOffset(qint64) 988d300 [qt] Rename formateNiceTimeOffset(qint64) to formatNiceTimeOffset(qint64) (practicalswift) * Merge bitcoin#11237: qt: Fixing division by zero in time remaining c8d38ab Refactor tipUpdate as per style guide (MeshCollider) 3b69a08 Fix division by zero in time remaining (MeshCollider) Pull request description: Fixes bitcoin#10291, bitcoin#11265 progressDelta may be 0 (or even negative according to 11265), this checks for that and prints unknown if it is, because we cannot calculate an estimate for the time remaining (would be infinite or negative). Tree-SHA512: bc5708e5ed6e4670d008219558c5fbb25709bd99a32c98ec39bb74f94a0b7fa058f3d03389ccdd39e6723e6b5b48e34b13ceee7c051c2db631e51d8ec3e1d68c
…tions.py If both numeric format specifiers and "others" are used, assume we're dealing with a Qt-formatted message. In the case of Qt formatting (see https://doc.qt.io/qt-5/qstring.html#arg) only numeric formats are replaced at all. This means "(percentage: %1%)" is valid (which was introduced in bitcoin#9461), without needing any kind of escaping that would be necessary for strprintf. Without this, this function would wrongly detect '%)' as a printf format specifier.
6 participants
Stumbled over this during some Core-SPV work.
After a fresh start (especially if no
peers.datfile is available), it can take a couple of seconds until we can start syncing. During this time, the user is not directly informed about the current state.Same problem appears when syncing headers. If you download the headers from a slow peer, this can take a couple of minutes.
Users can't see the reason why the sync hasn't started.
This PR fixes this by displaying the "Connecting to peers..." in the Status-Bar when no peers are available as well as it shows "Syncing Headers (%%)..." during the headers-sync phase and only if the headers-tip is older then 24*600.
Screenshots: