Update ldk-node dependency & expose bolt12 proofs

[benthecarman]

Update ldk-node and adapt payment events to its current API. Expose payment IDs in events and retain manual handling for unknown BOLT 11 payments.

Add BOLT 12 payer-proof creation to the gRPC, CLI, and MCP interfaces. Include the preimage and invoice in successful-payment events for stateless proof creation.

Use ldk-node pagination for payment history. Remove duplicate payment records from the ldk-server SQLite store, which now contains only forwarded-payment history.

[ldk-reviews-bot]

👋 Thanks for assigning @tankyleo as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[benthecarman]

Rebased and updated ldk-node to new commit with the pagination changes. Now using the paginated payments instead of the ldk-server version

[tnull]

Looks good, but we should probably also include the new channel type in list_channels now.

[tankyleo]

Codex wants to classify this to an InvalidRequestError rather than an InternalServerError. It's not perfect, but seems InvalidRequestError is more likely if we hit PayerProofCreationFailed

[benthecarman]

Yeah not perfect mapping, but fixed

[tankyleo]

I asked Codex to cast a broader net, here's what it found, feel free to dismiss aggressively, but all seemed worth taking a look to me.

[tankyleo]

Also confirmed this patch now keeps my fans quiet on mainnet

[tankyleo]

Codex found this can race with update_all_pollable_metrics and suggests to remove fn update_payments_count entirely and have update_all_pollable_metrics be the sole writer.

I then asked about the other metrics here's what it had to say:

_ Yes, but not all in the same way.

  - Payment counters had the strongest race: update_all_pollable_metrics() performed an absolute store(), while update_payments_count() performed fetch_add() on the same value. That could permanently double-count or lose an
    update until reconciliation.

  - Balance gauges can still race: both the polling task and payment event handlers call update_all_balances(). Because both write absolute snapshots, an older, slower call could overwrite a newer snapshot. This causes
    temporary staleness, not double-counting, and the next refresh corrects it.

  - Channel metrics avoid this particular race: total_channels_count is event-driven, while public/private channel counts are poll-driven. They do not concurrently update the same atomic value.
  - Peer count is polling-only.

  For complete consistency, balance refreshes should also be serialized or assigned to a single updater. Prometheus gauges usually tolerate brief staleness, whereas payment counters decreasing or double-counting is more
  operationally problematic.

[benthecarman]

thanks fixed

[tankyleo]

Codex suggests we initialize other metrics in addition to the total channels count:

    Initialize poll-only metrics at startup

    Seed peer and channel visibility gauges before the first delayed metrics poll.\n\nAI assistance: OpenAI Codex was used for this change.

diff --git a/ldk-server/src/util/metrics.rs b/ldk-server/src/util/metrics.rs
index 1c92b87..893b1b9 100644
--- a/ldk-server/src/util/metrics.rs
+++ b/ldk-server/src/util/metrics.rs
@@ -133,9 +133,20 @@ impl Metrics {
                        Err(e) => error!("Failed to initialize payment metrics: {e}"),
                }

-               let channels_count = node.list_channels().len() as i64;
-               self.total_channels_count.store(channels_count, Ordering::Relaxed);
+               let all_channels = node.list_channels();
+               self.total_channels_count.store(all_channels.len() as i64, Ordering::Relaxed);
+
+               let public_channels_count =
+                       all_channels.iter().filter(|channel_details| channel_details.is_announced).count()
+                               as i64;
+               self.total_public_channels_count.store(public_channels_count, Ordering::Relaxed);

+               let private_channels_count =
+                       all_channels.iter().filter(|channel_details| !channel_details.is_announced).count()
+                               as i64;
+               self.total_private_channels_count.store(private_channels_count, Ordering::Relaxed);
+
+               self.update_peer_count(node);
                self.update_all_balances(node);
        }

[benthecarman]

fixed, collect all before a delayed poll

[tankyleo]

Grok 4.6 found this: should we call event_node.event_handled() in these branches too ?

[bug] ldk-server/src/main.rs:728 _ send_payment_event skips event_handled() when payment-store lookup fails, which can stall the event queue

Then codex:

[P1] Resolve payment events when payment lookup fails _
With an active subscriber, Ok(None) and Err(_) return without calling event_handled().
Since LDK Node repeatedly returns the queue-head event until acknowledged,
a missing or unreadable payment record blocks every later event and creates a tight retry loop.
Emit a partial event with payment: None, or otherwise explicitly acknowledge/drop the event;
use controlled backoff if retrieval errors should be retried.

[tankyleo]

this is the suggested patch:

diff --git a/ldk-server/src/main.rs b/ldk-server/src/main.rs
index c5fd800..e1fab26 100644
--- a/ldk-server/src/main.rs
+++ b/ldk-server/src/main.rs
@@ -706,27 +706,23 @@ fn send_payment_event(
 ) {
        if event_sender.receiver_count() == 0 {
                debug!("No event subscribers connected, skipping payment event");
-               if let Err(e) = event_node.event_handled() {
-                       error!("Failed to mark event as handled: {e}");
+       } else {
+               match event_node.payment(payment_id) {
+                       Ok(Some(payment_details)) => {
+                               let payment = payment_to_proto(payment_details);
+
+                               let event = payment_to_event(payment);
+                               if let Err(e) = event_sender.send(EventEnvelope { event: Some(event) }) {
+                                       debug!("No event subscribers connected, skipping event: {e}");
+                               }
+                       },
+                       Ok(None) => error!("Unable to find payment with payment ID: {payment_id}"),
+                       Err(e) => error!("Failed to retrieve payment with payment ID {payment_id}: {e}"),
                }
-               return;
        }

-       match event_node.payment(payment_id) {
-               Ok(Some(payment_details)) => {
-                       let payment = payment_to_proto(payment_details);
-
-                       let event = payment_to_event(payment);
-                       if let Err(e) = event_sender.send(EventEnvelope { event: Some(event) }) {
-                               debug!("No event subscribers connected, skipping event: {e}");
-                       }
-
-                       if let Err(e) = event_node.event_handled() {
-                               error!("Failed to mark event as handled: {e}");
-                       }
-               },
-               Ok(None) => error!("Unable to find payment with payment ID: {payment_id}"),
-               Err(e) => error!("Failed to retrieve payment with payment ID {payment_id}: {e}"),
+       if let Err(e) = event_node.event_handled() {
+               error!("Failed to mark event as handled: {e}");
        }
 }

[benthecarman]

done

[benthecarman]

Also fixed the proto definition to be a single string for the PageToken and it's handling in the cli.

[tankyleo]

While chatting about how we call event_handled even if we hit a persistence read failure and fail to send the payment event in fn send_payment_event instead of not handling the event and replaying it hoping to forward it later, I eventually landed on this documentation update below. TLDR people should be aware notifications are currently best effort.

diff --git a/docs/api-guide.md b/docs/api-guide.md
index fb0e737..50cc052 100644
--- a/docs/api-guide.md
+++ b/docs/api-guide.md
@@ -216,8 +216,21 @@ See [Pagination](#pagination) below for how to page through results.
 | `SpliceNegotiated` | A channel splice was negotiated and the funding transaction is pending confirmation |
 | `SpliceNegotiationFailed` | A channel splice negotiation round failed                       |

-Events are broadcast to all connected subscribers. The server uses a bounded broadcast channel
-(capacity 1024). A slow subscriber that falls behind will miss events.
+> [!WARNING]
+> `SubscribeEvents` is a best-effort live notification stream. Events are not persisted for
+> subscribers, cannot be replayed after reconnecting, and have no client acknowledgement. Receipt
+> by the server's broadcast channel does not guarantee that a client received or processed an
+> event.
+
+Events are broadcast to all currently connected subscribers. The server uses a bounded broadcast
+channel (capacity 1024), so a slow subscriber that falls behind will miss events. Disconnected
+clients also miss events and receive only new events after reconnecting. If the server cannot read
+data required to construct an event, it logs the error and may skip that event so the event queue
+can continue processing.
+
+Treat events as notifications rather than authoritative history. After reconnecting, reconcile
+recoverable state with APIs such as `GetPaymentDetails`, `ListPayments`, `ListForwardedPayments`,
+and `ListChannels`. Some event-only fields cannot be recovered through these APIs.

[tankyleo]

Somewhat related to the above discussion, also landed on this update here:

-The payment is held in a pending state until you explicitly claim or fail it. **You must
-always handle each event.** If you do not, the HTLC will eventually time out. This can cause a
-force-closure of the channel.
+The payment is held in a pending state until you claim it, fail it, or its `claim_deadline` is
+reached. `PaymentClaimable` notifications are best-effort and are not replayed. If you miss the
+event or do not act before the deadline, LDK Node automatically fails the HTLC backward and the
+payment can no longer be claimed. Keep the subscriber healthy and resolve reported persistence
+errors before accepting further payments.

[tankyleo]

Came up with this here, worth applying to the proto definition too I think:

 /// This event is only emitted for payments created via `Bolt11ReceiveForHash`.
 /// Handle every event by its payment ID before `claim_deadline`.
 /// The same invoice can produce more than one event. Fail unexpected duplicate or late payments.
+/// Delivery through `SubscribeEvents` is best-effort and is not replayed. If the event is missed and
+/// the payment is not otherwise claimed or failed, LDK Node automatically fails the HTLC backward at
+/// `claim_deadline`.

[tankyleo]

In this file for pub struct SubscribeEventsRequest {} this diff below would pair with the documentation update I mentioned above. We'd also apply this to the documentation in the proto defintion at message SubscribeEventsRequest {}

-/// Subscribe to a stream of server events.
+/// Subscribe to a best-effort stream of new server events.
+///
+/// Events are not persisted for subscribers or replayed after reconnecting, and the server does not
+/// wait for client acknowledgement. Slow or disconnected subscribers may miss events. Reconcile
+/// recoverable state with the listing and detail APIs after reconnecting.
+///
+/// If a PaymentClaimable event is missed and the payment is not otherwise claimed or failed, LDK
+/// Node automatically fails the HTLC backward at its claim_deadline.

[tankyleo]

For struct ForwardedPayment, seems like outbound_amount_forwarded_msat does not need to be optional ?

-       #[prost(uint64, optional, tag = "4")]
-       pub outbound_amount_forwarded_msat: ::core::option::Option<u64>,
+       #[prost(uint64, tag = "4")]
+       pub outbound_amount_forwarded_msat: u64,

[tankyleo]

Looks like we are back to using ThreadRng here instead of OsRng, we should ask rust-bip39 to make a release before we ship this, cc @tnull

[tankyleo]

Let's expose amount_msat in HtlcLocator ?

 /// Identifies the channel and counterparty that an HTLC was processed with.
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[cfg_attr(feature = "serde", serde(rename_all = "snake_case"))]
 #[cfg_attr(feature = "serde", serde(default))]
 #[allow(clippy::derive_partial_eq_without_eq)]
 #[derive(Clone, PartialEq, ::prost::Message)]
 pub struct HtlcLocator {
        /// The channel that the HTLC was sent or received on.
        #[prost(string, tag = "1")]
        pub channel_id: ::prost::alloc::string::String,
+       /// The amount, in milli-satoshis, of the HTLC that was sent or received.
+       /// This can be unset for events serialized by LDK Node v0.7.0 and prior.
+       #[prost(uint64, optional, tag = "2")]
+       pub amount_msat: ::core::option::Option<u64>,
        /// The `user_channel_id` for the channel.
        /// This can be unset for older serialized events or if the payment was settled on-chain.
-       #[prost(string, optional, tag = "2")]
+       #[prost(string, optional, tag = "3")]
        pub user_channel_id: ::core::option::Option<::prost::alloc::string::String>,
        /// The node id of the counterparty for this HTLC.
        /// This can be unset for older serialized events.
-       #[prost(string, optional, tag = "3")]
+       #[prost(string, optional, tag = "4")]
        pub node_id: ::core::option::Option<::prost::alloc::string::String>,
 }

[tankyleo]

Let's forward the claimable_amount_msat to our subscribers here ?

[tankyleo]

Found this a little weird, perhaps it's a fix for ldk-node: when the user sets this field, and sets this field to an amount greater than the amount actually claimable, we go ahead and claim the htlc. Shouldn't we scream here instead ? ie you tried to claim x sats, but only y sats were available to claim.

would you consider making this field required instead of optional ? or perhaps we should drop the amount from claim_for_id entirely and have the user do the validation of the amount themselves, and decide themselves whether to fail or claim ?

[tankyleo]

nit: can we consolidate all these if let statements into a single if let after the match ?

[tankyleo]

One more comment, found with a quick "look for ways to simplify the code" with codex.

[tankyleo]

Let's put these into a helper method, DRY with update_all_pollable_metrics. Can also derive private count from channel count - public count so we avoid a traversal.