Allow specifying an error when failing back HTLC

[alecchendev]

Fixes #1570. This PR adds a new FailureCode enum, and a new function ChannelManager::fail_htlc_backwards_with_reason to allow the user to input their own failure reason (FailureCode) to form an HTLCFailReason to send to peers when failing back in response to an Event::PaymentClaimable. It also adds do_test_fail_htlc_backwards_with_reason and the test test_fail_htlc_backwards_with_reason.

Notes

• (DONE) Would like to DRY up between fail_htlc_backwards and fail_htlc_backwards_with_reason but because the msat + height data is found individually for each HTLC to form the error (in fail_htlc_backwards), not sure how I would.

To do

• (DONE) Add test(s). I was planning to test the new function similarly to fail_htlc_backwards, however I couldn't really find where fail_htlc_backwards was explicitly tested, mainly just used/indirectly tested throughout ln/functional_tests.rs. Still figuring what the best way to test this is.

Questions

• (DONE) Could anyone explain (or point me to a resource that explains) what the failure codes in HTLCFailReason::Reason are? I couldn’t find much in the docs/comments, I saw some stuff about failure codes in BOLT#2 for malformed HTLCs but this seems more general than that? Answer

This is my first PR on this repo so please forgive me if I'm asking something trivial! I'm going to try and participate in more code review to get more assimilated to the repo, but in the meantime if anyone is able to provide any feedback I'd greatly appreciate it.

[codecov-commenter]

Codecov Report

Base: 90.80% // Head: 90.75% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (6e485ae) compared to base (e0a0add).
Patch coverage: 93.44% of modified lines in pull request are covered.

❗ Current head 6e485ae differs from pull request most recent head 48aef2d. Consider uploading reports for the commit 48aef2d to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1948      +/-   ##
==========================================
- Coverage   90.80%   90.75%   -0.05%     
==========================================
  Files          98       98              
  Lines       51507    51733     +226     
  Branches    51507    51733     +226     
==========================================
+ Hits        46770    46952     +182     
- Misses       4737     4781      +44     

Impacted Files	Coverage Δ
lightning/src/util/events.rs	30.50% <ø> (ø)
lightning/src/ln/channelmanager.rs	87.24% <91.66%> (+0.05%)	⬆️
lightning/src/ln/onion_route_tests.rs	97.41% <93.87%> (-0.22%)	⬇️
lightning/src/ln/inbound_payment.rs	92.00% <0.00%> (-1.50%)	⬇️
lightning-invoice/src/utils.rs	96.94% <0.00%> (-0.83%)	⬇️
lightning/src/ln/functional_tests.rs	96.76% <0.00%> (-0.35%)	⬇️
lightning/src/chain/channelmonitor.rs	90.87% <0.00%> (-0.20%)	⬇️
lightning-invoice/src/lib.rs	87.37% <0.00%> (-0.11%)	⬇️
lightning/src/chain/keysinterface.rs	83.46% <0.00%> (ø)
lightning/src/ln/functional_test_utils.rs	91.21% <0.00%> (ø)
... and 3 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

[valentinewallace]

Could anyone explain (or point me to a resource that explains) what the failure codes in HTLCFailReason::Reason are?

Here are the possible failure codes/messages: https://github.com/lightning/bolts/blob/master/04-onion-routing.md#failure-messages

Re: testing, the expect_payment_failed macro in functional_test_utils checks a failed payment's error code and data :)

[alecchendev]

Thanks so much for the feedback @valentinewallace! I'll try to address most of the suggestions in about an hour or so.

[valentinewallace]

Looking pretty good to me. You'll want to amend your commit history at some point, we follow bitcoin core's guidelines: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

[valentinewallace]

Feel free to request a review once you've added tests or otherwise want another look

[alecchendev]

Will rebase tomorrow, but besides that I've mostly gotten through a new implementation that I think makes more sense as well as some tests.

I only implemented the four failure codes @TheBlueMatt mentioned (unknown_payment_details, temporary_node_failure, required_channel_feature_missing, expiry_too_soon) to start, but I still have to think about other ones I might want to add. Also might move the conversion from FailureCode to HTLCFailReason to a separate function especially if I add more.

In fail_htlc_backwards_with_reason, for the FailureCode::ExpiryTooSoon branch, I have a ton of unwraps, not super sure what's the best way to handle these--I see other places that are doing similar things (getting a channel from a short channel id) take different approaches, e.g. they sometimes just unwrap, sometimes propagate an error, sometimes handle it based on local context. Seems like it would be sorta unreasonable to panic when a user just tries to fail back an HTLC, so I'm thinking I might want to default to a certain failure code or maybe propagate an error so they can choose how to handle it. I would need to take some more time to think about which places it makes sense to just unwrap vs. properly handle the error, so suggestions welcome.

Also had some miscellaneous questions:

• For the check commits CI check, should I try to be passing that all the time, or mainly only after squashing at the end?
• While working on this I mainly just wanted to run the one test, but it took quite a while to compile because I assume it compiles all the other tests even if it doesn't use them...is there a way around this?

[TheBlueMatt]

Nice! This looks pretty great to me. Instead of merging in upstream changes, please rebase per the guide at https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes (and squash down fixup commits into one linear history that has logically separate commits that don't fix previous commits in the same PR).

[TheBlueMatt]

While working on this I mainly just wanted to run the one test, but it took quite a while to compile because I assume it compiles all the other tests even if it doesn't use them...is there a way around this?

Sadly, no. Rust has some ability to just recompile files that have changed, but its been on and off depending on the rust version due to various bugs. We also are (very slowly) working on breaking up our functional tests into more files to make this less painful. Speaking of which, please consider moving your new test to onion_route_tests :)

[alecchendev]

Instead of merging in upstream changes, please rebase per the guide at https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes

Oh I see, thanks.

(and squash down fixup commits into one linear history that has logically separate commits that don't fix previous commits in the same PR).

Just to clarify, should I be squashing any time I make changes in the PR or just at the end?

[TheBlueMatt]

Just to clarify, should I be squashing any time I make changes in the PR or just at the end?

It depends on the reviewer, in general we usually coordinate with reviewers by asking if they're ready for squashing, though that can delay PRs some so if its a relatively minor change feel free to just squash as you go - there's no hard rule here, we're flexible.

[alecchendev]

Btw I moved the code to fetch the corresponding error data to create the HTLCFailReason to a separate internal function get_htlc_fail_reason_from_failure_code. I rebased this to streamline the commits but realized leaving the separate commits temporarily would've probably been a helpful indicator, so just wanted to give a heads up.

[TheBlueMatt]

LGTM, I think. Please squash the fixup commit at the end back down into the second commit so we don't have commits fixing previous commits in the same PR.

[alecchendev]

Okay, I think the commits should be all fixed up 👍

[alecchendev]

Just squashed the first two commits together because the added function from the second commit was referenced in the first commit's documentation. Now it should be good I think.