Skip to content

Channel phase refactor, wrap channel phase, store wrapped phase in map #3418

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

Closed
wants to merge 4 commits into from
Closed

Channel phase refactor, wrap channel phase, store wrapped phase in map #3418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
60a3f36
Store ChannelWrappers in channel_by_id map
optout21 Nov 21, 2024
edffca2
Handle transition to Funded inline, without remove+readd from map
optout21 Nov 21, 2024
dd19f65
Fixes (review, build)
optout21 Nov 25, 2024
f99c2a5
Rename Channel to FundedChannel, ChannelWrapper to Channel
optout21 Dec 5, 2024
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
117 changes: 95 additions & 22 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1132,14 +1132,15 @@ pub(super) enum ChannelPhase<SP: Deref> where SP::Target: SignerProvider {
#[allow(dead_code)] // TODO(dual_funding): Remove once creating V2 channels is enabled.
UnfundedOutboundV2(OutboundV2Channel<SP>),
UnfundedInboundV2(InboundV2Channel<SP>),
Funded(Channel<SP>),
Funded(FundedChannel<SP>),
}

impl<'a, SP: Deref> ChannelPhase<SP> where
SP::Target: SignerProvider,
<SP::Target as SignerProvider>::EcdsaSigner: ChannelSigner,
{
pub fn context(&'a self) -> &'a ChannelContext<SP> {
#[inline]
pub fn context(&self) -> &ChannelContext<SP> {
match self {
ChannelPhase::Funded(chan) => &chan.context,
ChannelPhase::UnfundedOutboundV1(chan) => &chan.context,
Expand All @@ -1149,7 +1150,8 @@ impl<'a, SP: Deref> ChannelPhase<SP> where
}
}

pub fn context_mut(&'a mut self) -> &'a mut ChannelContext<SP> {
#[inline]
pub fn context_mut(&mut self) -> &mut ChannelContext<SP> {
match self {
ChannelPhase::Funded(ref mut chan) => &mut chan.context,
ChannelPhase::UnfundedOutboundV1(ref mut chan) => &mut chan.context,
Expand All @@ -1160,6 +1162,77 @@ impl<'a, SP: Deref> ChannelPhase<SP> where
}
}

/// A top-level channel struct, containing a channel phase
pub(super) struct Channel<SP: Deref> where SP::Target: SignerProvider {
/// The inner channel phase
/// Option is used to facilitate in-place replacement (see e.g. move_v2_to_funded),
/// but it is never None, ensured in new() and set_phase()
phase: Option<ChannelPhase<SP>>,
Comment on lines +1167 to +1170
Copy link
Contributor

Choose a reason for hiding this comment

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

See other comment about moving ChannelContext here. Seems we shouldn't need to use an Option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that logically it should not be Option, but I found no other way to transport the context from one phase to another. This may be Rust technicality. Option can be taken out and left empty, which is needed as I was not able to make the transfer in one atomic operation. Maybe another solution would be if ChannelContext had a default().

Copy link
Contributor

Choose a reason for hiding this comment

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

Would swap work?

}

impl<'a, SP: Deref> Channel<SP> where SP::Target: SignerProvider {
pub fn new(phase: ChannelPhase<SP>) -> Self {
Self {
phase: Some(phase),
}
}

#[inline]
pub fn phase(&self) -> &ChannelPhase<SP> {
self.phase.as_ref().unwrap()
}

#[inline]
pub fn phase_mut(&mut self) -> &mut ChannelPhase<SP> {
self.phase.as_mut().unwrap()
}

pub fn phase_take(self) -> ChannelPhase<SP> {
match self.phase {
Some(phase) => phase,
None => panic!("None phase"),
}
}

pub fn funded_channel(&self) -> Option<&FundedChannel<SP>> {
if let ChannelPhase::Funded(chan) = &self.phase.as_ref().unwrap() {
Some(chan)
} else {
None
}
}

/// Change the internal phase
#[inline]
pub fn set_phase(&mut self, new_phase: ChannelPhase<SP>) {
self.phase = Some(new_phase);
}

pub fn move_v2_to_funded(&mut self, signing_session: InteractiveTxSigningSession) -> Result<(), ChannelError> {
// We need a borrow to the phase field, but self is only a mut ref
let phase_inline = self.phase.take().unwrap();
let new_phase = match phase_inline {
ChannelPhase::UnfundedOutboundV2(chan) =>
ChannelPhase::Funded(chan.into_funded_channel(signing_session)?),
ChannelPhase::UnfundedInboundV2(chan) =>
ChannelPhase::Funded(chan.into_funded_channel(signing_session)?),
_ => phase_inline,
};
self.set_phase(new_phase);
Ok(())
}

#[inline]
pub fn context(&self) -> &ChannelContext<SP> {
self.phase().context()
}

#[inline]
pub fn context_mut(&mut self) -> &mut ChannelContext<SP> {
self.phase_mut().context_mut()
}
}

/// Contains all state common to unfunded inbound/outbound channels.
pub(super) struct UnfundedChannelContext {
/// A counter tracking how many ticks have elapsed since this unfunded channel was
Expand Down Expand Up @@ -1270,7 +1343,7 @@ pub(super) struct ChannelContext<SP: Deref> where SP::Target: SignerProvider {
/// the future when the signer indicates it may have a signature for us.
///
/// This flag is set in such a case. Note that we don't need to persist this as we'll end up
/// setting it again as a side-effect of [`Channel::channel_reestablish`].
/// setting it again as a side-effect of [`FundedChannel::channel_reestablish`].
signer_pending_commitment_update: bool,
/// Similar to [`Self::signer_pending_commitment_update`] but we're waiting to send either a
/// [`msgs::FundingCreated`] or [`msgs::FundingSigned`] depending on if this channel is
Expand Down Expand Up @@ -1661,7 +1734,7 @@ impl<SP: Deref> InitialRemoteCommitmentReceiver<SP> for InboundV1Channel<SP> whe
}
}

impl<SP: Deref> InitialRemoteCommitmentReceiver<SP> for Channel<SP> where SP::Target: SignerProvider {
impl<SP: Deref> InitialRemoteCommitmentReceiver<SP> for FundedChannel<SP> where SP::Target: SignerProvider {
fn context(&self) -> &ChannelContext<SP> {
&self.context
}
Expand Down Expand Up @@ -1928,7 +2001,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
if open_channel_fields.htlc_minimum_msat >= full_channel_value_msat {
return Err(ChannelError::close(format!("Minimum htlc value ({}) was larger than full channel value ({})", open_channel_fields.htlc_minimum_msat, full_channel_value_msat)));
}
Channel::<SP>::check_remote_fee(&channel_type, fee_estimator, open_channel_fields.commitment_feerate_sat_per_1000_weight, None, &&logger)?;
FundedChannel::<SP>::check_remote_fee(&channel_type, fee_estimator, open_channel_fields.commitment_feerate_sat_per_1000_weight, None, &&logger)?;

let max_counterparty_selected_contest_delay = u16::min(config.channel_handshake_limits.their_to_self_delay, MAX_LOCAL_BREAKDOWN_TIMEOUT);
if open_channel_fields.to_self_delay > max_counterparty_selected_contest_delay {
Expand Down Expand Up @@ -4211,7 +4284,7 @@ pub(super) struct DualFundingChannelContext {

// Holder designates channel data owned for the benefit of the user client.
// Counterparty designates channel data owned by the another channel participant entity.
pub(super) struct Channel<SP: Deref> where SP::Target: SignerProvider {
pub(super) struct FundedChannel<SP: Deref> where SP::Target: SignerProvider {
pub context: ChannelContext<SP>,
pub interactive_tx_signing_session: Option<InteractiveTxSigningSession>,
}
Expand Down Expand Up @@ -4281,7 +4354,7 @@ impl FailHTLCMessageName for msgs::UpdateFailMalformedHTLC {
}
}

impl<SP: Deref> Channel<SP> where
impl<SP: Deref> FundedChannel<SP> where
SP::Target: SignerProvider,
<SP::Target as SignerProvider>::EcdsaSigner: EcdsaChannelSigner
{
Expand Down Expand Up @@ -6073,7 +6146,7 @@ impl<SP: Deref> Channel<SP> where
if self.context.channel_state.is_peer_disconnected() {
return Err(ChannelError::close("Peer sent update_fee when we needed a channel_reestablish".to_owned()));
}
Channel::<SP>::check_remote_fee(&self.context.channel_type, fee_estimator, msg.feerate_per_kw, Some(self.context.feerate_per_kw), logger)?;
FundedChannel::<SP>::check_remote_fee(&self.context.channel_type, fee_estimator, msg.feerate_per_kw, Some(self.context.feerate_per_kw), logger)?;

self.context.pending_update_fee = Some((msg.feerate_per_kw, FeeUpdateState::RemoteAnnounced));
self.context.update_time_counter += 1;
Expand Down Expand Up @@ -8465,7 +8538,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
/// If this call is successful, broadcast the funding transaction (and not before!)
pub fn funding_signed<L: Deref>(
mut self, msg: &msgs::FundingSigned, best_block: BestBlock, signer_provider: &SP, logger: &L
) -> Result<(Channel<SP>, ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>), (OutboundV1Channel<SP>, ChannelError)>
) -> Result<(FundedChannel<SP>, ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>), (OutboundV1Channel<SP>, ChannelError)>
where
L::Target: Logger
{
Expand All @@ -8488,7 +8561,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {

log_info!(logger, "Received funding_signed from peer for channel {}", &self.context.channel_id());

let mut channel = Channel {
let mut channel = FundedChannel {
context: self.context,
interactive_tx_signing_session: None,
};
Expand Down Expand Up @@ -8673,7 +8746,7 @@ impl<SP: Deref> InboundV1Channel<SP> where SP::Target: SignerProvider {

pub fn funding_created<L: Deref>(
mut self, msg: &msgs::FundingCreated, best_block: BestBlock, signer_provider: &SP, logger: &L
) -> Result<(Channel<SP>, Option<msgs::FundingSigned>, ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>), (Self, ChannelError)>
) -> Result<(FundedChannel<SP>, Option<msgs::FundingSigned>, ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>), (Self, ChannelError)>
where
L::Target: Logger
{
Expand Down Expand Up @@ -8713,7 +8786,7 @@ impl<SP: Deref> InboundV1Channel<SP> where SP::Target: SignerProvider {

// Promote the channel to a full-fledged one now that we have updated the state and have a
// `ChannelMonitor`.
let mut channel = Channel {
let mut channel = FundedChannel {
context: self.context,
interactive_tx_signing_session: None,
};
Expand Down Expand Up @@ -8857,8 +8930,8 @@ impl<SP: Deref> OutboundV2Channel<SP> where SP::Target: SignerProvider {
}
}

pub fn into_channel(self, signing_session: InteractiveTxSigningSession) -> Result<Channel<SP>, ChannelError>{
let channel = Channel {
pub fn into_funded_channel(self, signing_session: InteractiveTxSigningSession) -> Result<FundedChannel<SP>, ChannelError>{
let channel = FundedChannel {
context: self.context,
interactive_tx_signing_session: Some(signing_session),
};
Expand Down Expand Up @@ -9051,8 +9124,8 @@ impl<SP: Deref> InboundV2Channel<SP> where SP::Target: SignerProvider {
self.generate_accept_channel_v2_message()
}

pub fn into_channel(self, signing_session: InteractiveTxSigningSession) -> Result<Channel<SP>, ChannelError>{
let channel = Channel {
pub fn into_funded_channel(self, signing_session: InteractiveTxSigningSession) -> Result<FundedChannel<SP>, ChannelError>{
let channel = FundedChannel {
context: self.context,
interactive_tx_signing_session: Some(signing_session),
};
Expand Down Expand Up @@ -9143,7 +9216,7 @@ impl Readable for AnnouncementSigsState {
}
}

impl<SP: Deref> Writeable for Channel<SP> where SP::Target: SignerProvider {
impl<SP: Deref> Writeable for FundedChannel<SP> where SP::Target: SignerProvider {
fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
// Note that we write out as if remove_uncommitted_htlcs_and_mark_paused had just been
// called.
Expand Down Expand Up @@ -9522,7 +9595,7 @@ impl<SP: Deref> Writeable for Channel<SP> where SP::Target: SignerProvider {
}

const MAX_ALLOC_SIZE: usize = 64*1024;
impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c ChannelTypeFeatures)> for Channel<SP>
impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c ChannelTypeFeatures)> for FundedChannel<SP>
where
ES::Target: EntropySource,
SP::Target: SignerProvider
Expand Down Expand Up @@ -9994,7 +10067,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
},
};

Ok(Channel {
Ok(FundedChannel {
context: ChannelContext {
user_id,

Expand Down Expand Up @@ -10151,7 +10224,7 @@ mod tests {
use crate::ln::channel_keys::{RevocationKey, RevocationBasepoint};
use crate::ln::channelmanager::{self, HTLCSource, PaymentId};
use crate::ln::channel::InitFeatures;
use crate::ln::channel::{AwaitingChannelReadyFlags, Channel, ChannelState, InboundHTLCOutput, OutboundV1Channel, InboundV1Channel, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator, HTLCUpdateAwaitingACK, commit_tx_fee_sat};
use crate::ln::channel::{AwaitingChannelReadyFlags, FundedChannel, ChannelState, InboundHTLCOutput, OutboundV1Channel, InboundV1Channel, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator, HTLCUpdateAwaitingACK, commit_tx_fee_sat};
use crate::ln::channel::{MAX_FUNDING_SATOSHIS_NO_WUMBO, TOTAL_BITCOIN_SUPPLY_SATOSHIS, MIN_THEIR_CHAN_RESERVE_SATOSHIS};
use crate::types::features::{ChannelFeatures, ChannelTypeFeatures, NodeFeatures};
use crate::ln::msgs;
Expand Down Expand Up @@ -10817,7 +10890,7 @@ mod tests {
let mut s = crate::io::Cursor::new(&encoded_chan);
let mut reader = crate::util::ser::FixedLengthReader::new(&mut s, encoded_chan.len() as u64);
let features = channelmanager::provided_channel_type_features(&config);
let decoded_chan = Channel::read(&mut reader, (&&keys_provider, &&keys_provider, 0, &features)).unwrap();
let decoded_chan = FundedChannel::read(&mut reader, (&&keys_provider, &&keys_provider, 0, &features)).unwrap();
assert_eq!(decoded_chan.context.pending_outbound_htlcs, pending_outbound_htlcs);
assert_eq!(decoded_chan.context.holding_cell_htlc_updates, holding_cell_htlc_updates);
}
Expand Down
Loading
Loading