[Qt] Improve progress display during headers-sync and peer-finding #9461

pull jonasschnelli wants to merge 1 commits into bitcoin:master from jonasschnelli:2017/01/qt_sync changing 4 files +18 −3
  1. jonasschnelli commented at 2:53 PM on January 3, 2017: contributor

    Stumbled over this during some Core-SPV work.

    After a fresh start (especially if no peers.dat file 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:

    <img width="948" alt="bildschirmfoto 2017-01-03 um 15 16 25" src="https://cloud.githubusercontent.com/assets/178464/21611330/b21400c6-d1cc-11e6-8627-59bbf149930a.png"> <img width="948" alt="bildschirmfoto 2017-01-03 um 15 32 41" src="https://cloud.githubusercontent.com/assets/178464/21611329/b1ee62e4-d1cc-11e6-9e76-bbb0b6487f75.png">

  2. jonasschnelli added the label GUI on Jan 3, 2017
  3. jonasschnelli force-pushed on Jan 3, 2017
  4. in src/qt/bitcoingui.cpp:None in 8757896cdb outdated 
     807 | +            {
 808 | +                // Case: not yet connected to peers
 809 | +                progressBarLabel->setText(tr("Connecting to peers..."));
 810 | +            }
 811 | +            else if (!updateHeadersSyncProgressLabel()) {
 812 | +                // Case: not Importing, not Reindexing and no network connection
    MarcoFalke commented at 4:34 PM on January 3, 2017:

    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.

  5. rebroad commented at 4:39 PM on January 3, 2017: contributor

    utACK

  6. MarcoFalke dismissed
  7. MarcoFalke commented at 4:47 PM on January 3, 2017: member

    Concept ACK 8757896cdb93db6e111835e345e615e0121093eb

  8. MarcoFalke commented at 4:56 PM on January 3, 2017: member

    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

  9. jonasschnelli force-pushed on Jan 6, 2017
  10. jonasschnelli commented at 9:46 AM on January 6, 2017: contributor

    Fixed @MarcoFalke findings...

  11. jonasschnelli added this to the milestone 0.14.0 on Jan 12, 2017
  12. in src/qt/bitcoingui.cpp:None in 862c6f18a7 outdated 
     747 | @@ -748,6 +748,19 @@ void BitcoinGUI::setNetworkActive(bool networkActive)
 748 |      updateNetworkState();
 749 |  }
 750 |  
 751 | +bool BitcoinGUI::updateHeadersSyncProgressLabel()
 752 | +{
 753 | +    int64_t headersTipTime = clientModel->getHeaderTipTime();
 754 | +    int headersTipHeight = clientModel->getHeaderTipHeight();
 755 | +    int estHeadersLeft = (GetTime() - headersTipTime)/600;
    sipa commented at 5:08 PM on January 15, 2017:

    Please don't hardcode the interblock time 600, but use Consensus::Params::nPowTargetTimespan.

    TheBlueMatt commented at 4:52 PM on January 18, 2017:

    I dont believe this was fixed?

  13. in src/qt/bitcoingui.cpp:None in 862c6f18a7 outdated 
     753 | +    int64_t headersTipTime = clientModel->getHeaderTipTime();
 754 | +    int headersTipHeight = clientModel->getHeaderTipHeight();
 755 | +    int estHeadersLeft = (GetTime() - headersTipTime)/600;
 756 | +    if (estHeadersLeft > 24)
 757 | +    {
 758 | +        progressBarLabel->setText(tr("Syncing Headers (%1%)...").arg(QString::number(100.0/(headersTipHeight+estHeadersLeft)*headersTipHeight, 'f', 1)));
    sipa commented at 5:09 PM on January 15, 2017:

    Please use spaces around operators.

  14. in src/qt/bitcoingui.cpp:None in 862c6f18a7 outdated 
     747 | @@ -748,6 +748,19 @@ void BitcoinGUI::setNetworkActive(bool networkActive)
 748 |      updateNetworkState();
 749 |  }
 750 |  
 751 | +bool BitcoinGUI::updateHeadersSyncProgressLabel()
    sipa commented at 5:09 PM on January 15, 2017:

    What does the boolean return value indicate? It seems unused.

    jonasschnelli commented at 7:06 AM on January 18, 2017:

    The boolean indicates whether the label was updated or not. This reduces redundant calls in bitcoingui.cpp. I'll document it a bit more.

    jonasschnelli commented at 7:07 AM on January 18, 2017:

    Nevermind. It's no longer in use! I'll fix that.

  15. in src/qt/bitcoingui.cpp:None in 862c6f18a7 outdated 
     747 | @@ -748,6 +748,19 @@ void BitcoinGUI::setNetworkActive(bool networkActive)
 748 |      updateNetworkState();
 749 |  }
 750 |  
 751 | +bool BitcoinGUI::updateHeadersSyncProgressLabel()
 752 | +{
 753 | +    int64_t headersTipTime = clientModel->getHeaderTipTime();
 754 | +    int headersTipHeight = clientModel->getHeaderTipHeight();
 755 | +    int estHeadersLeft = (GetTime() - headersTipTime)/600;
 756 | +    if (estHeadersLeft > 24)
    sipa commented at 5:10 PM on January 15, 2017:

    Where does the value 24 come from? Can you provide a named constant for it?

    jonasschnelli commented at 7:06 AM on January 18, 2017:

    Comes from #8821 over modaloverlay.cpp. I'll try to move this into a constant.

  16. MarcoFalke commented at 12:17 AM on January 18, 2017: member

    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?

  17. jonasschnelli force-pushed on Jan 18, 2017
  18. jonasschnelli commented at 7:38 AM on January 18, 2017: contributor

    Thanks @sipa for the review. Fixed all points. @MarcoFalke: We could postpone this for 0.15. This – simple – PR together with the new modal overlay, does guide the user much better during the first startup. Connecting to peers and syncing the first headers can take a couple of seconds... during this stage, I think it would be great to show a minimal progress update.

    Let's try to get one or two tested ACK's (or NACKs) today, and, if not, we untag it tomorrow.

  19. [Qt] Improve progress display during headers-sync and peer-finding 40ec7c7b0d
  20. in src/qt/bitcoingui.cpp:None in d9d029e965 outdated 
     803 | +            {
 804 | +                // Case: not yet connected to peers
 805 | +                progressBarLabel->setText(tr("Connecting to peers..."));
 806 | +            }
 807 | +            else
 808 | +                updateHeadersSyncProgressLabel();
    TheBlueMatt commented at 4:51 PM on January 18, 2017:

    I believe this is dead code - getBlockSource will always return at least BLOCK_SOURCE_NETWORK if getNumConnections is > 0.

  21. jonasschnelli force-pushed on Jan 19, 2017
  22. jonasschnelli commented at 8:11 AM on January 19, 2017: contributor

    Thanks for the review @TheBlueMatt. Updated and remove the dead-code part.

  23. in src/qt/bitcoingui.cpp:None in 40ec7c7b0d 
     747 | @@ -748,6 +748,15 @@ void BitcoinGUI::setNetworkActive(bool networkActive)
 748 |      updateNetworkState();
 749 |  }
 750 |  
 751 | +void BitcoinGUI::updateHeadersSyncProgressLabel()
 752 | +{
 753 | +    int64_t headersTipTime = clientModel->getHeaderTipTime();
 754 | +    int headersTipHeight = clientModel->getHeaderTipHeight();
 755 | +    int estHeadersLeft = (GetTime() - headersTipTime)/600;
    MarcoFalke commented at 10:16 AM on January 19, 2017:

    Nit: I know I initially put it there, but please change it to Consensus::Params::nPowTargetSpacing, as sipa requested.

  24. MarcoFalke commented at 10:17 AM on January 19, 2017: member

    I was trying to reproduce the scenario mentioned in the OP, but somehow it shows just right already in current master...

  25. TheBlueMatt commented at 4:15 PM on January 19, 2017: member

    utACK 40ec7c7b0d2a5a37de90635b676b16884b622dd6, would be nice to not use the 600 constant but it doesnt matter too much for now.

  26. MarcoFalke commented at 6:49 PM on January 19, 2017: member

    Tested ACK 40ec7c7

  27. jonasschnelli merged this on Jan 19, 2017
  28. jonasschnelli closed this on Jan 19, 2017
  29. jonasschnelli referenced this in commit b25068697f on Jan 19, 2017
  30. jtimon commented at 7:49 PM on January 19, 2017: contributor

    Posthumous concept ACK, nice.

  31. laanwj referenced this in commit 5e903a5ed9 on Feb 10, 2017
  32. codablock referenced this in commit 612f2fbc73 on Sep 9, 2017
  33. codablock referenced this in commit ac375a17d4 on Sep 11, 2017
  34. UdjinM6 referenced this in commit 91d99fcd3f on Sep 11, 2017
  35. lateminer referenced this in commit f7d567e4a4 on Jan 4, 2019
  36. MarcoFalke locked this on Sep 8, 2021
Contributors
jonasschnelliMarcoFalkerebroadsipaTheBlueMattjtimon
Labels
GUI

Milestone
0.14.0

Linked (view graph)
#9735 devtools: Handle Qt formatting characters edge-case in update-translations.py
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me