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.

Sign up for GitHub

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

Add a ManagerOptions type to allow for customizable options #102

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Add a ManagerOptions type to allow for customizable options #102

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c0e3dfb
add a ManagerOptions type
sosaucily Mar 29, 2023
caa0a40
Remove Derivative package and use standard Default trait; change refu…
sosaucily Apr 4, 2023
57af634
removed unused import
sosaucily Apr 4, 2023
891b986
derive copy/clone rather than writing it
sosaucily Apr 4, 2023
6d7fc60
switch refund_delay back to u32 to solve bug
sosaucily Apr 17, 2023
80a21fe
Merge branch 'feat/allow-customizable-num-confirmations' of github.co…
sosaucily Apr 24, 2023
9c393db
add back in consts; use none instead of Some(default)
sosaucily Apr 24, 2023
bb8361f
Merge branch 'master' into feat/allow-customizable-num-confirmations
sosaucily Apr 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions dlc-manager/src/channel/offered_channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,11 @@ pub struct OfferedChannel {
}

impl OfferedChannel {
pub(crate) fn get_offer_channel_msg(&self, offered_contract: &OfferedContract) -> OfferChannel {
pub(crate) fn get_offer_channel_msg(
&self,
offered_contract: &OfferedContract,
cet_nsequence: u32,
) -> OfferChannel {
let party_points = &self.party_points;
OfferChannel {
protocol_version: crate::conversion_utils::PROTOCOL_VERSION,
Expand Down Expand Up @@ -72,7 +76,7 @@ impl OfferedChannel {
refund_locktime: offered_contract.refund_locktime,
fee_rate_per_vb: offered_contract.fee_rate_per_vb,
fund_output_serial_id: offered_contract.fund_output_serial_id,
cet_nsequence: crate::manager::CET_NSEQUENCE,
cet_nsequence,
}
}

Expand Down
8 changes: 5 additions & 3 deletions dlc-manager/src/channel_updater.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use crate::{
verify_signed_contract_internal,
},
error::Error,
manager::RefundDelayWindow,
utils::get_new_temporary_id,
Blockchain, Signer, Time, Wallet,
};
Expand Down Expand Up @@ -72,7 +73,7 @@ pub fn offer_channel<C: Signing, W: Deref, B: Deref, T: Deref>(
counter_party: &PublicKey,
oracle_announcements: &[Vec<OracleAnnouncement>],
cet_nsequence: u32,
refund_delay: u32,
refund_delay: RefundDelayWindow,
wallet: &W,
blockchain: &B,
time: &T,
Expand Down Expand Up @@ -957,7 +958,7 @@ pub fn renew_offer<C: Signing, S: Deref, T: Deref>(
contract_input: &ContractInput,
oracle_announcements: Vec<Vec<OracleAnnouncement>>,
counter_payout: u64,
refund_delay: u32,
refund_delay: RefundDelayWindow,
peer_timeout: u64,
cet_nsequence: u32,
signer: &S,
Expand Down Expand Up @@ -1490,6 +1491,7 @@ pub fn offer_collaborative_close<C: Signing, S: Deref, T: Deref>(
counter_payout: u64,
signer: &S,
time: &T,
peer_timeout: u64,
) -> Result<(CollaborativeCloseOffer, Transaction), Error>
where
S::Target: Signer,
Expand Down Expand Up @@ -1535,7 +1537,7 @@ where
counter_payout,
offer_signature: close_signature,
close_tx: close_tx.clone(),
timeout: time.unix_time_now() + super::manager::PEER_TIMEOUT,
timeout: time.unix_time_now() + peer_timeout,
};
std::mem::swap(&mut state, &mut signed_channel.state);
signed_channel.roll_back_state = Some(state);
Expand Down
5 changes: 3 additions & 2 deletions dlc-manager/src/contract/offered_contract.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
use crate::conversion_utils::{
get_contract_info_and_announcements, get_tx_input_infos, BITCOIN_CHAINHASH, PROTOCOL_VERSION,
};
use crate::manager::RefundDelayWindow;
use crate::utils::get_new_serial_id;

use super::contract_info::ContractInfo;
Expand Down Expand Up @@ -80,7 +81,7 @@ impl OfferedContract {
offer_params: &PartyParams,
funding_inputs_info: &[FundingInputInfo],
counter_party: &PublicKey,
refund_delay: u32,
refund_delay: RefundDelayWindow,
cet_locktime: u32,
) -> Self {
let total_collateral = contract.offer_collateral + contract.accept_collateral;
Expand Down Expand Up @@ -111,7 +112,7 @@ impl OfferedContract {
fund_output_serial_id,
fee_rate_per_vb: contract.fee_rate,
cet_locktime,
refund_locktime: latest_maturity + refund_delay,
refund_locktime: latest_maturity + refund_delay.min,
counter_party: *counter_party,
}
}
Expand Down
3 changes: 2 additions & 1 deletion dlc-manager/src/contract_updater.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use crate::{
},
conversion_utils::get_tx_input_infos,
error::Error,
manager::RefundDelayWindow,
Blockchain, ChannelId, Signer, Time, Wallet,
};

Expand All @@ -29,7 +30,7 @@ pub fn offer_contract<C: Signing, W: Deref, B: Deref, T: Deref>(
secp: &Secp256k1<C>,
contract_input: &ContractInput,
oracle_announcements: Vec<Vec<OracleAnnouncement>>,
refund_delay: u32,
refund_delay: RefundDelayWindow,
counter_party: &PublicKey,
wallet: &W,
blockchain: &B,
Expand Down
112 changes: 74 additions & 38 deletions dlc-manager/src/manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,40 @@ use std::collections::HashMap;
use std::ops::Deref;
use std::string::ToString;

/// The number of confirmations required before moving the the confirmed state.
pub const NB_CONFIRMATIONS: u32 = 6;
/// The delay to set the refund value to.
pub const REFUND_DELAY: u32 = 86400 * 7;
/// The nSequence value used for CETs in DLC channels
pub const CET_NSEQUENCE: u32 = 288;
/// Timeout in seconds when waiting for a peer's reply, after which a DLC channel
/// is forced closed.
pub const PEER_TIMEOUT: u64 = 3600;
/// The options used to configure the DLC manager.
pub struct ManagerOptions {
/// The number of btc confirmations required before moving the DLC to the confirmed state.
pub nb_confirmations: u32,
/// The refund delay window to trigger the refund.
pub refund_delay: RefundDelayWindow,
/// The nSequence value used for CETs in DLC channels
pub cet_nsequence: u32,
/// Timeout in seconds when waiting for a peer's reply, after which a DLC channel
pub peer_timeout: u64,
}

/// The min and max value in seconds until a DLC will be refunded.
#[derive(Clone, Copy)]
pub struct RefundDelayWindow {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking through the code I'm not sure this is really useful, in some places you end up passing that when only one of the value is required.

/// The min delay in seconds to trigger the refund.
pub min: u32,
/// The max delay in seconds to trigger the refund.
pub max: u32,
}

impl Default for ManagerOptions {
fn default() -> Self {
Self {
nb_confirmations: 6,
refund_delay: RefundDelayWindow {
min: 86400 * 7,
max: 86400 * 14,
},
cet_nsequence: 288,
peer_timeout: 3600,
}
}
}

type ClosableContractInfo<'a> = Option<(
&'a ContractInfo,
Expand All @@ -71,6 +96,7 @@ where
chain_monitor: ChainMonitor,
time: T,
fee_estimator: F,
options: ManagerOptions,
}

macro_rules! get_object_in_state {
Expand Down Expand Up @@ -175,8 +201,10 @@ where
oracles: HashMap<XOnlyPublicKey, O>,
time: T,
fee_estimator: F,
options: Option<ManagerOptions>,
) -> Result<Self, Error> {
let init_height = blockchain.get_blockchain_height()?;
let options = options.unwrap_or_default();
Ok(Manager {
secp: secp256k1_zkp::Secp256k1::new(),
wallet,
Expand All @@ -185,6 +213,7 @@ where
oracles,
time,
fee_estimator,
options,
chain_monitor: ChainMonitor::new(init_height),
})
}
Expand Down Expand Up @@ -284,7 +313,7 @@ where
&self.secp,
contract_input,
oracle_announcements,
REFUND_DELAY,
self.options.refund_delay,
&counter_party,
&self.wallet,
&self.blockchain,
Expand Down Expand Up @@ -344,7 +373,11 @@ where
offered_message: &OfferDlc,
counter_party: PublicKey,
) -> Result<(), Error> {
offered_message.validate(&self.secp, REFUND_DELAY, REFUND_DELAY * 2)?;
offered_message.validate(
&self.secp,
self.options.refund_delay.min,
self.options.refund_delay.max,
)?;
let contract: OfferedContract =
OfferedContract::try_from_offer_dlc(offered_message, counter_party)?;
contract.validate()?;
Expand Down Expand Up @@ -474,7 +507,7 @@ where
let confirmations = self.blockchain.get_transaction_confirmations(
&contract.accepted_contract.dlc_transactions.fund.txid(),
)?;
if confirmations >= NB_CONFIRMATIONS {
if confirmations >= self.options.nb_confirmations {
self.store
.update_contract(&Contract::Confirmed(contract.clone()))?;
}
Expand Down Expand Up @@ -604,7 +637,7 @@ where
let confirmations = self
.blockchain
.get_transaction_confirmations(&broadcasted_txid)?;
if confirmations >= NB_CONFIRMATIONS {
if confirmations >= self.options.nb_confirmations {
let closed_contract = ClosedContract {
attestations: contract.attestations.clone(),
signed_cet: Some(contract.signed_cet.clone()),
Expand Down Expand Up @@ -655,7 +688,7 @@ where
};

return Ok(Contract::PreClosed(preclosed_contract));
} else if confirmations < NB_CONFIRMATIONS {
} else if confirmations < self.options.nb_confirmations {
let preclosed_contract = PreClosedContract {
signed_contract: contract.clone(),
attestations: Some(attestations),
Expand Down Expand Up @@ -733,14 +766,15 @@ where
contract_input,
&counter_party,
&oracle_announcements,
CET_NSEQUENCE,
REFUND_DELAY,
self.options.cet_nsequence,
self.options.refund_delay,
&self.wallet,
&self.blockchain,
&self.time,
)?;

let msg = offered_channel.get_offer_channel_msg(&offered_contract);
let msg =
offered_channel.get_offer_channel_msg(&offered_contract, self.options.cet_nsequence);

self.store.upsert_channel(
Channel::Offered(offered_channel),
Expand Down Expand Up @@ -821,7 +855,7 @@ where
&self.secp,
&mut signed_channel,
counter_payout,
PEER_TIMEOUT,
self.options.peer_timeout,
&self.wallet,
&self.time,
)?;
Expand All @@ -846,9 +880,9 @@ where
let msg = crate::channel_updater::settle_channel_accept(
&self.secp,
&mut signed_channel,
CET_NSEQUENCE,
self.options.cet_nsequence,
0,
PEER_TIMEOUT,
self.options.peer_timeout,
&self.wallet,
&self.time,
)?;
Expand Down Expand Up @@ -885,9 +919,9 @@ where
contract_input,
oracle_announcements,
counter_payout,
REFUND_DELAY,
PEER_TIMEOUT,
CET_NSEQUENCE,
self.options.refund_delay,
self.options.peer_timeout,
self.options.cet_nsequence,
&self.wallet,
&self.time,
)?;
Expand Down Expand Up @@ -926,8 +960,8 @@ where
&self.secp,
&mut signed_channel,
&offered_contract,
CET_NSEQUENCE,
PEER_TIMEOUT,
self.options.cet_nsequence,
self.options.peer_timeout,
&self.wallet,
&self.time,
)?;
Expand Down Expand Up @@ -1014,6 +1048,7 @@ where
counter_payout,
&self.wallet,
&self.time,
self.options.peer_timeout,
)?;

self.chain_monitor.add_tx(
Expand Down Expand Up @@ -1107,7 +1142,7 @@ where
if self
.blockchain
.get_transaction_confirmations(&buffer_tx.txid())?
> CET_NSEQUENCE
> self.options.cet_nsequence
{
let confirmed_contract =
get_contract_in_state!(self, &contract_id, Confirmed, None as Option<PublicKey>)?;
Expand All @@ -1131,10 +1166,10 @@ where
) -> Result<(), Error> {
offer_channel.validate(
&self.secp,
REFUND_DELAY,
REFUND_DELAY * 2,
CET_NSEQUENCE,
CET_NSEQUENCE * 2,
self.options.refund_delay.min,
self.options.refund_delay.max,
self.options.cet_nsequence,
self.options.cet_nsequence * 2,
Comment on lines +1184 to +1185
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I set about trying to make this change in the same way I did for refund_delay, but it became a huge rats nest. I created a struct called CETnSequenceWindow, but I couldn't import it from dlc-manager in dlc-messages because of a circular dependency. Then I tried just using a cet_nsequence_min and cet_nsequence_max, and handing those all the way down, but that also became really messy because that sometimes it expects just the "min" value, and it gets really confusing.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand why you need to use that in dlc-messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I think I was trying to pass the struct with the min/max values too far down , when I could have just been passing the u32.

Anyway, do you think it would be good to have a CETnSequenceWindow object with a min / max values, or just leave it as a u32 with default 288?

Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned above I feel like not having a separate object is simpler as in I think the max value is only required in few places.

)?;

let (channel, contract) = OfferedChannel::from_offer_channel(offer_channel, counter_party)?;
Expand Down Expand Up @@ -1182,7 +1217,7 @@ where
&offered_contract,
accept_channel,
//TODO(tibo): this should be parameterizable.
CET_NSEQUENCE,
self.options.cet_nsequence,
&self.wallet,
);

Expand Down Expand Up @@ -1334,9 +1369,9 @@ where
&self.secp,
&mut signed_channel,
settle_accept,
CET_NSEQUENCE,
self.options.cet_nsequence,
0,
PEER_TIMEOUT,
self.options.peer_timeout,
&self.wallet,
&self.time,
)?;
Expand Down Expand Up @@ -1536,8 +1571,8 @@ where
renew_accept,
&mut signed_channel,
&offered_contract,
CET_NSEQUENCE,
PEER_TIMEOUT,
self.options.cet_nsequence,
self.options.peer_timeout,
&self.wallet,
&self.time,
)?;
Expand Down Expand Up @@ -1790,7 +1825,7 @@ where
crate::channel_updater::on_collaborative_close_offer(
&mut signed_channel,
close_offer,
PEER_TIMEOUT,
self.options.peer_timeout,
&self.time,
)?;

Expand Down Expand Up @@ -2020,7 +2055,7 @@ where
&counter_revocation_sk,
&tx,
&self.wallet.get_new_address()?,
CET_NSEQUENCE,
self.options.cet_nsequence,
0,
fee_rate_per_vb,
is_offer,
Expand Down Expand Up @@ -2189,7 +2224,7 @@ where
mod test {
use dlc_messages::Message;
use mocks::{
dlc_manager::{manager::Manager, Oracle},
dlc_manager::{manager::Manager, manager::ManagerOptions, Oracle},
memory_storage_provider::MemoryStorage,
mock_blockchain::MockBlockchain,
mock_oracle_provider::MockOracle,
Expand Down Expand Up @@ -2229,6 +2264,7 @@ mod test {
oracles,
time,
blockchain.clone(),
Some(ManagerOptions::default()),
)
.unwrap()
}
Expand Down
Loading