Drop return value from `fail_htlc_backwards`, clarify docs
authorMatt Corallo <git@bluematt.me>
Tue, 19 Apr 2022 22:06:50 +0000 (22:06 +0000)
committerMatt Corallo <git@bluematt.me>
Sat, 28 May 2022 00:02:49 +0000 (00:02 +0000)
commita12d37e0633d94f1651cd24b656024b84f68e3cb
tree9ba647cdeb63a7df14508cb9dde649ae2309d631
parent11c2f12baaeb4fb8fcd3965eb1aaea7b8aa21722
Drop return value from `fail_htlc_backwards`, clarify docs

`ChannelManager::fail_htlc_backwards`' bool return value is quite
confusing - just because it returns false doesn't mean the payment
wasn't (already) failed. Worse, in some race cases around shutdown
where a payment was claimed before an unclean shutdown and then
retried on startup, `fail_htlc_backwards` could return true even
though (a duplicate copy of the same payment) was claimed, but the
claim event has not been seen by the user yet.

While its possible to use it correctly, its somewhat confusing to
have a return value at all, and definitely lends itself to misuse.

Instead, we should push users towards a model where they don't care
if `fail_htlc_backwards` succeeds - either they've locally marked
the payment as failed (prior to seeing any `PaymentReceived`
events) and will fail any attempts to pay it, or they have not and
the payment is still receivable until its timeout time is reached.

We can revisit this decision based on user feedback, but will need
to very carefully document the potential failure modes here if we
do.
fuzz/src/chanmon_consistency.rs
lightning/src/ln/chanmon_update_fail_tests.rs
lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_test_utils.rs
lightning/src/ln/functional_tests.rs
lightning/src/ln/onion_route_tests.rs