-
Notifications
You must be signed in to change notification settings - Fork 102
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
swap: stuck due to low fee (potential solutions) #3135
Comments
I'm not sure atm why we don't automatically increase evm tx fees. The philosophy behind not watching the swap initiation tx so much is that it's not usually a threat of losing funds. Plus it happens now so it's unlikely that, if we have tx fee info now, it isn't mined. In a few hours whenever we might need to send the next tx we expect fee to be unknown. Perhaps you are right though and we should monitor swap tx. Just monitor all the things. |
I want to add an Maybe we should also monitor init but I don't know since it usually isn't a problem (afaik). @buck54321 what do you think? |
I think these are all good ideas, but each one opens a can of worms. Automatically spending more than we have reserved is super-problematic, especially so for bots. I'll put some more thought into this and we can touch base again next week. Or feel free to bring it up on Matrix too. |
I see what you mean with respect to fee reserves - if wallet spends all its funds/reserves on initiating a bunch of Swaps there might not be enough left to Redeem/Refund to finish corresponding trades, I think one solution would be to hard-limit how many matches an order can have. This would allow to solve 2 issues:
So, lets say we limit matches per order at 16 (I don't see a strong reason for it to be much higher). Client would simply keep track of this and refuse to initiate 17th match and beyond. And Server would also need to permit revocation for an order that has reached its limit (perhaps it should issue server-side revocation in that case, I guess that mirrors existing "revocation due to client inactivity" mechanism nicely). As for fee reserves, we lock funds that cover |
It is possible for Swap transaction to get stuck for 8+ hours (and/or never get mined),
I'm not sure what happens in case such Swap transaction does get mined eventually - I'm assuming Bison wallet will realize it needs to refund it ASAP and will send a Refund transaction (for Maker that's safe behavior, for Taker that's safe until 20h into the Match),
but what happens in case such Swap transaction never gets mined is - corresponding Match ends up in a hanging state where "it thinks Swap happened" (even thought it actually didn't) and "it thinks Refund is coming" (even though Bison wallet cannot possibly refund it since there is nothing to refund); all the while wallet also thinks Swap transaction is pending in mempool (which is technically correct but might be confusing to a User, so it's probably best to clean it up after Match state is resolved one way or another), here is how such stuck Match looks like:
I think there are at least two different ways this can be addressed:
Another note, #3131 also probably depends on the resolution of this issue since
Swapping
balance (underLocked
section) keeps showing funds reserved by stuck Swap transaction even after I've sent another Polygon transaction with the same nonce that got mined (effectively replacing/invalidating that Swap transaction completely) - which is probably due to corresponding Match still expecting Refund that will never happen.Another aspect of this issue - it seems bumping fee for Swap transaction for EVM-based assets (through manual user-action request) isn't working correctly because if Swap
tx1
was replaced bytx2
(with higher fee) Match won't know about it and we keep trying to convince servertx1
is the Swap transaction to look for (we do refund Swaptx2
successfully in that case though, that's good 👍 )it seems Redeems/Refunds would suffer from exactly the same issue of Match being unaware of fee-bumped transaction - but the code handle this for Redeems/Refunds exists (see code around
ConfirmTransaction
in https://github.com/decred/dcrdex/pull/3082/files) ... and that probably means we needThe text was updated successfully, but these errors were encountered: