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

fix: monero wallet refresh #1596

Merged
merged 7 commits into from
Jun 10, 2024
Merged

fix: monero wallet refresh #1596

merged 7 commits into from
Jun 10, 2024

Conversation

delta1
Copy link
Collaborator

@delta1 delta1 commented Mar 26, 2024

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
  • Print the current sync height after each failed attempt at syncing to see how far we've come
  • 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

const GET_HEIGHT_INTERVAL: Duration = Duration::from_secs(5);
const RETRY_INTERVAL: Duration = Duration::from_secs(2);

let inner = self.inner.lock().await;
Copy link
Contributor

Choose a reason for hiding this comment

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

@binarybaron doing a bit of debugging on this, it appears this line never returns, at least for given_alice_and_bob_manually_refund_after_funds_locked_both_refund

Copy link
Contributor

Choose a reason for hiding this comment

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

Stack trace looks like

swap::monero::wallet::Wallet::refresh::{{closure}} wallet.rs:269
swap::monero::wallet::Wallet::create_from::{{closure}} wallet.rs:148
swap::protocol::alice::state::State3::refund_xmr::{{closure}} state.rs:527
swap::asb::recovery::refund::refund::{{closure}} refund.rs:82
alice_and_bob_refund_using_cancel_and_refund_command::given_alice_and_bob_manually_refund_after_funds_locked_both_refund::{{closure}}::{{closure}}::{{closure}} alice_and_bob_refund_using_cancel_and_refund_command.rs:76
alice_and_bob_refund_using_cancel_and_refund_command::harness::setup_test::{{closure}} mod.rs:140
alice_and_bob_refund_using_cancel_and_refund_command::given_alice_and_bob_manually_refund_after_funds_locked_both_refund::{{closure}} alice_and_bob_refund_using_cancel_and_refund_command.rs:82

Inside refund_xmr, immediately before the create_from call there is a call to watch_for_transfer which itself calls wait_for_confirmations which also locks the client mutex. From the little I know about these locks they should unlock when it goes out of scope, but perhaps there is something going on with the borrowing that keeps it in scope? Not sure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot. @delta1 do you know what exact issue could be? I haven't been able to figure it out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't even understand why the HTTP Clients needs to be a Mutex. That seems so unnecessary and complicates everything...

Co-Authored-By: binarybaron <[email protected]>
Co-Authored-By: Byron Hambly <[email protected]>
@binarybaron
Copy link
Collaborator

The tests are passing. Can you give this another review? @delta1 @Einliterflasche

@delta1
Copy link
Collaborator Author

delta1 commented Jun 7, 2024

The tests are passing.

No it seems the same tests are timing out

@binarybaron
Copy link
Collaborator

The tests are passing.

No it seems the same tests are timing out

Now they are passing :)

@binarybaron
Copy link
Collaborator

@delta1 I'm going to merge this. Tests are passing and the code is relatively simple. Please ping me if you want to revert something.

@binarybaron binarybaron merged commit 90494ba into master Jun 10, 2024
27 checks passed
@binarybaron binarybaron deleted the wallet-refresh branch June 10, 2024 16:53
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.

3 participants