Skip to content

fix: concurrency conflict in NEP6Wallet.ToJson #3527

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Oct 14, 2024
Merged

fix: concurrency conflict in NEP6Wallet.ToJson #3527

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e3c42ee
fix: concurrency conflict in NEP6Wallet.ToJson
nan01ab Oct 11, 2024
65cc69b
Update also ChangePasssword
shargon Oct 11, 2024
a8f5e27
Reduce lock time
shargon Oct 11, 2024
1fc006c
Merge branch 'master' into fix.concurrency-conflict-NEP6Wallet.ToJson
Oct 12, 2024
c2e6efa
Merge branch 'master' into fix.concurrency-conflict-NEP6Wallet.ToJson
Oct 14, 2024
3b418f2
Merge branch 'master' into fix.concurrency-conflict-NEP6Wallet.ToJson
shargon Oct 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions src/Neo/Wallets/NEP6/NEP6Wallet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,12 +293,18 @@ public override WalletAccount Import(string nep2, string passphrase, int N = 163
/// </summary>
public JObject ToJson()
{
NEP6Account[] accountValues;
lock (accounts)
Copy link
Member

@cschuchardt88 cschuchardt88 Oct 11, 2024

Choose a reason for hiding this comment

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

I prefer locking to be on its own object altogether. The reason for this is so that way it frees it up quicker and it doesn't Lock up when something wants to read it. Only prevents writing when saving the Json

Copy link
Member

@shargon shargon Oct 11, 2024

Choose a reason for hiding this comment

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

Lock time wasted procesing .ToJson

Copy link
Contributor Author

@nan01ab nan01ab Oct 12, 2024

Choose a reason for hiding this comment

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

This is not thread safe if ToJson and ChangePassword concurrently, because account.ToJson read fields of the Account and ChangePassword changes fields of the Account.

Copy link
Contributor Author

@nan01ab nan01ab Oct 13, 2024

Choose a reason for hiding this comment

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

Lock time wasted procesing .ToJson

This is not performance critical path, so excessive optimization is not needed.

{
accountValues = accounts.Values.ToArray();
}

return new()
{
["name"] = name,
["version"] = version.ToString(),
["scrypt"] = Scrypt.ToJson(),
["accounts"] = accounts.Values.Select(p => p.ToJson()).ToArray(),
["accounts"] = accountValues.Select(p => p.ToJson()).ToArray(),
["extra"] = extra
};
}
Expand Down Expand Up @@ -345,26 +351,28 @@ private bool VerifyPasswordInternal(string password)
public override bool ChangePassword(string oldPassword, string newPassword)
{
bool succeed = true;
NEP6Account[] accountsValues;
lock (accounts)
{
Parallel.ForEach(accounts.Values, (account, state) =>
{
if (!account.ChangePasswordPrepare(oldPassword, newPassword))
{
state.Stop();
succeed = false;
}
});
accountsValues = accounts.Values.ToArray();
}
Parallel.ForEach(accountsValues, (account, state) =>
Copy link
Contributor Author

@nan01ab nan01ab Oct 12, 2024

Choose a reason for hiding this comment

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

This is not thread safe if multi-ChangePassword requested concurrently, because of account.ChangePasswordPrepare is not thread safe.

Copy link
Contributor Author

@nan01ab nan01ab Oct 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Parallel.ForEach may speed up operations here, but may lead to more system overhead.

In general, it is sufficient to ensure correctness, and there is no need to over-optimize if it is not performance critical path.

Copy link
Member

@cschuchardt88 cschuchardt88 Oct 13, 2024

Choose a reason for hiding this comment

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

You should only use Parallel.ForEach if the process count is greater than 1 CPU. Otherwise you Task.Run. The overhead cost to much on single CPU or if the payload is small. Take in account docker

{
if (!account.ChangePasswordPrepare(oldPassword, newPassword))
{
state.Stop();
succeed = false;
}
});
if (succeed)
{
foreach (NEP6Account account in accounts.Values)
foreach (NEP6Account account in accountsValues)
account.ChangePasswordCommit();
password = newPassword.ToSecureString();
}
else
{
foreach (NEP6Account account in accounts.Values)
foreach (NEP6Account account in accountsValues)
account.ChangePasswordRollback();
}
return succeed;
Expand Down
Loading