Skip to content

Fix missing PaymentSent::fee_paid_msat#4651

Open
valentinewallace wants to merge 1 commit into
lightningdevkit:mainfrom
valentinewallace:2026-06-paymentsent-unwrap
Open

Fix missing PaymentSent::fee_paid_msat#4651
valentinewallace wants to merge 1 commit into
lightningdevkit:mainfrom
valentinewallace:2026-06-paymentsent-unwrap

Conversation

@valentinewallace
Copy link
Copy Markdown
Contributor

If an outbound payment was abandoned with htlcs in-flight and later claimed, we would previously have the PaymentSent::fee_paid_msat be set to None. This contradicted some docs on the event that stated the field would always be Some after 0.0.103.

Fixes #4639

If an outbound payment was abandoned with htlcs in-flight and later claimed, we
would previously have the PaymentSent::fee_paid_msat be set to None. This
contradicted some docs on the event that stated the field would always be Some
after 0.0.103.
@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented Jun 1, 2026

I've assigned @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.

@ldk-reviews-bot ldk-reviews-bot requested a review from tankyleo June 1, 2026 18:52
/// overstate the amount paid, though this is unlikely.
///
/// This is only `None` for payments initiated on LDK versions prior to 0.0.103.
/// May be `None` for payments initiated on LDK versions prior to 0.4.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This wording slightly overstates the scope. For payments that stayed in Retryable state (i.e., were never abandoned while in-flight), fee_paid_msat has been Some since 0.0.103. The None gap between 0.0.103 and 0.4 only applies to payments that were abandoned with HTLCs in-flight and then unexpectedly claimed.

Something like "May be None for payments abandoned on LDK versions prior to 0.4, or initiated prior to 0.0.103." would be more precise. Not blocking — the current "May be" phrasing is technically correct.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like keeping it simple but open to feedback

@ldk-claude-review-bot
Copy link
Copy Markdown
Collaborator

Review Summary

The fix is correct. The core logic — preserving pending_fee_msat during the RetryableAbandoned transition and keeping it in sync as failed paths are removed — is sound. I verified the key invariants:

  1. In mark_abandoned(), get_pending_fee_msat() is called while still in Retryable state (before the transition), so the fee is correctly captured.
  2. In handle_pay_route_err(), remove() is called before auto-abandonment (mark_abandoned), so the fee is decremented while still in Retryable state for the auto-abandon case. For the manual-abandon case (abandon_early=true), the new Abandoned branch in remove() handles it.
  3. send_payment_success() reads get_pending_fee_msat() before calling mark_fulfilled(), so the fee is captured before transitioning to Fulfilled (which doesn't track fees).
  4. The serialization with TLV field 5 (odd = optional) is backward-compatible — old nodes without this field will deserialize pending_fee_msat as None.

The test assertion change from Some(None) to Some(Some(2_000)) in the existing test_inconsistent_mpp_params is also correct — that test's payment gets auto-abandoned when the mismatched MPP path fails (since retry_strategy is None), so it was hitting the same Abandoned-state bug even without explicit abandonment.

Inline comments posted:

  • lightning/src/events/mod.rs:1204 — doc precision: the None gap between 0.0.103 and 0.4 only applies to abandoned-then-claimed payments, not all payments.

No bugs found.

@valentinewallace
Copy link
Copy Markdown
Contributor Author

Fuzz failure looks pre-existing, seems to error with "Expected disconnect"

@ldk-reviews-bot
Copy link
Copy Markdown

🔔 1st Reminder

Hey @tankyleo! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Event::PaymentSent::fee_paid_msat can be None despite docs to the contrary

3 participants