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

Monero wallet refresh fix #1487

Merged
merged 12 commits into from
Mar 25, 2024
Merged

Monero wallet refresh fix #1487

merged 12 commits into from
Mar 25, 2024

Conversation

binarybaron
Copy link
Collaborator

This PR changes the following behaviour in the refresh functionality of the monero wallet

  • Allows for multiple retries because in some cases users have experienced an issue where the wallet rpc returns no connection to daemon even though the daemon is available. I'm not 100% sure why this happens but retrying often fixes the issue
  • Attempt to print the current sync height while the wallet is syncing. This only works to some degree because the monero-wallet-rpc stops responding (or takes a long time to respond) while it's refreshing
  • The monero-wallet-rpc is started with the --no-initial-sync flag which ensures that as soon as it's started, it's ready to respond to requests
  • The monero-wallet-rpc was upgraded to v0.18.3.1 because this PR Wallet refresh improvements monero-project/monero#8941 has improved some of the issues mentioned above

This PR is part of a larger effort to fix this issue #1432

@binarybaron binarybaron requested a review from delta1 December 14, 2023 16:30
@binarybaron binarybaron force-pushed the monero-wallet-refresh-fix branch 2 times, most recently from 345e6c0 to c499e9d Compare December 14, 2023 16:37
@binarybaron
Copy link
Collaborator Author

This is still a draft. Please don't merge yet :)

@delta1 delta1 marked this pull request as draft December 14, 2023 18:06
@binarybaron binarybaron force-pushed the monero-wallet-refresh-fix branch from b9c7736 to 7697f42 Compare December 29, 2023 12:35
@binarybaron
Copy link
Collaborator Author

@delta1 This is ready for review

@binarybaron binarybaron force-pushed the monero-wallet-refresh-fix branch from 7697f42 to 61ae757 Compare December 29, 2023 12:41
This commit changes the following behaviour in the refresh functionality of the monero wallet
- Allows for multiple retries because in some cases users have experienced an issue where the wallet rpc returns `no connection to daemon` even though the daemon is available. I'm not 100% sure why this happens but retrying often fixes the issue
- Attempt to print the current sync height while the wallet is syncing. This only works to some degree because the `monero-wallet-rpc` stops responding (or takes a long time to respond) while it's refreshing
- The `monero-wallet-rpc` is started with the `--no-initial-sync` flag which ensures that as soon as it's started, it's ready to respond to requests
@binarybaron binarybaron force-pushed the monero-wallet-refresh-fix branch from 61ae757 to 48abcd5 Compare December 29, 2023 12:54
@delta1 delta1 marked this pull request as ready for review December 29, 2023 13:38
Copy link
Collaborator

@delta1 delta1 left a comment

Choose a reason for hiding this comment

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

Nice LGTM. Do you want to add a note to the changelog? At least for bumping the Monero version

@delta1
Copy link
Collaborator

delta1 commented Feb 5, 2024

seems like some tests are now running into the 6 hour CI time limit?

@delta1 delta1 self-requested a review February 5, 2024 20:18
@binarybaron
Copy link
Collaborator Author

Nice LGTM. Do you want to add a note to the changelog? At least for bumping the Monero version

Good catch. I'll add one

@delta1
Copy link
Collaborator

delta1 commented Feb 7, 2024

@binarybaron a few of the ci jobs are not working with this change

@binarybaron binarybaron merged commit d8dacbd into master Mar 25, 2024
18 of 23 checks passed
@binarybaron binarybaron deleted the monero-wallet-refresh-fix branch March 25, 2024 14:08
@delta1
Copy link
Collaborator

delta1 commented Mar 25, 2024

@binarybaron a few of the ci jobs are not working with this change

@binarybaron this PR had broken one or more CI jobs.

@binarybaron
Copy link
Collaborator Author

Okay. I'll revert this PR if the CI jobs fail on master

@binarybaron
Copy link
Collaborator Author

@delta1 Okay it seems like some of the tests will time out. Looking at the logs it seems that the CI is stuck at refreshing the Monero wallet which seems related to this PR. I have no idea why.

When refreshing the CI calls this

pub async fn refresh(&self, max_attempts: usize) -> Result<Refreshed> {

function with max_attempts=1 .

This line

tracing::info!(name = %self.name, attempt=i, "Syncing Monero wallet");

is never reached. Nowhere in the logs is this log message to be found. The other tokio task which is responsible for logging the sync progress is called and prints periodic log messages.

Do you have any idea what the issue could be here? I'm clueless. Here is the entire function for reference:

pub async fn refresh(&self, max_attempts: usize) -> Result<Refreshed> {
const GET_HEIGHT_INTERVAL: Duration = Duration::from_secs(5);
const RETRY_INTERVAL: Duration = Duration::from_secs(2);
let inner = self.inner.lock().await;
// Cloning this is relatively cheap because reqwest::Client is a wrapper around an Arc
let inner_clone = inner.clone();
let wallet_name_clone = self.name.clone();
let refresh_task = tokio::task::spawn(async move {
loop {
let height = inner_clone.get_height().await;
match height {
Err(error) => {
tracing::debug!(name = %wallet_name_clone, %error, "Failed to get current Monero wallet sync height");
}
Ok(height) => {
tracing::debug!(name = %wallet_name_clone, current_sync_height = height.height, "Syncing Monero wallet");
}
}
tokio::time::sleep(GET_HEIGHT_INTERVAL).await;
}
});
let refresh_result = tokio::select! {
biased;
_ = refresh_task => {
unreachable!("Current sync height refresh task should never finish")
}
refresh_result = async {
for i in 1..=max_attempts {
tracing::info!(name = %self.name, attempt=i, "Syncing Monero wallet");
let result = inner.refresh().await;
match result {
Ok(refreshed) => {
tracing::info!(name = %self.name, "Monero wallet synced");
return Ok(refreshed);
}
Err(error) => {
let attempts_left = max_attempts - i;
tracing::warn!(attempt=i, %attempts_left, name = %self.name, %error, "Failed to sync Monero wallet");
if attempts_left == 0 {
tracing::error!(name = %self.name, %error, "Failed to sync Monero wallet");
return Err(error);
}
}
}
tokio::time::sleep(RETRY_INTERVAL).await;
}
unreachable!("Loop should always return before it breaks")
} => {
refresh_result
}
};
Ok(refresh_result?)
}
}

@binarybaron binarybaron restored the monero-wallet-refresh-fix branch March 25, 2024 15:26
delta1 added a commit that referenced this pull request Mar 26, 2024
@delta1
Copy link
Collaborator

delta1 commented Mar 26, 2024

revert in #1593 so we can recreate this while debugging

@delta1
Copy link
Collaborator

delta1 commented Mar 26, 2024

recreated this PR in #1594

delta1 added a commit that referenced this pull request Mar 26, 2024
…-refresh-fix

Revert "Monero wallet refresh fix (#1487)"
delta1 added a commit that referenced this pull request Mar 26, 2024
this is a squashed commit of the changes proposed by @binarybaron in #1487

Co-Authored-By: binarybaron <86064887+binarybaron@users.noreply.github.com>
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.

2 participants