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

[VDG] Change PrivacyWarnings to IAsyncEnumerable #11163

Merged

Conversation

ichthus1604
Copy link
Collaborator

@ichthus1604 ichthus1604 commented Jul 31, 2023

@soosr soosr self-requested a review July 31, 2023 12:22
@ichthus1604 ichthus1604 marked this pull request as ready for review August 1, 2023 16:08
@ichthus1604
Copy link
Collaborator Author

@soosr I figured out the problem was due to the early disposal of the CancellationTokenSources.

Marked as ready for review.

{
public List<PrivacyWarning> Warnings { get; } = new();
public List<PrivacySuggestion> Suggestions { get; } = new();
private List<IAsyncEnumerable<PrivacyItem>> _combinedResults = new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit, but more like a question... Can we somehow prioritize the items here? Because by the testing it seems to me during processing if the first item is a ChangeAvoidance item (and it takes seconds to finish), every other suggestion is waiting to be shown, despite their computation being already done.

We know that always the computation of the ChangeAvoidance items will take the most so put them at the end of the processing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@soosr Can you share the steps to reproduce this? Adding a 5 second delay in VerifyChangeAsync() line 218, I get all the other suggestions properly shown in the flyout and then the change-related ones appear in the end.

The IAsyncEnumerables are added to a regular List which is then iterated over, so we should get the results in the same order we insert them:

result
	.Combine(VerifyLabels(info, transactionResult))
	.Combine(VerifyPrivacyLevel(info, transactionResult))
	.Combine(VerifyConsolidation(transactionResult))
	.Combine(VerifyUnconfirmedInputs(transactionResult))
	.Combine(VerifyCoinjoiningInputs(transactionResult))
	.Combine(VerifyChangeAsync(info, transactionResult, _linkedCancellationTokenSource));

Unless I'm missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I added a 5 seconds delay before this line

transaction = _wallet.BuildChangelessTransaction(

Just to simulate if the BnB algorithm runs longer (it can run up to 15 seconds).
After this, I did my testing by repeatedly creating a transaction where I know only the Label Management and a Less and a More change avoidance suggestion is possible.

I noticed that sometimes the Label Management option appears later. Taking a look at the logs I could see that when this scenario happened it always appeared after a change avoidance calculation was done.

Copy link
Collaborator

@soosr soosr left a comment

Choose a reason for hiding this comment

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

tACK

@soosr
Copy link
Collaborator

soosr commented Aug 7, 2023

Merging since #11163 (comment) is not a blocker. @ichthus1604 think about it, you might have a good idea.

@soosr soosr merged commit 301ca96 into zkSNACKs:master Aug 7, 2023
7 checks passed
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.

[VDG] Change avoidance calculation blocks all the transaction warnings
2 participants