-
Notifications
You must be signed in to change notification settings - Fork 416
Integrate Splicing with Quiescence #4019
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
base: main
Are you sure you want to change the base?
Changes from all commits
2a6501a
37edb4c
29a160b
c301cd6
0b9d8b6
5fc4466
2d825f1
dcd99da
19912ef
524eecf
b37b5e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2448,13 +2448,46 @@ impl PendingSplice { | |
} | ||
} | ||
|
||
pub(crate) struct SpliceInstructions { | ||
adjusted_funding_contribution: SignedAmount, | ||
our_funding_inputs: Vec<FundingTxInput>, | ||
our_funding_outputs: Vec<TxOut>, | ||
change_script: Option<ScriptBuf>, | ||
funding_feerate_per_kw: u32, | ||
locktime: u32, | ||
original_funding_txo: OutPoint, | ||
} | ||
|
||
impl_writeable_tlv_based!(SpliceInstructions, { | ||
(1, adjusted_funding_contribution, required), | ||
(3, our_funding_inputs, required_vec), | ||
(5, our_funding_outputs, required_vec), | ||
(7, change_script, option), | ||
(9, funding_feerate_per_kw, required), | ||
(11, locktime, required), | ||
(13, original_funding_txo, required), | ||
}); | ||
|
||
pub(crate) enum QuiescentAction { | ||
// TODO: Make this test-only once we have another variant (as some code requires *a* variant). | ||
Splice(SpliceInstructions), | ||
#[cfg(any(test, fuzzing))] | ||
DoNothing, | ||
} | ||
|
||
pub(crate) enum StfuResponse { | ||
Stfu(msgs::Stfu), | ||
#[cfg_attr(not(splicing), allow(unused))] | ||
SpliceInit(msgs::SpliceInit), | ||
} | ||
|
||
#[cfg(any(test, fuzzing))] | ||
impl_writeable_tlv_based_enum_upgradable!(QuiescentAction, | ||
(99, DoNothing) => {}, | ||
(0, DoNothing) => {}, | ||
{1, Splice} => (), | ||
); | ||
#[cfg(not(any(test, fuzzing)))] | ||
impl_writeable_tlv_based_enum_upgradable!(QuiescentAction,, | ||
{1, Splice} => (), | ||
); | ||
|
||
/// Wrapper around a [`Transaction`] useful for caching the result of [`Transaction::compute_txid`]. | ||
|
@@ -5982,7 +6015,7 @@ fn get_v2_channel_reserve_satoshis(channel_value_satoshis: u64, dust_limit_satos | |
fn check_splice_contribution_sufficient( | ||
channel_balance: Amount, contribution: &SpliceContribution, is_initiator: bool, | ||
funding_feerate: FeeRate, | ||
) -> Result<Amount, ChannelError> { | ||
) -> Result<Amount, String> { | ||
let contribution_amount = contribution.value(); | ||
if contribution_amount < SignedAmount::ZERO { | ||
let estimated_fee = Amount::from_sat(estimate_v2_funding_transaction_fee( | ||
|
@@ -5996,10 +6029,10 @@ fn check_splice_contribution_sufficient( | |
if channel_balance >= contribution_amount.unsigned_abs() + estimated_fee { | ||
Ok(estimated_fee) | ||
} else { | ||
Err(ChannelError::Warn(format!( | ||
"Available channel balance {} is lower than needed for splicing out {}, considering fees of {}", | ||
channel_balance, contribution_amount.unsigned_abs(), estimated_fee, | ||
))) | ||
Err(format!( | ||
"Available channel balance {channel_balance} is lower than needed for splicing out {}, considering fees of {estimated_fee}", | ||
contribution_amount.unsigned_abs(), | ||
)) | ||
} | ||
} else { | ||
check_v2_funding_inputs_sufficient( | ||
|
@@ -6066,7 +6099,7 @@ fn estimate_v2_funding_transaction_fee( | |
fn check_v2_funding_inputs_sufficient( | ||
contribution_amount: i64, funding_inputs: &[FundingTxInput], is_initiator: bool, | ||
is_splice: bool, funding_feerate_sat_per_1000_weight: u32, | ||
) -> Result<u64, ChannelError> { | ||
) -> Result<u64, String> { | ||
let estimated_fee = estimate_v2_funding_transaction_fee( | ||
funding_inputs, &[], is_initiator, is_splice, funding_feerate_sat_per_1000_weight, | ||
); | ||
|
@@ -6089,10 +6122,9 @@ fn check_v2_funding_inputs_sufficient( | |
|
||
let minimal_input_amount_needed = contribution_amount.saturating_add(estimated_fee as i64); | ||
if (total_input_sats as i64) < minimal_input_amount_needed { | ||
Err(ChannelError::Warn(format!( | ||
"Total input amount {} is lower than needed for contribution {}, considering fees of {}. Need more inputs.", | ||
total_input_sats, contribution_amount, estimated_fee, | ||
))) | ||
Err(format!( | ||
"Total input amount {total_input_sats} is lower than needed for contribution {contribution_amount}, considering fees of {estimated_fee}. Need more inputs.", | ||
)) | ||
} else { | ||
Ok(estimated_fee) | ||
} | ||
|
@@ -10749,9 +10781,13 @@ where | |
/// - `change_script`: an option change output script. If `None` and needed, one will be | ||
/// generated by `SignerProvider::get_destination_script`. | ||
#[cfg(splicing)] | ||
pub fn splice_channel( | ||
pub fn splice_channel<L: Deref>( | ||
&mut self, contribution: SpliceContribution, funding_feerate_per_kw: u32, locktime: u32, | ||
) -> Result<msgs::SpliceInit, APIError> { | ||
logger: &L, | ||
) -> Result<Option<msgs::Stfu>, APIError> | ||
where | ||
L::Target: Logger, | ||
{ | ||
if self.holder_commitment_point.current_point().is_none() { | ||
return Err(APIError::APIMisuseError { | ||
err: format!( | ||
|
@@ -10781,8 +10817,6 @@ where | |
}); | ||
} | ||
|
||
// TODO(splicing): check for quiescence | ||
|
||
let our_funding_contribution = contribution.value(); | ||
if our_funding_contribution == SignedAmount::ZERO { | ||
return Err(APIError::APIMisuseError { | ||
|
@@ -10877,8 +10911,58 @@ where | |
} | ||
} | ||
|
||
let prev_funding_input = self.funding.to_splice_funding_input(); | ||
let original_funding_txo = self.funding.get_funding_txo().ok_or_else(|| { | ||
debug_assert!(false); | ||
APIError::APIMisuseError { | ||
err: "Channel isn't yet fully funded".to_owned(), | ||
} | ||
})?; | ||
|
||
let (our_funding_inputs, our_funding_outputs, change_script) = contribution.into_tx_parts(); | ||
|
||
let action = QuiescentAction::Splice(SpliceInstructions { | ||
adjusted_funding_contribution, | ||
our_funding_inputs, | ||
our_funding_outputs, | ||
change_script, | ||
funding_feerate_per_kw, | ||
locktime, | ||
original_funding_txo, | ||
}); | ||
self.propose_quiescence(logger, action) | ||
.map_err(|e| APIError::APIMisuseError { err: e.to_owned() }) | ||
Comment on lines
+10932
to
+10933
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to break the check portion out of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, also if there's a splice already queued up waiting on quiescence, the user will get back a "Channel is already quiescing" error which isn't very clear. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think its fine? It only fails if the channel isn't ready yet or the channel is already pending a quiescent action (currently only splicing), both of which are already checked in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could update the error message in |
||
} | ||
|
||
#[cfg(splicing)] | ||
fn send_splice_init( | ||
&mut self, instructions: SpliceInstructions, | ||
) -> Result<msgs::SpliceInit, String> { | ||
let SpliceInstructions { | ||
adjusted_funding_contribution, | ||
our_funding_inputs, | ||
our_funding_outputs, | ||
change_script, | ||
funding_feerate_per_kw, | ||
locktime, | ||
original_funding_txo, | ||
} = instructions; | ||
|
||
if self.funding.get_funding_txo() != Some(original_funding_txo) { | ||
// This should be unreachable once we opportunistically merge splices if the | ||
// counterparty initializes a splice. | ||
return Err("Funding changed out from under us".to_owned()); | ||
Comment on lines
+10950
to
+10953
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this is preventing a user-initiated splice from happening if the counterparty happens to splice first. Why do we want this? Presumably, if they spliced, they still want to splice-in/out funds from their side of the channel and that hasn't happened yet. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't splice in that case anyway until the first splice locks, so I'm not really sure its worth holding the pending splice around in that case? Also the current code would break cause we need to recalculate if the splice as proposed is possible, and what the fees should be. But I didn't bother fixing it cause I'm not sure we should wait 6 blocks then do it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could initiate an RBF to do our splice but that's not supported yet either. Without that or being able to contribute on an incoming There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, seems like we should. We don't yet have a |
||
} | ||
|
||
// Check if a splice has been initiated already. | ||
// Note: only a single outstanding splice is supported (per spec) | ||
if self.pending_splice.is_some() { | ||
return Err(format!( | ||
"Channel {} cannot be spliced, as it has already a splice pending", | ||
self.context.channel_id(), | ||
)); | ||
} | ||
|
||
let prev_funding_input = self.funding.to_splice_funding_input(); | ||
let funding_negotiation_context = FundingNegotiationContext { | ||
is_initiator: true, | ||
our_funding_contribution: adjusted_funding_contribution, | ||
|
@@ -11034,6 +11118,10 @@ where | |
ES::Target: EntropySource, | ||
L::Target: Logger, | ||
{ | ||
if !self.context.channel_state.is_quiescent() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also check the counterparty is the quiescence initiator? I guess once we can contribute back it doesn't matter, might just be more relevant when there's another quiescent action that's possible to perform. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We no longer track who is the initiator directly as its clear during the message flow what's going on. In this case, we'll just have to reject if the |
||
return Err(ChannelError::WarnAndDisconnect("Quiescence needed to splice".to_owned())); | ||
} | ||
|
||
let our_funding_contribution = SignedAmount::from_sat(our_funding_contribution_satoshis); | ||
let splice_funding = self.validate_splice_init(msg, our_funding_contribution)?; | ||
|
||
|
@@ -11073,6 +11161,11 @@ where | |
})?; | ||
debug_assert!(interactive_tx_constructor.take_initiator_first_message().is_none()); | ||
|
||
// TODO(splicing): if quiescent_action is set, integrate what the user wants to do into the | ||
// counterparty-initiated splice. For always-on nodes this probably isn't a useful | ||
// optimization, but for often-offline nodes it may be, as we may connect and immediately | ||
// go into splicing from both sides. | ||
|
||
let funding_pubkey = splice_funding.get_holder_pubkeys().funding_pubkey; | ||
|
||
self.pending_splice = Some(PendingSplice { | ||
|
@@ -11821,23 +11914,21 @@ where | |
); | ||
} | ||
|
||
#[cfg(any(test, fuzzing))] | ||
#[cfg(any(splicing, test, fuzzing))] | ||
#[rustfmt::skip] | ||
pub fn propose_quiescence<L: Deref>( | ||
&mut self, logger: &L, action: QuiescentAction, | ||
) -> Result<Option<msgs::Stfu>, ChannelError> | ||
) -> Result<Option<msgs::Stfu>, &'static str> | ||
where | ||
L::Target: Logger, | ||
{ | ||
log_debug!(logger, "Attempting to initiate quiescence"); | ||
|
||
if !self.context.is_usable() { | ||
return Err(ChannelError::Ignore( | ||
"Channel is not in a usable state to propose quiescence".to_owned() | ||
)); | ||
return Err("Channel is not in a usable state to propose quiescence"); | ||
} | ||
if self.quiescent_action.is_some() { | ||
return Err(ChannelError::Ignore("Channel is already quiescing".to_owned())); | ||
return Err("Channel already has a pending quiescent action and cannot start another"); | ||
} | ||
|
||
self.quiescent_action = Some(action); | ||
|
@@ -11858,7 +11949,7 @@ where | |
|
||
// Assumes we are either awaiting quiescence or our counterparty has requested quiescence. | ||
#[rustfmt::skip] | ||
pub fn send_stfu<L: Deref>(&mut self, logger: &L) -> Result<msgs::Stfu, ChannelError> | ||
pub fn send_stfu<L: Deref>(&mut self, logger: &L) -> Result<msgs::Stfu, &'static str> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May as well return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we'd end up with strictly more |
||
where | ||
L::Target: Logger, | ||
{ | ||
|
@@ -11872,9 +11963,7 @@ where | |
if self.context.is_waiting_on_peer_pending_channel_update() | ||
|| self.context.is_monitor_or_signer_pending_channel_update() | ||
{ | ||
return Err(ChannelError::Ignore( | ||
"We cannot send `stfu` while state machine is pending".to_owned() | ||
)); | ||
return Err("We cannot send `stfu` while state machine is pending") | ||
} | ||
|
||
let initiator = if self.context.channel_state.is_remote_stfu_sent() { | ||
|
@@ -11900,7 +11989,7 @@ where | |
#[rustfmt::skip] | ||
pub fn stfu<L: Deref>( | ||
&mut self, msg: &msgs::Stfu, logger: &L | ||
) -> Result<Option<msgs::Stfu>, ChannelError> where L::Target: Logger { | ||
) -> Result<Option<StfuResponse>, ChannelError> where L::Target: Logger { | ||
if self.context.channel_state.is_quiescent() { | ||
return Err(ChannelError::Warn("Channel is already quiescent".to_owned())); | ||
} | ||
|
@@ -11931,7 +12020,10 @@ where | |
self.context.channel_state.set_remote_stfu_sent(); | ||
|
||
log_debug!(logger, "Received counterparty stfu proposing quiescence"); | ||
return self.send_stfu(logger).map(|stfu| Some(stfu)); | ||
return self | ||
.send_stfu(logger) | ||
.map(|stfu| Some(StfuResponse::Stfu(stfu))) | ||
.map_err(|e| ChannelError::Ignore(e.to_owned())); | ||
} | ||
|
||
// We already sent `stfu` and are now processing theirs. It may be in response to ours, or | ||
|
@@ -11972,6 +12064,13 @@ where | |
"Internal Error: Didn't have anything to do after reaching quiescence".to_owned() | ||
)); | ||
}, | ||
Some(QuiescentAction::Splice(_instructions)) => { | ||
#[cfg(splicing)] | ||
return self.send_splice_init(_instructions) | ||
.map(|splice_init| Some(StfuResponse::SpliceInit(splice_init))) | ||
.map_err(|e| ChannelError::WarnAndDisconnect(e.to_owned())); | ||
}, | ||
#[cfg(any(test, fuzzing))] | ||
Some(QuiescentAction::DoNothing) => { | ||
// In quiescence test we want to just hang out here, letting the test manually | ||
// leave quiescence. | ||
|
@@ -12004,7 +12103,10 @@ where | |
|| (self.context.channel_state.is_remote_stfu_sent() | ||
&& !self.context.channel_state.is_local_stfu_sent()) | ||
{ | ||
return self.send_stfu(logger).map(|stfu| Some(stfu)); | ||
return self | ||
.send_stfu(logger) | ||
.map(|stfu| Some(stfu)) | ||
.map_err(|e| ChannelError::Ignore(e.to_owned())); | ||
} | ||
|
||
// We're either: | ||
|
@@ -16205,8 +16307,8 @@ mod tests { | |
2000, | ||
); | ||
assert_eq!( | ||
format!("{:?}", res.err().unwrap()), | ||
"Warn: Total input amount 100000 is lower than needed for contribution 220000, considering fees of 1746. Need more inputs.", | ||
res.err().unwrap(), | ||
"Total input amount 100000 is lower than needed for contribution 220000, considering fees of 1746. Need more inputs.", | ||
); | ||
} | ||
|
||
|
@@ -16241,8 +16343,8 @@ mod tests { | |
2200, | ||
); | ||
assert_eq!( | ||
format!("{:?}", res.err().unwrap()), | ||
"Warn: Total input amount 300000 is lower than needed for contribution 298032, considering fees of 2522. Need more inputs.", | ||
res.err().unwrap(), | ||
"Total input amount 300000 is lower than needed for contribution 298032, considering fees of 2522. Need more inputs.", | ||
); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: could just expect this, we checked for liveness above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh, let's just fix the stupid API so that we don't have to do either.