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

asb fails to watch Monero lock transaction properly unless monero-wallet-rpc and asb are restarted #652

Open
sethforprivacy opened this issue Aug 19, 2021 · 5 comments
Labels
bug Something isn't working

Comments

@sethforprivacy
Copy link
Contributor

sethforprivacy commented Aug 19, 2021

I have now run into this twice, and wanted to be sure to open an issue as it seems to be a persistent issue.

For some swaps (2/10 for me, so far) the Monero lock transaction details are not properly gathered/watched by the asb with the following logs:

asb:

DEBUG swap{id=REDACTED}: Failed to retrieve tx from blockchain: JSON-RPC request failed with code -1: Failed to get transaction from daemon: JSON-RPC request failed with code -1: Failed to get transaction from daemon txid=REDACTED

monero-wallet-rpc:

2021-08-19 14:59:38.698	[RPC0]	ERROR	wallet.wallet2	src/wallet/wallet2.cpp:11442	!ok || (res.txs.size() != 1 && res.txs_as_hex.size() != 1). THROW EXCEPTION: error::wallet_internal_error

A couple more notes:

  • You cannot restart monero-wallet-rpc without also restarting asb, as the asb tool cannot properly load the wallet file when running.
  • The transaction gets sent despite the bug, just does not get seen and so the swap hangs until you restart the ASB.
  • Restarting the ASB so far seems to resolve almost all issues, but there may be some work that can be done to recover without restarting the service, but not sure here.
@sethforprivacy
Copy link
Contributor Author

99% sure I was the other side of this. Let me know if you need any logs.

Any logs you have would be great, just be sure to redact any swap ID/TXID as desired before posting here to preserve your own privacy!

@thomaseizinger
Copy link
Contributor

Thanks for reporting. This seems to be an issue with monero-wallet-rpc from what I can see.

I guess a meaningful thing to address here would be to allow for monero-wallet-rpc to be restarted without restarting the ASB. I will look into that.

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Aug 20, 2021

Failed to get transaction from daemon

That is an error message from monero-wallet-rpc. Some quick web searching reveals that this just happens occasionally. I don't think there is anything we can do to fix this on our side.

To confirm that: Were you able to fix this issue by just restarting the ASB? Or did you restart monero-wallet-rpc as well?

What we can do is what I suggested above: Make it possible to restart monero-wallet-rpc without having to restart the ASB.

thomaseizinger added a commit that referenced this issue Aug 20, 2021
Previously, we relied on the wallet in the `monero-wallet-rpc` daemon
to be loaded as we do on startup. As a consequence of this expectation,
restarting `monero-wallet-rpc` to fix bugs like #652 resulted in the
ASB no longer operating correctly.

To fix this, we now load the wallet on-demand in case the daemon responds
with the error code -13.

Ideally, we would implement this behaviour generically using the proxy
pattern on the `MoneroWalletRpc` trait. Unfortunately, when attempting
to do so we uncover a limitation in the design of `jsonrpc_client`.
This limitation is tracked in thomaseizinger/rust-jsonrpc-client#47.
Once fixed, we can implement this logic in a more robust way that is not
tied to the `check_tx_key` RPC call but applies to any RPC call automatically.
@thomaseizinger
Copy link
Contributor

thomaseizinger commented Aug 20, 2021

What we can do is what I suggested above: Make it possible to restart monero-wallet-rpc without having to restart the ASB.

I've opened a PR that should allow this: #657

I don't know how exactly you are operating your daemons but as a general approach, I think you will need some kind of monitoring that alerts you of issues like this. In this particular case, you would have to monitor the number of errors being logged in the monero-wallet-rpc daemon's log. As a result of too many errors, you could implemented an automated restart strategy. With the linked PR, the ASB should play nicely with such a behaviour now.

We are also logging a particular log message on warn level now for this exact case, so you can also monitor for that if you want.

@sethforprivacy
Copy link
Contributor Author

What we can do is what I suggested above: Make it possible to restart monero-wallet-rpc without having to restart the ASB.

I've opened a PR that should allow this: #657

I don't know how exactly you are operating your daemons but as a general approach, I think you will need some kind of monitoring that alerts you of issues like this. In this particular case, you would have to monitor the number of errors being logged in the monero-wallet-rpc daemon's log. As a result of too many errors, you could implemented an automated restart strategy. With the linked PR, the ASB should play nicely with such a behaviour now.

We are also logging a particular log message on warn level now for this exact case, so you can also monitor for that if you want.

Glad to hear it! The new ASB log will be helpful as well as the RPC logs are a bit noisy.

I'm working on actionable monitoring as we speak via Zabbix, but the more specific logs I can get, the better 👍

Will test out the new version once it's built!

@thomaseizinger thomaseizinger changed the title [Bug] asb fails to watch Monero lock transaction properly unless monero-wallet-rpc and asb are restarted asb fails to watch Monero lock transaction properly unless monero-wallet-rpc and asb are restarted Aug 21, 2021
@thomaseizinger thomaseizinger added the bug Something isn't working label Aug 21, 2021
thomaseizinger added a commit that referenced this issue Aug 26, 2021
Previously, we relied on the wallet in the `monero-wallet-rpc` daemon
to be loaded as we do on startup. As a consequence of this expectation,
restarting `monero-wallet-rpc` to fix bugs like #652 resulted in the
ASB no longer operating correctly.

To fix this, we now load the wallet on-demand in case the daemon responds
with the error code -13.

Ideally, we would implement this behaviour generically using the proxy
pattern on the `MoneroWalletRpc` trait. Unfortunately, when attempting
to do so we uncover a limitation in the design of `jsonrpc_client`.
This limitation is tracked in thomaseizinger/rust-jsonrpc-client#47.
Once fixed, we can implement this logic in a more robust way that is not
tied to the `check_tx_key` RPC call but applies to any RPC call automatically.
thomaseizinger added a commit that referenced this issue Aug 26, 2021
Previously, we relied on the wallet in the `monero-wallet-rpc` daemon
to be loaded as we do on startup. As a consequence of this expectation,
restarting `monero-wallet-rpc` to fix bugs like #652 resulted in the
ASB no longer operating correctly.

To fix this, we now load the wallet on-demand in case the daemon responds
with the error code -13.

Ideally, we would implement this behaviour generically using the proxy
pattern on the `MoneroWalletRpc` trait. Unfortunately, when attempting
to do so we uncover a limitation in the design of `jsonrpc_client`.
This limitation is tracked in thomaseizinger/rust-jsonrpc-client#47.
Once fixed, we can implement this logic in a more robust way that is not
tied to the `check_tx_key` RPC call but applies to any RPC call automatically.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants