Skip to content

[RFC] event: store the outpoint when is_manual_broadcast #3259

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

Merged
merged 1 commit into from
Aug 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
61 changes: 53 additions & 8 deletions lightning/src/events/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,10 @@ use crate::util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReada
use crate::util::string::UntrustedString;

use bitcoin::{Transaction, OutPoint};
use bitcoin::locktime::absolute::LockTime;
use bitcoin::script::ScriptBuf;
use bitcoin::hashes::Hash;
use bitcoin::hashes::sha256::Hash as Sha256;
use bitcoin::secp256k1::PublicKey;
use bitcoin::transaction::Version;
use crate::io;
use core::time::Duration;
use core::ops::Deref;
Expand All @@ -50,6 +48,38 @@ use crate::sync::Arc;
#[allow(unused_imports)]
use crate::prelude::*;

/// `FundingInfo` holds information about a channel's funding transaction.
///
/// When LDK is set to manual propagation of the funding transaction
/// (via [`ChannelManager::unsafe_manual_funding_transaction_generated`),
/// LDK does not have the full transaction data. Instead, the `OutPoint`
/// for the funding is provided here.
///
/// [`ChannelManager::unsafe_manual_funding_transaction_generated`]: crate::ln::channelmanager::ChannelManager::unsafe_manual_funding_transaction_generated
#[derive(Debug, PartialEq, Eq, Clone)]
pub enum FundingInfo {
/// The full funding `Transaction`.
Tx {
/// The funding transaction
transaction: Transaction
},
/// The `OutPoint` of the funding.
OutPoint {
/// The outpoint of the funding
outpoint: transaction::OutPoint
},
}

impl_writeable_tlv_based_enum!(FundingInfo,
(0, Tx) => {
(0, transaction, required)
},
(1, OutPoint) => {
(1, outpoint, required)
}
);


/// Some information provided on receipt of payment depends on whether the payment received is a
/// spontaneous payment or a "conventional" lightning payment that's paying an invoice.
#[derive(Clone, Debug, PartialEq, Eq)]
Expand Down Expand Up @@ -1257,7 +1287,7 @@ pub enum Event {
/// The channel_id of the channel which has been closed.
channel_id: ChannelId,
/// The full transaction received from the user
transaction: Transaction
funding_info: FundingInfo,
},
/// Indicates a request to open a new channel by a peer.
///
Expand Down Expand Up @@ -1541,11 +1571,18 @@ impl Writeable for Event {
(9, channel_funding_txo, option),
});
},
&Event::DiscardFunding { ref channel_id, ref transaction } => {
&Event::DiscardFunding { ref channel_id, ref funding_info } => {
11u8.write(writer)?;

let transaction = if let FundingInfo::Tx { transaction } = funding_info {
Some(transaction)
} else {
None
};
write_tlv_fields!(writer, {
(0, channel_id, required),
(2, transaction, required)
(2, transaction, option),
(4, funding_info, required),
Copy link
Contributor

@valentinewallace valentinewallace Aug 27, 2024

Choose a reason for hiding this comment

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

Tried to look into ways to not break downgrades for this (i.e. writing a dummy or partially-dummy tx if unsafe_manual_funding_transaction_generated was used) but it doesn't seem worth it. So just noting that we'll need a release note for this since I don't see one in pending_changelog (can happen in follow-up).

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 can ask how the pending_changelog works? I could make the followup, sorry if I missed it

Copy link
Collaborator

Choose a reason for hiding this comment

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

We just put things in there to remember to include them in the changelog. I realized there was another issue so i went ahead and did the followup at #3275

})
},
&Event::PaymentPathSuccessful { ref payment_id, ref payment_hash, ref path } => {
Expand Down Expand Up @@ -1924,12 +1961,20 @@ impl MaybeReadable for Event {
11u8 => {
let mut f = || {
let mut channel_id = ChannelId::new_zero();
let mut transaction = Transaction{ version: Version::TWO, lock_time: LockTime::ZERO, input: Vec::new(), output: Vec::new() };
let mut transaction: Option<Transaction> = None;
let mut funding_info: Option<FundingInfo> = None;
read_tlv_fields!(reader, {
(0, channel_id, required),
(2, transaction, required),
(2, transaction, option),
(4, funding_info, option),
});
Ok(Some(Event::DiscardFunding { channel_id, transaction } ))

let funding_info = if let Some(tx) = transaction {
FundingInfo::Tx { transaction: tx }
} else {
funding_info.ok_or(msgs::DecodeError::InvalidValue)?
};
Ok(Some(Event::DiscardFunding { channel_id, funding_info } ))
};
f()
},
Expand Down
15 changes: 13 additions & 2 deletions lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ use bitcoin::secp256k1::{SecretKey,PublicKey};
use bitcoin::secp256k1::Secp256k1;
use bitcoin::{secp256k1, Sequence};

use crate::events::FundingInfo;
use crate::blinded_path::message::{MessageContext, OffersContext};
use crate::blinded_path::NodeIdLookUp;
use crate::blinded_path::message::{BlindedMessagePath, ForwardNode};
Expand Down Expand Up @@ -3509,6 +3510,7 @@ where
let _ = self.chain_monitor.update_channel(funding_txo, &monitor_update);
}
let mut shutdown_results = Vec::new();
let mut is_manual_broadcast = false;
if let Some(txid) = shutdown_res.unbroadcasted_batch_funding_txid {
let mut funding_batch_states = self.funding_batch_states.lock().unwrap();
let affected_channels = funding_batch_states.remove(&txid).into_iter().flatten();
Expand All @@ -3518,6 +3520,10 @@ where
if let Some(peer_state_mutex) = per_peer_state.get(&counterparty_node_id) {
let mut peer_state = peer_state_mutex.lock().unwrap();
if let Some(mut chan) = peer_state.channel_by_id.remove(&channel_id) {
// We override the previous value, so we could change from true -> false,
// but this is fine because if a channel has manual_broadcast set to false
// we should choose the safier condition.
is_manual_broadcast = chan.context().is_manual_broadcast();
update_maps_on_chan_removal!(self, &chan.context());
shutdown_results.push(chan.context_mut().force_shutdown(false, ClosureReason::FundingBatchClosure));
}
Expand All @@ -3541,9 +3547,14 @@ where
channel_funding_txo: shutdown_res.channel_funding_txo,
}, None));

if let Some(transaction) = shutdown_res.unbroadcasted_funding_tx {
let funding_info = if is_manual_broadcast {
shutdown_res.channel_funding_txo.map(|outpoint| FundingInfo::OutPoint{ outpoint })
} else {
shutdown_res.unbroadcasted_funding_tx.map(|transaction| FundingInfo::Tx{ transaction })
};
if let Some(funding_info) = funding_info {
pending_events.push_back((events::Event::DiscardFunding {
channel_id: shutdown_res.channel_id, transaction
channel_id: shutdown_res.channel_id, funding_info
}, None));
}
}
Expand Down
Loading