Add HTLCHandlingFailed event

[jurvis]

Addresses #1392

When forward_htlcs is empty, forward_event does not get set, so I assume that means that represents an instance where forwarding has failed since we have no HTLCs to forward (?). may be completely off here.

if possible, I will like some help finding the test where this event is suppose to be called 😄 thanks!

edit: functional_tests helped me find those failing tests 😅 will fix

[jurvis]

hey @TheBlueMatt, some of the tests affected by this change use the expect_pending_htlcs_forwardable_ignore! macro that hardcodes a count of events it expects to 1.

this makes sense; since we only send the PendingHTLCsForwardable event once when we fail backward. however, we probably need a new macro for expecting potentially the additional count of PaymentForwardingFailed events. Before I work on this change, I just wanted to ask and see if you may be aware of a better way of doing this + if the place in ChannelManager where I push PaymentForwardedFailed even makes sense at this current moment.

thanks!

[TheBlueMatt]

As for the test macros/updates, yes, this kind of change tends to end up with a ton of test changes spewn throughout. Hopefully my comment below may make it simplier (actually just update the expect_pending_htlcs_forwardable_ignore/expect_pending_htlcs_forwardable macro(s) to detect the new event, with either a new argument to indicate at the test-sites if its required or something like PaymentFailedConditions to set a list of conditions and pass that in.

[jurvis]

@TheBlueMatt got it! thanks for the guidance 😄

[codecov-commenter]

Codecov Report

Merging #1403 (6c5af58) into main (5cca9a0) will increase coverage by 0.90%.
The diff coverage is 92.24%.

❗ Current head 6c5af58 differs from pull request most recent head 8f0d9b0. Consider uploading reports for the commit 8f0d9b0 to get more accurate results

@@ Coverage Diff @@## main #1403 +/- ##
==========================================
+ Coverage 90.87% 91.77% +0.90% 
==========================================
Files 80 80 Lines 44645 49595 +4950 Branches 44645 49595 +4950 ==========================================
+ Hits 40569 45518 +4949 - Misses 4076 4077 +1 

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	68.18% <ø> (ø)
lightning/src/util/events.rs	39.50% <46.15%> (-0.04%)	⬇️
lightning/src/ln/channelmanager.rs	87.99% <83.92%> (+2.90%)	⬆️
lightning/src/ln/functional_tests.rs	98.30% <97.70%> (+1.20%)	⬆️
lightning/src/chain/chainmonitor.rs	97.97% <100.00%> (+0.01%)	⬆️
lightning/src/chain/channelmonitor.rs	90.97% <100.00%> (+0.01%)	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	97.71% <100.00%> (ø)
lightning/src/ln/channel.rs	88.78% <100.00%> (+0.03%)	⬆️
lightning/src/ln/functional_test_utils.rs	96.78% <100.00%> (+1.54%)	⬆️
lightning/src/ln/monitor_tests.rs	100.00% <100.00%> (ø)
... and 14 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cca9a0...8f0d9b0. Read the comment docs.

[jurvis]

I think I have implemented this PR to the fullest of my knowledge of how things work... I added some comments in places I want to call out because I'm not 100% confident of them.

Also need some help in places, specifically with identifying when to not send our node_id as the sink_node_id

[jurvis]

update: still working on this PR. stepping away to catch up with some other deadlines. hope to make my next iteration by end of the week :)

[TheBlueMatt]

Looks like this is starting to shape up! Let me know when you want another round of review on this.

[jurvis]

@TheBlueMatt yeah I just got a little stuck finding the counterparty_node_id at some of the fail_htlc_backwards_internal call-sites and had a question in an old comment thread: #1403 (comment)

[jurvis]

@TheBlueMatt not a PR I have super high confidence about getting right the first time, but will like to see how I can improve :D

[TheBlueMatt]

A number of comments about data provided and structure before I jump into reviewing all the creation sites for correctness.

[TheBlueMatt]

Looks like this needs rebase.

[jurvis]

Please don't rebase PRs unless there's a conflict, it makes it very difficult to track changes to the PR vs upstream.

oops, sorry. thought I was being helpful, lol.

[jurvis]

@TheBlueMatt I saw your comment in my email notifications but can't find it on GitHub for this line

you said:

Rather than asserting that the destination is in the expected set, lets assert that the destination is exactly the last entry in the destination vec, and remove it from the vec.

I'm looking to correct this, but when you say that "the destination is exactly the last entry in the destination vec," is this what you mean?:

$crate::util::events::Event::HTLCHandlingFailed { ref failed_next_destination, .. } => {
let destination = destinations.pop().unwrap();
assert_eq!(&destination, failed_next_destination);
},

because that seems to imply that the event order when calling get_and_clear_pending_events() is in reverse chronological order, but I don't think that's how it is right now 🤔 or am I misunderstanding something?

[TheBlueMatt]

Yea, sorry, I deleted that comment because I'm not sure we need/want tests to assert on a precise event ordering given a set of HTLCs get failed at the same time. In general we do want to avoid tests that just check that each of a series of events is in some set - we want to make sure all of the set get returned - but we don't want to be overly prescriptive in event ordering in case we change it in the future.

[TheBlueMatt]

Code changes look good. Please make sure to wrap your commit messages (including descriptions) at 70-80 lines long.

[jkczyz]

Largely looks good. Just a bunch of nits.

[jurvis]

cleaned up commit messages

[TheBlueMatt]

Code LGTM, some doc notes but this should be basically ready.