-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PriceNode: Add support for multiple ExchangeRateProviders #4315
PriceNode: Add support for multiple ExchangeRateProviders #4315
Conversation
92bc006
to
09845a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NACK. I just had a few minutes to run through these changes right now, and so have added some mostly style-related change requests. I'd like to actually run and test everything myself as well, but will have to get to that later.
pricenode/src/main/java/bisq/price/spot/ExchangeRateProvider.java
Outdated
Show resolved
Hide resolved
pricenode/src/main/java/bisq/price/spot/ExchangeRateProvider.java
Outdated
Show resolved
Hide resolved
pricenode/src/main/java/bisq/price/spot/ExchangeRateProvider.java
Outdated
Show resolved
Hide resolved
pricenode/src/main/java/bisq/price/spot/ExchangeRateService.java
Outdated
Show resolved
Hide resolved
pricenode/src/main/java/bisq/price/spot/providers/Poloniex.java
Outdated
Show resolved
Hide resolved
Nit: It's probably better to mark a PR as a draft using GitHub's built-in support than to prefix it with [WIP]. Perhaps the @bisq-network/bisq-maintainers have something to say about that as well. |
There has been some discussion in the project issue (bisq-network/projects#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? |
306bca7
to
38680ca
Compare
Update sanity check methods to allow for deeper and more comprehensive validations of the input data. Accept full ExchangeRateProviders in the method signatures, instead of just the provider prefix, to allow for more complex sanity checks within those validation methods.
Add support for aggregate rates in the ExchangeRateService. If multiple ExchangeRateProviders contain rates for the same currency, then these rates will be automatically aggregated (averaged) into one. This allows the service to transparently scale to multiple providers for any specific currency. The clients index the rates received from the pricenode by currency code, which means they expect at most a single rate per currency. By aggregating rates from multiple providers into one per currency, the ExchangeRateService provides more accurate price data. At the same time, the service API data structure remains intact, thus preserving backward compatibility with all clients.
Add support for a few exchanges to demonstrate and test the pricenode aggregate rates. The chose exchanges were selected because they each provide a varied list of fiat and altcoins, with a substantial overlap between them. This provides a robust initial set of datapoints and scenarios for aggregate rates.
Revert from latest v5.0.0 to v4.2.2, since the newer version libraries are compiled with Java 11, so they cannot be used as part of the Bisq build process which still partially relies on Java 10.
Update comments to reflect bisq-network/style#5 guideline
Update the name of exception variables to ex for consistency and better readability.
Simplify if-else block to avoid redundant use of else-if in combination with an empty check and a return statement.
Remove Order annotation from rate providers, which was used in the case that multiple providers would retrieve rates for the same currency. The ExchangeRateService now handles such scenarios, thus eliminating the need for deciding provider precedence via the Order annotation.
Remove public modifier in their class definitions to preserve their package-private scope.
Give a more accurate name to the abstract test class which contains common methods used by all ExchangeRateProvider tests, like BinanceTest or KrakenTest. Mark this test class as abstract, to indicate that it should not be run as a standalone test.
Retrieve the exchange rates in bulk, when possible. This reduces the number of calls the pricenode makes to the exchange API from N = "number of exchange rates to retrieve" to N = 1. The replaced approach, which made a separate call to the exchange API for each exchange rate, was sometimes failing due to reaching API rate limits.
Reuse sets of supported currencies between pricenode classes and tests.
38680ca
to
7fc5191
Compare
Good points. I put some thought into how this PR can be tested, see the updated PR description above. To summarize the key points:
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. (Update 22.07.2020: Updated onion addresses of test live instances + added track 2 script content) (Update 23.07.2020: Consolidated relevant tables / scripts and moved them to PR description above. Also added specific tests.) |
Disclaimer: I've taken about 15 minutes to go through everything you've posted above. I haven't been hands on with it, and I haven't scrutinized it closely enough to really poke holes in it, but I wanted to get back sooner than later. With that said, in general, the thinking and approach looks great; I'm impressed by the amount of effort you've put into it already. One question I don't believe you addressed above: what actually happens in the failure mode of the price tolerance being out of range, i.e. over 1% difference? How do users know what went wrong, and how, if at all, can they recover? The answer to this question shouldn't be much different than the status quo, i.e. before applying this patch, but it's just a failure mode I've never personally seen. Do we give people good error messages, etc? It could be argued that that's out of scope for this change, but since this change considerably increases the likelihood of this failure mode occurring, we should make sure the failure is a graceful as it can be. So I don't have much more detailed feedback, other than that this seems directionally correct. Ideally, you'd find someone to pair up with this on and really complete the testing together in real-time, as opposed to just asking someone to review the plan. Two sets of eyes / minds would probably really help in a situation like this (and would probably be more fun less tedious than doing it alone). Any volunteers, @bisq-network/bisq-devs? Maybe pairing up on this is something you could bring up on the next dev call (I haven't been tracking the plan for those calls, just an idea). Another note would be that once whatever diligence is done on the testing front and it seems reasonable to roll this out, I'd suggest having everyone involved (pricenode operators, bisq maintainers) primed and ready to roll back as soon as we start seeing something unexpected. For example, make sure the changes in this PR are as easy to revert as possible (multiple commits are fine—you can revert a whole merge, just want to make sure it doesn't have any extra stuff that might make the revert complicated). And make sure that the pricenode operators know that this upgrade is a relatively risky one and that we might call for a rollback (or upgrade to the new, reverted version) in short order. I hope that helps, I realize it's all pretty high level and perhaps obvious, but that's about as much time as I have to spend on this for now. |
@cbeams thanks for the feedback.
Good point, I hadn't specifically considered it, but I will add a test which ensures this edge case fails gracefully. Technically, any quick and massive price movement of any currency would cause this (for example, a 3% move within 1-2 minutes). Since pricenodes don't poll the exchangs at the exact same times, but perhaps a few seconds apart from each other, in such a case different pricenodes could get considerably different inputs from exchanges and therefore report rates to Bisq clients which may be "out of range" (more than 1% apart across different instances). However, after 1-2 minutes, when all pricenodes get a chance to poll the exchanges again and update their rates, they would then all be at the relatively same level again, reporting rates "within range". About testing, I'll write out the specific steps of the tests and ask for tester volunteers on keybase. About rolling out, you're right, I'll coordinate with @wiz to make sure this is done safely (and there's a contingency rollback plan, in case smth goes wrong). Thanks again for the input. It is a tedious PR, but hopefully the effort will be worth it. |
Removed |
Ran all tests (0-4) in a local regtest setup, all successful. Will coordinate with other volunteers to see if tests also work fine for them (or if the steps are simple and clear enough). |
Thanks for your hard work on this PR. I can't wait to start reviewing it. My first thoughts are:
|
Thanks for the efforts on this PR. Rolling this out will need caution, but the main risks are usability. That could probably be helped by making sure the client handles faults better, as has been discussed. I also have concerns regarding the loss of currencies. In particular the latin american ones seem to be missing. We should make an effort to not reduce usability in any market. Getting any market started is really tedious work and we should make sure to not lose any of that. I haven't read the code yet, but could we run this in parallel with BA for those currencies we don't yet have our own aggregation for? |
ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good to me.
Requiring Java 11 is likely not a problem as we're never shipping this, all price node operators build from source. I can't see any other issues with that either, but still worth keeping in mind.
There are still heaps of codacy complaints. Some seem legit, some are probably just settings that are too strict.
@@ -57,7 +57,7 @@ configure(subprojects) { | |||
junitVersion = '4.12' | |||
jupiterVersion = '5.3.2' | |||
kotlinVersion = '1.3.41' | |||
knowmXchangeVersion = '4.3.3' | |||
knowmXchangeVersion = '4.4.2' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New import, haven't seen any mention of this in the discussion. Anything new carries a risk, and perhaps more so when upgrading versions as the project in question already knows it's being used by bisq. In this case this is for price nodes so bisq wallets are not affected, lowering the risk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the library that abstract away the different exchange integrations. Since we want to have the most up-to-date integration possible (e.g. broadest set of exchanges, support for most recent exchange API versions, etc) it makes sense to use the latest version of this lib.
Removed unused imports from pricenode classes.
Apply various changes in coding style, required by the Codacy check.
Rewrite a few generic parts of the code to be more specific in what they handle, or how they handle the resulting data structure.
233af83
to
d972a75
Compare
@sqrrm I addressed your comments above. Also, the Codacy checks now all pass. |
Okay, all 5 of the @bisq-network/pricenode-operators have created instances of this PR branch, so we are ready to begin the full testing using our staging environment. Onion hostnames are listed in bisq-network/projects#35 (comment) |
Pending merge of bisq-network#4315 and rough consensus to proceed with migration plan in bisq-network/projects#35 wizpriceje6q5tdrxkyiazsgu7irquiqjy2dptezqhrtu7l2qelqktid (@wiz) emzyprwrcz22h2fzhsbqd3deoe5hkqdm5yfy4geuqcdqqicgoor65iad (@Emzy) 6b7jpqiy2ejq3m7jskem6cuverg7xelhhuf3d2nvooucayoeapo2m3qd (@devinbileck) aprcndeiwdrkbf4fq7iozxbd27dl72oeo76n7zmjwdi4z34agdrnheyd (@mrosseel) ro7nv73awqs3ga2qtqeqawrjpbxwarsazznszvr6whv7tes5ehffopid (@alexej996)
Pending merge of bisq-network#4315 and rough consensus to proceed with migration plan in bisq-network/projects#35 wizpriceje6q5tdrxkyiazsgu7irquiqjy2dptezqhrtu7l2qelqktid (@wiz) emzyprwrcz22h2fzhsbqd3deoe5hkqdm5yfy4geuqcdqqicgoor65iad (@Emzy) devinpndvdwll4wiqcyq5e7itezmarg7rzicrvf6brzkwxdm374kmmyd (@devinbileck) aprcndeiwdrkbf4fq7iozxbd27dl72oeo76n7zmjwdi4z34agdrnheyd (@mrosseel) ro7nv73awqs3ga2qtqeqawrjpbxwarsazznszvr6whv7tes5ehffopid (@alexej996)
Pending merge of bisq-network#4315 and rough consensus to proceed with migration plan in bisq-network/projects#35 wizpriceje6q5tdrxkyiazsgu7irquiqjy2dptezqhrtu7l2qelqktid (@wiz) emzypricpidesmyqg2hc6dkwitqzaxrqnpkdg3ae2wef5znncu2ambqd (@Emzy) devinpndvdwll4wiqcyq5e7itezmarg7rzicrvf6brzkwxdm374kmmyd (@devinbileck) aprcndeiwdrkbf4fq7iozxbd27dl72oeo76n7zmjwdi4z34agdrnheyd (@mrosseel) ro7nv73awqs3ga2qtqeqawrjpbxwarsazznszvr6whv7tes5ehffopid (@alexej996)
@sqrrm as we discussed on Keybase, the only "issue" I've found so far is the precision of the prices from the pricenodes being much more precise now that they're calculated averages. Other than the prices looking a bit strange with lots of decimals it shouldn't cause any actual issues, right? |
@sqrrm If you watch the messages in keybase://chat/bisq#ops-alerts you can see the new pricenodes are quite stable, in sync with each other, using the new price indices. The only concern is the $100 difference from BA index and Bisq index for USD, EUR, and a few other markets which we can warn users about by using an in-app broadcast message. |
From what I see most feeds have a diff of .5% or something in that range. Would be good to know why it's such a consistent large difference. It's good that we have a consistent feed, but it might be a problem if it's a fair bit off the BA feed. Also looking at some of the major exchanges their prices are closer to the BA feed than the Bisq index. Perhaps volume weighting is needed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
I haven't tested it myself, but several contributors are now running the nodes and we can see a consistent feed. To deploy this we still need to tune the feed, but the PR as such looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK - this PR completes Phase 1 of the project bisq-network/projects#35 - We will tweak the index weighting in another PR later as part of Phase 2 of the project after testing is completed.
I've gone back through the nits in my NACK at #4315 (review) and approved the changes. Thanks, all. |
Extend ExchangeRateService in pricenode to add basic support for multiple providers per currency.
When the service detects that multiple providers have exchange rates for the same currency, it automatically aggregates (averages) these rates into a single ExchangeRate.
The client thus receives a single ExchangeRate per currency, regardless of the number of providers which gathered exchange rates for it (be it a single provider, or multiple). This ensures compatibility with previous and existing clients.
The purpose of this WIP is to test and review whether:
Addresses bisq-network/projects#35
(Update 21.07.2020: Add overview of supported currencies + providers)
Supported Currencies and Exchange Providers
In this PR, rates for the following currencies are retrieved from the following exchange providers:
Table 1: Fiat currencies and corresponding exchange providers supported by this PR
Table 2: Altcoins and corresponding exchange providers supported by this PR
(Update 23.07.2020: Simplify and consolidate test overview)
Testing Approach
Tests should specifically check two things:
Table 3: Overview of areas to test
Click to show...
New Pricenode is compatible with existing Bisq clients
Pricenode price feeds are reliable / usable in Bisq trades
/getAllMarketPrices
API on the pricenodes (implemented byExchangeRateController.getAllMarketPrices()
) to retrieve the currency exchange rates.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.Perform a few regtest trades in different configurations. If trades successful, then test is considered successful.
Useful Pricenode Instance Types
A few specific types of pricenodes can help more easily achieve or simulate the different conditions we need for the tests.
These pricenode types are ONLY used to simplify tests, they don't matter for the actual roll-out.
Table 4: Suggested types of test pricenode instances
Click to show...
(Why is such a pricenode useful for tests? What does it simulate?)
(How often each exchange provider will poll the exchange)
(Adjustments on top of the PR code, to make the instance suitable for tests)
Artificially inflate returned rates by +5%
(Test branch containing the modifications mentioned above)
(For testers who want to setup their own instance. Auto-install script tested with debian 10.)
Show...
curl -s https://raw.githubusercontent.com/cd2357/bisq/pricenode-test-setup-1m/pricenode/install_pricenode_debian.sh | sudo bash
Show...
curl -s https://raw.githubusercontent.com/cd2357/bisq/pricenode-test-setup-3m/pricenode/install_pricenode_debian.sh | sudo bash
Show...
curl -s https://raw.githubusercontent.com/cd2357/bisq/pricenode-test-setup-1m-up5pc/pricenode/install_pricenode_debian.sh | sudo bash
(Existing live instances, for test purposes. Warning: no uptime guarantees, best-effort availability)
mifoy7xvoeoyvtoi.onion
(Pricenode-PR-1m-A)lzlxdqzj6t7dxjha.onion
(Pricenode-PR-1m-B)jzfd6tanhnc65fhh.onion
(Pricenode-PR-3m-A)p3s7gpl42iq67llv.onion
(Pricenode-PR-3m-B)ii6vfecmp7cf6gky.onion
(Pricenode-PR-1m-up5%)Test Cases
Test 0
Goal / Scenario
This test checks that prices differ by less than 1% across different pricenode instances, in normal situations.
Prerequisites
bash
,torsocks
,curl
andjq
installedcompare-pricenode-rates.sh
and populate it with the following content:Snippet 1: Content of Test 0 script
Show Snippet 1...
chmod +x compare-pricenode-rates.sh
Steps
./compare-pricenode-rates.sh
-1.00%
and1.00%
Pricenode-PR-1m-up5pc
are expected to be roughly around5%
Here is a sample output of the script:
Snippet 2: Sample output for Test 0 script
Click to show...
Success criteria
The test is successful if, in the table outputted by the script, all the deltas in columns
Pricenode-PR-1m-A
Pricenode-PR-1m-B
Pricenode-PR-3m-A
Pricenode-PR-3m-B
have a value between
-1.00%
and1.00%
.Tests 1-4
Goals / Scenarios
Both instances are healthy, polling exchanges every 1 minute
Pricenode-PR-1m-A
Pricenode-PR-1m-B
One uses a healthy instance (polling every minute), the other uses an instance with intermittent connectivity to exchanges, which is simulated by a 3 minute polling interval
Pricenode-PR-1m-A
Pricenode-PR-3m-A
Both instances have intermittent connectivity to exchanges, which is simulated by a 3 minute polling interval
Pricenode-PR-3m-A
Pricenode-PR-3m-B
1%
between Maker and Taker, for any reasonOne instance is healthy (polling every minute), the other one also polls every minute but returns artificially inflated rates by
+5%
Pricenode-PR-1m-A
Pricenode-PR-1m-up5pc
How to execute
temp-Bisq-PR-4315
folder to free up disk spacePrerequisites
gradlew
to build different artefactsbitcoind
andbitcoin-cli
installedgit
andgit-lfs
installedmkdir temp-Bisq-PR-4315
cd temp-Bisq-PR-4315
git clone https://github.com/bisq-network/bisq.git
cd bisq
make
init_aliases.sh
with the following content:Snippet 3: init_aliases.sh script for Tests 1-4
Click to show Snippet 3...
Steps
All commands below are executed in the
.../temp-Bisq-PR-4315/bisq
folder.1. Start regtest bitcoin daemon
make bitcoind
bitcoind
daemon2. Start 1st seednode
source init_aliases.sh && start-seednode-1
3. Start 2nd seednode
source init_aliases.sh && start-seednode-2
4. Start mediator
source init_aliases.sh && start-mediator
Cmd+N
orCtrl+N
(a popup will appear)Cmd+D
orCtrl+D
(a popup will appear)5. Start 1st trader (Alice)
source init_aliases.sh && start-alice-test-X
X
is the number of the test (e.g.1
,2
,3
or4
)X
will cause different pricenodes to be used, as per each test scenario6. Start 2nd trader (Bob)
source init_aliases.sh && start-bob-test-X
X
is the number of the test (e.g.1
,2
,3
or4
)X
will cause different pricenodes to be used, as per each test scenario7. Optional: If Alice and Bob wallets are empty, generate some regtest BTC for them
make block
8. Perform a trade between Alice and Bob
9. Stop all test daemons and processes
bitcoind
,seednode
,seednode2
)Success criteria
Test 5
Goal / Scenario
This test checks that a pricenode is still running and provides a price feed, in case any exchange API is suddenly unreachable or returns an unexpected HTTP error code.
Prerequisites
1. A running (test) pricenode
Steps
1. Choose a currency that is supported by a single provider
ANG
is only covered by Bitpay2. Ensure the pricenode is up and running
ANG
(as a child of thedata
array)3. Make that API endpoint completely unreachable from the pricenode
/etc/hosts
file and add a line containing127.0.0.1 bitpay.com
4. Monitor the pricende log until it shows failed connections to Bitpay
sudo journalctl -u bisq-pricenode -e
5. Open http://localhost:8080/getAllMarketPrices on the pricenode
ANG
Snippet 1: Sample log output indicating unreachable exchange API endpoint
Click to show...
Success criteria
The test is successful if in the last step, the pricenode
ANG