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

Refactor WhiteList #9963

Merged
merged 8 commits into from Jan 19, 2023
Merged

Refactor WhiteList #9963

merged 8 commits into from Jan 19, 2023

Conversation

molnard
Copy link
Collaborator

@molnard molnard commented Jan 18, 2023

WhiteList will be able to check and release coins one by one - right at the time when requested. This is required for further work with CoinVerifier.

Szpoti
Szpoti previously approved these changes Jan 18, 2023
Copy link
Collaborator

@Szpoti Szpoti left a comment

Choose a reason for hiding this comment

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

LGTM

adamPetho
adamPetho previously approved these changes Jan 18, 2023
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.

tACK

WalletWasabi/WabiSabi/Backend/Banning/Whitelist.cs Outdated Show resolved Hide resolved
if (!string.IsNullOrEmpty(WhitelistFilePath))
{
var toFile = GetInnocents().Select(innocent => innocent.ToString());
File.WriteAllLines(WhitelistFilePath, toFile);
var toFile = Innocents.Values.Select(innocent => innocent.ToString());
Copy link
Collaborator

Choose a reason for hiding this comment

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

One thing to be aware here Innoncents being ConcurrentDictionary means that Innoncents can change at any point even during iteration (in WriteToFileIfChangedAsync or RemoveAllExpired).

Here it looks OK because it looks like Innocents.Values represents a moment in time snapshot of values.

Still I find that pretty hard to reason about. Having a simple lock + a simple Dictionary<OutPoint, Innocent> is imho much easier to understand and verify correctness.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pretty hard to reason about

Less code.

much easier to understand and verify correctness.

I did not touch that part here it was using ConcurrentDictionary before the PR for this reason it is out of scope here. Do you have a specific use case that can be problematic?

@kiminuo
Copy link
Collaborator

kiminuo commented Jan 18, 2023

It would be nice to rename UtxoWhitelistTests -> WhitelistTests. There are no new tests.

@kiminuo
Copy link
Collaborator

kiminuo commented Jan 18, 2023

	public void Add(Alice alice)
		=> Save(new Innocent(alice.Coin.Outpoint, DateTimeOffset.UtcNow));

seems to be unused.

@molnard molnard dismissed stale reviews from adamPetho and Szpoti via 4c354ba January 19, 2023 10:41
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.

LGTM

@molnard molnard merged commit 454a677 into zkSNACKs:master Jan 19, 2023
@molnard molnard deleted the imprwhitelist branch January 19, 2023 15:19
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