fix(swap): Monero wallet thread safety

[Einliterflasche]

Our current approach to thread safety regarding monero::Wallet, which we pass around as Arc<monero::Wallet>, is that it contains a Mutex::monero_rpc::wallet::Client and acquires that before using the monero wallet rpc process. Since it does that in each function, calling i.e. create_from_and_load from create_from_keys_and_sweep results in the lock being acquired, released and then acquired again, even though we expect an atomic operation. This results in a race condition where another thread acquires the lock after it is first released, even though the function isn't finished yet.

We need to migrate to an API where we can guarantee that multiple functions can be called without releasing the lock. The best way to do this would be to always pass Arc<Mutex<monero::Wallet>> and have the caller lock the mutex. Any call to the monero::Wallet would then be guaranteed to happen within a single lock acquisition. However, this would introduce a lot of (small) changes, since we use Arc<monero::Wallet> quite a lot.

@binarybaron can you think of another way to fix this with less friction, that is similarly robust?

[binarybaron]

I need to think about this a bit more but how long would you like to keep the lock? Over the duration of which process ?

[Einliterflasche]

This race condition is the cause of the refund failures of #274. If we take a look at these logs:

{"timestamp":"2025-03-26T21:53:28.464127523Z","level":"INFO","fields":{"message":"Syncing Monero wallet","name":"asb-wallet","attempt":1},"span":{"id":"773...","name":"swap"},"spans":[{"id":"773...","name":"swap"}]}
{"timestamp":"2025-03-26T21:54:44.403850664Z","level":"INFO","fields":{"message":"Syncing Monero wallet","name":"asb-wallet","attempt":1},"span":{"id":"50f...","name":"swap"},"spans":[{"id":"50f...","name":"swap"}]}
{"timestamp":"2025-03-26T21:54:58.127906703Z","level":"INFO","fields":{"message":"Monero wallet synced","name":"asb-wallet"},"span":{"id":"773...","name":"swap"},"spans":[{"id":"773...","name":"swap"}]}
{"timestamp":"2025-03-26T21:54:58.128939883Z","level":"INFO","fields":{"message":"Monero wallet synced","name":"asb-wallet"},"span":{"id":"50f...","name":"swap"},"spans":[{"id":"50f...","name":"swap"}]}
{"timestamp":"2025-03-26T21:54:59.207398055Z","level":"INFO","fields":{"message":"Monero transferred back to default wallet","tx":"2ab...","monero_address":"4BA..."},"span":{"id":"773...","name":"swap"},"spans":[{"id":"773...","name":"swap"}]}
{"timestamp":"2025-03-26T21:54:59.290489473Z","level":"WARN","fields":{"message":"Failed to refund Monero. We will retry in 0 seconds","swap_id":"50f...","error":"Failed to transfer Monero to default wallet\n\nCaused by:\n    0: JSON-RPC request failed with code -4: No unlocked balance in the specified account\n    1: JSON-RPC request failed with code -4: No unlocked balance in the specified account"},"span":{"id":"50f...","name":"swap"},"spans":[{"id":"50f...","name":"swap"}]}

Then the asb is trying to sync 2 monero wallets at the same, which is not possible with monero-wallet-rpc. We need cannot release the lock unless we finish the complete sequence (opening wallet, sync, sweep) or else monero-wallet-rpc will be in an unexpected (wrong) state and cause refund issues etc.
This is triggered any time the asb tries to refund 2 swaps whose timelock expires in the same block, i.e. often (for busy makers).

[binarybaron]

This race condition is the cause of the refund failures of #274. If we take a look at these logs:

{"timestamp":"2025-03-26T21:53:28.464127523Z","level":"INFO","fields":{"message":"Syncing Monero wallet","name":"asb-wallet","attempt":1},"span":{"id":"773...","name":"swap"},"spans":[{"id":"773...","name":"swap"}]}
{"timestamp":"2025-03-26T21:54:44.403850664Z","level":"INFO","fields":{"message":"Syncing Monero wallet","name":"asb-wallet","attempt":1},"span":{"id":"50f...","name":"swap"},"spans":[{"id":"50f...","name":"swap"}]}
{"timestamp":"2025-03-26T21:54:58.127906703Z","level":"INFO","fields":{"message":"Monero wallet synced","name":"asb-wallet"},"span":{"id":"773...","name":"swap"},"spans":[{"id":"773...","name":"swap"}]}
{"timestamp":"2025-03-26T21:54:58.128939883Z","level":"INFO","fields":{"message":"Monero wallet synced","name":"asb-wallet"},"span":{"id":"50f...","name":"swap"},"spans":[{"id":"50f...","name":"swap"}]}
{"timestamp":"2025-03-26T21:54:59.207398055Z","level":"INFO","fields":{"message":"Monero transferred back to default wallet","tx":"2ab...","monero_address":"4BA..."},"span":{"id":"773...","name":"swap"},"spans":[{"id":"773...","name":"swap"}]}
{"timestamp":"2025-03-26T21:54:59.290489473Z","level":"WARN","fields":{"message":"Failed to refund Monero. We will retry in 0 seconds","swap_id":"50f...","error":"Failed to transfer Monero to default wallet\n\nCaused by:\n    0: JSON-RPC request failed with code -4: No unlocked balance in the specified account\n    1: JSON-RPC request failed with code -4: No unlocked balance in the specified account"},"span":{"id":"50f...","name":"swap"},"spans":[{"id":"50f...","name":"swap"}]}

Then the asb is trying to sync 2 monero wallets at the same, which is not possible with monero-wallet-rpc. We need cannot release the lock unless we finish the complete sequence (opening wallet, sync, sweep) or else monero-wallet-rpc will be in an unexpected (wrong) state and cause refund issues etc. This is triggered any time the asb tries to refund 2 swaps whose timelock expires in the same block, i.e. often (for busy makers).

You are totally correct here. I have encountered this myself on my dev server.

[binarybaron]

This also means we cannot provide quotes while we are syncing a refund wallet. Which is ok, I guess but is properly not expected behaviour.

[Einliterflasche]

Combined with #288 this would probanly mostly resolve the issue, though? We could also return the cached quote while working on creating another one.

[binarybaron]

Combined with #288 this would probanly mostly resolve the issue, though? We could also return the cached quote while working on creating another one.

I'd stay with the current policy of only returning a quote if the TTL is accurate. We don't want to return the same outdated quote over a long period (if e.g the syncing takes a long time). A user cannot initiate a swap while we're syncing anyway because we need to sync our main asb_wallet before initiating a swap (for the wallet snapshot)

This will be fixed once #244 is merged. No need to prevent to have concurrency, if we don't support it.

We should still merge this PR though (if it wasn't clear)

[binarybaron]

We can add a timeout for the acquisition of the Mutex lock for some operation.

If we want to serve a quote and can't lock the Monero wallet for >60s it might not make sense anymore to even bother with responding anymore.

[Einliterflasche]

We can add a timeout for the acquisition of the Mutex lock for some operation.

If we want to serve a quote and can't lock the Monero wallet for >60s it might not make sense anymore to even bother with responding anymore.

I think we just waited for the lock (without timeout) before this change too. But you're right, it totally makes sense to implement this here.

[Einliterflasche]

Currently, there still seems to be an instance of overly long lock holding and subsequent starvation of other tasks in wait_for_confirmations. Why this happens currently eludes me.

[Einliterflasche]

Tests are passing now, could you review this @binarybaron @delta1?

Edit: there is one test (reopens_wallet_in_case_not_available) which is probably failing

[binarybaron]

Tests are passing now, could you review this @binarybaron @delta1?

Edit: there is one test (reopens_wallet_in_case_not_available) which is probably failing

I'll review this aspa.

Our testnet asb is running this branch.

[binarybaron]

@Einliterflasche Have you added the changes from #260?

[Einliterflasche]

Not yet

[binarybaron]

some comments about Mutex / RwLock

[binarybaron]

LGTM but let's do some manual testing before merging this.

[Einliterflasche]

Edit: there is one test (reopens_wallet_in_case_not_available) which is probably failing

The test runs fine on my machine, but timed out in the action. I'll rerun the action, but it should be fine.

[binarybaron]

I keep getting this error when trying to do a testnet swap: Failed to complete swap: Fee per byte returned by electrum server is negative: -1. This may indicate that fee estimation is not supported by this server

This isn't related to your changes but it's preventing me from testing this PR. Can you try doing a swap?

[Einliterflasche]

I'm adding some changes from #260 into this PR.

[Einliterflasche]

There was a deadlock possibility in wait_for_confirmations_with in one match arm, which I fixed now. I also verified that no other match arm can exhibit that behaviour.

[binarybaron]

There was a deadlock possibility in wait_for_confirmations_with in one match arm, which I fixed now. I also verified that no other match arm can exhibit that behaviour.

Awesome. LGTM. Is this good to merge?

[Einliterflasche]

I think so. Since you agree, I'll go ahead and merge this.

[binarybaron]

Please also add a changelog entry.

Awesome that we finally fixed this 💪🚀