-
Notifications
You must be signed in to change notification settings - Fork 19
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
add delay between xmr auto-funding reattempts #733
add delay between xmr auto-funding reattempts #733
Conversation
@@ -1137,6 +1137,7 @@ fn attempt_transition_to_end( | |||
break; | |||
} else { | |||
warn!("{} | Auto-funding Monero transaction failed with {}, retrying, {} retries left", &swap_id.bright_blue_italic(), err, retries); | |||
tokio::time::sleep(std::time::Duration::from_secs(retries)).await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fear this introduces too much of a delay. In the worst case, it blocks the entire daemon for 55 seconds. If you really need this, I would at the least lower this to either 10's or 100's of ms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the worst case, it blocks the entire daemon for 55 seconds.
Don't exaggerate: it's only 45! :P
Reduced to 10ms increments in 82764f4, so upper bound 450 ms. If this catches most failures within the first two retries, let's cut by another order of magnitude.
@Lederstrumpf do we still want to add some delays, or did the recent refactors also helped on this? |
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
stats from last run: [2022-12-28T18:46:35Z INFO farcaster_node::farcasterd::stats] Swapped(4241) | Refunded(33) / Punished(0) | Aborted(0) | Initialized(4314) / AwaitingFundingXMR(0) / AwaitingFundingBTC(0) / FundedXMR(2125) / FundedBTC(2150) / FundingCanceledXMR(8) / FundingCanceledBTC(0) Autofunding reattempt stats ( 2 Auto-funding Monero transaction failed, pushing to cli, use `swap-cli needs-funding Monero` to retrieve address and amount
1 Auto-funding Monero transaction failed with Server error: Failed to get height, retrying, 1 retries left
1 Auto-funding Monero transaction failed with Server error: Failed to get height, retrying, 2 retries left
1 Auto-funding Monero transaction failed with Server error: Failed to get height, retrying, 3 retries left
1 Auto-funding Monero transaction failed with Server error: Failed to get height, retrying, 4 retries left
1 Auto-funding Monero transaction failed with Server error: Failed to get height, retrying, 5 retries left
1 Auto-funding Monero transaction failed with Server error: Failed to get height, retrying, 6 retries left
1 Auto-funding Monero transaction failed with Server error: Failed to get height, retrying, 7 retries left
1 Auto-funding Monero transaction failed with Server error: Failed to get height, retrying, 8 retries left
9 Auto-funding Monero transaction failed with Server error: Failed to get height, retrying, 9 retries left
2 Auto-funding Monero transaction failed with Server error: failed to get output distribution, retrying, 1 retries left
2 Auto-funding Monero transaction failed with Server error: failed to get output distribution, retrying, 2 retries left
3 Auto-funding Monero transaction failed with Server error: failed to get output distribution, retrying, 3 retries left
3 Auto-funding Monero transaction failed with Server error: failed to get output distribution, retrying, 4 retries left
3 Auto-funding Monero transaction failed with Server error: failed to get output distribution, retrying, 5 retries left
3 Auto-funding Monero transaction failed with Server error: failed to get output distribution, retrying, 6 retries left
4 Auto-funding Monero transaction failed with Server error: failed to get output distribution, retrying, 7 retries left
4 Auto-funding Monero transaction failed with Server error: failed to get output distribution, retrying, 8 retries left
4 Auto-funding Monero transaction failed with Server error: failed to get output distribution, retrying, 9 retries left So of the 13 reattempt sequences, 2 failures weren't caught by current reattempts and ended up in a refund. That's 0.05% of the swaps of the run, or 6% of failures. As @TheCharlatan observed, adding a delay on order of seconds can lead to massive congestion for all running swaps. The failure rate's too low to warrant this, but also high enough to warrant at least some delay, imo (lest liquidity providers run an external autofunding monitor, which I think is too much if we can handle with small fc daemon delay). I've decreased the delay hundredfold and will add to next fc.dev run. If it catches most failures, we can try another order of magnitude decrease in the delay. If it doesn't catch most failures, imo PR should be closed, and liquidity providers handle externally if need be. |
well: this asynced & @TheCharlatan already merged ^^. Will revert if delay doesn't address the autofund failures. |
On #707, the only non-intentional refunds I've found so far have been from xmr auto-funding.
Unless due to insufficient unlocked money, they've typically been down to temporary monerod irresponsiveness.
We reattempt autofunding for monero as of #686.
When excluding insufficient unlocked money, last week's swaps' statistical distribution of auto-funding reattempts until success (or resignation, if 10 attempts didn't suffice) is the following:
So we're succeeding about 1/3rd of the time on the first reattempt, else it most likely simply fails. Right now, we're reattempting the call right away, so it mostly occurs within a few milliseconds of the last. But based on the statistical distribution, I suspect we'd do much better by introducing a delay, similar as done for transaction polling.
I'm using a gradually increasing delay amount here to better capture the tail end of failures.
Failure example:
farcasterd:
monero-wallet-rpc: