-
Notifications
You must be signed in to change notification settings - Fork 492
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
Pre-calculate Privacy Suggestions for changeless transaction #11512
Pre-calculate Privacy Suggestions for changeless transaction #11512
Conversation
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.
tACK, just a nit.
WalletWasabi.Fluent/Models/Transactions/PrivacySuggestionsModel.cs
Outdated
Show resolved
Hide resolved
@adamPetho pls check and merge if 👌 |
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 PR reintroduces #11031
I have a rather ugly solution to not introduce back that issue. #11512 (review) PrivacySuggestionsModel.cs public async IAsyncEnumerable<PrivacyItem> BuildPrivacySuggestionsAsync(TransactionInfo info, BuildTransactionResult transactionResult, [EnumeratorCancellation] CancellationToken cancellationToken)
{
using CancellationTokenSource singleRunCts = new();
lock (_lock)
{
_singleRunCancellationTokenSource?.Cancel();
_linkedCancellationTokenSource?.Cancel();
_singleRunCancellationTokenSource = singleRunCts;
CancellationTokenSource timeoutCts = new(TimeSpan.FromSeconds(15));
CancellationTokenSource linkedCts = CancellationTokenSource.CreateLinkedTokenSource(timeoutCts.Token, singleRunCts.Token, cancellationToken);
_linkedCancellationTokenSource = linkedCts;
}
using (await _asyncLock.LockAsync(CancellationToken.None))
{
foreach (var item in VerifyLabels(info, transactionResult))
{
yield return item;
}
foreach (var item in VerifyPrivacyLevel(info, transactionResult))
{
yield return item;
}
foreach (var item in VerifyConsolidation(transactionResult))
{
yield return item;
}
foreach (var item in VerifyUnconfirmedInputs(transactionResult))
{
yield return item;
}
foreach (var item in VerifyCoinjoiningInputs(transactionResult))
{
yield return item;
}
var changeItems = new List<PrivacyItem>();
try
{
await foreach (var item in VerifyChangeAsync(info, transactionResult, _linkedCancellationTokenSource).ConfigureAwait(false))
{
changeItems.Add(item); // Can't yield return in try-catch block
}
}
catch (OperationCanceledException)
{
Logger.LogTrace("Operation was cancelled.");
}
finally
{
lock (_lock)
{
_singleRunCancellationTokenSource = null;
}
}
foreach (var item in changeItems)
{
yield return item;
}
}
} We can even Then we can have this inside PrivacySuggestionsFlyoutViewModel.cs await foreach(var result in _privacySuggestionsModel.BuildPrivacySuggestionsAsync(info, transaction, cancellationToken))
{
if (result is PrivacyWarning warning)
{
Warnings.Add(warning);
}
if (result is PrivacySuggestion suggestion)
{
Suggestions.Add(suggestion);
}
} WDYT @soosr ? I tested it and looks to work well. If it's acceptable, then I can push my changes. |
Please make a final version that you show |
Check now pls and address |
catch (OperationCanceledException) | ||
await foreach (var item in VerifyChangeAsync(info, transactionResult, _linkedCancellationTokenSource).ConfigureAwait(false)) | ||
{ | ||
Logger.LogTrace("Operation was cancelled."); | ||
yield return item; | ||
} | ||
finally |
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.
I removed this because it's irrelevant, the excpetion was never catched because it was always caught inside.
https://github.com/zkSNACKs/WalletWasabi/blob/33233a26c839aa06771847c6dd8099e61f7df45b/WalletWasabi/Blockchain/TransactionBuilding/ChangelessTransactionCoinSelector.cs#L91-L98
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.
tACK
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.
LGTM
result.Add(VerifyLabels(info, transactionResult)); | ||
result.Add(VerifyPrivacyLevel(info, transactionResult)); | ||
result.Add(VerifyConsolidation(transactionResult)); | ||
result.Add(VerifyUnconfirmedInputs(transactionResult)); | ||
result.Add(VerifyCoinjoiningInputs(transactionResult)); |
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.
Note for the future:
if one of these is a heavy operation, it will block all the other results.
This is not the case at the moment.
WalletWasabi.Fluent/ViewModels/Wallets/Send/PrivacySuggestionsFlyoutViewModel.cs
Outdated
Show resolved
Hide resolved
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
Pre-calculate Privacy Suggestions for changeless transaction (cherry picked from commit 6ab6747)
…ions" This reverts commit 5d95a47.
Alternate of #11496
This PR removes redundant
PrivacySuggestionsResult
class, and pre-calculates thePrivacyWarning
s andPrivacySuggestion
s, so we don't iterate through the list twice unnecessarily, callingVerifyChangeAsync
both times. Instead useOfType
on the already calculatedPrivacyItems
.