Uh oh!
There was an error while loading. Please reload this page.
Include height to incorrect_or_unknown_payment_details failure - #596
Merged
TheBlueMatt merged 2 commits intoApr 20, 2020
Merged
Conversation
TheBlueMatt
left a comment
Collaborator
There was a problem hiding this comment.
Yea, in the future we should probably have structs in onion_utils which ensure that we are always passing the right data for each possible error type instead of the ad-hoc stuff right now. Certainly thats a lot more work, so should probably be a separate PR. As for a test, the following patch adds a test for this:
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index cec173e5e..03db866ad 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -1851,9 +1851,9 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
match &onion_error {
&HTLCFailReason::LightningError { ref err } => {
#[cfg(test)]
- let (channel_update, payment_retryable, onion_error_code) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone());
+ let (channel_update, payment_retryable, onion_error_code, onion_error_data) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone());
#[cfg(not(test))]
- let (channel_update, payment_retryable, _) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone());
+ let (channel_update, payment_retryable, _, _) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone());
// TODO: If we decided to blame ourselves (or one of our channels) in
// process_onion_failure we should close that channel as it implies our
// next-hop is needlessly blaming us!
@@ -1869,13 +1869,17 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
payment_hash: payment_hash.clone(),
rejected_by_dest: !payment_retryable,
#[cfg(test)]
- error_code: onion_error_code
+ error_code: onion_error_code,
+#[cfg(test)]
+ error_data: onion_error_data
}
);
},
&HTLCFailReason::Reason {
#[cfg(test)]
ref failure_code,
+#[cfg(test)]
+ ref data,
.. } => {
// we get a fail_malformed_htlc from the first hop
// TODO: We'd like to generate a PaymentFailureNetworkUpdate for temporary
@@ -1890,6 +1894,8 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
rejected_by_dest: path.len() == 1,
#[cfg(test)]
error_code: Some(*failure_code),
+#[cfg(test)]
+ error_data: Some(data.clone()),
}
);
}
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index 1be967d93..21d868603 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -5326,7 +5326,7 @@ fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case:
let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
- if let &Event::PaymentFailed { payment_hash:_, ref rejected_by_dest, ref error_code } = &events[0] {
+ if let &Event::PaymentFailed { payment_hash:_, ref rejected_by_dest, ref error_code, error_data: _ } = &events[0] {
assert_eq!(*rejected_by_dest, !expected_retryable);
assert_eq!(*error_code, expected_error_code);
} else {
@@ -6914,9 +6914,20 @@ fn test_check_htlc_underpaying() {
let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
- if let &Event::PaymentFailed { payment_hash:_, ref rejected_by_dest, ref error_code } = &events[0] {
+ if let &Event::PaymentFailed { payment_hash:_, ref rejected_by_dest, ref error_code, ref error_data } = &events[0] {
assert_eq!(*rejected_by_dest, true);
assert_eq!(error_code.unwrap(), 0x4000|15);
+ // 10_000 msat as u64, followed by a height of 99 as u32
+ assert_eq!(&error_data.as_ref().unwrap()[..], &[
+ ((10_000u64 >> 7*8) & 0xff) as u8,
+ ((10_000u64 >> 6*8) & 0xff) as u8,
+ ((10_000u64 >> 5*8) & 0xff) as u8,
+ ((10_000u64 >> 4*8) & 0xff) as u8,
+ ((10_000u64 >> 3*8) & 0xff) as u8,
+ ((10_000u64 >> 2*8) & 0xff) as u8,
+ ((10_000u64 >> 1*8) & 0xff) as u8,
+ ((10_000u64 >> 0*8) & 0xff) as u8,
+ 0, 0, 0, 99]);
} else {
panic!("Unexpected event");
}
diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs
index ff89782be..dc3b2f902 100644
--- a/lightning/src/ln/onion_utils.rs
+++ b/lightning/src/ln/onion_utils.rs
@@ -317,11 +317,13 @@ pub(super) fn build_first_hop_failure_packet(shared_secret: &[u8], failure_type:
/// Process failure we got back from upstream on a payment we sent (implying htlc_source is an
/// OutboundRoute).
/// Returns update, a boolean indicating that the payment itself failed, and the error code.
-pub(super) fn process_onion_failure<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, logger: &Arc<Logger>, htlc_source: &HTLCSource, mut packet_decrypted: Vec<u8>) -> (Option<msgs::HTLCFailChannelUpdate>, bool, Option<u16>) {
+#[inline]
+pub(super) fn process_onion_failure<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, logger: &Arc<Logger>, htlc_source: &HTLCSource, mut packet_decrypted: Vec<u8>) -> (Option<msgs::HTLCFailChannelUpdate>, bool, Option<u16>, Option<Vec<u8>>) {
if let &HTLCSource::OutboundRoute { ref path, ref session_priv, ref first_hop_htlc_msat } = htlc_source {
let mut res = None;
let mut htlc_msat = *first_hop_htlc_msat;
let mut error_code_ret = None;
+ let mut error_packet_ret = None;
let mut next_route_hop_ix = 0;
let mut is_from_final_node = false;
@@ -356,6 +358,7 @@ pub(super) fn process_onion_failure<T: secp256k1::Signing>(secp_ctx: &Secp256k1<
let error_code = byte_utils::slice_to_be16(&error_code_slice);
error_code_ret = Some(error_code);
+ error_packet_ret = Some(err_packet.failuremsg[2..].to_vec());
let (debug_field, debug_field_size) = errors::get_onion_debug_field(error_code);
@@ -456,11 +459,11 @@ pub(super) fn process_onion_failure<T: secp256k1::Signing>(secp_ctx: &Secp256k1<
}
}).expect("Route that we sent via spontaneously grew invalid keys in the middle of it?");
if let Some((channel_update, payment_retryable)) = res {
- (channel_update, payment_retryable, error_code_ret)
+ (channel_update, payment_retryable, error_code_ret, error_packet_ret)
} else {
// only not set either packet unparseable or hmac does not match with any
// payment not retryable only when garbage is from the final node
- (None, !is_from_final_node, None)
+ (None, !is_from_final_node, None, None)
}
} else { unreachable!(); }
}
diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs
index 2c00a1335..8f3460567 100644
--- a/lightning/src/util/events.rs
+++ b/lightning/src/util/events.rs
@@ -96,6 +96,8 @@ pub enum Event {
rejected_by_dest: bool,
#[cfg(test)]
error_code: Option<u16>,
+#[cfg(test)]
+ error_data: Option<Vec<u8>>,
},
/// Used to indicate that ChannelManager::process_pending_htlc_forwards should be called at a
/// time in the future.
@@ -142,12 +144,16 @@ impl Writeable for Event {
&Event::PaymentFailed { ref payment_hash, ref rejected_by_dest,
#[cfg(test)]
ref error_code,
+ #[cfg(test)]
+ ref error_data,
} => {
4u8.write(writer)?;
payment_hash.write(writer)?;
rejected_by_dest.write(writer)?;
#[cfg(test)]
error_code.write(writer)?;
+ #[cfg(test)]
+ error_data.write(writer)?;
},
&Event::PendingHTLCsForwardable { time_forwardable: _ } => {
5u8.write(writer)?;
@@ -186,6 +192,8 @@ impl MaybeReadable for Event {
rejected_by_dest: Readable::read(reader)?,
#[cfg(test)]
error_code: Readable::read(reader)?,
+ #[cfg(test)]
+ error_data: Readable::read(reader)?,
})),
5u8 => Ok(Some(Event::PendingHTLCsForwardable {
time_forwardable: Duration::from_secs(0)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
D4nte
commented
Apr 19, 2020
ContributorAuthor
Yes it's what I thought. I'll aim to do a follow-up PR that does it. Thanks for the test. |
D4nteforce-pushed
the
586-update-htlc-error-code-content
branch
2 times, most recently
from
April 19, 2020 20:46
a4d1080 to
d55738cCompare`incorrect_or_unknown_payment_details` failure message, `0x4000 (PERM) | 15`, should include the following data: - [u64:htlc_msat] - [u32:height] This patches ensure that the height is included in all the occurrences of this failure message.
Ensure that the best know blockchain height is included in the data of `incorrect_or_unknown_payment_details` message failure.
D4nteforce-pushed
the
586-update-htlc-error-code-content
branch
from
April 19, 2020 22:33
d55738c to
6e2c9b1CompareTheBlueMatt
commented
Apr 19, 2020
Collaborator
Hmm, please drop the formatting commit. We'll probably want to do that all in one go (#425), its someone needless churn to do it as we go. |
Uh oh!
There was an error while loading. Please reload this page.
D4nteforce-pushed
the
586-update-htlc-error-code-content
branch
from
April 19, 2020 23:40
6e2c9b1 to
236887dCompareD4nte
commented
Apr 19, 2020
ContributorAuthor
Done! |
TheBlueMatt added a commit
to TheBlueMatt/rust-lightning
that referenced
this pull request
Apr 20, 2020
TheBlueMatt added a commit
to TheBlueMatt/rust-lightning
that referenced
this pull request
Apr 20, 2020
TheBlueMatt added a commit
to TheBlueMatt/rust-lightning
that referenced
this pull request
Apr 24, 2020
TheBlueMatt added a commit
to TheBlueMatt/rust-lightning
that referenced
this pull request
Apr 24, 2020
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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Resolves#586.
To prepare the work on this PR, I reviewed all the instantiations of
HTLCFailReason::Reasonagainst the latest BOLT spec and checked whether it includes the spec'd data.I have not reviewed in details whether or not the failure message was instantiated against the right conditions as the issue only mentioned content.
Edit: I intend to leverage type safety with a follow-up PR as per Matt's comment.