mycelium-com / wallet-android

Mycelium Bitcoin Wallet for Android
http://mycelium.com
Other
665 stars 322 forks source link

When to enable features that require connectivity #505

Closed dscotese closed 5 years ago

dscotese commented 5 years ago

This morning, I was near a Jack in the Box that has spotty WiFi. It was not working, so Mycelium's BTC/USD indicator when you're entering an amount was disabled (well done!) So I disabled my WiFi (on my mobile) and it connected to 4G instead. This enabled the BTC/USD indicator and I proceeded to send some BTC. It used a rate that was way too high (about 2% off) and a fee that was way too low (about 50% off). I believe there's a bug that can be fixed here: The last-update time for fees and rate probably has a timestamp, and the wallet should NOT enable a disabled feature when connectivity returns, but rather wait until that timestamp is updated. Otherwise, it gives the user (me, in this case) a false sense of security regarding exchange rate and fee-selection.

Steps to reproduce:

  1. Connect your mobile to a WiFi network so that Mycelium can pick up an exchange rate and whatever data it needs to suggest fees.
  2. Verify that your wallet will convert from BTC to USD on the Send screen and note that it uses an exchange rate that we will later call "stale".
  3. Disable the Wifi router's connection to the Internet (so that your mobile still thinks it's connected).
  4. Wait until the exchange rate changes significantly. (This is easier if you have a test source for it - then just go change the rate on the test source).
  5. Verify that your wallet will no longer convert from BTC to USD (because it doesn't have a live connection to the exchange source).
  6. Perform this step and the next one quickly: Disable your mobile's WiFi connection so that it uses the cellular data network to connect to the exchange source,
  7. Notice that the BTC indicator on the Send screen is enabled and click it and enter some amount (of USD) and note the amount of BTC it calculates.

In the last step, I believe the calculation will reflect the stale exchange rate instead of the newest one, at least sometimes, and more often if you add cellular band noise so your cellular data is slower and takes longer to collect the data.

Proposed fix: instead of checking that we are connected to the Internet to determine if the converter should be enabled, check the age of the last query to the exchange source. This way, just connecting will not trick the user (as it tricked me). The user will have to wait until the wallet has collected fresh data.

Giszmo commented 5 years ago

Hi. Thanks for reporting.

So, the mining fee estimator is something that usually doesn't fluctuate dramatically in a short time. Here we use a 2h timeout and if the last estimate is older than 2h, we show a warning. Otherwise it's still good. Note here that even if some deliberate "stress test" were to happen now, the fee estimator would not react dramatically within 2h and there is no way to predict that somebody will pay a higher fee than me sustained for weeks, based on a backlog that is enough for 2 blocks now.

For exchange rates there, is also a timeout. It is 5 minutes. Of course, especially if your exchange rate provider uses an exchange with poor liquidity, you might easily get 2% swings in 5 minutes but so will the price fluctuate between hitting send and the transaction confirming. Given the block time is 10 minutes I guess up to 5 minutes old rates should generally be acceptable.

There is also a minimum age of the rate. That is 5 seconds. If you hit refresh too fast, the price will not refresh and given most rate provider apis don't provide us with more frequent updates than once per minute, that is also by far enough.

In summary, most of the time for most people, the price should be pretty accurate with the parameters currently in use and even with much stricter settings, there would be room for fluctuation but with many more people complaining about exchange rate not being available when needed.

If you think we missed something, please feel free to re-open the issue.

dscotese commented 5 years ago

@Giszmo - I don't see how to re-open this.

It makes sense that there is a timeout and a warning, but it seems something isn't working properly. I KNOW I got a stale rate, and I attribute that to the peculiar record of connectivity:

  1. Connected, but Internet unavailable and so the converter is disabled because it knows we need to see the Internet,
  2. Mometarily NOT Connected (because I turned off WiFi)
  3. Connected again through the cellular network.
  4. Internet available so the converter is ENABLED, but it still has a stale price (NOT DETECTED = BUG), it was obvious to me that the converter became enabled too soon after I re-established the availability of the Internet. The bug might only happen when the device detects connection to a network and that network is not connected to the Internet.
Giszmo commented 5 years ago
  1. go to spend -> amount entry
  2. make sure that fiat option is available (USD for example)
  3. switch on airplane mode
  4. kill mycelium
  5. open mycelium and check again the amount entry: USD is available

Rates are only shown based on being stale or not. Not based on internet connectivity.

dscotese commented 5 years ago

@Giszmo I know that my request to convert was performed using a stale price right after I established internet connectivity (by disabling WiFi) because I had to send a second transaction seven minutes later (2019-03-25 19:13:17 GMT) to make up the difference (you can see this by searching for 3HkLKoixKtXez6CqmLoE2i5TcELxMi44ez on a block explorer - they are the only two incoming transactions). I sent over 2% more in the second transaction which means the price difference was at least 2% and the difference between the high and the low for that whole hour was less than 1%. That is how I knew the first conversion used a stale price. The feerate in the larger transaction (which is the one I broadcasted first) was too small and caused that transaction to wait five blocks to get confirmed.

Maybe the code that enables the "fiat option" checks for connectivity instead of freshness of the rate... or maybe a fresh rate allows the fiat option to become enabled before the rate used by that feature has been updated from the fresh rate... or maybe there is a race condition between the UI thread and the thread that reads fresh rates. In any case, something went wrong.

The instructions I provided match what I did, so that may be the only way to reproduce the bug, and even then, it could be rare. I just know that it happened and figured it's something you'd want to fix.