feat: Reject dlc channel, settle and renew offer #1935
Conversation
holzeis
force-pushed
the
fix/reject-dlc-channel
branch
5 times, most recently
from
6ca2d1e to
14cccb5
Compare
There was a problem hiding this comment.
LGTM overall.
I think it would be good to clean up the second to last commit and maybe split it up into several commits. I say this because as it currently stands I think the commit is: adding a new feature, changing the policy of a related feature and refactoring a bunch of stuff.
| // TODO(holzeis): if an dlc channel gets rejected we have to deal with the | ||
| // counterparty as well. |
There was a problem hiding this comment.
the order gets matched with the maker. if the setup with the taker fails, the maker setup should also fail.
| @@ -175,6 +194,7 @@ impl DlcHandler { | |||
|
|
|||
| // TODO(bonomat): we should verify that the proposed amount is acceptable | |||
| self.node | |||
| .inner | |||
| .accept_dlc_channel_collaborative_close(channel_id)?; | |||
There was a problem hiding this comment.
I guess here we are happy with the original policy of continuing with the channel close after a reconnect.
There was a problem hiding this comment.
hmm.. good question, I did not look at that. but I think there is no risk of a reserved utxo being used in the meantime as well as a price shift that may impact the originators intend.
To clarify, I think rejecting a pending offer on reconnect is just the first step, as we should as well add a corresponding handling on the coordinator side. Otherwise it would be far to easy for somebody to write a custom client that holds the dlc message for ~1h (3600 seconds), check how the price moved and then either accept or reject the offer.
However, that would require more work as rust-dlc currently does not support a cancel flow from accepted. Also its hard to tell if the user tries to game the counterparty or if its just a technical issue.
| tracing::info!( | ||
| channel_id = offer.temporary_channel_id.to_hex(), | ||
| "Automatically accepting dlc channel offer" | ||
| ); | ||
| self.process_dlc_channel_offer(offer.temporary_channel_id, action)?; | ||
| self.process_dlc_channel_offer(&offer.temporary_channel_id)?; | ||
| } | ||
| ChannelMessage::SettleOffer(offer) => { | ||
| self.inner | ||
| .accept_dlc_channel_collaborative_settlement(&offer.channel_id) | ||
| .with_context(|| { | ||
| format!( | ||
| "Failed to accept DLC channel close offer for channel {}", | ||
| hex::encode(offer.channel_id) | ||
| ) | ||
| })?; | ||
| tracing::info!( | ||
| channel_id = offer.channel_id.to_hex(), | ||
| "Automatically accepting settle offer" | ||
| ); | ||
| self.process_settle_offer(&offer.channel_id)?; | ||
| } | ||
| ChannelMessage::RenewOffer(r) => { | ||
| let channel_id_hex = r.channel_id.to_hex(); | ||
|
|
||
| tracing::info!( | ||
| channel_id = %channel_id_hex, | ||
| channel_id = r.channel_id.to_hex(), | ||
| "Automatically accepting renew offer" | ||
| ); | ||
|
|
||
| // TODO: This is generally not safe. The coordinator will even send | ||
| // `RenewOffer`s in order to roll over, and these will not even be triggered | ||
| // by a user action. | ||
| let (accept_renew_offer, counterparty_pubkey) = | ||
| self.inner.dlc_manager.accept_renew_offer(&r.channel_id)?; | ||
|
|
||
| self.send_dlc_message( | ||
| counterparty_pubkey, | ||
| Message::Channel(ChannelMessage::RenewAccept(accept_renew_offer)), | ||
| )?; | ||
|
|
||
| let expiry_timestamp = OffsetDateTime::from_unix_timestamp( | ||
| r.contract_info.get_closest_maturity_date() as i64, | ||
| )?; | ||
| position::handler::handle_channel_renewal_offer(expiry_timestamp)?; | ||
| self.process_renew_offer(&r.channel_id, expiry_timestamp)?; |
There was a problem hiding this comment.
❓Are these changes meant to be a straight refactor? Might be good to split this out into a separate commit if so. Otherwise I can review this more carefully.
There was a problem hiding this comment.
I am sorry it became a mixture of a refactoring and adding features. However, I've split the changed policy for dealing with pending offers into a separate commit. I hope that makes it clearer.
holzeis
force-pushed
the
fix/reject-dlc-channel
branch
from
14cccb5 to
7278def
Compare
Otherwise the message is not recorded in `dlc_messages` and `last_outbound_dlc_messages`
Depends on a rust change, making the `ChannelMessage` serializable, which significantly simplifies the logic on our side.
This way we get the logs from rust-dlc (or any other lib that is using log) on the mobile app.
Adds a proper handling to reject invalid or failed dlc channel, settle and renew offers. That includes producing the correct reject message through rust-dlc and updating the 10101 state correspondingly.
If the app (on reconnect) identifies that there is a pending offer (dlc channel, settle offer or renew offer) it will automatically try to reject it I think this is the safest we can do given that 1. for dlc channels the utxo might not be reserved anymore and 2. (more importantly) the price might have changed again, so we probably don't want to continue a dlc protocol that started some days ago.
holzeis
force-pushed
the
fix/reject-dlc-channel
branch
from
7278def to
ca191ca
Compare
Adds a handling for dlc channel offer, settle offer and renew offer. If the corresponding accept handling fails the protocol now attempts to reject the offer.
If the app identifies that there is a pending offer (dlc channel, settle offer or renew offer) it will automatically try to reject it. I think this is the safest we can do given that 1. for dlc channels the utxo might not be reserved anymore and 2. (more importantly) the price might have changed again, so we probably don't want to continue a dlc protocol that started some days ago.
rust-dlchas been enhanced for a reject dlc channel offer flow allowing to reject dlc channel offers.This change also adds a handling for updating the app layer (app, coordinator) state of the order and position.
Added rust-dlc changes
RenewOfferedstate: p2pderivatives/rust-dlc@8c13abffixes #1918
fixes #1919