Provide send_payment idempotency guarantees
#1761
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportBase: 90.74% // Head: 90.74% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #1761 +/- ##
=======================================
Coverage 90.74% 90.74%
=======================================
Files 87 87
Lines 47353 47353
Branches 47353 47353
=======================================
Hits 42969 42969
Misses 4384 4384 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
TheBlueMatt
force-pushed
the
2022-10-user-idempotency-token
branch
from
89e64ce to
248c622
Compare
|
Question for reviewers - should we even expose the PaymentId, or should we just use PaymentHash as the token automatically? |
I mean we get idempotency for free like that. Also, I'm not sure of the historical context, but were But I don't see any reason we can't just use the Will follow up with code review after other reviewers chime in on the conceptual part :) |
|
Yea, thinking about it more I think i'll rewrite this to use payment hashes. It makes way more sense that way, even if its gonna be more annoying for test updates. |
|
Looks like we didn't use For So, I think we can just use |
|
I think we can consider that separately. What I was thinking of doing here was keeping the code as it existed before, but where the PaymentId is always just the PaymentHash, rather than a new value. We can, separately, remove the concept of PaymentId entirely and just use the PaymentHash everywhere. |
TheBlueMatt
force-pushed
the
2022-10-user-idempotency-token
branch
from
248c622 to
bad822e
Compare
|
Pushed an update to use the payment hash for the ID. Failed to get all tests passing before lunch but should be a quick fix afterwards. |
TheBlueMatt
force-pushed
the
2022-10-user-idempotency-token
branch
from
bad822e to
6fbf31a
Compare
|
Okay, should work now, rewrote it to use payment hashes, which is way less code. |
TheBlueMatt
force-pushed
the
2022-10-user-idempotency-token
branch
from
6fbf31a to
248c622
Compare
|
Reverted to the previous version with a required payment_id. |
|
It might be useful for clients to provide an idempotency-token(paymentID) while requesting send_payment. Main reason being, customer's service applications are always working in some context which could be e-commerce checkOut workflow(orderId), rideBooking (rideId), funds withdraw (withdrawlId). They might want to use that as an idempotency token for ease. This is also the industry standard for idempotency, whether it is aws, google-cloud, stripe, square payments public api's. All use idempotency-keys. And might be developer friendly in this case. Key advantages:
|
TheBlueMatt
force-pushed
the
2022-10-user-idempotency-token
branch
from
248c622 to
7014bb2
Compare
|
Delayed the timeout a bit to 7 ticks (6-7 minutes by default). |
We could just make the This gives flexibility but doesn't make it a strict requirement. |
|
I'm not a huge fan of making the parameter an |
I forgot to write this option but yeah basically just that is cool too lol |
TheBlueMatt
force-pushed
the
2022-10-user-idempotency-token
branch
from
7014bb2 to
1610699
Compare
jkczyz
reviewed
Oct 12, 2022
lightning/src/ln/channelmanager.rs
Outdated
Comment on lines
2608
to
2606
| // Only public for testing, this should otherwise never be called direcly | ||
| pub(crate) fn add_new_pending_payment(&self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId, total_msat: u64) -> Result<(), PaymentSendFailure> { |
There was a problem hiding this comment.
Hmm, I included it to emphasize that this method will fail if the payment isn't new it'll fail.
lightning/src/ln/payment_tests.rs
Outdated
| nodes[0].node.timer_tick_occurred(); | ||
| nodes[0].node.timer_tick_occurred(); | ||
|
|
||
| nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap(); |
There was a problem hiding this comment.
We unwrap a (), not sure what we could assert?
lightning/src/util/events.rs
Outdated
Comment on lines
360
to
361
| /// If you have given up retrying this payment and wish to fail it, you MUST call | ||
| /// [`ChannelManager::abandon_payment`] or memory related to payment tracking will leak. |
There was a problem hiding this comment.
Does abandon_payment need to be called for each PaymentPathFailed event?
There was a problem hiding this comment.
Hmm, tried to rephrase, let me know if its clearer now.
Comment on lines
+320
to
+408
| /// payment is no longer retryable due to [`ChannelManager::abandon_payment`] having been | ||
| /// called for the corresponding payment. |
There was a problem hiding this comment.
Hmm... seems odd to even have this event then?
There was a problem hiding this comment.
Hmm? abandon_payment docs (I think) make it clear that the payment has not actually failed until you see this event. Do you have suggested clearer wording?
There was a problem hiding this comment.
Ah, right. Forgot that this would be required.
TheBlueMatt
force-pushed
the
2022-10-user-idempotency-token
branch
from
1610699 to
b9ba988
Compare
|
Ugh, I pushed back to the original version, then overwrote that again, so some of the review comments were bogus, force-pushed back to the original version again. |
dunxen
reviewed
Oct 17, 2022
lightning-invoice/src/utils.rs
Outdated
| @@ -519,14 +519,16 @@ where | |||
| fn send_payment( | |||
| &self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret> | |||
| ) -> Result<PaymentId, PaymentSendFailure> { | |||
| self.send_payment(route, payment_hash, payment_secret) | |||
| let payment_id = PaymentId(payment_hash.0); | |||
There was a problem hiding this comment.
Might be a dumb question, but why do we always use the hash here instead of adding a payment ID param to the above signature in the trait?
When we deal with the Payer API is it that we don't want to expose these internal things or just not to break stuff downstream?
There was a problem hiding this comment.
Well, the InvoicePayer already refuses to retry a payment with a duplicate payment hash (at runtime), so it seemed "correct" to continue using that as the payment's ID. And, yes, it seemed nicer for users to not have to provide a payment id by default (its in a trait, though, so the user can always wrap the trait).
jkczyz
reviewed
Oct 17, 2022
TheBlueMatt
force-pushed
the
2022-10-user-idempotency-token
branch
from
beb94cf to
26ecbb3
Compare
TheBlueMatt
force-pushed
the
2022-10-user-idempotency-token
branch
from
d791438 to
7988029
Compare
|
Rebased on the new edition: $ git range-diff -U3 d6321e6e11b3e1b11e26617d3bab0b8c21da0b5b...d79143867e0716fe4982c79ceab64e597471a5c0 fc9a4c22d195a75ad5942eed271757f285452214...7988029fbe9e95aca23e8561700f38f9380e82af
1: b8577c593 ! 1: 4e5a8137b Provide a `reset_on_reload` TLV field serialization type
@@ lightning/src/util/ser_macros.rs: macro_rules! decode_tlv {
+ ($reader: expr, $field: ident, (reset_on_reload, $value: expr)) => {{
+ }};
($reader: expr, $field: ident, required) => {{
- $field = ::util::ser::Readable::read(&mut $reader)?;
+ $field = $crate::util::ser::Readable::read(&mut $reader)?;
}};
@@ lightning/src/util/ser_macros.rs: macro_rules! decode_tlv {
}};
@@ lightning/src/util/ser_macros.rs: macro_rules! decode_tlv {
// If it is provided, it will be called with the custom type and the `FixedLengthReader` containing
// the message contents. It should return `Ok(true)` if the custom message is successfully parsed,
@@ lightning/src/util/ser_macros.rs: macro_rules! decode_tlv_stream {
- let length: ser::BigSize = ser::Readable::read(&mut stream_ref)?;
+ let length: ser::BigSize = $crate::util::ser::Readable::read(&mut stream_ref)?;
let mut s = ser::FixedLengthReader::new(&mut stream_ref, length.0);
match typ.0 {
- $($type => {
@@ lightning/src/util/ser_macros.rs: macro_rules! init_tlv_based_struct_field {
};
@@ lightning/src/util/ser_macros.rs: macro_rules! init_tlv_field_var {
($field: ident, (default_value, $default: expr)) => {
- let mut $field = ::util::ser::OptionDeserWrapper(None);
+ let mut $field = $crate::util::ser::OptionDeserWrapper(None);
};
+ ($field: ident, (reset_on_reload, $value: expr)) => {
+ let $field;
+ };
($field: ident, required) => {
- let mut $field = ::util::ser::OptionDeserWrapper(None);
+ let mut $field = $crate::util::ser::OptionDeserWrapper(None);
};
2: 801891e52 ! 2: 9f8783cb5 Allow users to specify the `PaymentId` for new outbound payments
@@ lightning-invoice/src/utils.rs: mod test {
## lightning/src/chain/chainmonitor.rs ##
@@ lightning/src/chain/chainmonitor.rs: mod tests {
- use ::{get_htlc_update_msgs, get_local_commitment_txn, get_revoke_commit_msgs, get_route_and_payment_hash,
unwrap_send_err};
- use chain::{ChannelMonitorUpdateStatus, Confirm, Watch};
- use chain::channelmonitor::LATENCY_GRACE_PERIOD_BLOCKS;
-- use ln::channelmanager::{self, PaymentSendFailure};
-+ use ln::channelmanager::{self, PaymentSendFailure, PaymentId};
- use ln::functional_test_utils::*;
- use ln::msgs::ChannelMessageHandler;
- use util::errors::APIError;
+ use crate::{get_htlc_update_msgs, get_local_commitment_txn, get_revoke_commit_msgs, get_route_and_payment_
hash, unwrap_send_err};
+ use crate::chain::{ChannelMonitorUpdateStatus, Confirm, Watch};
+ use crate::chain::channelmonitor::LATENCY_GRACE_PERIOD_BLOCKS;
+- use crate::ln::channelmanager::{self, PaymentSendFailure};
++ use crate::ln::channelmanager::{self, PaymentSendFailure, PaymentId};
+ use crate::ln::functional_test_utils::*;
+ use crate::ln::msgs::ChannelMessageHandler;
+ use crate::util::errors::APIError;
@@ lightning/src/chain/chainmonitor.rs: mod tests {
// If the ChannelManager tries to update the channel, however, the ChainMonitor will pass
// the update through to the ChannelMonitor which will refuse it (as the channel is closed).
@@ lightning/src/chain/chainmonitor.rs: mod tests {
## lightning/src/chain/channelmonitor.rs ##
@@ lightning/src/chain/channelmonitor.rs: mod tests {
- use ln::{PaymentPreimage, PaymentHash};
- use ln::chan_utils;
- use ln::chan_utils::{HTLCOutputInCommitment, ChannelPublicKeys, ChannelTransactionParameters, HolderCommit
mentTransaction, CounterpartyChannelTransactionParameters};
-- use ln::channelmanager::{self, PaymentSendFailure};
-+ use ln::channelmanager::{self, PaymentSendFailure, PaymentId};
- use ln::functional_test_utils::*;
- use ln::script::ShutdownScript;
- use util::errors::APIError;
+ use crate::ln::{PaymentPreimage, PaymentHash};
+ use crate::ln::chan_utils;
+ use crate::ln::chan_utils::{HTLCOutputInCommitment, ChannelPublicKeys, ChannelTransactionParameters, Holde
rCommitmentTransaction, CounterpartyChannelTransactionParameters};
+- use crate::ln::channelmanager::{self, PaymentSendFailure};
++ use crate::ln::channelmanager::{self, PaymentSendFailure, PaymentId};
+ use crate::ln::functional_test_utils::*;
+ use crate::ln::script::ShutdownScript;
+ use crate::util::errors::APIError;
@@ lightning/src/chain/channelmonitor.rs: mod tests {
// If the ChannelManager tries to update the channel, however, the ChainMonitor will pass
// the update through to the ChannelMonitor which will refuse it (as the channel is closed).
@@ lightning/src/chain/channelmonitor.rs: mod tests {
## lightning/src/ln/chanmon_update_fail_tests.rs ##
@@ lightning/src/ln/chanmon_update_fail_tests.rs: use bitcoin::network::constants::Network;
- use chain::channelmonitor::{ANTI_REORG_DELAY, ChannelMonitor};
- use chain::transaction::OutPoint;
- use chain::{ChannelMonitorUpdateStatus, Listen, Watch};
--use ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentSendFailur
e};
-+use ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentSendFailur
e, PaymentId};
- use ln::channel::AnnouncementSigsState;
- use ln::msgs;
- use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler};
+ use crate::chain::channelmonitor::{ANTI_REORG_DELAY, ChannelMonitor};
+ use crate::chain::transaction::OutPoint;
+ use crate::chain::{ChannelMonitorUpdateStatus, Listen, Watch};
+-use crate::ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentSendFailure};
++use crate::ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentSendFailure, PaymentId};
+ use crate::ln::channel::AnnouncementSigsState;
+ use crate::ln::msgs;
+ use crate::ln::msgs::{ChannelMessageHandler, RoutingMessageHandler};
@@ lightning/src/ln/chanmon_update_fail_tests.rs: fn test_simple_monitor_permanent_update_fail() {
let (route, payment_hash_1, _, payment_secret_1) = get_route_and_payment_hash!(&nodes[0], nodes[1], 1000000);
@@ lightning/src/ln/channelmanager.rs: mod tests {
assert!(regex::Regex::new(r"Payment secret is required for multi-path payments").unwrap().is_match(err)) },
_ => panic!("unexpected error")
@@ lightning/src/ln/channelmanager.rs: pub mod bench {
- use chain::Listen;
- use chain::chainmonitor::{ChainMonitor, Persist};
- use chain::keysinterface::{KeysManager, KeysInterface, InMemorySigner};
-- use ln::channelmanager::{self, BestBlock, ChainParameters, ChannelManager, PaymentHash, PaymentPreimage};
-+ use ln::channelmanager::{self, BestBlock, ChainParameters, ChannelManager, PaymentHash, PaymentPreimage, PaymentId};
- use ln::features::{InitFeatures, InvoiceFeatures};
- use ln::functional_test_utils::*;
- use ln::msgs::{ChannelMessageHandler, Init};
+ use crate::chain::Listen;
+ use crate::chain::chainmonitor::{ChainMonitor, Persist};
+ use crate::chain::keysinterface::{KeysManager, KeysInterface, InMemorySigner};
+- use crate::ln::channelmanager::{self, BestBlock, ChainParameters, ChannelManager, PaymentHash, PaymentPreimage};
++ use crate::ln::channelmanager::{self, BestBlock, ChainParameters, ChannelManager, PaymentHash, PaymentPreimage, PaymentId};
+ use crate::ln::functional_test_utils::*;
+ use crate::ln::msgs::{ChannelMessageHandler, Init};
+ use crate::routing::gossip::NetworkGraph;
@@ lightning/src/ln/channelmanager.rs: pub mod bench {
let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner());
let payment_secret = $node_b.create_inbound_payment_for_hash(payment_hash, None, 7200).unwrap();
@@ lightning/src/ln/functional_tests.rs: fn test_update_fee_with_fundee_update_add_
assert_eq!(added_monitors.len(), 0);
@@ lightning/src/ln/functional_tests.rs: fn holding_cell_htlc_counting() {
let mut payments = Vec::new();
- for _ in 0..::ln::channel::OUR_MAX_HTLCS {
+ for _ in 0..crate::ln::channel::OUR_MAX_HTLCS {
let (route, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[2], 100000);
- nodes[1].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap();
+ nodes[1].node.send_payment(&route, payment_hash, &Some(payment_secret), PaymentId(payment_hash.0)).unwrap();
@@ lightning/src/ln/functional_tests.rs: fn do_test_max_dust_htlc_exposure(dust_out
*feerate_lock = *feerate_lock * 10;
## lightning/src/ln/monitor_tests.rs ##
-@@ lightning/src/ln/monitor_tests.rs: use chain::channelmonitor::{ANTI_REORG_DELAY, Balance};
- use chain::transaction::OutPoint;
- use chain::chaininterface::LowerBoundedFeeEstimator;
- use ln::channel;
--use ln::channelmanager::{self, BREAKDOWN_TIMEOUT};
-+use ln::channelmanager::{self, BREAKDOWN_TIMEOUT, PaymentId};
- use ln::msgs::ChannelMessageHandler;
- use util::events::{Event, MessageSendEvent, MessageSendEventsProvider, ClosureReason, HTLCDestination};
+@@ lightning/src/ln/monitor_tests.rs: use crate::chain::channelmonitor::{ANTI_REORG_DELAY, Balance};
+ use crate::chain::transaction::OutPoint;
+ use crate::chain::chaininterface::LowerBoundedFeeEstimator;
+ use crate::ln::channel;
+-use crate::ln::channelmanager::{self, BREAKDOWN_TIMEOUT};
++use crate::ln::channelmanager::{self, BREAKDOWN_TIMEOUT, PaymentId};
+ use crate::ln::msgs::ChannelMessageHandler;
+ use crate::util::events::{Event, MessageSendEvent, MessageSendEventsProvider, ClosureReason, HTLCDestination};
@@ lightning/src/ln/monitor_tests.rs: fn chanmon_fail_from_stale_commitment() {
let (update_a, _, chan_id_2, _) = create_announced_chan_between_nodes(&nodes, 1, 2, channelmanager::provided_init_features(), channelmanager::provided_init_features());
@@ lightning/src/ln/monitor_tests.rs: fn test_balances_on_local_commitment_htlcs()
let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
## lightning/src/ln/onion_route_tests.rs ##
-@@ lightning/src/ln/onion_route_tests.rs: use chain::channelmonitor::{ChannelMonitor, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PER
- use chain::keysinterface::{KeysInterface, Recipient};
- use ln::{PaymentHash, PaymentSecret};
- use ln::channel::EXPIRE_PREV_CONFIG_TICKS;
--use ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, HTLCForwardInfo, CLTV_FAR_FAR_AWAY, MIN_CLTV_EXPIRY_DELTA, PendingHTLCInfo, PendingHTLCRouting};
-+use ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, HTLCForwardInfo, CLTV_FAR_FAR_AWAY, MIN_CLTV_EXPIRY_DELTA, PendingHTLCInfo, PendingHTLCRouting, PaymentId};
- use ln::onion_utils;
- use routing::gossip::{NetworkUpdate, RoutingFees, NodeId};
- use routing::router::{get_route, PaymentParameters, Route, RouteHint, RouteHintHop};
+@@ lightning/src/ln/onion_route_tests.rs: use crate::chain::channelmonitor::{ChannelMonitor, CLTV_CLAIM_BUFFER, LATENCY_GR
+ use crate::chain::keysinterface::{KeysInterface, Recipient};
+ use crate::ln::{PaymentHash, PaymentSecret};
+ use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
+-use crate::ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, HTLCForwardInfo, CLTV_FAR_FAR_AWAY, MIN_CLTV_EXPIRY_DELTA, PendingHTLCInfo, PendingHTLCRouting};
++use crate::ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, HTLCForwardInfo, CLTV_FAR_FAR_AWAY, MIN_CLTV_EXPIRY_DELTA, PendingHTLCInfo, PendingHTLCRouting, PaymentId};
+ use crate::ln::onion_utils;
+ use crate::routing::gossip::{NetworkUpdate, RoutingFees, NodeId};
+ use crate::routing::router::{get_route, PaymentParameters, Route, RouteHint, RouteHintHop};
@@ lightning/src/ln/onion_route_tests.rs: fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case:
}
@@ lightning/src/ln/payment_tests.rs: fn get_ldk_payment_preimage() {
## lightning/src/ln/priv_short_conf_tests.rs ##
@@
- use chain::{ChannelMonitorUpdateStatus, Watch};
- use chain::channelmonitor::ChannelMonitor;
- use chain::keysinterface::{Recipient, KeysInterface};
--use ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, MIN_CLTV_EXPIRY_DELTA};
-+use ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, MIN_CLTV_EXPIRY_DELTA, PaymentId};
- use routing::gossip::RoutingFees;
- use routing::router::{PaymentParameters, RouteHint, RouteHintHop};
- use ln::features::ChannelTypeFeatures;
+ use crate::chain::{ChannelMonitorUpdateStatus, Watch};
+ use crate::chain::channelmonitor::ChannelMonitor;
+ use crate::chain::keysinterface::{Recipient, KeysInterface};
+-use crate::ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, MIN_CLTV_EXPIRY_DELTA};
++use crate::ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, MIN_CLTV_EXPIRY_DELTA, PaymentId};
+ use crate::routing::gossip::RoutingFees;
+ use crate::routing::router::{PaymentParameters, RouteHint, RouteHintHop};
+ use crate::ln::features::ChannelTypeFeatures;
@@ lightning/src/ln/priv_short_conf_tests.rs: fn test_priv_forwarding_rejection() {
.with_route_hints(last_hops);
let (route, our_payment_hash, our_payment_preimage, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], payment_params, 10_000, TEST_FINAL_CLTV);
@@ lightning/src/ln/priv_short_conf_tests.rs: fn test_0conf_channel_with_async_moni
## lightning/src/ln/shutdown_tests.rs ##
@@
- use chain::keysinterface::KeysInterface;
- use chain::transaction::OutPoint;
--use ln::channelmanager::{self, PaymentSendFailure};
-+use ln::channelmanager::{self, PaymentSendFailure, PaymentId};
- use routing::router::{PaymentParameters, get_route};
- use ln::msgs;
- use ln::msgs::{ChannelMessageHandler, ErrorAction};
+ use crate::chain::keysinterface::KeysInterface;
+ use crate::chain::transaction::OutPoint;
+-use crate::ln::channelmanager::{self, PaymentSendFailure};
++use crate::ln::channelmanager::{self, PaymentSendFailure, PaymentId};
+ use crate::routing::router::{PaymentParameters, get_route};
+ use crate::ln::msgs;
+ use crate::ln::msgs::{ChannelMessageHandler, ErrorAction};
@@ lightning/src/ln/shutdown_tests.rs: fn updates_shutdown_wait() {
let route_1 = get_route(&nodes[0].node.get_our_node_id(), &payment_params_1, &nodes[0].network_graph.read_only(), None, 100000, TEST_FINAL_CLTV, &logger, &scorer, &random_seed_bytes).unwrap();
let payment_params_2 = PaymentParameters::from_node_id(nodes[0].node.get_our_node_id()).with_features(channelmanager::provided_invoice_features());
3: a5e93baa4 ! 3: 5d411aa75 Delay removal of fulfilled outbound payments for a few timer ticks
@@ lightning/src/ln/channelmanager.rs: impl_writeable_tlv_based_enum_upgradable!(Pe
(0, session_privs, required),
## lightning/src/ln/payment_tests.rs ##
-@@ lightning/src/ln/payment_tests.rs: use chain::channelmonitor::{ANTI_REORG_DELAY, ChannelMonitor, LATENCY_GRACE_PERI
- use chain::transaction::OutPoint;
- use chain::keysinterface::KeysInterface;
- use ln::channel::EXPIRE_PREV_CONFIG_TICKS;
--use ln::channelmanager::{self, BREAKDOWN_TIMEOUT, ChannelManager, ChannelManagerReadArgs, MPP_TIMEOUT_TICKS, MIN_CLTV_EXPIRY_DELTA, PaymentId, PaymentSendFailure};
-+use ln::channelmanager::{self, BREAKDOWN_TIMEOUT, ChannelManager, ChannelManagerReadArgs, MPP_TIMEOUT_TICKS, MIN_CLTV_EXPIRY_DELTA, PaymentId, PaymentSendFailure, IDEMPOTENCY_TIMEOUT_TICKS};
- use ln::msgs;
- use ln::msgs::ChannelMessageHandler;
- use routing::router::{PaymentParameters, get_route};
+@@ lightning/src/ln/payment_tests.rs: use crate::chain::channelmonitor::{ANTI_REORG_DELAY, ChannelMonitor, LATENCY_GRA
+ use crate::chain::transaction::OutPoint;
+ use crate::chain::keysinterface::KeysInterface;
+ use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
+-use crate::ln::channelmanager::{self, BREAKDOWN_TIMEOUT, ChannelManager, ChannelManagerReadArgs, MPP_TIMEOUT_TICKS, MIN_CLTV_EXPIRY_DELTA, PaymentId, PaymentSendFailure};
++use crate::ln::channelmanager::{self, BREAKDOWN_TIMEOUT, ChannelManager, ChannelManagerReadArgs, MPP_TIMEOUT_TICKS, MIN_CLTV_EXPIRY_DELTA, PaymentId, PaymentSendFailure, IDEMPOTENCY_TIMEOUT_TICKS};
+ use crate::ln::msgs;
+ use crate::ln::msgs::ChannelMessageHandler;
+ use crate::routing::router::{PaymentParameters, get_route};
@@ lightning/src/ln/payment_tests.rs: fn onchain_failed_probe_yields_event() {
}
assert!(found_probe_failed);
4: 0e879c0c4 ! 4: eabfc3d2a Stop timing out payments automatically, requiring abandon_payment
@@ lightning/src/ln/channelmanager.rs: where
fn get_relevant_txids(&self) -> Vec<Txid> {
## lightning/src/ln/functional_tests.rs ##
-@@ lightning/src/ln/functional_tests.rs: use chain::transaction::OutPoint;
- use chain::keysinterface::{BaseSign, KeysInterface};
- use ln::{PaymentPreimage, PaymentSecret, PaymentHash};
- use ln::channel::{commitment_tx_base_weight, COMMITMENT_TX_WEIGHT_PER_HTLC, CONCURRENT_INBOUND_HTLC_FEE_BUFFER, FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE, MIN_AFFORDABLE_HTLC_COUNT};
--use ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, PaymentId, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, PAYMENT_EXPIRY_BLOCKS};
-+use ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, PaymentId, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA};
- use ln::channel::{Channel, ChannelError};
- use ln::{chan_utils, onion_utils};
- use ln::chan_utils::{OFFERED_HTLC_SCRIPT_WEIGHT, htlc_success_tx_weight, htlc_timeout_tx_weight, HTLCOutputInCommitment};
+@@ lightning/src/ln/functional_tests.rs: use crate::chain::transaction::OutPoint;
+ use crate::chain::keysinterface::{BaseSign, KeysInterface};
+ use crate::ln::{PaymentPreimage, PaymentSecret, PaymentHash};
+ use crate::ln::channel::{commitment_tx_base_weight, COMMITMENT_TX_WEIGHT_PER_HTLC, CONCURRENT_INBOUND_HTLC_FEE_BUFFER, FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE, MIN_AFFORDABLE_HTLC_COUNT};
+-use crate::ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, PaymentId, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, PAYMENT_EXPIRY_BLOCKS};
++use crate::ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, PaymentId, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA};
+ use crate::ln::channel::{Channel, ChannelError};
+ use crate::ln::{chan_utils, onion_utils};
+ use crate::ln::chan_utils::{OFFERED_HTLC_SCRIPT_WEIGHT, htlc_success_tx_weight, htlc_timeout_tx_weight, HTLCOutputInCommitment};
@@ lightning/src/ln/functional_tests.rs: fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
mine_transaction(&nodes[1], &revoked_local_txn[0]);
check_added_monitors!(nodes[1], 1);
5: b434d509f = 5: 820978a77 Allow users to specify the `PaymentId` used in `InvoicePayer`
6: d79143867 = 6: 7988029fb Add a pending changelog entry for the past few commits |
TheBlueMatt
force-pushed
the
2022-10-user-idempotency-token
branch
from
7988029 to
c69c574
Compare
|
Slipping to 113 - this is gonna need some nontrivial followup in the same release - redoing the error codes, reworking when we start tracking payment in-flight state, etc. |
lightning/src/ln/payment_tests.rs
Outdated
| nodes[0].node.timer_tick_occurred(); | ||
| check_send_rejected!(); | ||
|
|
||
| // However, after some time has passed (at least more than one timer tick), a duplicate payment |
There was a problem hiding this comment.
Suggested change
| // However, after some time has passed (at least more than one timer tick), a duplicate payment | |
| // However, after some time has passed, a duplicate payment |
Just found this a bit confusing given 7 timer ticks pass
There was a problem hiding this comment.
Well, the point is we just tried, above, after one timer tick, and it didn't pass.
Comment on lines
+14
to
+16
| * When downgrading to a version of LDK prior to THIS_VERSION_XXX when there are | ||
| resolved payments waiting for a small timeout, the payments may not be | ||
| removed, preventing payments with the same `PaymentId`. |
There was a problem hiding this comment.
Re: the question from #1761 (comment) I'm good with including this note
TheBlueMatt
force-pushed
the
2022-10-user-idempotency-token
branch
from
c69c574 to
cdcbc59
Compare
|
Okay, addressed Val's comment and added one somewhat nontrivial rework fixup commit to address #1761 (comment). It simplifies things a bit and makes things less race-y (not that there was material risk to begin with, but still). Apologies to reviewers this has gone through a lot of iterations :( |
|
This will still need a followup to provide a "Idempotency Failure" error code, however. |
In c986e52, an `MppId` was added to `HTLCSource` objects as a way of correlating HTLCs which belong to the same payment when the `ChannelManager` sees an HTLC succeed/fail. This allows it to have awareness of the state of all HTLCs in a payment when it generates the ultimate user-facing payment success/failure events. This was used in the same PR to avoid generating duplicative success/failure events for a single payment. Because the field was only used as an internal token to correlate HTLCs, and retries were not supported, it was generated randomly by calling the `KeysInterface`'s 32-byte random-fetching function. This also provided a backwards-compatibility story as the existing HTLC randomization key was re-used for older clients. In 28eea12 `MppId` was renamed to the current `PaymentId` which was then used expose the `retry_payment` interface, allowing users to send new HTLCs which are considered a part of an existing payment. At no point has the payment-sending API seriously considered idempotency, a major drawback which leaves the API unsafe in most deployments. Luckily, there is a simple solution - because the `PaymentId` must be unique, and because payment information for a given payment is held for several blocks after a payment completes/fails, it represents an obvious idempotency token. Here we simply require the user provide the `PaymentId` directly in `send_payment`, allowing them to use whatever token they may already have for a payment's idempotency token.
Previously, once a fulfilled outbound payment completed and all associated HTLCs were resolved, we'd immediately remove the payment entry from the `pending_outbound_payments` map. Now that we're using the `pending_outbound_payments` map for send idempotency, this presents a race condition - if the user makes a redundant `send_payment` call at the same time that the original payment's last HTLC is resolved, the user would reasonably expect the `send_payment` call to fail due to our idempotency guarantees. However, because the `pending_outbound_payments` entry is being removed, if it completes first the `send_payment` call will succeed even though the user has not had a chance to see the corresponding `Event::PaymentSent`. Instead, here, we delay removal of `Fulfilled` `pending_outbound_payments` entries until several timer ticks have passed without any corresponding event or HTLC pending.
When the `abandon_payment` flow was added there was some concern that upgrading users may not migrate to the new flow, causing memory leaks in the pending-payment tracking. While this is true, now that we're relying on the pending_outbound_payments map for `send_payment` idempotency, the risk of removing a payment prematurely goes up from "spurious retry failure" to "sending a duplicative payment", which is much worse. Thus, we simply remove the automated payment timeout here, explicitly requiring that users call `abandon_payment` when they give up retrying a payment.
In order to allow users to pass a custom idempotency key to the `send*` methods in `InvoicePayer`, we have to pipe the `PaymentId` through to the `Payer` methods, which we do here. By default, existing `InvoicePayer` methods use the `PaymentHash` as the `PaymentId`, however we also add duplicate `send*_with_id` methods which allow users to pass a custom `PaymentId`. Finally, appropriate documentation updates are made to clarify idempotency guarantees.
TheBlueMatt
force-pushed
the
2022-10-user-idempotency-token
branch
from
cdcbc59 to
0df712a
Compare
|
Squashed with only minor fixups from Val: $ git diff-tree -U1 cdcbc59ec 0df712aae
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index e15a1324e..2e7e0d3c3 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -2667,3 +2667,3 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
debug_assert!(false, "This can't happen as the payment was added by callers");
- path_res = Err(APIError::APIMisuseError { err: "Internal error: payment dissapeared during processing. Please report this bug!".to_owned() });
+ path_res = Err(APIError::APIMisuseError { err: "Internal error: payment disappeared during processing. Please report this bug!".to_owned() });
}
@@ -3648,4 +3648,4 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
match ev {
- events::Event::PaymentSent { payment_id: Some(ev_payment_id), .. }|
- events::Event::PaymentPathSuccessful { payment_id: ev_payment_id, .. }|
+ events::Event::PaymentSent { payment_id: Some(ev_payment_id), .. } |
+ events::Event::PaymentPathSuccessful { payment_id: ev_payment_id, .. } |
events::Event::PaymentPathFailed { payment_id: Some(ev_payment_id), .. } => {
diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs
index 5d13e6184..ca2208ce1 100644
--- a/lightning/src/ln/payment_tests.rs
+++ b/lightning/src/ln/payment_tests.rs
@@ -1382,3 +1382,2 @@ fn abandoned_send_payment_idempotent() {
// However, we can reuse the PaymentId immediately after we `abandon_payment`.
-
nodes[0].node.send_payment(&route, second_payment_hash, &Some(second_payment_secret), payment_id).unwrap(); |
valentinewallace
approved these changes
Nov 2, 2022
jkczyz
approved these changes
Nov 2, 2022
G8XSU
approved these changes
Nov 3, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In c986e52, an
MppIdwas addedto
HTLCSourceobjects as a way of correlating HTLCs which belongto the same payment when the
ChannelManagersees an HTLCsucceed/fail. This allows it to have awareness of the state of all
HTLCs in a payment when it generates the ultimate user-facing
payment success/failure events. This was used in the same PR to
avoid generating duplicative success/failure events for a single
payment.
Because the field was only used as an internal token to correlate
HTLCs, and retries were not supported, it was generated randomly by
calling the
KeysInterface's 32-byte random-fetching function.This also provided a backwards-compatibility story as the existing
HTLC randomization key was re-used for older clients.
In 28eea12
MppIdwas renamed tothe current
PaymentIdwhich was then used expose theretry_paymentinterface, allowing users to send new HTLCs whichare considered a part of an existing payment.
At no point has the payment-sending API seriously considered
idempotency, a major drawback which leaves the API unsafe in most
deployments. Luckily, there is a simple solution - because the
PaymentIdmust be unique, and because payment information for agiven payment is held for several blocks after a payment
completes/fails, it represents an obvious idempotency token.
Here we simply reuse the
PaymentHashas thePaymentIdinsend_payment, ensuring idempotency of payments with a given hash.Note that we continue to not store payment data indefinitely, and thus have to tweak the times at which outbound pending payments are removed from the map - for failed payments we no longer automatically time them out ever, and require
abandon_paymentunconditionally. For successful payments, we continue to remove them after all HTLCs are resolevd, but we do so only after the user has processed thePaymentSentevent and some time has passed.