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

Send & Chain: Localize refund flow to swap handlers #407

Merged
merged 28 commits into from
Sep 20, 2024

Conversation

hydra-yse
Copy link
Collaborator

@hydra-yse hydra-yse commented Jul 23, 2024

This PR aims at fixing the refund flow loop, since it used to block the status stream thread while waiting for lockups to be confirmed. The logic has been moved to the swap handlers, so they are now independent from the swapper (currently only used for broadcasting). The logic for constructing the refund tx (only Liquid) has also been moved to the SDK side due to problems with lowball, and the requirement to fetch from our Esplora.

The current improvements make cooperative refunds happen almost instantaneously, and with no reliance on the swapper. In case of no Boltz status update, they will be refunded non-cooperatively on locktime expiry (via LiquidSdk::track_pending_swaps).

@hydra-yse hydra-yse self-assigned this Jul 23, 2024
@ok300 ok300 linked an issue Jul 24, 2024 that may be closed by this pull request
Copy link
Collaborator

@dangeross dangeross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think by not explicitly checking the swap expiration and first relying on the swap state you are assuming Boltz is always being compliant (or functional). If swap a state is not changed to RefundPending (for outgoing chain/send swaps) in the stream updates, then there is never an attempt to refund

lib/core/src/sdk.rs Outdated Show resolved Hide resolved
lib/core/src/sdk.rs Outdated Show resolved Hide resolved
@roeierez
Copy link
Member

@hydra-yse IMO the coopertive refund which should in theory always succeed should be executed immeditely as soon as we understand we need it so having it as part of the status stream as before makes sense to me.
I do agree that making the non cooperative refund only in one place (the watcher task) is a good idea since anyway we will execute it only on exiration and we are not relying on boltz statuses for that but only on chain information.

@roeierez
Copy link
Member

roeierez commented Aug 4, 2024

The code as is is not clear and hard to understand (not because of this PR) so I certainly understand the motivation to do some cleanup here.
IMO a clean approach would be:

  1. Create a new module (file) refund.rs
  2. In there we will have two structures (SendSwapRefunder, ChainSwapRefunder)
  3. Each of them has two functions:
  • Refund(swaps: []swap) - will attempt first cooperatively and if fail and timeout is applicable will try non cooperatively.
  • RefundExpired() - which will query the db for those that are refundables and use the former method to refund.

Now our two cases can use the above functinoality:

  1. From the status stream when need to refund just call Refund (swaps).
  2. From a background thread that runs periodically and only calls RefundExpired().

What do you think @hydra-yse ?

lib/core/src/chain_swap.rs Show resolved Hide resolved
@hydra-yse
Copy link
Collaborator Author

hydra-yse commented Aug 27, 2024

There are still some checks/tests to do, so I will be leaving it as draft for now, though I would appreciate a review on the interface changes 😄 . @roeierez I'll clarify later today why I think creating a separate refund.rs sounds good but has some practical issues, and why I decided to opt for this approach instead.
Note: I've still taken inspiration for the refund and refund_expired methods you suggested, as they made the whole call structure easier to read.

@hydra-yse hydra-yse force-pushed the yse-refund-loop-fix branch 2 times, most recently from 69518c6 to 9113ce1 Compare August 27, 2024 06:15
@hydra-yse
Copy link
Collaborator Author

@dangeross I've also addressed Pending swap tracking in 9113ce1, so that even if Boltz doesn't return the right state we can still try non-cooperatively after the expiry. Just have to propagate the changes to chain swaps.

Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks a lot better IMO. I also like the fact that every module (chainswap, send_swap) takes care of its own refunds.
I still need to do another pass.

lib/core/src/persist/chain.rs Outdated Show resolved Hide resolved
@hydra-yse
Copy link
Collaborator Author

hydra-yse commented Aug 29, 2024

A note about 2d65bbb: I think waiting 10 minutes is a reasonable tradeoff which ends up saving us both bandwidth and computation time. Since the current method attempts a non-cooperative refund even when the swap is Pending (therefore even for swaps which may become successful soon) I think it's not worth trying to refund straight away but rather just wait some time for the swap to complete.

feat: add background thread cooperative refunding

feat: simplify refund flow and isolate to state handler

fix: rename `StateHandler` to `Handler`

fix: dart doc

fix: consider Pending swaps when refunding

feat: add creation time check before swap expiry

Setting a wait time of 10 minutes avoids fetching chain data and some
extra computation each time the background thread picks up on the swap

feat: isolate refund flow to swap handlers

feat: add `get_transaction_hex` method
@hydra-yse hydra-yse marked this pull request as ready for review September 8, 2024 23:19
Some(&refund_tx_id),
)
.await?;
match swap.user_lockup_tx_id {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment for @dangeross, I've reverted the script history check here to avoid re-fetching. It occurs anyway when reconstructing the refund transaction (utils::new_lbtc_refund_tx) so there shouldn't be any problems there.

@hydra-yse hydra-yse changed the title Send & Chain: Localize refund flow to background thread Send & Chain: Localize refund flow to swap handlers Sep 9, 2024
@roeierez
Copy link
Member

@hydra-yse is this ready for another review? If so can you please rebase the conflicts?

lib/core/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

lib/core/src/refund/chain.rs Outdated Show resolved Hide resolved
lib/core/src/chain_swap.rs Show resolved Hide resolved
lib/core/src/chain_swap.rs Show resolved Hide resolved
Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hydra-yse hydra-yse merged commit 783c5ac into main Sep 20, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send: Improve refund blocking mechanism
4 participants