[bisq-network/bisq] [WIP] PriceNode: Add support for multiple ExchangeRateProviders (#4315)

cd2357 notifications at github.com
Tue Jul 21 17:39:19 UTC 2020


> There has been some discussion in the project issue ([bisq-network/projects#35](https://github.com/bisq-network/projects/issues/35)) about how this change will impact the the way we tolerance differences between prices on the maker and taker side. ("allowance" as @sqrrm called it). Is this something you gave further thought to, @cd2357?
> 
> In any case, I wouldn't want to see this change merged and rolled out without a real live testing effort. We really need to try to break this before rolling it out. The changes look reasonable from what little time I've given to reviewing them, but there's no substitute for a real test plan. Could you lay out how you want to do that?

Good points.

I put some thought into how this PR can be tested, here is what I came up with:

|  | Track 1:<br/><br/>New Pricenode is compatible with existing Bisq clients | Track 2:<br/><br/>Pricenode price feeds are reliable / usable in Bisq trades |
| -- | -- | -- |
| **Context** | Bisq clients query the `/getAllMarketPrices` API on the pricenodes (implemented by `ExchangeRateController.getAllMarketPrices()`) to retrieve the currency exchange rates.<br/><br/>For the current Bisq clients to be compatible with the pricenode code in this PR, the new pricenode should use the same data structure as current pricenodes when returning price data. | `Offer.PRICE_TOLERANCE` defines the maximum acceptable price difference between the maker and taker in a trade to be 1%. If maker/taker local prices differ by more than that, the trade will throw an exception and will not be created. |
| **Reason why this matters** | This PR is only usable if existing Bisq clients in the wild can use them as pricenodes. | Different pricenode instances running the PR code could face different circumstances that can influence their price feeds. For example, intermittent connectivity to one or more of the exchanges, etc. If the maker and taker use pricenodes that return prices which are more than 1% apart, trades in Bisq cannot be created. |
| **Testing: General approach** | Start Bisq clients that use pricenodes based on this PR. If prices are successfully retrieved, test is successful.<br/><br/>Perform a few regtest trades in different configurations. If trades successful, then test is considered successful. | Check prices reported by different pricenodes, at different times, under different conditions. If prices are within <1% of each other, test is successful. |
| **Testing: Detailed steps** | TBD<br/><br/>Start a regtest trade, where maker/taker use:<br/>- the same pricenode (e.g. same instance of a Pricenode-PR-1m)<br/>- each a different pricenode of same type (e.g. two instances of Pricenode-PR-1m)<br/>- each a different pricenode of different types (e.g. one Pricenode-PR-1m and one Pricenode-PR-3m), to simulate occasional connectivity issues between some pricenodes and some exchanges)<br/>- one using a normal pricenode (e.g. a Pricenode-PR-1m), the other using an artificial pricenode with an artificially inflated price (e.g. a Pricenode-PR-1m-up5pc) to simulate that trades are indeed prevented if taker/maker prices are more than 1% apart; scenario should not happen but could occur during fast/huge rapid price moves; if pricenodes poll the exchanges at different times, they get different prices; even if they poll 30s apart from each other, the price could theoretically have moved more than 1% in that timeframe | TBD<br/><br/>Suggestion 1: Manually run a script that compares prices across different “test pricenodes” running the PR code. If script output indicates price differences of <1%, then test is successful. (See below for sample output of this script. If approach accepted, script will be shared, TBD where/how)<br/><br/>Suggestion 2: Create a monitoring script that retrieves prices from relevant pricenodes, automatically, regulary, over a predefined period of time + logs or indicates if/when/which prices from which pricenodes differed more than 1% (Perhaps by a modified version of the `PriceNodeStats` metrics monitor? TBD) |

Suggested types of pricenode instances (for tests) :

|  | Pricenode-PR-1m | Pricenode-PR-3m | Pricenode-PR-1m-up5pc |
| --- | --- | --- | --- |
| **Description / Purpose**<br /><br />(Why is such a pricenode useful for tests? What does it simulate?) | Simulates a vanilla pricenode, based on this PR | Simulates a pricenode based on this PR, but which has intermittent connectivity to some/all of the exchanges. The higher polling rate simulates that exchanges are only occasionally polled successfuly to retrieve fresh price data | Simulates a pricenode based on this PR, but which returns artificially inflated rates (+5%). Can be used to test how a maker/taker setup behaves, should they have massively different prices for the traded currency. Since this situation is not expected to occur (and does not occur normally), this type of pricenode helps to simulate it. |
| **Polling rate**<br /><br />(How often each exchange provider will poll the exchange) | 1 min | 3 mins | 1 min |
| **Modifications**<br /><br />(Adjustments on top of the PR code, to make the instance suitable for tests) | Disable BitcoinAverage | Disable BitcoinAverage | Disable BitcoinAverage<br /><br />Artificially inflate returned rates by +5% |
| **Branch**<br /><br />(Test branch containing the modifications mentioned above) | [pricenode-test-setup-1m](https://github.com/cd2357/bisq/tree/pricenode-test-setup-1m/) | [pricenode-test-setup-3m](https://github.com/cd2357/bisq/tree/pricenode-test-setup-3m/) | [pricenode-test-setup-1m-up5pc](https://github.com/cd2357/bisq/tree/pricenode-test-setup-1m-up5pc/) |
| **How to install**<br /><br />(For testers who want to setup their own instance. Auto-install script tested with debian 10.) | <details><summary>Show...</summary>`curl -s https://raw.githubusercontent.com/cd2357/bisq/pricenode-test-setup-1m/pricenode/install_pricenode_debian.sh \| sudo bash`</details> | <details><summary>Show...</summary>`curl -s https://raw.githubusercontent.com/cd2357/bisq/pricenode-test-setup-3m/pricenode/install_pricenode_debian.sh \| sudo bash`</details> | <details><summary>Show...</summary>`curl -s https://raw.githubusercontent.com/cd2357/bisq/pricenode-test-setup-1m-up5pc/pricenode/install_pricenode_debian.sh \| sudo bash`</details> |
| **Current live test instances**<br /><br />(Existing live instances, for test purposes. Warning: no uptime guarantees, best-effort availability) | `a4l35kmpdg5dydbl.onion` (Pricenode-PR-1m-A)<br /><br />`am53iepqny57izih.onion` (Pricenode-PR-1m-B) | `ylb4chzvvx466cms.onion` (Pricenode-PR-3m-A)<br/><br/>`kjuslac3rq4qxkml.onion` (Pricenode-PR-3m-B) | `rk4i2adtkmukoh4j.onion` (Pricenode-PR-1m-up5%) |

Track 2 / Manual script sample output (executed against test instances mentioned above) : 

(The script output indicates that, at the moment when it was executed, the pricenode instances reported prices within less than 1% of each other, as expected).

```
-----------------------------------------------------------------------------------------------------------------------------------------
Code   |Pricenode-PR-1m-A|          Pricenode-PR-1m-B|          Pricenode-PR-3m-A|          Pricenode-PR-3m-B|       Pricenode-PR-1m-up5%
-----------------------------------------------------------------------------------------------------------------------------------------
"AUD"  |   13220.90000000|   13220.90000000 ( +0.00%)|   13220.90000000 ( +0.00%)|   13220.90000000 ( +0.00%)|   13881.94500000 ( +5.00%)
"BEAM" |       0.00004160|       0.00004160 ( +0.00%)|       0.00004170 ( +0.24%)|       0.00004160 ( +0.00%)|       0.00004368 ( +5.00%)
"CAD"  |   12569.90000000|   12569.90000000 ( +0.00%)|   12569.90000000 ( +0.00%)|   12569.90000000 ( +0.00%)|   13198.39500000 ( +5.00%)
"CHF"  |    8756.90000000|    8756.90000000 ( +0.00%)|    8756.90000000 ( +0.00%)|    8756.90000000 ( +0.00%)|    9194.74500000 ( +5.00%)
"DAI"  |       0.00010821|       0.00010821 ( +0.00%)|       0.00010821 ( +0.00%)|       0.00010821 ( +0.00%)|       0.00011362 ( +5.00%)
"DASH" |       0.00760342|       0.00760442 ( +0.01%)|       0.00760442 ( +0.01%)|       0.00760442 ( +0.01%)|       0.00798359 ( +5.00%)
"DCR"  |       0.00164523|       0.00164973 ( +0.27%)|       0.00164873 ( +0.21%)|       0.00164973 ( +0.27%)|       0.00173222 ( +5.29%)
"DOGE" |       0.00000035|       0.00000035 ( +0.00%)|       0.00000035 ( +0.00%)|       0.00000035 ( +0.00%)|       0.00000036 ( +5.00%)
"ETC"  |       0.00065952|       0.00065957 ( +0.01%)|       0.00065755 ( -0.30%)|       0.00065803 ( -0.23%)|       0.00069250 ( +5.00%)
"ETH"  |       0.02607944|       0.02607675 ( -0.01%)|       0.02607875 ( -0.00%)|       0.02607800 ( -0.01%)|       0.02738112 ( +4.99%)
"EUR"  |    8152.69000000|    8152.69000000 ( +0.00%)|    8148.59666667 ( -0.05%)|    8154.45666667 ( +0.02%)|    8560.28950000 ( +5.00%)
"GBP"  |    7356.59279286|    7356.59279286 ( +0.00%)|    7354.67666667 ( -0.03%)|    7356.59279286 ( +0.00%)|    7724.42243251 ( +5.00%)
"JPY"  | 1000805.50000000| 1000805.50000000 ( +0.00%)| 1000580.50000000 ( -0.02%)| 1001057.51825581 ( +0.03%)| 1050845.77500000 ( +5.00%)
"LTC"  |       0.00468795|       0.00468764 ( -0.01%)|       0.00468661 ( -0.03%)|       0.00468764 ( -0.01%)|       0.00492228 ( +5.00%)
"NAV"  |       0.00001379|       0.00001379 ( +0.00%)|       0.00001379 ( +0.00%)|       0.00001379 ( +0.00%)|       0.00001448 ( +5.00%)
"NGN"  | 4297561.00000000| 4300694.00000000 ( +0.07%)| 4294770.00000000 ( -0.06%)| 4300694.00000000 ( +0.07%)| 4512439.05000000 ( +5.00%)
"PIVX" |       0.00004865|       0.00004865 ( +0.00%)|       0.00004880 ( +0.31%)|       0.00004865 ( +0.00%)|       0.00005108 ( +5.00%)
"RUB"  |  661466.00000000|  661466.00000000 ( +0.00%)|  662185.00000000 ( +0.11%)|  661466.00000000 ( +0.00%)|  694539.30000000 ( +5.00%)
"TRY"  |   64391.00000000|   64391.00000000 ( +0.00%)|   64452.00000000 ( +0.09%)|   64391.00000000 ( +0.00%)|   67610.55000000 ( +5.00%)
"UAH"  |  260390.00000000|  260390.00000000 ( +0.00%)|  260390.00000000 ( +0.00%)|  260390.00000000 ( +0.00%)|  273409.50000000 ( +5.00%)
"USD"  |    9355.35000000|    9353.30000000 ( -0.02%)|    9351.55000000 ( -0.04%)|    9355.95000000 ( +0.01%)|    9822.17250000 ( +4.99%)
"XMR"  |       0.00758023|       0.00758347 ( +0.04%)|       0.00758597 ( +0.08%)|       0.00758397 ( +0.05%)|       0.00796107 ( +5.02%)
"XZC"  |       0.00061690|       0.00061690 ( +0.00%)|       0.00061390 ( -0.49%)|       0.00061690 ( +0.00%)|       0.00064775 ( +5.00%)
"ZAR"  |  167000.00000000|  167000.00000000 ( +0.00%)|  167000.00000000 ( +0.00%)|  167000.00000000 ( +0.00%)|  175350.00000000 ( +5.00%)
"ZEC"  |       0.00659196|       0.00659246 ( +0.01%)|       0.00657860 ( -0.20%)|       0.00659196 ( +0.00%)|       0.00692156 ( +5.00%)
"ZEN"  |       0.00086500|       0.00086490 ( -0.01%)|       0.00086880 ( +0.44%)|       0.00086490 ( -0.01%)|       0.00090825 ( +5.00%)
```

----
To summarize:
* I found two things that should be tested (testing tracks 1 and 2 above) :
  * whether the new response of the `/getAllMarketPrices/` pricenode API is compatible with current Bisq clients, and
  * whether the new pricenodes have relatively stable price outputs across different instances (reported rates are within <1%, even if some instances may face intermittent connectivity issues to some exchanges, or poll at slightly different times than other pricenode instances)

For testing track 1, a few regtest scenarios are probably good enough to confirm things work as expected.

For testing track 2, a script can query different pricenode instances to help the tester visually confirm that, at the point of running the script, prices are within <1%. This should confirm that the computed exchange rates across pricenode instances are stable enough, and that the Bisq "price delta allowance" / "price tolerance" is not violated. A sample output of the script is included above. A few sample "Pricenode-PR" instances (test pricenodes based on this PR) are also provided, for test purposes.

@cbeams : Is this a generally good direction for the tests?

If yes, I can flesh out some of the TBDs above and prepare more detailed step-by-step test instructions. I can also provide the price comparison script I used for track 2.

If no, what am I missing? Are there other considerations, in addition to testing tracks 1 and 2 above? Thanks.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/4315#issuecomment-662005207
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200721/71aec8e1/attachment-0001.html>


More information about the bisq-github mailing list