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

Aborting input registrations: 'WrongPhase' gives "Waiting for confirmed funds" in musicbox #10313

Closed
MarnixCroes opened this issue Mar 20, 2023 · 7 comments · Fixed by #10697
Closed
Assignees
Labels

Comments

@MarnixCroes
Copy link
Collaborator

General Description

I started coinjoin, Waiting for other participants ended
then the message Waiting for confirmed funds was displayed in the musicbox. while all my coins are confirmed.

I checked logs:
INFO CoinJoinClient.CreateRegisterAndConfirmCoinsAsync (406) Round (680fac436000f72ec49d2d4ed40f4f00ce64ac29df96c9fdcca5b49305cf211a): Aborting input registrations: 'WrongPhase'.

it continued normally after a couple seconds

Wasabi Version

2.0.3rc4

@molnard
Copy link
Collaborator

molnard commented Mar 20, 2023

I guess the round was load balanced or just aborted for some other reason. This is OK, the coordinator can abort for any reason. The log message could be confusing. Do you see something regarding the reason in the logs?

@MarnixCroes
Copy link
Collaborator Author

I guess the round was load balanced or just aborted for some other reason. This is OK, the coordinator can abort for any reason. The log message could be confusing. Do you see something regarding the reason in the logs?

nothing else in the log,

the issue is more about the incorrect musicbox message though

@wieslawsoltes
Copy link
Collaborator

wieslawsoltes commented May 15, 2023

So upon my investigation the Waiting for confirmed fund corresponds to error CoinjoinError.NoCoinsToMix which actually can be 3 different errors:

  1. So I see these can be either message ""No coin was selected from candidate coins" but no idea what real reason is or might have been reason "...Probably it was not economical"?
throw new CoinJoinClientException(CoinjoinError.NoCoinsToMix, $"No coin was selected from '{coinCandidates.Count()}' number of coins. Probably it was not economical, total amount of coins were: {Money.Satoshis(coinCandidates.Sum(c => c.Amount))} BTC.");

https://github.com/zkSNACKs/WalletWasabi/blob/e25fea57e6d377ef181d104b036a5cb4af52024f/WalletWasabi/WabiSabi/Client/CoinJoinClient.cs#L170

  1. The message maybe should indicate "The coordinator rejected all inputs"or something more user friendly ?
throw new CoinJoinClientException(CoinjoinError.NoCoinsToMix, $"The coordinator rejected all {smartCoins.Count()} inputs.");

https://github.com/zkSNACKs/WalletWasabi/blob/e25fea57e6d377ef181d104b036a5cb4af52024f/WalletWasabi/WabiSabi/Client/CoinJoinClient.cs#L313

  1. Possible message "No candidate coins available to mix." for music box ?
throw new CoinJoinClientException(CoinjoinError.NoCoinsToMix, "No candidate coins available to mix.");

https://github.com/zkSNACKs/WalletWasabi/blob/e25fea57e6d377ef181d104b036a5cb4af52024f/WalletWasabi/WabiSabi/Client/CoinJoinManager.cs#L194

Solution:
So to make music box display proper message we would need 3 distinct errors in CoinjoinError and handle those in UI ?

@wieslawsoltes
Copy link
Collaborator

wieslawsoltes commented May 15, 2023

Upon further investigation CoinjoinError.AllCoinsPrivate is also used in 3 different district places but is reported as one issue. Might be also confusing for music box messages to users.

  1. This actually writes into the log: "All mixed!", while music box message is "Hurray! Your funds are private"
    https://github.com/zkSNACKs/WalletWasabi/blob/e25fea57e6d377ef181d104b036a5cb4af52024f/WalletWasabi/WabiSabi/Client/CoinJoinManager.cs#L186

  2. So this error is related to comment "If coin candidates are already private and the user doesn't override the StopWhenAllMixed, then don't mix." so we should probably indicate it to user as comment suggests?
    https://github.com/zkSNACKs/WalletWasabi/blob/e25fea57e6d377ef181d104b036a5cb4af52024f/WalletWasabi/WabiSabi/Client/CoinJoinManager.cs#L200

  3. This one actually might not stop the process, it's notifications and might be retried. Seems to be proper message actaully as it's using wallet.IsWalletPrivateAsync()
    https://github.com/zkSNACKs/WalletWasabi/blob/e25fea57e6d377ef181d104b036a5cb4af52024f/WalletWasabi/WabiSabi/Client/CoinJoinManager.cs#L458

@wieslawsoltes
Copy link
Collaborator

wieslawsoltes commented May 15, 2023

Possible solution to solve confusing or wrong music box messages related to coin join errors:

  1. So I think we might need to split CoinjoinError.NoCoinsToMix into 3 separate errors based on this : Aborting input registrations: 'WrongPhase' gives "Waiting for confirmed funds" in musicbox #10313 (comment)

  2. So I think we might need to split CoinjoinError.AllCoinsPrivate into 3 separate errors based on this :
    Aborting input registrations: 'WrongPhase' gives "Waiting for confirmed funds" in musicbox #10313 (comment)

@wieslawsoltes
Copy link
Collaborator

I have opened #10697 with quick fix so at least make message closer to the truth :)

@yahiheb
Copy link
Collaborator

yahiheb commented Jun 19, 2023

Possible solution to solve confusing or wrong music box messages related to coin join errors:

  1. So I think we might need to split CoinjoinError.NoCoinsToMix into 3 separate errors based on this : Aborting input registrations: 'WrongPhase' gives "Waiting for confirmed funds" in musicbox #10313 (comment)
  2. So I think we might need to split CoinjoinError.AllCoinsPrivate into 3 separate errors based on this :
    Aborting input registrations: 'WrongPhase' gives "Waiting for confirmed funds" in musicbox #10313 (comment)

@soosr The 1st one was fixed by #10697, should we do anything regarding the 2nd one?

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 a pull request may close this issue.

5 participants