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

Add SuggestedMaxAmount #7748

Merged
merged 17 commits into from
Apr 18, 2022
Merged

Add SuggestedMaxAmount #7748

merged 17 commits into from
Apr 18, 2022

Conversation

molnard
Copy link
Collaborator

@molnard molnard commented Apr 7, 2022

Backend

  • Added parameter MaxSuggestedAmount to Round.
  • The amount is changed according to a counter.
  • The blame round always inherits the MaxSuggestedAmount from the failed round.
  • The counter is incremented after a round successfully progressing forward to connection confirmation. Why?

(1) If the round goes to conn confirmation, Arena will open another round to have at least one round in input reg. That round will be created with the upcoming MaxSuggestedAmount value. This will make sure that we keep changing the counter. THIS is what I do now.
(2) The other option can be that we increase the counter after a round was successful. In this case, it is possible to have more rounds created with the same counter.

Client

How should the client handle the value here?
https://github.com/molnard/WalletWasabi/blob/master/WalletWasabi/WabiSabi/Client/CoinJoinClient.cs#L119

Current solution: apply the filtering by using the maxSuggestedAmount and if the number of coins drops below the target number of coins - so we will have a worse solution than before, then do not take part in the round and wait for a bigger maxSuggestedAmount.

@molnard molnard requested review from nopara73 and lontivero and removed request for nopara73 April 7, 2022 15:08
@pull-request-size pull-request-size bot added size/L and removed size/M labels Apr 8, 2022
@molnard molnard marked this pull request as ready for review April 8, 2022 15:02
@molnard molnard requested a review from nopara73 April 8, 2022 15:02
@nopara73
Copy link
Contributor

nopara73 commented Apr 9, 2022

Backend logic ACK.

var txParams = new MultipartyTransactionParameters(
roundParameters.FeeRate,
roundParameters.CoordinationFeeRate,
allowedAmounts,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is allowedAmounts supposed to be twice here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes we allow the same value on input and on output side as well.

WalletWasabi/WabiSabi/Client/CoinJoinClient.cs Outdated Show resolved Hide resolved

// Check if after taking the suggestion into account, the situation worsen?
filteredCoins = missingCoinsWithOriginal < missingCoinsWithSuggested
? throw new InvalidOperationException("Skipping the round for more optimal mixing, waiting for one with bigger suggested amount.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
? throw new InvalidOperationException("Skipping the round for more optimal mixing, waiting for one with bigger suggested amount.")
? throw new InvalidOperationException("Skipping the round for more optimal mixing, waiting for one with bigger suggested input value.")

Round r = new(roundParams);
Rounds.Add(r);
}
}

internal static Money GetMaxSuggestedAmount(int roundCounter)
{
Money baseAmount = Money.Coins(0.1m);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe smaller? Not sure.

Suggested change
Money baseAmount = Money.Coins(0.1m);
Money baseAmount = Money.Coins(0.01m);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added to WabiSabiConfig as a parameter. the current default is 0.1 still

@molnard
Copy link
Collaborator Author

molnard commented Apr 13, 2022

Before I continue with this, I need to know if it is necessary to add MaxSuggestedAmount while we already have a MaxInput amount in roundparameters. Here is the concept:

#7777

If we never gonna register bigger coin that the suggested then this is much simpler. @nopara73 @lontivero

@nopara73
Copy link
Contributor

If we never gonna register bigger coin that the suggested then this is much simpler.

We will. It's just a "suggestion".

@molnard molnard self-assigned this Apr 14, 2022
@molnard
Copy link
Collaborator Author

molnard commented Apr 14, 2022

@nopara73 @lontivero @MaxHillebrand this is ready, pls take a final look. I had to change a couple of things because of tests but the logic is still the same.

Copy link
Contributor

@nopara73 nopara73 left a comment

Choose a reason for hiding this comment

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

NACK until we go through the client logic carefully together. Good job btw.

Copy link
Contributor

@nopara73 nopara73 left a comment

Choose a reason for hiding this comment

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

ACK

@nopara73 nopara73 merged commit 05b776b into WalletWasabi:master Apr 18, 2022
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

4 participants