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

wallet2: fail to establish daemon cxn == "Disconnected" cxn status [release] #8585

Merged
merged 1 commit into from
Sep 26, 2022

Conversation

j-berman
Copy link
Collaborator

When the wallet fails to establish a connection to a daemon either because the daemon is non-existent or offline, sometimes the wallet will incorrectly think the failure is caused by incompatible versions. Reported by @hinto-janaiyo here (also the cause of the "Wrong version" status in #8583).

The primary cause of the bug is undefined behavior introduced in #8544 when initializing wallet_is_outdated.

Separate from the above bug: in this PR, I also reduced wallet.cpp's default connection timeout from 30s to 20s, which matches wallet2 and helps avoid an infinite "Connecting" loop in the GUI.

Copy link
Contributor

@hinto-janai hinto-janai left a comment

Choose a reason for hiding this comment

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

Tested, these show correct messages now:

  • v0.18 wallet -> v0.17 daemon
  • v0.18 wallet -> offline daemon

Didn't know it was just missing = false, I thought that was just valid syntax :D

This is probably a different issue but the actual interval values seem to be multiplied by 1.5x:

  • Connection timeout takes 30s not 20s
  • Refresh interval takes 15s not 10s

The hangs after each message funnily enough also lead to the wallet locking itself for inactivity anywhere from 30 seconds to immediately after the initial user prompt

@selsta
Copy link
Collaborator

selsta commented Sep 22, 2022

@hinto-janaiyo did you test the CLI wallet or the GUI?

@hinto-janai
Copy link
Contributor

@selsta Above was CLI, just tested GUI with monero-project/monero-gui#4036 + this PR, it show the correct error messages and the time interval is also increased. The wallet lock doesn't really apply I guess since the GUI default is 10min vs 1:30min in CLI, and you can still interact in the GUI while it's connecting

@jeffro256
Copy link
Contributor

For those who want to reproduce the bad behavior, where in this PR @j-berman added = false, change it to = true. C++ initialization is terrible :(. I wonder if there is something suppressing the uninitialized warning?

@j-berman
Copy link
Collaborator Author

Thank you for helping test @hinto-janaiyo, much appreciated.

@jeffro256 I get the warning if I try to read the variable in the line after it's declared, so seems it's something to do with how the variable is used in connect -- generally has the feel of a compiler bug to me. Also unfortunate that my system seems to default initialize the value to false (in order to repro this comment, I either had to explicitly initialize as true like you suggested, or call set_daemon multiple times; I specifically tested all those cases when I worked on it).

@j-berman j-berman mentioned this pull request Sep 24, 2022
@luigi1111 luigi1111 merged commit 0bef426 into monero-project:release-v0.18 Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants