Use LocalHTLCFailureReason in Onion Processing #3744
Conversation
|
👋 Thanks for assigning @valentinewallace as a reviewer! |
|
@joostjager anything else that you'd like to add here? |
There was a problem hiding this comment.
The only comment that I think may not be in this PR is #3601 (comment)
If that one still applies now of course.
Addressed in 2cc8799 |
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
|
✅ Added second reviewer: @wpaulino |
wpaulino
requested review from
valentinewallace
and removed request for
wpaulino
|
@joostjager your approval came in before I could address last comment last comment - diff here 🙏 |
| self.failure_code() == LocalHTLCFailureReason::IncorrectPaymentDetails.failure_code() | ||
| || *self == LocalHTLCFailureReason::FinalIncorrectCLTVExpiry | ||
| || *self == LocalHTLCFailureReason::FinalIncorrectHTLCAmount | ||
| || *self == LocalHTLCFailureReason::MPPTimeout |
There was a problem hiding this comment.
I think this is intended to only apply to errors that directly map from onion codes, but we could also include InvalidKeysendPreimage and PaymentSecretRequired here if we expect this method to be used elsewhere in the future.
There was a problem hiding this comment.
InvalidKeysendPreimage will already be covered by self.failure_code() == LocalHTLCFailureReason::IncorrectPaymentDetails.failure_code()
PaymentSecretRequired
I think I'll update the doc a bit to note that this is only dealing with bolt 04 codes? Because we'd never get this variant as a sender, and RequiredNodeFeature (which it maps to) isn't a recipient-specific error.
lightning/src/ln/onion_utils.rs
Outdated
| error_code, | ||
| error_code.failure_code(), | ||
| debug_field, | ||
| log_bytes!(&err_packet.failuremsg[4..4 + debug_field_size]), | ||
| description | ||
| error_code |
There was a problem hiding this comment.
nit: no need to log error_code twice, similar below
There was a problem hiding this comment.
I thought it would be nice to log the variant name ({:?}) and the string ({}) so that the variant was easy to look up.
Will update based on whether we keep display.
| cur_height, outgoing_cltv_value, msg.cltv_expiry, | ||
| ) { | ||
| return Err(InboundHTLCErr { | ||
| msg: err_msg, | ||
| msg: "incoming cltv check failed", |
There was a problem hiding this comment.
Just a note here for review: this msg loses a bit of granularity - previously it had the descriptive string, and now it's just got this generic statement because we can't get a &str from format!({:?}, reason)'s String that lives long enough (afaik).
I think that this is okay because peel_payment_onion isn't used anywhere in the codebase, and the new reason is now available to provide the information that's lost in the string. If it's not ok, I can revert check_incoming_htlc_cltv to still return &str, LocalHTLCFailureReason.
There was a problem hiding this comment.
Seems legit 👍 Thanks for the diligence!
|
Dropped |
|
Looks like this needs another test fix :( seems one of the formerly logged strings is being asserted on still? Also good with a squash! |
Squash should fix it, fixup commit contained the test fix so the unsquashed commit's build check failed 👍 nvm: enum variant name is logged twice not the expected once - diff to fix |
In some places where we have LocalHTLCFailureReason, the string we're passing around is redundant. This isn't the case in all uses of LocalHTLCFailureReason, for example InboundHTLCError provides additional information about payload and blinding related errors that isn't captured in LocalHTLCFailureReason. In these cases we keep the error string, tolerating some duplication.
These error codes were removed from the specification seven years ago to prevent probing, so we don't need to handle these cases anymore.
De-duplicate use of u16 failure codes by using a macro that will match against each variant's failure_code instead.
This PR contains some of the follow ups that didn't make it into #3601:
DisplayforLocalHTLCFailureReasonLocalHTLCFailureReasonin onion error processing, removing the last few places where we were using raw u16 valuesfailure_codeandfrom<u16>to dedup codes