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

Revert "Reduce the RangeProof by 5 bits" #9591

Merged

Conversation

lontivero
Copy link
Collaborator

@lontivero lontivero commented Nov 21, 2022

This reverts commit 751483d (PR #7646). Reverts the range proof limitation

The limit is relaxed back from 1343750000000 to 4300000000000. Previously this value was restricted because
we suspected the increasing delay time observed during credential reissuance requests was due to high cpu usage, however that was not proven but assumed to be true.

Note that this doesn't solve the problem but kick it down the road because now someone could ask "what if I have more than 43000 bitcoins in one single utxo?" and the only way to solve now and forever would be to increase it another 12 bits. The problem with that is that we would be imposing a big cost to everybody just in case Satoshi one day decides to consolidate all his coins in one single utxo and start mixing with Wasabi.

[JsonProperty(PropertyName = "MaxRegistrableAmount", DefaultValueHandling = DefaultValueHandling.Populate)]
[JsonConverter(typeof(MoneyBtcJsonConverter))]
public Money MaxRegistrableAmount { get; set; } = Money.Satoshis(ProtocolConstants.MaxAmountPerAlice);
public Money MaxRegistrableAmount { get; set; } = Money.Coins(43_000m);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change? You are also changing ProtocolConstants.MaxAmountPerAlice in the PR so why not keep using it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because this is a revert, i mean, the result of doing git revert <commit> . If I get the ACK then i will continue with it, otherwise not.

@turbolay
Copy link
Collaborator

Is there a way to effectively measure the impact of this?

@lontivero
Copy link
Collaborator Author

Yes, there is a WalketWasabi.Benchmark project somewhere. I will find it and share it.

@yahiheb
Copy link
Collaborator

yahiheb commented Nov 22, 2022

Yes, there is a WalketWasabi.Benchmark project somewhere. I will find it and share it.

#2439

https://github.com/zkSNACKs/WasabiBenchmark

@lontivero
Copy link
Collaborator Author

lontivero commented Nov 22, 2022

It seems there is not real difference in processing time:

// * Summary *

BenchmarkDotNet=v0.13.2, OS=nixos 22.05
AMD Ryzen 7 5800H with Radeon Graphics, 1 CPU, 16 logical and 8 physical cores
.NET SDK=6.0.300
  [Host]     : .NET 6.0.5 (6.0.522.21309), X64 RyuJIT AVX2
  .NET 6.0   : .NET 6.0.5 (6.0.522.21309), X64 RyuJIT AVX2

Runtime=.NET 6.0

|  Method |        Job | Toolchain |    upperBound |     Mean |   Error |  StdDev | Rank |
|-------- |----------- |---------- |-------------- |---------:|--------:|--------:|-----:|
| Process |   .NET 6.0 |   Default | 1343750000000 | 501.0 ms | 1.21 ms | 1.07 ms |    1 |
| Process |   .NET 6.0 |   Default | 2150000000000 | 505.2 ms | 3.53 ms | 3.31 ms |    1 |
| Process |   .NET 6.0 |   Default | 4300000000000 | 512.7 ms | 1.55 ms | 1.29 ms |    2 |

code: https://github.com/zkSNACKs/WasabiBenchmark

@lontivero
Copy link
Collaborator Author

There was an extra zero in the upper bound limit so, it was wrong.

|  Method |        Job | Toolchain |    upperBound |     Mean |   Error |  StdDev | Rank |
|-------- |----------- |---------- |-------------- |---------:|--------:|--------:|-----:|
| Process |   .NET 6.0 |   Default |  134375000000 | 452.7 ms | 1.27 ms | 1.12 ms |    1 |
| Process |   .NET 6.0 |   Default | 2150000000000 | 501.6 ms | 1.75 ms | 1.46 ms |    2 |
| Process |   .NET 6.0 |   Default | 4300000000000 | 519.9 ms | 1.91 ms | 1.59 ms |    3 |

@lontivero lontivero force-pushed the revert-range-proof-size-reduction branch from 5dd0c55 to eb007ab Compare November 22, 2022 14:25
@lontivero
Copy link
Collaborator Author

Note: Even when this is a server-side change only, those who don't update will not be able to participate with more than 1343.75btc because the DependencyGraph uses the ProtocolConstants.MaxAmountPerAlice instead of using the value specified by the RoundParameters .

@lontivero
Copy link
Collaborator Author

Btw reviewers, the benchmark project is updated and you can run it and verify the numbers are correct.

@molnard
Copy link
Collaborator

molnard commented Dec 6, 2022

Deployed to TestNet

Copy link
Collaborator

@adamPetho adamPetho left a comment

Choose a reason for hiding this comment

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

Successful CJs on client side, nothing suspicious in the server side logs.

@molnard
Copy link
Collaborator

molnard commented Dec 9, 2022

Next Monday we can deploy this to the master. Before that, I will merge this PR.

@molnard
Copy link
Collaborator

molnard commented Dec 12, 2022

Tested.

@molnard molnard merged commit 97f11e9 into WalletWasabi:master Dec 12, 2022
@molnard
Copy link
Collaborator

molnard commented Dec 12, 2022

Deployed to MainNet

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

Successfully merging this pull request may close these issues.

None yet

5 participants