oxen-io / oxen-mobile-wallet

A Mobile Wallet for Oxen
MIT License
28 stars 22 forks source link

oxen mobile wallet sometimes takes extremely long time to sync the last block #31

Open ghost opened 1 year ago

ghost commented 1 year ago

Oxen iOS wallet version 1.10.1 (or any previous versions, or Oxen Android wallet, this is an issue since long ago)

For example, it might show SYNCHRONIZING (1194492/1194492) for entire 2mins, then it receives the next block;

then it shows SYNCHRONIZING (1194493/1194493) for another 2mins;

then it receives the next block, syncing for another 2mins, etc.

As a result, it could spend 30 mins syncing for the "last block". During that syncing time, it is impossible to make any transaction. In other words, isRefreshing() seems always return true.

I tried both wifi network and mobile data network, it happens for both. The pattern is quite random.

I tried "Menu->Reconnect" and it seems not helping. Update: "Menu->Reconnect" seems increase the probability of reproducing the bug.

I'm not the only person with this issue cause I heard other users complaining about the same thing as well.

The desktop wallet works fine.

If anyone gives me some hints I might be able to debug by myself. Thanks. Update: see analysis and potential fix in the following comments

ghost commented 1 year ago

I can reproduce it on both iPhone and Android.

Here are some screenshots from a fresh new Android from browser stack https://app-live.browserstack.com

os: Android os_version: 13.0 device: Google Pixel 7

Screen Shot 2023-01-07 at 9 58 48 pm Screen Shot 2023-01-07 at 9 59 43 pm Screen Shot 2023-01-07 at 10 01 36 pm
ghost commented 1 year ago

Dear team, where can I find the source code of native function is_refreshing?

https://github.com/oxen-io/oxen-mobile-wallet/blob/trunk/oxen_coin/lib/src/native/wallet.dart#L53

final isRefreshingNative = oxenApi
    .lookup<NativeFunction<is_refreshing>>('is_refreshing')
    .asFunction<IsRefreshing>();

Thanks!

ghost commented 1 year ago

Am I right is_refreshing is the same as isRefreshing?

https://github.com/oxen-io/oxen-core/blob/dev/src/wallet/api/wallet2_api.h#L738

    /**
     * @brief refreshing - returns true if a refresh is currently underway; this is done *without*
     * requiring a lock, unlike most other wallet-interacting functions.
     *
     * @param max_wait - the maximum time to try to obtain the refresh thread lock.  If this time
     * expires without acquiring a lock, we return true, otherwise we return false.  Defaults to
     * 50ms; can be set to 0ms to always return immediately.
     *
     * @return - true if the refresh thread is currently active, false otherwise.  If true, very few
     * other methods here will work (i.e. they will block until the refresh finishes).  The most
     * notably non-blocking, thread-safe methods that can be used when this returns true are
     * blockChainHeight and daemonBlockChainHeight.
     */
    virtual bool isRefreshing(std::chrono::milliseconds max_wait = std::chrono::milliseconds{50}) = 0;

I don't understand why the name doesn't match exactly, did I miss some .h files and .def files?

ghost commented 1 year ago

Ah I found it:

https://github.com/oxen-io/oxen-mobile-wallet/blob/trunk/oxen_coin/ios/Classes/oxen_api.cpp#L482

    EXPORT
    bool is_refreshing()
    {
        return get_current_wallet()->isRefreshing();
    }
ghost commented 1 year ago

Updated, v2:

diff --git a/oxen_coin/lib/wallet.dart b/oxen_coin/lib/wallet.dart
index 164855c..142b09b 100644
--- a/oxen_coin/lib/wallet.dart
+++ b/oxen_coin/lib/wallet.dart
@@ -123,6 +123,7 @@ class SyncListener {
   int _cachedBlockchainHeight = 0;
   int _lastKnownBlockHeight = 0;
   int _initialSyncHeight = 0;
+  bool _pendingNewBlocks = false;

   Future<int> getNodeHeightOrUpdate(int baseHeight) async {
     if (_cachedBlockchainHeight < baseHeight || _cachedBlockchainHeight == 0) {
@@ -136,6 +137,7 @@ class SyncListener {
     _cachedBlockchainHeight = 0;
     _lastKnownBlockHeight = 0;
     _initialSyncHeight = 0;
+    _pendingNewBlocks = false;
     _updateSyncInfoTimer ??=
         Timer.periodic(Duration(milliseconds: 1200), (_) async {
       // var syncHeight = getSyncingHeight();
@@ -153,6 +155,11 @@ class SyncListener {
       final bchHeight = await getNodeHeightOrUpdate(syncHeight);

       if (_lastKnownBlockHeight == syncHeight) {
+        final refreshing = isRefreshing();
+        if (_pendingNewBlocks && !refreshing) {
+          onNewBlock.call(syncHeight, bchHeight, refreshing);
+          _pendingNewBlocks = false;
+        }
         return;
       }

@@ -171,6 +178,9 @@ class SyncListener {

       // 1. Actual new height; 2. Blocks left to finish; 3. Progress in percents;
       onNewBlock.call(syncHeight, bchHeight, refreshing);
+      if (refreshing) {
+        _pendingNewBlocks = true;
+      }
     });
   }

I believe this fix the issue for me, but I'm not familiar with the entire code base and I'm not a dart developer, please carefully review my change @jagerman @ryanzhao before merging.

BTW this comment is outdated

 // 1. Actual new height; 2. Blocks left to finish; 3. Progress in percents;
ghost commented 1 year ago

I make up a theory to explain the issue and the fix:

Backgrounds

Oxen mobile dart wallet syncs to Oxen core every 1.2 seconds

oxen_coin/lib/wallet.dart:        Timer.periodic(Duration(milliseconds: 1200), (_) async {

By default, Oxen core refreshes every 10 seconds

src/wallet/api/wallet.cpp:    static const int    DEFAULT_REFRESH_INTERVAL_MILLIS = 1000 * 10;
src/wallet/api/wallet.cpp:    static const int    DEFAULT_REMOTE_NODE_REFRESH_INTERVAL_MILLIS = 1000 * 10;

Oxen decentralized network generates a new block every 120 seconds (2 minutes)

Note that 120 seconds is a perfect multiple of 10 seconds, as well as a perfect multiple of 1.2 seconds, this combination will create some interesting effects which we will see soon.

Code analysis

Original code without patching:

135   void start() {
136     _cachedBlockchainHeight = 0;
137     _lastKnownBlockHeight = 0;
138     _initialSyncHeight = 0;
139     _updateSyncInfoTimer ??=
140         Timer.periodic(Duration(milliseconds: 1200), (_) async {
141       // var syncHeight = getSyncingHeight();
142       //
143       // if (syncHeight <= 0) {
144       //   syncHeight = getCurrentHeight();
145       // }
146
147       final syncHeight = getCurrentHeight();
148
149       if (_initialSyncHeight <= 0) {
150         _initialSyncHeight = syncHeight;
151       }
152
153       final bchHeight = await getNodeHeightOrUpdate(syncHeight);
154
155       if (_lastKnownBlockHeight == syncHeight) {
156         return;
157       }
158
159       _lastKnownBlockHeight = syncHeight;
160
161       if (syncHeight < 0 || bchHeight < syncHeight) {
162         return;
163       }
164
165       final refreshing = isRefreshing();
166       if (!refreshing) {
167         if (isNewTransactionExist()) {
168           onNewTransaction.call();
169         }
170       }
171
172       // 1. Actual new height; 2. Blocks left to finish; 3. Progress in percents;
173       onNewBlock.call(syncHeight, bchHeight, refreshing);
174     });
175   }

Sometimes, soon after Oxen mobile dart wallet detects a new update from Oxen core, it is possible that Oxen core returns true for the following isRefreshing() call, which is unlucky.

If we are unlucky enough, we might end up calling onNewBlock.call(syncHeight, bchHeight, refreshing); with refreshing == true again and again, because:

  1. As the function name suggests, we don't call isRefreshing() and onNewBlock.call() unless the Oxen mobile dart wallet detects a remote block height change. (Notice the return statement in line 156 and line 162)

  2. If Oxen decentralized network updates precisely every 120 seconds, then a remote block height change is always fetched by Oxen core 120 secs after the previous fetch, almost precisely. During this 120 secs, Oxen core refreshes 12 times (120/10=12), 11 of them do not fetch any new blocks, 1 of them get a new block.

  3. Every Oxen core update is further detected by Oxen mobile dart wallet every 120 secs, almost precisely. During this 120 secs, Oxen mobile dart wallet syncs with Oxen core 100 times (120/1.2=100), however 99 of syncs do not detect any new block, only 1 of them triggers a isRefreshing() call. As a result, every isRefreshing() is also called almost precisely 120 secs after the previous isRefreshing() call.

To summarize, since the on chain new block generation time is 120 seconds, which coincidently is a common multiple of Oxen core refreshing interval (10 seconds) and Oxen mobile dart wallet syncing interval (1.2 seconds), as a result the isRefreshing() call and the actual Oxen core refresh always fit into the same time pattern. If we are so unlucky that a isRefreshing() query happens at the same time when Oxen core is refreshing, then the next isRefreshing() query is almost guaranteed to be called simultaneously with another Oxen core refreshing.

In other words, if isRefreshing() returns true once, it might returns true forever, which means we might fall into the loop of calling onNewBlock.call() with refreshing == true forever. This also means the mobile wallet might show SYNCHRONIZING (<latest block id>/<latest block id>) forever and it is impossible for a user to make any transaction in this state.

In reality, new block generation time on chain is not always exactly 120 secs, and human interaction with mobile app might also change the syncing point, as a result one might still be able to escape from the endless loop after enough waiting and random interactions.

(I'm struggling to explain the pattern clearer, might need some diagram or some animation. Someone can give a hand? Please let me know if you have any questions)

Fix

As a solution, I introduce the _pendingNewBlocks variable to capture this unlucky edge case and insert a separate onNewBlock.call() in the later dart wallet syncing cycles to save this unlucky case. Does it make sense to you @jagerman @ryanzhao?

jagerman commented 1 year ago

This has been an issue for a while, thanks for diving into this!

I can see how that would fix the issue, but I wonder if we couldn't make a "better" fix for this by having the "refreshing" status triggered separately from onNewBlock.

I've got some other things on my plate for this week but I'll see if I can look into this in more detail next week.

ghost commented 1 year ago

Thanks, this issue seems to get worse in recent months, I wonder if the on-chain block generation interval becomes more "accurate" recently?

I'm surprised no one else reported this bug before, maybe I'm one of the few only "real" users of Oxen as a currency. With this bug Oxen as a currency has become useless to someone who can not afford to buy a laptop, it has a real impact on someone's business and life, more than most people living in first-world countries can imagine, please reserve some priority for it, thanks very much!

jagerman commented 1 year ago

I'm surprised no one else reported this bug before

I've definitely noticed it before.

With this bug Oxen as a currency has become useless

Is this preventing use of the wallet? It's been fairly low priority because, as far as I can see, it's mainly a UI glitch rather than a functional one (i.e. you should still be able to send/receive/etc. perfectly well even if it still in the "synchronizing" status).

ghost commented 1 year ago

Is this preventing use of the wallet? It's been fairly low priority because, as far as I can see, it's mainly a UI glitch rather than a functional one (i.e. you should still be able to send/receive/etc. perfectly well even if it still in the "synchronizing" status).

It is preventing the use of the wallet for me and my team members. (I'm the only one capable to describe a bug in English.) I'm surprised you said sending is still working in the "synchronizing" status.

When the wallet is in "synchronizing" status, I'm not able to send because no matter how hard I try swiping the "Send Oxen" slider button right, it will bounce back, the "Send Oxen" slider button never worked for me in "synchronizing" status.

Now you said sending should be working, which becomes more confusing to me, I wonder if there are some separate subtile UI glitches preventing me from using the "Send Oxen" slider button. I'll try to attach a video soon.

This also brings up another complaint: the "Send Oxen" slider button is visually cool but unfortunately a bad experience for me, it doesn't give me the feeling of "confirmation" when swiping right. Every time it bounces back, I always doubt if I didn't swipe hard enough and keep trying again and again.

ghost commented 1 year ago

As this video shows, the wallet is in "synchronizing" status, all input transactions have enough confirmation blocks, but the "Send Oxen" slider button doesn't work, I tried 5 times and gave up.

Another poor UI experience to complain is, when finishing typing the address and amount, there's no obvious way to view the "Send Oxen" slider button because it's hidden by the soft keyboard, the user needs to carefully tap the "Your wallet" area to hide the soft keyboard and show the "Send Oxen" slider button.

https://user-images.githubusercontent.com/194342/212152581-fd4bd415-3876-4e0a-bf03-7eca336ea3f4.mp4

ghost commented 1 year ago

ping @jagerman

ghost commented 1 year ago

ping @jagerman

I saw the amazing progressed made in the libsession-util library, hopefully you are eventually freed a little bit to take a look at this one again! This is a serious one, thanks!

ghost commented 1 year ago

If it takes too long to implement a perfect solution, maybe a quick workaround should be considered. The mobile wallet is very hard to use and very confusing to new users with this bug. Please, thanks!

ghost commented 1 year ago

ping

Other users in the Oxen Community complains about this bug as well, it's not just me.

@jagerman is it possible to assign the mobile wallet bug to someone else in the team if you can't free yourself from other projects? or would you want me to proposal a better patch following your idea in https://github.com/oxen-io/oxen-mobile-wallet/issues/31#issuecomment-1379030496?

ghost commented 1 year ago

ping @jagerman

There seems to be an important security fix in oxen-core https://github.com/oxen-io/oxen-core/pull/1643

If we are going to update oxen-core, is that a good opportunity to also review this GUI issue again?

ghost commented 1 year ago

ping @jagerman @KeeJef

I have a working patch in https://github.com/oxen-io/oxen-mobile-wallet/issues/31#issuecomment-1374581463 about 7 months ago and I'm still hoping for getting this fixed.

The Oxen core is working properly, it's just the UI part not functioning, and a potential fixed is given, begging for attentions...

jagerman commented 11 months ago

Sorry for this taking so long; I'll see about getting this patch added this week.

jagerman commented 10 months ago

I spent some time last week on this, but ran into Dart/flutter dependency hell (as in #35) that is going to take a bit more time/effort to sort out.