Merge #1421: Refresh cache more often in GUI while wallet or blockchain is syncing

918909db235ff8a0886a9f74d7ac3b1caec83324 gui: include last poll for cache refresh interval (Michael Mallan)
c915970c7b00adbd27c0d8a20116d1bda57a8d2c gui(home): pass sync status directly (Michael Mallan)
92a4a4f8dcaa75bc7181767b1958e39c94e8ad3f gui(cache): store last poll at startup (Michael Mallan)
acf14c6d734fc238dc1f44a4b5bab1f3f6411c9b gui: refresh cache more often while syncing (Michael Mallan)
e419784e9f2da76e8e86c03c8773f0dcb2a883da gui: move sync status function to wallet module (Michael Mallan)

Pull request description:

  This is to resolve #1414.

  In the end, I felt it was simple enough to include commits that cover both parts of #1414 (the short-term change and the follow-up).

  I start with the short-term change (setting the refresh interval ignoring the last poll logic) and then follow with commits that cover all syncing statuses.

  I don't use as high a refresh frequency for a remote backend as for a local backend, but these values can be easily changed if required.

ACKs for top commit:
  pythcoiner:
    tACK [918909d](918909db23)

Tree-SHA512: 6b47f315b0d50b2898435247b288562413e0f0abf520747edc67166c66153f7432e65ae5b3337bba67c71ade7181110e5dbfbc58df3b8510bfb5d9eb29b2bb02

gui/src/app/cache.rs

@@ -10,7 +10,10 @@ pub struct Cache {
     pub coins: Vec<Coin>,
     pub rescan_progress: Option<f64>,
     pub sync_progress: f64,
+    /// The most recent `last_poll_timestamp`.
     pub last_poll_timestamp: Option<u32>,
+    /// The `last_poll_timestamp` when starting the application.
+    pub last_poll_at_startup: Option<u32>,
 }
 
 /// only used for tests.
@@ -24,6 +27,7 @@ impl std::default::Default for Cache {
             rescan_progress: None,
             sync_progress: 1.0,
             last_poll_timestamp: None,
+            last_poll_at_startup: None,
         }
     }
 }

gui/src/app/mod.rs

@@ -32,6 +32,7 @@ use state::{
     CoinsPanel, CreateSpendPanel, Home, PsbtsPanel, ReceivePanel, RecoveryPanel, State,
     TransactionsPanel,
 };
+use wallet::{sync_status, SyncStatus};
 
 use crate::{
     app::{cache::Cache, error::Error, menu::Menu, wallet::Wallet},
@@ -66,10 +67,13 @@ impl Panels {
             home: Home::new(
                 wallet.clone(),
                 &cache.coins,
-                cache.blockheight,
+                sync_status(
-                cache.sync_progress,
+                    daemon_backend.clone(),
-                cache.last_poll_timestamp,
+                    cache.blockheight,
-                daemon_backend.clone(),
+                    cache.sync_progress,
+                    cache.last_poll_timestamp,
+                    cache.last_poll_at_startup,
+                ),
             ),
             coins: CoinsPanel::new(&cache.coins, wallet.main_descriptor.first_timelock_value()),
             transactions: TransactionsPanel::new(wallet.clone()),
@@ -227,19 +231,31 @@ impl App {
     pub fn subscription(&self) -> Subscription<Message> {
         Subscription::batch(vec![
             time::every(Duration::from_secs(
-                // LianaLite has no rescan feature, the cache refresh loop is only
+                match sync_status(
-                // to fetch the new block height tip, which for a synced wallet
+                    self.daemon.backend(),
-                // (height > 0) is only used to warn user about recovery availability.
+                    self.cache.blockheight,
-                if self.daemon.backend() == DaemonBackend::RemoteBackend
+                    self.cache.sync_progress,
-                    && self.cache.blockheight > 0
+                    self.cache.last_poll_timestamp,
-                {
+                    self.cache.last_poll_at_startup,
-                    120
+                ) {
-                // For the rescan feature, we set a higher frequency of cache refresh
+                    SyncStatus::BlockchainSync(_) => 5, // Only applies to local backends
-                // to give to user an up-to-date view of the rescan progress.
+                    SyncStatus::WalletFullScan
-                // For a remote backend, we refresh cache more often while height is 0
+                        if self.daemon.backend() == DaemonBackend::RemoteBackend =>
-                // to detect sooner that syncing has finished.
+                    {
-                } else {
+                        10
-                    10
+                    } // If remote backend, don't ping too often
+                    SyncStatus::WalletFullScan | SyncStatus::LatestWalletSync => 3,
+                    SyncStatus::Synced => {
+                        if self.daemon.backend() == DaemonBackend::RemoteBackend {
+                            // Remote backend has no rescan feature. For a synced wallet,
+                            // cache refresh is only used to warn user about recovery availability.
+                            120
+                        } else {
+                            // For the rescan feature, we refresh more often in order
+                            // to give user an up-to-date view of the rescan progress.
+                            10
+                        }
+                    }
                 },
             ))
             .map(|_| Message::Tick),
@@ -267,6 +283,7 @@ impl App {
                 let daemon = self.daemon.clone();
                 let datadir_path = self.cache.datadir_path.clone();
                 let network = self.cache.network;
+                let last_poll_at_startup = self.cache.last_poll_at_startup;
                 Command::perform(
                     async move {
                         // we check every 10 second if the daemon poller is alive
@@ -285,6 +302,7 @@ impl App {
                             rescan_progress: info.rescan_progress,
                             sync_progress: info.sync,
                             last_poll_timestamp: info.last_poll_timestamp,
+                            last_poll_at_startup, // doesn't change
                         })
                     },
                     Message::UpdateCache,

gui/src/app/state/mod.rs

@@ -25,17 +25,14 @@ use super::{
     menu::Menu,
     message::Message,
     view,
-    wallet::{SyncStatus, Wallet},
+    wallet::{sync_status, SyncStatus, Wallet},
 };
 
 pub const HISTORY_EVENT_PAGE_SIZE: u64 = 20;
 
-use crate::{
+use crate::daemon::{
-    daemon::{
+    model::{remaining_sequence, Coin, HistoryTransaction, Labelled},
-        model::{remaining_sequence, Coin, HistoryTransaction, Labelled},
+    Daemon,
-        Daemon, DaemonBackend,
-    },
-    node::NodeType,
 };
 pub use coins::CoinsPanel;
 use label::LabelsEdited;
@@ -76,49 +73,9 @@ pub fn redirect(menu: Menu) -> Command<Message> {
     })
 }
 
-fn sync_status(
-    daemon_backend: DaemonBackend,
-    blockheight: i32,
-    sync_progress: f64,
-    last_poll: Option<u32>,
-    last_poll_at_startup: Option<u32>,
-) -> SyncStatus {
-    if sync_progress < 1.0 {
-        return SyncStatus::BlockchainSync(sync_progress);
-    } else if blockheight <= 0 {
-        // If blockheight <= 0, then this is a newly created wallet.
-        // If user imported descriptor and is using a local bitcoind, a rescan
-        // will need to be performed in order to see past transactions and so the
-        // syncing status could be misleading as it could suggest the rescan is
-        // being performed.
-        // For external daemon or if we otherwise don't know the node type,
-        // treat it the same as bitcoind to be sure we don't mislead the user.
-        if daemon_backend == DaemonBackend::RemoteBackend
-            || daemon_backend == DaemonBackend::EmbeddedLianad(Some(NodeType::Electrum))
-        {
-            return SyncStatus::WalletFullScan;
-        }
-    }
-    // For an existing wallet with any local node type, if the first poll has
-    // not completed, then the wallet has not yet caught up with the tip.
-    // An existing wallet with remote backend remains synced so we can ignore it.
-    // If external daemon, we cannot be sure it will return last poll as it
-    // depends on the version, so assume it won't unless the last poll at
-    // startup is set.
-    // TODO: should we check the daemon version at GUI startup?
-    else if last_poll <= last_poll_at_startup
-        && (daemon_backend.is_embedded()
-            || (daemon_backend == DaemonBackend::ExternalLianad && last_poll_at_startup.is_some()))
-    {
-        return SyncStatus::LatestWalletSync;
-    }
-    SyncStatus::Synced
-}
-
 pub struct Home {
     wallet: Arc<Wallet>,
     sync_status: SyncStatus,
-    last_poll_at_startup: Option<u32>,
     balance: Amount,
     unconfirmed_balance: Amount,
     remaining_sequence: Option<u32>,
@@ -133,14 +90,7 @@ pub struct Home {
 }
 
 impl Home {
-    pub fn new(
+    pub fn new(wallet: Arc<Wallet>, coins: &[Coin], sync_status: SyncStatus) -> Self {
-        wallet: Arc<Wallet>,
-        coins: &[Coin],
-        blockheight: i32,
-        sync_progress: f64,
-        last_poll: Option<u32>,
-        daemon_backend: DaemonBackend,
-    ) -> Self {
         let (balance, unconfirmed_balance) = coins.iter().fold(
             (Amount::from_sat(0), Amount::from_sat(0)),
             |(balance, unconfirmed_balance), coin| {
@@ -154,18 +104,9 @@ impl Home {
             },
         );
 
-        let sync_status = sync_status(
-            daemon_backend,
-            blockheight,
-            sync_progress,
-            last_poll,
-            last_poll,
-        );
-
         Self {
             wallet,
             sync_status,
-            last_poll_at_startup: last_poll,
             balance,
             unconfirmed_balance,
             remaining_sequence: None,
@@ -179,22 +120,6 @@ impl Home {
             processing: false,
         }
     }
-
-    fn sync_status(
-        &self,
-        daemon_backend: DaemonBackend,
-        blockheight: i32,
-        sync_progress: f64,
-        last_poll: Option<u32>,
-    ) -> SyncStatus {
-        sync_status(
-            daemon_backend,
-            blockheight,
-            sync_progress,
-            last_poll,
-            self.last_poll_at_startup,
-        )
-    }
 }
 
 impl State for Home {
@@ -305,11 +230,12 @@ impl State for Home {
             },
             Message::UpdatePanelCache(is_current, Ok(cache)) => {
                 let wallet_was_syncing = !self.sync_status.is_synced();
-                self.sync_status = self.sync_status(
+                self.sync_status = sync_status(
                     daemon.backend(),
                     cache.blockheight,
                     cache.sync_progress,
                     cache.last_poll_timestamp,
+                    cache.last_poll_at_startup,
                 );
                 // If this is the current panel, reload it if wallet is no longer syncing.
                 if is_current && wallet_was_syncing && self.sync_status.is_synced() {

gui/src/app/wallet.rs

@@ -2,7 +2,9 @@ use std::collections::{HashMap, HashSet};
 use std::path::Path;
 use std::sync::Arc;
 
-use crate::{app::settings, hw::HardwareWalletConfig, signer::Signer};
+use crate::{
+    app::settings, daemon::DaemonBackend, hw::HardwareWalletConfig, node::NodeType, signer::Signer,
+};
 
 use liana::{miniscript::bitcoin, signer::HotSigner};
 
@@ -206,3 +208,50 @@ impl SyncStatus {
         self == &SyncStatus::Synced
     }
 }
+
+/// Get the [`SyncStatus`].
+///
+/// The `last_poll_at_startup` is the timestamp of the last poll
+/// of the blockchain when the application was first loaded, while
+/// `last_poll` refers to the most recent poll.
+///
+/// `sync_progress` is the blockchain synchronization progress as
+/// a number between `0.0` and `1.0`.
+pub fn sync_status(
+    daemon_backend: DaemonBackend,
+    blockheight: i32,
+    sync_progress: f64,
+    last_poll: Option<u32>,
+    last_poll_at_startup: Option<u32>,
+) -> SyncStatus {
+    if sync_progress < 1.0 {
+        return SyncStatus::BlockchainSync(sync_progress);
+    } else if blockheight <= 0 {
+        // If blockheight <= 0, then this is a newly created wallet.
+        // If user imported descriptor and is using a local bitcoind, a rescan
+        // will need to be performed in order to see past transactions and so the
+        // syncing status could be misleading as it could suggest the rescan is
+        // being performed.
+        // For external daemon or if we otherwise don't know the node type,
+        // treat it the same as bitcoind to be sure we don't mislead the user.
+        if daemon_backend == DaemonBackend::RemoteBackend
+            || daemon_backend == DaemonBackend::EmbeddedLianad(Some(NodeType::Electrum))
+        {
+            return SyncStatus::WalletFullScan;
+        }
+    }
+    // For an existing wallet with any local node type, if the first poll has
+    // not completed, then the wallet has not yet caught up with the tip.
+    // An existing wallet with remote backend remains synced so we can ignore it.
+    // If external daemon, we cannot be sure it will return last poll as it
+    // depends on the version, so assume it won't unless the last poll at
+    // startup is set.
+    // TODO: should we check the daemon version at GUI startup?
+    else if last_poll <= last_poll_at_startup
+        && (daemon_backend.is_embedded()
+            || (daemon_backend == DaemonBackend::ExternalLianad && last_poll_at_startup.is_some()))
+    {
+        return SyncStatus::LatestWalletSync;
+    }
+    SyncStatus::Synced
+}

gui/src/loader.rs

@@ -407,7 +407,9 @@ pub async fn load_application(
         blockheight: info.block_height,
         coins,
         sync_progress: info.sync,
+        // Both last poll fields start with the same value.
         last_poll_timestamp: info.last_poll_timestamp,
+        last_poll_at_startup: info.last_poll_timestamp,
         ..Default::default()
     };
 

gui/src/main.rs

@@ -462,7 +462,9 @@ pub fn create_app_with_remote_backend(
             sync_progress: 1.0, // Remote backend is always synced
             datadir_path: datadir.clone(),
             blockheight: wallet.tip_height.unwrap_or(0),
-            last_poll_timestamp: None, // We ignore this field for remote backend.
+            // We ignore last poll fields for remote backend.
+            last_poll_timestamp: None,
+            last_poll_at_startup: None,
         },
         Arc::new(
             Wallet::new(wallet.descriptor)