Skip to content

Add unsafe_funding_transaction_generated #3056

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

Closed
Closed
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
54 changes: 54 additions & 0 deletions lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4618,6 +4618,53 @@ where
self.batch_funding_transaction_generated(&[(temporary_channel_id, counterparty_node_id)], funding_transaction)
}


/// Unsafe: This method wont check if the funding transaction
/// is signed ie. if the witness data is empty or not. It is the
/// caller's responsibility to ensure that the funding transaction
/// is final.
///
/// If you wish to use a safer method, use [`ChannelManager::funding_transaction_generated`]
///
/// Call this upon creation of a funding transaction for the given channel.
///
/// Returns an [`APIError::APIMisuseError`] if the funding_transaction spent non-SegWit outputs
/// or if no output was found which matches the parameters in [`Event::FundingGenerationReady`].
///
/// Returns [`APIError::APIMisuseError`] if the funding transaction is not final for propagation
/// across the p2p network.
///
/// Returns [`APIError::ChannelUnavailable`] if a funding transaction has already been provided
/// for the channel or if the channel has been closed as indicated by [`Event::ChannelClosed`].
///
/// May panic if the output found in the funding transaction is duplicative with some other
/// channel (note that this should be trivially prevented by using unique funding transaction
/// keys per-channel).
///
/// Do NOT broadcast the funding transaction yourself. When we have safely received our
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, sorry for the confusion, my comment at #3024 (comment) wasn't really intended to be that we'd expect users to handle this crazy hacky indirection, but rather we could push the broadcast out via the Event described in that PR but just don't use a config knob and rather do it via a new channel funding method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the goal of this pr is to introduce an "unsafe_funding_tx_generated" that wouldnt check if the witness data is empty (https://github.com/lightningdevkit/rust-lightning/pull/3056/files?diff=split&w=0#diff-645d12c7269d09caab57d2e0c1fd34f672ee259994c86ad4bad7f4da9183671cR4682) but call batch_funding_transaction_generated_intern directly.
is it the only blocker for lightningdevkit/ldk-node#257.

Beside the top lines regarding unsafe, the docs are the same as the original funding_tx_generated.

I see how both PRs are related and introduce two separate functions(which is fine as is) but could be used in the same scenario(ie payjoin) but I would like to get this one merged first and unblock the ldk-node pr and complete the funding signed pr afterwards if that makes sense..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, it seems a bit weird to introduce this here and then rewrite it one PR later to do something totally different? ISTM we could land #3024 without it taking toooo long.

/// counterparty's signature the funding transaction will automatically be broadcast via the
/// [`BroadcasterInterface`] provided when this `ChannelManager` was constructed.
///
/// Note that this includes RBF or similar transaction replacement strategies - lightning does
/// not currently support replacing a funding transaction on an existing channel. Instead,
/// create a new channel with a conflicting funding transaction.
///
/// Note to keep the miner incentives aligned in moving the blockchain forward, we recommend
/// the wallet software generating the funding transaction to apply anti-fee sniping as
/// implemented by Bitcoin Core wallet. See <https://bitcoinops.org/en/topics/fee-sniping/>
/// for more details.
///
/// [`Event::FundingGenerationReady`]: crate::events::Event::FundingGenerationReady
/// [`Event::ChannelClosed`]: crate::events::Event::ChannelClosed
/// [`ChannelManager::funding_transaction_generated`]: crate::ln::channelmanager::ChannelManager::funding_transaction_generated
pub fn unsafe_funding_transaction_generated(&self, temporary_channel_id: &ChannelId, counterparty_node_id: &PublicKey, funding_transaction: Transaction) -> Result<(), APIError> {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);

let temporary_channels = &[(temporary_channel_id, counterparty_node_id)];
return self.batch_funding_transaction_generated_intern(temporary_channels, funding_transaction);

}

/// Call this upon creation of a batch funding transaction for the given channels.
///
/// Return values are identical to [`Self::funding_transaction_generated`], respective to
Expand All @@ -4641,6 +4688,13 @@ where
}
}
}
if result != Ok(()) { return result; }

return self.batch_funding_transaction_generated_intern(temporary_channels, funding_transaction);
}

fn batch_funding_transaction_generated_intern(&self, temporary_channels: &[(&ChannelId, &PublicKey)], funding_transaction: Transaction) -> Result<(), APIError> {
let mut result = Ok(());
if funding_transaction.output.len() > u16::max_value() as usize {
result = result.and(Err(APIError::APIMisuseError {
err: "Transaction had more than 2^16 outputs, which is not supported".to_owned()
Expand Down
33 changes: 33 additions & 0 deletions lightning/src/ln/functional_test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1133,6 +1133,39 @@ pub fn create_coinbase_funding_transaction<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>,
internal_create_funding_transaction(node, expected_counterparty_node_id, expected_chan_value, expected_user_chan_id, true)
}

pub fn create_funding_tx_without_witness_data<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>,
expected_counterparty_node_id: &PublicKey, expected_chan_value: u64, expected_user_chan_id: u128) -> (ChannelId, Transaction, OutPoint) {
let chan_id = *node.network_chan_count.borrow();

let events = node.node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::FundingGenerationReady { ref temporary_channel_id, ref counterparty_node_id, ref channel_value_satoshis, ref output_script, user_channel_id } => {
assert_eq!(counterparty_node_id, expected_counterparty_node_id);
assert_eq!(*channel_value_satoshis, expected_chan_value);
assert_eq!(user_channel_id, expected_user_chan_id);

let dummy_outpoint = bitcoin::OutPoint { txid: bitcoin::Txid::from_slice(&[1; 32]).unwrap(), vout: 0 };
let dummy_script_sig = bitcoin::ScriptBuf::new();
let dummy_sequence = bitcoin::Sequence::ZERO;
let dummy_witness = bitcoin::Witness::new();
let input = vec![TxIn {
previous_output: dummy_outpoint,
script_sig: dummy_script_sig,
sequence: dummy_sequence,
witness: dummy_witness,
}];

let tx = Transaction { version: chan_id as i32, lock_time: LockTime::ZERO, input, output: vec![TxOut {
value: *channel_value_satoshis, script_pubkey: output_script.clone(),
}]};
let funding_outpoint = OutPoint { txid: tx.txid(), index: 0 };
(*temporary_channel_id, tx, funding_outpoint)
},
_ => panic!("Unexpected event"),
}
}

fn internal_create_funding_transaction<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>,
expected_counterparty_node_id: &PublicKey, expected_chan_value: u64, expected_user_chan_id: u128,
coinbase: bool) -> (ChannelId, Transaction, OutPoint) {
Expand Down
27 changes: 27 additions & 0 deletions lightning/src/ln/functional_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3743,6 +3743,33 @@ fn test_peer_disconnected_before_funding_broadcasted() {
, [nodes[0].node.get_our_node_id()], 1000000);
}

#[test]
fn test_unsafe_funding_transaction_generated() {
let chanmon_cfgs = create_chanmon_cfgs(2);
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);

let expected_temporary_channel_id = nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 1_000_000, 500_000_000, 42, None, None).unwrap();
let open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel);
let accept_channel = get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id());
nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), &accept_channel);

let (temporary_channel_id, tx, _funding_output) = create_funding_tx_without_witness_data(&nodes[0], &nodes[1].node.get_our_node_id(), 1_000_000, 42);
assert_eq!(temporary_channel_id, expected_temporary_channel_id);

assert!(nodes[0].node.funding_transaction_generated(&temporary_channel_id, &nodes[1].node.get_our_node_id(), tx.clone()).is_err());
assert!(nodes[0].node.unsafe_funding_transaction_generated(&temporary_channel_id, &nodes[1].node.get_our_node_id(), tx.clone()).is_ok());
let node_0_msg_events = nodes[0].node.get_and_clear_pending_msg_events();
match node_0_msg_events[0] {
MessageSendEvent::SendFundingCreated { ref node_id, .. } => {
assert_eq!(node_id, &nodes[1].node.get_our_node_id());
},
_ => panic!("Unexpected event"),
}
}

#[test]
fn test_simple_peer_disconnect() {
// Test that we can reconnect when there are no lost messages
Expand Down
Loading