[Andezion]

Changelog-None

Important

26.09 FREEZE August 5th: Non-bugfix PRs not ready by this date will wait for 26.12.

RC1 is scheduled on August 17th

The final release is scheduled for September 7th.

Summary

Classify locally-failed forwards with a precise root cause (failure_reason)

forward_event/listforwards currently only expose the raw BOLT4 onion failcode for local_failed forwards, WIRE_TEMPORARY_CHANNEL_FAILURE - which is the same code for at least three unrelated causes (insufficient outgoing liquidity, too many HTLCs, dust limit, peer offline...). Operators can't tell "we're out of outgoing liquidity" from other failure classes

This adds a new optional failure_reason string field (insufficient_outgoing_liquidity, outgoing_peer_offline, cltv_expiry_too_far, ...) derived as close to the real root cause as possible - including carrying the precise cause out of channeld's channel_add_htlc result instead of discarding it, and fixing a gap where a forward that failed because the outgoing subdaemon died mid-send never appeared in listforwards at all

Purely additive - existing failcode/failreason fields and old DB rows are untouched. Includes a new pytest reproducing the insufficient-liquidity case end-to-end and doc/schema updates.

Checklist

Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:

• [ x] The changelog has been updated in the relevant commit(s) according to the guidelines.
• [ x] Tests have been added or modified to reflect the changes.
• [ x] Documentation has been reviewed and updated as needed.
• [ x] Related issues have been listed and linked, including any that this PR closes.
• Important All PRs must consider how to reverse any persistent changes for tools/lightning-downgrade

[nepet]

Thanks a lot for working on this, @Andezion! Knowing why a forward failed locally, and especially being able to tell "we didn't have enough outbound liquidity" apart from everything else, is really useful for anyone running a routing node. The approach is the right one: channeld reports the reason in offer_htlc_reply, lightningd stores it with the forward, and listforwards exposes it without changing failcode/failreason. I also like that a forward is now recorded when the outgoing subdaemon dies mid-offer, that was a real gap.

I built the branch and ran the tests locally. Once the new test is fixed (see the inline comment), a forward over a drained channel is recorded as insufficient_outgoing_liquidity from start to finish, so the core mechanism works. The unit tests (run-wallet, run-db, run-full_channel), test_forward_event_notification and test_forward_local_failed_stats pass as well.

I've left inline comments on the details. These are the ones I'd consider necessary before merging:

1. The new test can't run yet (tests/test_pay.py).
2. insufficient_outgoing_liquidity is also reported when the peer, as the channel opener, can't afford the commitment fee for one more HTLC, even though we have plenty of outbound balance. I reproduced it and suggested a separate reason, with a test (channeld/channeld.c).
3. The new migration needs a revert for lightning-downgrade (wallet/migrations.c).
4. The generated gRPC Python files should be regenerated with the pinned tool versions (node_pb2_grpc.py). That's probably the check-compiled-source failure.

The rest are smaller suggestions and nits.

A few things I couldn't attach to a line:

The forward_event schema. failure_reason is also sent in the forward_event notification, because it shares the serializer with listforwards, but doc/schemas/notification/forward_event.json isn't updated. As a result the notification docs don't mention the field, and since msggen generates the gRPC ForwardEventNotification from that file, gRPC SubscribeForwardEvent subscribers never receive it. The tests don't catch this because that schema's response object doesn't set additionalProperties: false. Could you add the property right after failreason and regenerate? If you add a reason for the opener-fee case, it goes in both enums.

      "failure_reason": {
        "type": "string",
        "enum": [
          "insufficient_outgoing_liquidity",
          "too_many_htlcs",
          "dust_limit",
          "htlc_below_minimum",
          "htlc_above_maximum",
          "fee_insufficient",
          "cltv_incorrect",
          "cltv_expiry_too_soon",
          "cltv_expiry_too_far",
          "unknown_next_peer",
          "outgoing_peer_offline",
          "channel_failed_permanent",
          "invalid_onion",
          "channel_shutting_down",
          "max_htlc_value_in_flight"
        ],
        "description": [
          "The root cause of the local failure, more specific than *failcode*/*failreason* (not always known)."
        ],
        "added": "v26.12"
      },

Commits and changelog.

• This adds a user-visible field, so the commits need a Changelog-Added: line instead of Changelog-None, for example:

  Changelog-Added: JSON-RPC: `listforwards` and the `forward_event` notification include `failure_reason` for locally failed forwards.
  

• Please squash "fix forward_failure_reason crash risk and misclassifications" into the commit it fixes, and use subsystem prefixes such as channeld:, lightningd:, wallet: and doc: instead of add:. Splitting the work into one commit per step (the new wire field, storing the reason in the database, the JSON output and schema, the test) would make it much easier to review.
• failure_reason in doc/schemas/listforwards.json needs "added": "v26.12".

Rebase. master has moved on quite a bit. When I tried the merge, the only conflict was in the generated node_pb2.py, which goes away once it's regenerated. master has also added a migration after /* ^v26.09 */, so please check that yours still comes last after rebasing.

CI. Several jobs are red: two of the "First Integration Tests" shards, check-compiled-source, "Update examples in doc schemas", the min-BTC clang build and the macOS smoke test. The logs had expired by the time I looked, so I can't say for sure what failed, but I'd expect the broken test and the generated files to explain a good part of it. It would be good to see CI green after the next push.

Thanks again, this is going to be really, really useful! Happy to help with any of it, and I can share the repro test from the inline comment as a file if that's easier.

[nepet]

This test fails before it reaches the code it's meant to test. LightningNode.pay() doesn't take a route argument (its signature is pay(self, dst, amt, label=None)), and it also requires dst to be a direct peer, while l3 is two hops away. The getroute call further down fails as well, because getroute is deprecated in this tree.

With this change the test passes for me locally:

Suggested change

	l1.pay(l3, 2_500_000_000, route=True)
	# Now ask for more than l2 has left to spend on c23, but well within
	# the channels advertised capacity/htlc_maximum_msat, so getroute()
	# still finds it
	amt = 2_000_000_000
	inv = l3.rpc.invoice(amt, "insufficient_liquidity", "desc")
	payment_hash = inv['payment_hash']
	route = l1.rpc.getroute(l3.info['id'], amt, 1)['route']
	l1.rpc.xpay(l3.rpc.invoice(2_500_000_000, 'drain', 'drain')['bolt11'])
	# Now ask for more than l2 has left to spend on c23. getroute is
	# deprecated, so build the two-hop route by hand: the failure happens
	# at l2, so the exact delays don't matter.
	amt = 2_000_000_000
	inv = l3.rpc.invoice(amt, "insufficient_liquidity", "desc")
	payment_hash = inv['payment_hash']
	c12 = l1.get_channel_scid(l2)
	route = [{'amount_msat': amt + 10**6, 'id': l2.info['id'], 'delay': 30, 'channel': c12},
	{'amount_msat': amt, 'id': l3.info['id'], 'delay': 20, 'channel': c23}]

[Andezion]

thanks! done

[nepet]

CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED doesn't only mean that we lack outbound liquidity. In add_htlc() (channeld/full_channel.c, lines 836-881), the block that runs when we offer an HTLC also checks whether the channel opener can pay for the extra HTLC output: its room above the reserve, the 660 sat for the anchors, and the higher commitment fee. If our peer opened the channel, that's the peer's balance, not ours.

I reproduced it: l3 opens a 1,000,000 sat channel to l2 and pushes 990,000 sat to l2, keeping only its reserve. On l2, listpeerchannels shows a spendable_msat of 980,000,000 for that channel, yet forwarding 100,000 sat over it fails and is recorded as insufficient_outgoing_liquidity. That's a false positive in exactly the signal this PR adds, and it contradicts listpeerchannels. It's a common situation on nodes where clients open channels to us and then spend most of their balance.

I'd suggest giving that case its own error and its own reason. In channeld/full_channel_error.h:

	/* Can't afford it */
	CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED,
	/* The remote opener can't afford the commitment fee (or anchors)
	 * for one more HTLC output, although we can afford the HTLC */
	CHANNEL_ERR_OPENER_CANNOT_AFFORD_FEE,

In add_htlc(), pick the error once at the top of the if (sender == LOCAL) block and use it for the four failure returns inside it. When we are the opener, that block checks our own balance, so the existing error is still correct there:

		if (sender == LOCAL) {
			/* If they opened the channel, their balance pays for
			 * the extra output, so this isn't our liquidity */
			enum channel_add_err fee_err
				= channel->opener == sender
				? CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED
				: CHANNEL_ERR_OPENER_CANNOT_AFFORD_FEE;

In common/forward_failure_reason.h, append a value (the enum is stored in the database, so only append) and name it peer_cannot_afford_fee in forward_failure_reason_name():

	/* CHANNEL_ERR_OPENER_CANNOT_AFFORD_FEE: the peer opened the outgoing
	 * channel and can't pay the commitment fee for another HTLC output.
	 * Our own balance was sufficient */
	FORWARD_FAIL_PEER_CANNOT_AFFORD_FEE = 16,

Then add a case here and in the same switch in tests/plugins/channeld_fakenet.c. The wire error stays temporary_channel_failure, so payers see no difference:

	case CHANNEL_ERR_OPENER_CANNOT_AFFORD_FEE:
		failwiremsg = towire_temporary_channel_failure(inmsg, NULL);
		failstr = tal_fmt(inmsg, "Peer (opener) cannot afford commitment fee - HTLC fee: %s",
				  fmt_amount_sat(inmsg, htlc_fee));
		reason = FORWARD_FAIL_PEER_CANNOT_AFFORD_FEE;
		goto failed;

Finally, add peer_cannot_afford_fee to the failure_reason enum in the schemas and regenerate. It would also help to say in the description of insufficient_outgoing_liquidity that it means our own balance on the outgoing channel.

Test for this case, adapted from my repro (with the current code it gets insufficient_outgoing_liquidity)

@pytest.mark.openchannel('v1')
def test_forward_failure_peer_cannot_afford_fee(node_factory, bitcoind):
    """Peer opened the outgoing channel and sits at its reserve: we have
    plenty of outbound, but the peer can't pay the fee for another HTLC
    output.  That must not be reported as our liquidity."""
    l1, l2, l3 = node_factory.get_nodes(3)
    l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
    l3.rpc.connect(l2.info['id'], 'localhost', l2.port)
    l1.fundchannel(l2, 10**7)
    l3.fundwallet(10**7)
    cap = 10**6
    l3.rpc.fundchannel(l2.info['id'], cap, push_msat=(cap - cap // 100) * 1000)
    mine_funding_to_announce(bitcoind, [l1, l2, l3])
    for a, b in ((l1, l2), (l2, l3)):
        wait_for(lambda: only_one(a.rpc.listpeerchannels(b.info['id'])['channels'])['state'] == 'CHANNELD_NORMAL')
    c12, c23 = l1.get_channel_scid(l2), l2.get_channel_scid(l3)

    amt = 100_000_000
    assert only_one(l2.rpc.listpeerchannels(l3.info['id'])['channels'])['spendable_msat'] > amt
    inv = l3.rpc.invoice(amt, 'peerfee', 'peerfee')
    route = [{'amount_msat': amt + 10**6, 'id': l2.info['id'], 'delay': 30, 'channel': c12},
             {'amount_msat': amt, 'id': l3.info['id'], 'delay': 20, 'channel': c23}]
    l1.rpc.sendpay(route, inv['payment_hash'], payment_secret=inv['payment_secret'])
    with pytest.raises(RpcError):
        l1.rpc.waitsendpay(inv['payment_hash'])

    fwd = only_one(l2.rpc.listforwards(status='local_failed')['forwards'])
    assert fwd['failure_reason'] == 'peer_cannot_afford_fee'

[Andezion]

niiice, thank you!! i added CHANNEL_ERR_OPENER_CANNOT_AFFORD_FEE, chosen once at the top of the sender == LOCAL block, and FORWARD_FAIL_PEER_CANNOT_AFFORD_FEE = 16 / peer_cannot_afford_fee, appended at the end of the enum. The new case is in both channeld and fakenet, and the wire error is still temporary_channel_failure. The schema descriptions now say that insufficient_outgoing_liquidity means our own balance. i wrote test_forward_local_failed_peer_cannot_afford_fee from your description - l3 opens and pushes everything but its reserve, and we check that spendable_msat is still enough

[nepet]

This migration needs a revert, like the other recent ones. Without it, lightning-downgrade skips this step but still sets the database version back, so the column stays, and the next upgrade fails when it tries to add the existing column again. (The downgrade checkbox in the PR description is still unticked.)

Suggested change

	{SQL("ALTER TABLE forwards ADD reason INTEGER DEFAULT NULL"), NULL},
	{SQL("ALTER TABLE forwards ADD reason INTEGER DEFAULT NULL"), NULL,
	SQL("ALTER TABLE forwards DROP COLUMN reason"), NULL},

A case in tests/test_downgrade.py would be nice if it's cheap to add.

[Andezion]

done!!

[nepet]

This file and primitives_pb2.py look like they were regenerated with an older grpcio-tools/protobuf: the GRPC_GENERATED_VERSION guard and every _registered_method=True are gone here, and primitives_pb2.py lost its protobuf runtime version check. When I regenerated them with the versions pinned in the repo (grpcio 1.75.1, protobuf 6.32.1), both came out identical to master, so this PR shouldn't change them at all.

Could you restore them and regenerate with the pinned tools?

git checkout master -- contrib/pyln-grpc-proto/pyln/grpc/node_pb2_grpc.py \
                       contrib/pyln-grpc-proto/pyln/grpc/primitives_pb2.py
uv sync
uv run make

Afterwards only node_pb2.py should differ from master. That should also fix the check-compiled-source CI job, and it removes the rebase conflict on these files.

[Andezion]

You were right, i had an old grpcio-tools installed((. i regenerated with the pinned versions through uv. node_pb2_grpc.py and primitives_pb2.py are now identical to master, and only node_pb2.py differs

[nepet]

Splitting out CHANNEL_ERR_MAX_HTLC_VALUE_IN_FLIGHT_EXCEEDED also changes what we send on the wire when the in-flight limit is hit: required_node_feature_missing becomes temporary_channel_failure. I think that's the right change, because required_node_feature_missing is a permanent node-level error and makes payers avoid our whole node. But it's a behaviour change other implementations can see, so it shouldn't be hidden in a fixup.

Could you move it into its own commit (the new enum value, the return change in full_channel.c, and the cases here and in channeld_fakenet.c) with a changelog line, for example:

channeld: return temporary_channel_failure when max_htlc_value_in_flight is exceeded

We used to return required_node_feature_missing, a permanent node
failure, which makes payers exclude our whole node for what is a
temporary channel condition.

Changelog-Fixed: Protocol: forwarding an HTLC that would exceed the outgoing channel's `max_htlc_value_in_flight_msat` now fails with `temporary_channel_failure` instead of `required_node_feature_missing`.

[Andezion]

done!!

[nepet]

test_forward_local_failed_stats already produces five different local failures, so one more assert covers most of the new reasons for free. These are the values a local run of the test records:

Suggested change

	assert [s.get('out_channel') for s in stats['forwards']] == [c23, c24, c25, None, c24]
	assert [s.get('out_channel') for s in stats['forwards']] == [c23, c24, c25, None, c24]
	# And we record why each of them failed
	assert [s.get('failure_reason') for s in stats['forwards']] == ['unknown_next_peer', 'fee_insufficient', 'htlc_above_maximum', 'invalid_onion', 'channel_failed_permanent']

[Andezion]

added!

[nepet]

nit: could you initialise this? Today every failure path in send_htlc_out() sets it, but a future early return that forgets to would write an uninitialised value to the database.

Suggested change

	enum forward_failure_reason reason;
	enum forward_failure_reason reason = FORWARD_FAIL_UNKNOWN;

[Andezion]

Done, it's initialised to FORWARD_FAIL_UNKNOWN!!

[nepet]

nit: this second if block mostly repeats the first one. The first block already handles local_failed (together with failed) and already lists failure_reason, and both else branches (lines 368 and 453) now allow failure_reason on offered and settled forwards, where lightningd never sets it.

I think this would be simpler:

• Move the full failure_reason definition (type, enum, description) to line 345 in the first block, next to failcode and failreason, and remove this second block.
• Remove "failure_reason": {} from the first block's else (line 368).

The schema then also allows failure_reason on failed forwards, but lightningd only ever sets it for local_failed, and the description can say so.

[Andezion]

Simplified as you suggested. The full definition now sits next to failcode/failreason in the local_failed/failed block, the second block is gone, and so is the entry in the else. The description says the field is only set for local_failed

[nepet]

Hey, just some hints on how to get CI green:

1. Generated files are out of date (check-compiled-source, macOS smoke test)

make check-gen-updated regenerates the Rust bindings and gets a diff. The new enums in cln-rpc/src/model.rs (ListforwardsForwardsFailureReason) and cln-rpc/src/notifications.rs (ForwardEventFailureReason) are missing the doc comment that msggen builds from the schema description. It looks like the schema was edited after the bindings were generated. Running make gen and amending the result into the lightningd: commit should fix it.

2. test_splice_stored_feerate_repaired_on_upgrade fails on startup (all the integration shards)

**BROKEN** lightningd: Error executing statement: wallet/db.c:64:
ALTER TABLE forwards ADD reason INTEGER DEFAULT NULL: duplicate column name: reason

This isn't a bug in your migration. The test assumes the two feerate-clamping migrations are the last ones and rewinds with UPDATE version SET version = version - 2. With your migration appended, that rewind re-runs your ALTER TABLE instead of the clamps. Changing it to - 3 would just break again with the next migration. Instead, make the test rewind to the absolute DB version just before the two clamp migrations (master's current DB version minus 2). Ideally that goes in its own small commit before yours.