Skip to content

Remove UserConfig::accept_mpp_keysend #3439

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

Merged
merged 1 commit into from
Dec 5, 2024
Merged

Remove UserConfig::accept_mpp_keysend #3439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 3 additions & 9 deletions lightning/src/ln/blinded_payment_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1214,11 +1214,9 @@ fn conditionally_round_fwd_amt() {
#[test]
#[cfg(async_payments)]
fn blinded_keysend() {
let mut mpp_keysend_config = test_default_channel_config();
mpp_keysend_config.accept_mpp_keysend = true;
let chanmon_cfgs = create_chanmon_cfgs(3);
let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, Some(mpp_keysend_config)]);
let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
let chan_upd_1_2 = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0).0.contents;
Expand Down Expand Up @@ -1254,11 +1252,9 @@ fn blinded_keysend() {
#[test]
#[cfg(async_payments)]
fn blinded_mpp_keysend() {
let mut mpp_keysend_config = test_default_channel_config();
mpp_keysend_config.accept_mpp_keysend = true;
let chanmon_cfgs = create_chanmon_cfgs(4);
let node_cfgs = create_node_cfgs(4, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, Some(mpp_keysend_config)]);
let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]);
let nodes = create_network(4, &node_cfgs, &node_chanmgrs);

create_announced_chan_between_nodes(&nodes, 0, 1);
Expand Down Expand Up @@ -1315,11 +1311,9 @@ fn blinded_mpp_keysend() {
#[test]
#[cfg(async_payments)]
fn invalid_keysend_payment_secret() {
let mut mpp_keysend_config = test_default_channel_config();
mpp_keysend_config.accept_mpp_keysend = true;
let chanmon_cfgs = create_chanmon_cfgs(3);
let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, Some(mpp_keysend_config)]);
let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
let chan_upd_1_2 = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0).0.contents;
Expand Down
79 changes: 8 additions & 71 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4323,7 +4323,7 @@ where
let current_height: u32 = self.best_block.read().unwrap().height;
match create_recv_pending_htlc_info(next_hop_data, shared_secret, msg.payment_hash,
msg.amount_msat, msg.cltv_expiry, None, allow_underpay, msg.skimmed_fee_msat,
current_height, self.default_configuration.accept_mpp_keysend)
current_height)
{
Ok(info) => {
// Note that we could obviously respond immediately with an update_fulfill_htlc
Expand Down Expand Up @@ -5750,7 +5750,7 @@ where
match create_recv_pending_htlc_info(hop_data,
incoming_shared_secret, payment_hash, outgoing_amt_msat,
outgoing_cltv_value, Some(phantom_shared_secret), false, None,
current_height, self.default_configuration.accept_mpp_keysend)
current_height)
{
Ok(info) => phantom_receives.push((
prev_short_channel_id, prev_counterparty_node_id, prev_funding_outpoint,
Expand Down Expand Up @@ -6061,10 +6061,6 @@ where
log_trace!(self.logger, "Failing new {} HTLC with payment_hash {} as we already had an existing {} HTLC with the same payment hash", log_keysend(is_keysend), &payment_hash, log_keysend(!is_keysend));
fail_htlc!(claimable_htlc, payment_hash);
}
if !self.default_configuration.accept_mpp_keysend && is_keysend && !claimable_payment.htlcs.is_empty() {
log_trace!(self.logger, "Failing new keysend HTLC with payment_hash {} as we already had an existing keysend HTLC with the same payment hash and our config states we don't accept MPP keysend", &payment_hash);
fail_htlc!(claimable_htlc, payment_hash);
}
if let Some(earlier_fields) = &mut claimable_payment.onion_fields {
if earlier_fields.check_merge(&mut onion_fields).is_err() {
fail_htlc!(claimable_htlc, payment_hash);
Expand Down Expand Up @@ -14202,7 +14198,6 @@ where
#[cfg(test)]
mod tests {
use bitcoin::hashes::Hash;
use bitcoin::hashes::sha256::Hash as Sha256;
use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey};
use core::sync::atomic::Ordering;
use crate::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, ClosureReason};
Expand Down Expand Up @@ -14419,26 +14414,16 @@ mod tests {

#[test]
fn test_keysend_dup_payment_hash() {
do_test_keysend_dup_payment_hash(false);
do_test_keysend_dup_payment_hash(true);
}

fn do_test_keysend_dup_payment_hash(accept_mpp_keysend: bool) {
// (1): Test that a keysend payment with a duplicate payment hash to an existing pending
// outbound regular payment fails as expected.
// (2): Test that a regular payment with a duplicate payment hash to an existing keysend payment
// fails as expected.
// (3): Test that a keysend payment with a duplicate payment hash to an existing keysend
// payment fails as expected. When `accept_mpp_keysend` is false, this tests that we
// reject MPP keysend payments, since in this case where the payment has no payment
// secret, a keysend payment with a duplicate hash is basically an MPP keysend. If
// `accept_mpp_keysend` is true, this tests that we only accept MPP keysends with
// payment secrets and reject otherwise.
// payment fails as expected. We only accept MPP keysends with payment secrets and reject
// otherwise.
let chanmon_cfgs = create_chanmon_cfgs(2);
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let mut mpp_keysend_cfg = test_default_channel_config();
mpp_keysend_cfg.accept_mpp_keysend = accept_mpp_keysend;
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, Some(mpp_keysend_cfg)]);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
create_announced_chan_between_nodes(&nodes, 0, 1);
let scorer = test_utils::TestScorer::new();
Expand Down Expand Up @@ -14618,53 +14603,6 @@ mod tests {
nodes[1].logger.assert_log_contains("lightning::ln::channelmanager", "Payment preimage didn't match payment hash", 1);
}

#[test]
fn test_keysend_msg_with_secret_err() {
// Test that we error as expected if we receive a keysend payment that includes a payment
// secret when we don't support MPP keysend.
let mut reject_mpp_keysend_cfg = test_default_channel_config();
reject_mpp_keysend_cfg.accept_mpp_keysend = false;
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, Some(reject_mpp_keysend_cfg)]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);

let payer_pubkey = nodes[0].node.get_our_node_id();
let payee_pubkey = nodes[1].node.get_our_node_id();

let _chan = create_chan_between_nodes(&nodes[0], &nodes[1]);
let route_params = RouteParameters::from_payment_params_and_value(
PaymentParameters::for_keysend(payee_pubkey, 40, false), 10_000);
let network_graph = nodes[0].network_graph;
let first_hops = nodes[0].node.list_usable_channels();
let scorer = test_utils::TestScorer::new();
let random_seed_bytes = chanmon_cfgs[1].keys_manager.get_secure_random_bytes();
let route = find_route(
&payer_pubkey, &route_params, &network_graph, Some(&first_hops.iter().collect::<Vec<_>>()),
nodes[0].logger, &scorer, &Default::default(), &random_seed_bytes
).unwrap();

let test_preimage = PaymentPreimage([42; 32]);
let test_secret = PaymentSecret([43; 32]);
let payment_hash = PaymentHash(Sha256::hash(&test_preimage.0).to_byte_array());
let session_privs = nodes[0].node.test_add_new_pending_payment(payment_hash,
RecipientOnionFields::secret_only(test_secret), PaymentId(payment_hash.0), &route).unwrap();
nodes[0].node.test_send_payment_internal(&route, payment_hash,
RecipientOnionFields::secret_only(test_secret), Some(test_preimage),
PaymentId(payment_hash.0), None, session_privs).unwrap();
check_added_monitors!(nodes[0], 1);

let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
assert_eq!(updates.update_add_htlcs.len(), 1);
assert!(updates.update_fulfill_htlcs.is_empty());
assert!(updates.update_fail_htlcs.is_empty());
assert!(updates.update_fail_malformed_htlcs.is_empty());
assert!(updates.update_fee.is_none());
nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &updates.update_add_htlcs[0]);

nodes[1].logger.assert_log_contains("lightning::ln::channelmanager", "We don't support MPP keysend payments", 1);
}

#[test]
fn test_multi_hop_missing_secret() {
let chanmon_cfgs = create_chanmon_cfgs(4);
Expand Down Expand Up @@ -15299,7 +15237,7 @@ mod tests {
if let Err(crate::ln::channelmanager::InboundHTLCErr { err_code, .. }) =
create_recv_pending_htlc_info(hop_data, [0; 32], PaymentHash([0; 32]),
sender_intended_amt_msat - extra_fee_msat - 1, 42, None, true, Some(extra_fee_msat),
current_height, node[0].node.default_configuration.accept_mpp_keysend)
current_height)
{
assert_eq!(err_code, 19);
} else { panic!(); }
Expand All @@ -15318,7 +15256,7 @@ mod tests {
let current_height: u32 = node[0].node.best_block.read().unwrap().height;
assert!(create_recv_pending_htlc_info(hop_data, [0; 32], PaymentHash([0; 32]),
sender_intended_amt_msat - extra_fee_msat, 42, None, true, Some(extra_fee_msat),
current_height, node[0].node.default_configuration.accept_mpp_keysend).is_ok());
current_height).is_ok());
}

#[test]
Expand All @@ -15338,8 +15276,7 @@ mod tests {
payment_secret: PaymentSecret([0; 32]), total_msat: 100,
}),
custom_tlvs: Vec::new(),
}, [0; 32], PaymentHash([0; 32]), 100, 23, None, true, None, current_height,
node[0].node.default_configuration.accept_mpp_keysend);
}, [0; 32], PaymentHash([0; 32]), 100, 23, None, true, None, current_height);

// Should not return an error as this condition:
// https://github.com/lightning/bolts/blob/4dcc377209509b13cf89a4b91fde7d478f5b46d8/04-onion-routing.md?plain=1#L334
Expand Down
17 changes: 5 additions & 12 deletions lightning/src/ln/onion_payment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ pub(super) fn create_fwd_pending_htlc_info(
pub(super) fn create_recv_pending_htlc_info(
hop_data: msgs::InboundOnionPayload, shared_secret: [u8; 32], payment_hash: PaymentHash,
amt_msat: u64, cltv_expiry: u32, phantom_shared_secret: Option<[u8; 32]>, allow_underpay: bool,
counterparty_skimmed_fee_msat: Option<u64>, current_height: u32, accept_mpp_keysend: bool,
counterparty_skimmed_fee_msat: Option<u64>, current_height: u32
) -> Result<PendingHTLCInfo, InboundHTLCErr> {
let (
payment_data, keysend_preimage, custom_tlvs, onion_amt_msat, onion_cltv_expiry,
Expand Down Expand Up @@ -227,13 +227,6 @@ pub(super) fn create_recv_pending_htlc_info(
msg: "Payment preimage didn't match payment hash",
});
}
if !accept_mpp_keysend && payment_data.is_some() {
return Err(InboundHTLCErr {
err_code: 0x4000|22,
err_data: Vec::new(),
msg: "We don't support MPP keysend payments",
});
}
PendingHTLCRouting::ReceiveKeysend {
payment_data,
payment_preimage,
Expand Down Expand Up @@ -282,7 +275,7 @@ pub(super) fn create_recv_pending_htlc_info(
/// [`Event::PaymentClaimable`]: crate::events::Event::PaymentClaimable
pub fn peel_payment_onion<NS: Deref, L: Deref, T: secp256k1::Verification>(
msg: &msgs::UpdateAddHTLC, node_signer: NS, logger: L, secp_ctx: &Secp256k1<T>,
cur_height: u32, accept_mpp_keysend: bool, allow_skimmed_fees: bool,
cur_height: u32, allow_skimmed_fees: bool,
) -> Result<PendingHTLCInfo, InboundHTLCErr>
where
NS::Target: NodeSigner,
Expand Down Expand Up @@ -333,7 +326,7 @@ where
onion_utils::Hop::Receive(received_data) => {
create_recv_pending_htlc_info(
received_data, shared_secret, msg.payment_hash, msg.amount_msat, msg.cltv_expiry,
None, allow_skimmed_fees, msg.skimmed_fee_msat, cur_height, accept_mpp_keysend,
None, allow_skimmed_fees, msg.skimmed_fee_msat, cur_height
)?
}
})
Expand Down Expand Up @@ -576,7 +569,7 @@ mod tests {
let msg = make_update_add_msg(amount_msat, cltv_expiry, payment_hash, onion);
let logger = test_utils::TestLogger::with_id("bob".to_string());

let peeled = peel_payment_onion(&msg, &bob, &logger, &secp_ctx, cur_height, true, false)
let peeled = peel_payment_onion(&msg, &bob, &logger, &secp_ctx, cur_height, false)
.map_err(|e| e.msg).unwrap();

let next_onion = match peeled.routing {
Expand All @@ -587,7 +580,7 @@ mod tests {
};

let msg2 = make_update_add_msg(amount_msat, cltv_expiry, payment_hash, next_onion);
let peeled2 = peel_payment_onion(&msg2, &charlie, &logger, &secp_ctx, cur_height, true, false)
let peeled2 = peel_payment_onion(&msg2, &charlie, &logger, &secp_ctx, cur_height, false)
.map_err(|e| e.msg).unwrap();

match peeled2.routing {
Expand Down
21 changes: 7 additions & 14 deletions lightning/src/ln/payment_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,11 +432,9 @@ fn do_test_keysend_payments(public_node: bool, with_retry: bool) {

#[test]
fn test_mpp_keysend() {
let mut mpp_keysend_config = test_default_channel_config();
mpp_keysend_config.accept_mpp_keysend = true;
let chanmon_cfgs = create_chanmon_cfgs(4);
let node_cfgs = create_node_cfgs(4, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, Some(mpp_keysend_config)]);
let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]);
let nodes = create_network(4, &node_cfgs, &node_chanmgrs);

create_announced_chan_between_nodes(&nodes, 0, 1);
Expand Down Expand Up @@ -478,19 +476,14 @@ fn test_mpp_keysend() {
}

#[test]
fn test_reject_mpp_keysend_htlc() {
// This test enforces that we reject MPP keysend HTLCs if our config states we don't support
// MPP keysend. When receiving a payment, if we don't support MPP keysend we'll reject the
// payment if it's keysend and has a payment secret, never reaching our payment validation
// logic. To check that we enforce rejecting MPP keysends in our payment logic, here we send
fn test_reject_mpp_keysend_htlc_mismatching_secret() {
// This test enforces that we reject MPP keysend HTLCs if the payment_secrets between MPP parts
// don't match. To check that we enforce rejecting MPP keysends in our payment logic, here we send
// keysend payments without payment secrets, then modify them by adding payment secrets in the
// final node in between receiving the HTLCs and actually processing them.
let mut reject_mpp_keysend_cfg = test_default_channel_config();
reject_mpp_keysend_cfg.accept_mpp_keysend = false;

let chanmon_cfgs = create_chanmon_cfgs(4);
let node_cfgs = create_node_cfgs(4, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, Some(reject_mpp_keysend_cfg)]);
let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]);
let nodes = create_network(4, &node_cfgs, &node_chanmgrs);
let chan_1_id = create_announced_chan_between_nodes(&nodes, 0, 1).0.contents.short_channel_id;
let chan_2_id = create_announced_chan_between_nodes(&nodes, 0, 2).0.contents.short_channel_id;
Expand Down Expand Up @@ -571,7 +564,7 @@ fn test_reject_mpp_keysend_htlc() {
match forward_info.routing {
PendingHTLCRouting::ReceiveKeysend { ref mut payment_data, .. } => {
*payment_data = Some(msgs::FinalOnionHopData {
payment_secret: PaymentSecret([42; 32]),
payment_secret: PaymentSecret([43; 32]), // Doesn't match the secret used above
total_msat: amount * 2,
});
},
Expand Down Expand Up @@ -4282,7 +4275,7 @@ fn peel_payment_onion_custom_tlvs() {
};
let peeled_onion = crate::ln::onion_payment::peel_payment_onion(
&update_add, &chanmon_cfgs[1].keys_manager, &chanmon_cfgs[1].logger, &secp_ctx,
nodes[1].best_block_info().1, true, false
nodes[1].best_block_info().1, false
).unwrap();
assert_eq!(peeled_onion.incoming_amt_msat, Some(amt_msat));
match peeled_onion.routing {
Expand Down
13 changes: 0 additions & 13 deletions lightning/src/util/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -844,17 +844,6 @@ pub struct UserConfig {
/// [`ChannelManager::get_intercept_scid`]: crate::ln::channelmanager::ChannelManager::get_intercept_scid
/// [`Event::HTLCIntercepted`]: crate::events::Event::HTLCIntercepted
pub accept_intercept_htlcs: bool,
/// If this is set to `false`, when receiving a keysend payment we'll fail it if it has multiple
/// parts. If this is set to `true`, we'll accept the payment.
///
/// Setting this to `true` will break backwards compatibility upon downgrading to an LDK
/// version prior to 0.0.116 while receiving an MPP keysend. If we have already received an MPP
/// keysend, downgrading will cause us to fail to deserialize [`ChannelManager`].
///
/// Default value: `false`
///
/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
pub accept_mpp_keysend: bool,
/// If this is set to `true`, the user needs to manually pay [`Bolt12Invoice`]s when received.
///
/// When set to `true`, [`Event::InvoiceReceived`] will be generated for each received
Expand All @@ -881,7 +870,6 @@ impl Default for UserConfig {
accept_inbound_channels: true,
manually_accept_inbound_channels: false,
accept_intercept_htlcs: false,
accept_mpp_keysend: false,
manually_handle_bolt12_invoices: false,
}
}
Expand All @@ -901,7 +889,6 @@ impl Readable for UserConfig {
accept_inbound_channels: Readable::read(reader)?,
manually_accept_inbound_channels: Readable::read(reader)?,
accept_intercept_htlcs: Readable::read(reader)?,
accept_mpp_keysend: Readable::read(reader)?,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey sorry, I think this might be why fuzz failed? Looking into it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably the no-changes test failed?

manually_handle_bolt12_invoices: Readable::read(reader)?,
})
}
Expand Down
3 changes: 3 additions & 0 deletions pending_changelog/3439-remove-accept-mpp-keysend-cfg.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Backwards Compatibility
* The presence of pending inbound MPP keysend payments breaks downgrades to LDK versions 0.0.115 and
earlier.
Loading