Start pricenode providers asynchronously

[ghost]

Fixes #4441

This guarantees that the pricenode will run even when there is a problem with a provider at startup. It simply calls the initial refresh() asynchronously rather than blocking the bean startup routine. To my point of view there is no difference between the first and subsequent calls to refresh, since providers can fail at any time. When rates are requested any non-initialized provider is ignored.

However I have not taken ownership of the pricenode and only have a superficial understanding of how the data is calculated. I did enough testing to check that the price and fee data looks good, and that the automated tests pass. There could certainly be edge cases that I'm not aware of. @cd2357 is certainly free to flame me if this is a stupid change, as he put a lot more work into it than I did.

[wiz]

Thanks for working on this. I think the main concern is what price do you return if there is no data for a given asset? In that case I assume you don't return anything, or a null value, right?

[ghost]

If there is no data for a given asset, that asset would not be present in the JSON data.

For example, running with only two providers: Kraken and Luno, the rate for ZAR is not present in the JSON data. ZAR is provided by Luno (not Kraken), but Luno service is down. If the Binance provider is included, the ZAR rate is present.

[ripcurlx]

@cd2357 Could you please give this code changes a quick ACK?

[cd2357]

My main concern is this:

• let's say a pricenode cannot connect to one or more exchanges, but still starts up
  • this means that, worst case, it won't provide a price feed for some "unlucky" currencies / assets, which were only available on the currently-not-reachable exchanges
• a Bisq node connects to this pricenode for price data
• if this node wants to start a trade (or has any ongoing trade) in any of the above "unlucky" currencies, this could result in failed trades, or the inability to create/accept new trades. Also, not sure what other side-effects this would have for clients in general (NPEs, errors in UI or trade protocol, etc).

This is now avoided by the simple fact that a pricenode always provides a price for all known / expected currencies :

• If an exchange is not reachable during pricenode startup, then the startup fails until that exchange is reachable again (not ideal, I know, but at least protects Bisq nodes from the scenario above).
• If a pricenode starts with all exchanges reachable, but some exchange becomes unreachable later on, the scenario above is still not an issue, because that pricenode will serve the "cached" (or last known) price for the affected currencies or assets -- until the problematic exchange comes back online again. Actually, I think the pricenode will provide an aggregated price for the affected currencies (if the aggregate price is an average of 3 prices from 3 exchanges, and one exchange goes offline, then the pricenode will serve an aggregate price based on the current data from the 2 working exchanges + the "cached" latest known price from the 3rd exchange, until that exchange comes back online).

I admit this is mainly a theoretical concern (I didn't test it out and see errors), but it seems reasonable that these risks are possible. Most "at risk" currencies are those with a small number of "inputs" in the aggregate price, like those depending on 1-2-3 exchanges: https://bisq.wiki/Bisq_Price_Indices . For example, lots of fiat currencies have only 1 provider (BitPay).

How do you see this?

Is this an acceptable risk, or maybe am I over-estimating it?

If not, how could it be mitigated? One possible solution that comes to mind is that pricenodes periodically persist to disk the latest-known prices for all supported currencies, then if they are restarted and some exchanges are temporarily unreachable, they could report the "cached" price for those currencies until the faulty exchanges come back online. (This cache approach is already in place, but the prices are cached in-memory, not persisted on-disk.)

cc @bisq-network/pricenode-operators

[ghost]

Good points. When rates are not available Bisq displays the price as of the last trade:

If you try and take an offer this message prevents it:

Likewise making an offer it forces you to enter a fixed price:

Or of you had an already published market rate offer; your price feed is down and someone tries to take it they get this:

[cd2357]

What happens if

• user creates market rate offer (when pricefeed for that currency is available)
• user disables offer
• pricefeed for that currency becomes unavailable (user restarts Bisq, chooses a pricenode without that feed)
• user re-enables offer

Does re-enabling the offer fail graciously?

Also, if shortly after that the pricefeed becomes available again (Bisq client didn't restart, kept using same pricenode, but not pricenode starts providing the needed pricefeed) -- will the client detect that and allow the user to re-enable the offer / create new offers in that currency?

[ghost]

Per your first question, re-enabling the offer does not fail, you can disable/enable at will. The status of the pricefeed is not taken into account. The trading protocol would prevent a market based offer from being taken due to the price feed being N/A.

Per your second question, offers can still be created when the pricefeed is down. If the pricefeed fails while Bisq is running, the user can still create market based offers. If Bisq starts up with no pricefeed available the user is only allowed to create fixed price offers.

[cd2357]

In that case, it sounds good from my side. Thanks for the detailed answers @jmacxx.

@wiz do you want to coordinate more extensive tests on this first? Or is this fine and can be merged?

[ripcurlx]

As pricenode providers are running from specific master versions anyways I'm not merging this for v1.5.2. Waiting for testing by @wiz.

[ripcurlx]

@wiz ☝️

[ghost]

@wiz did this get installed to production? It seemed like a high priority when you were complaining about it asking for it to be fixed.

[wiz]

@jmacxx sorry for the late reply, yes it was a high priority because it can cause pricenodes to completely fail to startup. I apologize I haven't had time to properly ACK the PR but IIRC it's running on my pricenode server for a while now. @Emzy would you mind testing/reviewing this PR ?

[Emzy]

I have this PR as a test running on my pricenode "emzypricpidesmyqg2hc6dkwitqzaxrqnpkdg3ae2wef5znncu2ambqd.onion"

[ripcurlx]

@Emzy @wiz As soon as I have an ACK from either of you I'll move forward and merge this PR.

[Emzy]

ACK
No issues on my production pricenode.

[ripcurlx]

utACK - based on #4890 (comment)