lightningd: don't force-close when fulfilled HTLC removal is in progress

[vincenzopalazzo]

Fixes #8899.

Two commits, red then green: the first adds reproducer tests (they fail
on master with the force-close below), the second stops the force-close
and adapts the two existing tests which relied on it.

The bug

CLN force-closes the channel when a fulfilled incoming HTLC hits its
deadline, even though removal is already in progress:

lightningd: Peer permanent failure in CHANNELD_NORMAL:
  Fulfilled HTLC 0 SENT_REMOVE_HTLC cltv 119 hit deadline

But if the HTLC has reached SENT_REMOVE_HTLC (or later), channeld has
been told to send update_fulfill_htlc upstream, or will be on reconnect:
the preimage is in the DB, so onchaind can always claim on-chain, the
cooperative path is cheaper and faster, and if the peer goes on-chain,
onchaind handles it. Force-closing just burns fees.

After the fix we log an UNUSUAL and let the normal state machine finish
the removal:

lightningd: UNUSUAL: Fulfilled HTLC 0 SENT_REMOVE_HTLC cltv 119
  hit deadline, but removal already in progress

The tests

• test_fulfilled_htlc_deadline_no_force_close (reproducer): l1->l2->l3
  line graph, pay, disconnect l2 from l1 right before update_fulfill_htlc
  is sent (-WIRE_UPDATE_FULFILL_HTLC), so the incoming HTLC is stuck in
  SENT_REMOVE_HTLC; mining to the deadline used to force-close.
• test_fulfilled_htlc_deadline_reconnect: same, but l2 reconnects after
  the deadline fires and completes the removal cooperatively.
• test_htlc_no_force_close / test_htlc_in_timeout are adapted: the
  fulfilling peer no longer force-closes, the offering peer does, and
  onchaind claims via the preimage.

(Also dropped an unrelated event-notifications.md doc blob that had
accidentally landed on this branch.)

[vincenzopalazzo]

Hi @daywalker90 👋 — this replaces #9408 (identical commits) and fixes #8899: nodes force-closing healthy channels on the fulfilled-HTLC removal deadline race (real-world logs in the issue from a BTCPay node). This is the most release-critical of my three v26.09 PRs — could you take a look before the RC? #9427 and #9428 are the other two.

[Andezion]

As i see hin->preimage for an incoming HTLC is set in exactly one place, fulfill_htlc(), and the very next line unconditionally bumps the state:

hin->preimage = tal_dup(hin, struct preimage, preimage);
htlc_in_update_state(channel, hin, SENT_REMOVE_HTLC);

And htlc_in_update_state() -> state_update_ok() enforces newstate == oldstate + 1, state is strictly forward-only. There is no other assignment to hin->preimage anywhere in the codebase (checked wallet.c dbload, htlc_end.c constructor/destructor) that doesn't go through this path or restore both fields together from the db. So the invariant hin->preimage != NULL => hin->hstate >= SENT_REMOVE_HTLC holds unconditionally, for the entire lifetime of hin?

The loop this PR touches already has, right above the new code:

if (!hin->preimage)
    continue;

So given the invariant, by the time execution reaches the new if (hin->hstate >= SENT_REMOVE_HTLC) check, it is always true? And the channel_fail_permanent(...) call for the "Fulfilled HTLC" case is now unreachable?
Or what is a concrete hin state where hin->preimage != NULL and hin->hstate < SENT_REMOVE_HTLC?