Skip to content
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

BSIP67: Dynamic Market Fees #133

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

OpenLedgerApp
Copy link
Contributor

@OpenLedgerApp OpenLedgerApp commented Dec 17, 2018

Dear BitShares Community,

We would like to introduce the Dynamic Market Fees BSIP.
The purpose is to support high-volume trading and market makers. Thus making it more profitable for them. We believe it will bring more people to BitShares.

As per BitShares Core Team request we have spent some time drafting this BSIP.
And here's the resulting BSIP:
https://github.com/openledger/bsips/blob/bsip-dynamicmarketfees/bsip-00XX%20Dynamic%20market%20fees.md

The pull request is here:
#133
Forum thread is here:
https://bitsharestalk.org/index.php?topic=27589.0
Please share your opinion.
If you think it might help BitShares, please voice your opinion and vote for it, when the worker is created.

Sincerely,
Denis Sokolov
OpenLedger

@pmconrad
Copy link
Contributor

Please squash this PR and use a sensible commit message.

One of the ways to increase the trade volume can be Dynamic Market Fees that will provide additional discounts for active traders.
For example, to set lower fees for trading larger volumes.

So, instead of market fee, an asset owner can optionally set up dynamic_market_fee_percent property that may look like a following example table.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please elaborate. Since the given table is only meant as an example, how is that property to be set up and evaluated, and which degrees of freedom does this allow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A table is an example. UIA owner can edit this table, set up a percent and trading volume.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That still doesnt define what the degrees of freedom are

@pmconrad
Copy link
Contributor

This proposal introduces a significant cost in terms of CPU and RAM for all node operators. IMO this needs to be balanced against the potential benefits.
I agree that this might incentivize market makers, but it is unclear if this can justify the significant cost.

@OpenLedgerApp
Copy link
Contributor Author

That's a good point! We have thought about it. We are planning to check and monitor performance KPIs at the beginning of implementation. It will allow us to make sure CPU and RAM are tracked. If we face some issues and we don't have any turnaround, it will be indicated ASAP.

@ryanRfox
Copy link
Contributor

This proposal introduces a significant cost in terms of CPU and RAM for all node operators. IMO this needs to be balanced against the potential benefits.
I agree that this might incentivize market makers, but it is unclear if this can justify the significant cost.

@sschiessl-bcp has suggested elsewhere that we add a Risks section to the BSIP Template. @pmconrad may I request your craft your comment above into a paragraph highlighting the potential risk(s) you foresee? My desire is the identified risks will be added to the PR by @OpenLedgerApp

@pmconrad
Copy link
Contributor

Suggestion:

Risks

CPU Overhead

The proposed fee calculations will have to be performed on every trade. This adds significant burden on top of the already quite heavyweight market engine. While this is probably not a big problem with today's transaction volume, it will further limit how far our chain scales in terms of tx/s.

RAM Overhead

The proposed calculations require logging the trade volume of all traders over a period of time, separately for each traded asset. With both the number of users and the number of assets growing over time, the required amount of RAM will grow squarely over time. This does not scale well.

@ryanRfox
Copy link
Contributor

Thank you @pmconrad for adding the Risks section. May I request @OpenLedgerApp update the PR to include the Risks just prior to the Summary for Shareholders?

@ryanRfox
Copy link
Contributor

My thoughts on calculation impacts/mitigation:

  1. I now understand why performing the calculation on every order would be problematic. Can we calculate a static table hourly, daily so the market can just lookup the fee?
  2. I now understand that storing complete order history for a month to use in calculation will consume substantial RAM. Can we mitigate this by calculating it into a volume average per hour, day?
  3. What other options can be explored to reduce the scaling impacts within this specification?
  4. I feel dynamic market fees are a valuable addition to the protocol and may provide an incentive for further adoption, so hope we continue to debate this topic.

@OpenLedgerApp
Copy link
Contributor Author

@pmconrad Thanks for the input! we will add this section to the BSIP description.
@ryanRfox we are analyzing your thoughts and will provide our feedback.
In any case, we should think how we can have some metrics in advance (or at the beginning of implementation) to decrease RISKS of this BSIP.

@pmconrad
Copy link
Contributor

pmconrad commented Feb 1, 2019

  1. I now understand why performing the calculation on every order would be problematic. Can we calculate a static table hourly, daily so the market can just lookup the fee?

We must update the trade volume per user and asset for every trade.

  1. I now understand that storing complete order history for a month to use in calculation will consume substantial RAM. Can we mitigate this by calculating it into a volume average per hour, day?

This is already described in the section "30-day volume method". It requires two values per user and asset.

@sschiessl-bcp
Copy link
Collaborator

sschiessl-bcp commented Feb 21, 2019

I would very much like this BSIP in two steps. First, introduce maker-taker, then maybe dynamic fees like indicated above.

Limit order would have fixed fee plus market fee, both for maker and taker. But, when filled

  • maker may receive a refund for fixed fee (maybe 50%, committee-parameter)
  • asset owner can define how much % of the market fee is refunded to the maker

Thoughts?

@xeroc
Copy link
Member

xeroc commented Feb 22, 2019

I would very much like this BSIP in two steps. First, introduce maker-taker, then maybe dynamic fees like indicated above.

This!
"Keep it simple stupid" and do one thing at a time. Also, a BSIP that proposes only maker/taker fees will be much easier to get approved than something that introduces it together with something that adds significant additional burden to the chain

@OpenLedgerApp
Copy link
Contributor Author

We have made a prototype in order to measure performance

As you can see in the picture attached, our worker has no significant influence on BitShares performance:
image

For more details please refer the prototype. Here is the link - https://github.com/openledger/bitshares-core/commits/issue-dmf

@abitmore
Copy link
Member

@OpenLedgerApp try test with 1M users? and around 1K trading pairs (although there can be more)?

@OpenLedgerApp
Copy link
Contributor Author

OpenLedgerApp commented Mar 27, 2019

@OpenLedgerApp try test with 1M users? and around 1K trading pairs (although there can be more)?

There is a result:
dmf-perf-result

The sources are located - https://github.com/openledger/bitshares-core/commits/issue-dmf

@ryanRfox
Copy link
Contributor

ryanRfox commented Apr 3, 2019

Sorry, I'm not understanding the graph. What is the X-axis representing? It seems like there are both red and blue plots for about 2/3 of the data, but only blue when the value jump. What accounts for the jump? Are there red values as well?

@pmconrad
Copy link
Contributor

pmconrad commented Apr 3, 2019

AFAICS your test creates 1 M users but only 1000 traders, and it trades only 1 asset pair.

@OpenLedgerApp
Copy link
Contributor Author

OpenLedgerApp commented Apr 19, 2019

AFAICS your test creates 1 M users but only 1000 traders, and it trades only 1 asset pair.

We have made test with 100 asset pair.

As you can see in the picture attached, our worker has no significant influence on BitShares performance:

image

sources are located - https://github.com/openledger/bitshares-core/commits/issue-dmf

@pmconrad
Copy link
Contributor

Now you have 100 assets and 1000 traders, but each trader is using only 1 or 2 assets. Like abit said, please repeat your test with 1M traders and 1k trading pairs where each trader trades in a significant portion of these pairs.

Also, please explain your graphics, as Ryan has requested. I don't understand them either.

@OpenLedgerApp
Copy link
Contributor Author

We will update our test in the near future.

@OpenLedgerApp
Copy link
Contributor Author

OpenLedgerApp commented Apr 29, 2019

Now you have 100 assets and 1000 traders, but each trader is using only 1 or 2 assets. Like abit said, please repeat your test with 1M traders and 1k trading pairs where each trader trades in a significant portion of these pairs.

We have modified the tests. There are 100 traders and 100 assets where each trader trades 100 assets. We have got the following results: Origin bitshares takes 2095465 milliseconds, and with DMF logic 2099182 milliseconds.
You are welcome to change tests' parameters and rerun them:
// How many traders to create
const static auto traders_count = 100;
// Count of cycles for trading
const static auto iterations = 1;
// How many assets to sell/buy
const static auto uia_to_sell = 20;
// How many assets to create
const static auto ui_assets_count = 100;

New sources are located openledger/bitshares-core@e4f9f44

Also, please explain your graphics, as Ryan has requested. I don't understand them either.

Red line shows how much time origin bitshares take and an other line shows how much time the same test takes with DMF logic.

@abitmore
Copy link
Member

abitmore commented Jun 9, 2019

@ryanRfox please assign BSIP number.

@ryanRfox ryanRfox changed the title BSIP Draft: Dynamic Market Fees BSIP67: Dynamic Market Fees Jun 9, 2019
@OpenLedgerApp
Copy link
Contributor Author

OpenLedgerApp commented Jul 26, 2019

We would like to put our BSIP on voting.
The prototype is ready for voting. We have finished all test and made Pull Request.
There are some worries about computation resources.
That why, we have made several performance tests, which showed good results.

What should we do to put this BSIP on voting?

@ryanRfox please let us know. Thanks in advance!

account_id_type owner;
asset_id_type asset;
timestamp first_trade_date; //first trade date for this asset
share_type total_volume; //total bought asset volume
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use share_type for this data due to the high possibility of overflow. I guess a 128-bit data type would be sufficient, and in the implementation still need to deal with overflows.

@abitmore
Copy link
Member

@OpenLedgerApp please update the document and rename the file with the assigned BSIP number.

@sschiessl-bcp
Copy link
Collaborator

Please address all comments, and update your document.

Also I am still wondering if only maker/taker differentiation should be an option as well when voting as the simple version.

Copy link
Contributor

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For more realistic data I have slightly modified your existing implementation to compute the trade statistics for all trades since the beginning of time while still applying the old market fee calculation rules.
As of block 35M there are roundabout 800,000 trade statistics objects generated which is about 32MB raw, plus index overhead. Replay time is slightly worse but acceptable IMO (1-2%). Both are acceptable IMO. Also, there is some room for improvement in the implementation, e. g. trade statistics should be removed if there were no trades within the last 30 days.

IMO this BSIP is not ready for voting, because the specification is virtually non-existent. Please see #170 for an example what the "Specification" section should contain.
It is also confusing because it presents several different options without making it clear which option is the one that will be implemented. Please focus on one option only. Considered alternatives should be mentioned briefly in the "Discussion" section, together with a reason why they were dismissed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants