breez / breez-sdk-liquid

MIT License
20 stars 5 forks source link

Support address re-use in Receive Chain Swap #432

Closed ok300 closed 5 days ago

ok300 commented 3 months ago

Scenario:

Example instance: testnet swap 7u5RFB8HQNdp

On SDK startup:

[2024-08-02 10:21:36.862 INFO breez_sdk_liquid::chain_swap:142] Chain Swap 7u5RFB8HQNdp has 5000 confirmed and 0 unconfirmed sats
[2024-08-02 10:21:36.862 INFO breez_sdk_liquid::chain_swap:153] Chain Swap 7u5RFB8HQNdp has 5000 unspent sats. Setting the swap to refundable
[2024-08-02 10:21:36.862 INFO breez_sdk_liquid::chain_swap:547] Transitioning Chain swap 7u5RFB8HQNdp to Refundable (server_lockup_tx_id = None, user_lockup_tx_id = None, claim_tx_id = None), refund_tx_id = None)
dangeross commented 3 months ago

This should be correct, as the additionally sent BTC amount needs to be refunded back to the sender using prepare_refund / refund

ok300 commented 3 months ago

Don't we also need the matching Boltz status for a swap to be actually refundable?

For the swap states swap.expired where bitcoin were sent and transaction.lockupFailed, the user needs to submit a refund transaction to reclaim the locked chain bitcoin. https://docs.boltz.exchange/v/api/lifecycle#chain-swaps

The Boltz status is transaction.claimed.

roeierez commented 2 months ago

@ok300 why do we care about the swapper status here? If we know use added more funds to that address then it seems right to me to mark it as refundable. Where do you see an issue here?

ok300 commented 2 months ago

True, we don't need it. I thought we need the swapper status to agree with our status (refundable), but the refund doesn't need interaction as we can build the refund tx ourselves.

Then the key question is: do we support address re-use?

If yes:

roeierez commented 2 months ago

I think the important items here are:

  1. Make sure we mark as refundable a swap in such case (that has another utxo after use or expiration). This will ensure the user won't loose funds
  2. The refund should consider all inputs and not only the first utxo.

I am not sure if we need atm to change the refund_tx_id field to be a vector as we can define it as the last_refund_tx_id (unless it has a bad affect on our logic)

ok300 commented 2 months ago

I ran some tests and have two questions:

1) If we have a Receive Chain Swap that is

then non-cooperative refund will only work after timeout_block_height. This means until then, these funds cannot be used or refunded, even though we know the swap can only be refunded.

Q: Is this desired behavior?

2) I noticed we only monitor chain swaps for this amount of blocks

/// Number of blocks to monitor a swap after its timeout block height
pub const CHAIN_SWAP_MONITORING_PERIOD_BITCOIN_BLOCKS: u32 = 4320;

This means if address re-use happens after this time (30 days), we will not pick it up.

Q: should I remove this and instead let it monitor chain swaps forever?

roeierez commented 2 months ago

: Is this desired behavior?

It is the only option. non-cooperative refund can't work within the lock height range.

should I remove this and instead let it monitor chain swaps forever?

It is a tradeoff so we won't get into performance issues. Users can use the rescan_swaps themselves in that cases (or periodically).

ok300 commented 2 months ago

: Is this desired behavior?

It is the only option. non-cooperative refund can't work within the lock height range.

Forgot to mention: this also means list_refundables only shows it as refundable after the lock height timeout.

In normal cases (no address re-use), refundable swaps are shown in list_refundables as soon as we know it can be refunded, because

So it is correct (and the only possible way) that list_refundables only shows such swaps as refundable after the lock height timeout.

I will document this in the docs.

ok300 commented 1 month ago

Short update:

Testing the situation above (address re-use + attempt refund after lock height timeout) I found that: