Farcasterd: Add 10 retries for Monero auto-funding

[TheCharlatan]

No description provided.

[h4sh3d]

I guess one of the main reasons auto-funding would fail is because no utxo is available. Do we want to add a small timeout (or did I missed it) between the 10 trials?

[h4sh3d]

I see that "not enough money" will exit early and is probably the returned message went no utxo is available. By available I mean e.g. unlocked for monero.

Maybe it is impossible to know that and the current code cannot get better.

[Lederstrumpf]

I see that "not enough money" will exit early and is probably the returned
message went no utxo is available. By available I mean e.g. unlocked for monero.

Maybe it is impossible to know that and the current code cannot get better.

@h4sh3d you're correct that this PR does not mitigate the "not enough money" issue, nor "not enough unlocked money".

That's also not the issue I'd like this PR to address (unless of course can by printing XMR :P) - the only one I want solved for now is when the monero daemon intermittently doesn't respond. Auto-funding failing because no utxos are available means either

1. not enough funds, so no waiting will help, unless auto-funding address is funded later by Alice.
2. insufficient unlocked funds, so we'll wait for at least another block. In this case, I'd prefer that we retrigger the autofunding attempt when processing the Event::HeightChanged syncer event.

I'm (very slightly) opposed to providing reattempted autofunding in case of 2. as an optional configuration (instead of pushing it to needs-funding, as it will currently do), since these auto-funding failures don't have safety implications and I'd rather have operators of automated instances make their setup more robust by ensuring a stable liquidity supply than providing a mechanism for automatically processing such backlogs, masking an issue that they can avoid.
My philosophy for this is similar to the over-/under-funding discussion #534, but it's less applicable here, so my opposition in this case is much weaker, and am happy to be convinced otherwise :)

[Lederstrumpf]

(sorry for double post - trying out new external github tooling)

[Lederstrumpf]

tACK, thank you very much for this change!

Over the last 1184 swaps, I've had 24 initial autofunding failures from no connection to daemon on the maker and 47. On the maker's much more powerful setup, all of its 24 autofunding reattempts worked within one retry. On the parallel takers' less powerful setup (which also has much more congestion due to 40 parallel monero-wallet-rpcs), all but 6 of the initial autofunding failures were handled within one retry, the others within two. So this PR's solution of simply reattempting works incredibly well, and its conservative bound of 10 retries is appropriate imo :)

As mentioned in my last comment, I think the only question is whether we also handle not enough unlocked money. Once others weigh in on this, I'm more than happy to move ahead with merging.

[Lederstrumpf]

this doesn't catch "not enough unlocked money", which is futile to reattempt prior to a block height increase.

so I'd either

• add a match for this (catchall not enough, or not enough money || not enough unlocked money, or negated no connection to daemon) or
• handle not enough unlocked money in a separate conditional which inserts to self.funding_xmr with the autofund bool = true (to avoid potential double-funding via a manual attempt) and delays handling insufficient unlocked money by reattempting upon a block height increase

[Lederstrumpf]

resolved in 4b32ea4

[Lederstrumpf]

(sorry for double post - trying out new external github tooling)

[h4sh3d]

I agree that this patch should only take of retrying in case the RPC is not responding for any reasons and not for asset management (not enough money, unlocked money, etc.) This second case should either be external or at a level above when blockchain pace is known.

[h4sh3d]

cACK

[TheCharlatan]

ACK - thank you for the patches @Lederstrumpf