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

[Protocol3] Decrease merkle tree depths #49

Closed
Brechtpd opened this issue Mar 27, 2019 · 7 comments
Closed

[Protocol3] Decrease merkle tree depths #49

Brechtpd opened this issue Mar 27, 2019 · 7 comments
Assignees
Milestone

Comments

@Brechtpd
Copy link
Contributor

Our current tree depth sizes:

Accounts: 24 (16,777,216 unique accounts / exchange)
Tokens: 12 (4096 unique tokens / exchange)
Trade History: 16 (65536 slots / tokenS / account)

Some of these are larger than needed. Updating these trees in the circuits is our main bottleneck next to data availability (which will be optional). By decreasing these we lower the number of constraints which increases our achievable throughput.

I propose the following:

Accounts: 20 (1,048,576 unique accounts / exchange)
Tokens: [8, 10] (256 - 1024 unique tokens / exchange)
TradeHistory: [12, 14] (4096 - 16384 slots / tokenS / account)

The Tokens tree is updated the most frequent so lowering this is the most important, then the Accounts tree and then the TradeHistory tree.

@dong77
Copy link
Contributor

dong77 commented Mar 28, 2019

I agree with smaller trees. 20 for accounts, 8 for tokens, and 14 for trading history seems ok for the near term. In the long term, way may even allow DEX to config these numbers on exchange creation.

@dong77
Copy link
Contributor

dong77 commented Apr 2, 2019

Will any of these depths "configs" be visible on smart contract level?

@Brechtpd
Copy link
Contributor Author

Brechtpd commented Apr 2, 2019

The only 'logic' that needs to be changed is in the withdrawFromMerkleTree where the length of the merkle proof changes. The genesis merkle root value will also change.
There are also a few places where tokenID is assumed to need 16bits and we only need 8bits, but I don't think we should change that in the smart contract because savings for this will be insignificant (and it will be easier to switch back to something bigger again if needed).

This shouldn't be a lot of work and should decrease the number of constraints by ~10-15%.

@Brechtpd
Copy link
Contributor Author

Brechtpd commented Apr 2, 2019

It's also not really possible to make these configurable for every exchange. Every different configuration would need its own set of circuits.

@dong77
Copy link
Contributor

dong77 commented Apr 2, 2019

I totally agree. I would only suggest these configs are somehow refected by those constant variables in the smart contract.

@dong77 dong77 removed the 3.0 label Apr 5, 2019
@dong77 dong77 added this to the 3.0 milestone Apr 5, 2019
@Brechtpd
Copy link
Contributor Author

Brechtpd commented Apr 6, 2019

Done in #115

Constraints/ring decreased from 650,000 -> 525,000, a nice 20% reduction.
I also got the correct constraint numbers of the circuit without data-availability, which is a bit cheaper than with data-availability: max throughput increased from 350 rings/second to 450 rings/second.

@dong77
Copy link
Contributor

dong77 commented Apr 6, 2019

This is great. I'll close this issue then.

@dong77 dong77 closed this as completed Apr 6, 2019
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

No branches or pull requests

2 participants