Skip to content

Conversation

@tnull
Copy link
Collaborator

@tnull tnull commented Oct 16, 2025

Recent LDK changes made skimmed_fee_msat a required field of the LSPS service handler's payment_forwarded API, which seemed reasonable given that the field is available since LDK 0.0.122. However, when updating LDK Node we introduced a debug_assert that checked the field to be always set, which is wrong, as it's only set post-0.0.122 if there was some fee skimmed. Here we fix this oversight.

Recent LDK changes made `skimmed_fee_msat` a required field of the LSPS
service handler's `payment_forwarded` API, which seemed reasonable given
that the field is available since LDK 0.0.122. However, when updating
LDK Node we introduced a `debug_assert` that checked the field to be
*always* set, which is wrong, as it's only set post-0.0.122 *if* there
was some fee skimmed. Here we fix this oversight.
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Oct 16, 2025

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

@tnull
Copy link
Collaborator Author

tnull commented Oct 16, 2025

Relatedly, we might want to change the API for 0.2 final still, but that's tbd. over at lightningdevkit/rust-lightning#4162

@tnull tnull added this to the 0.7 milestone Oct 16, 2025
"We expect skimmed_fee_msat to be set since LDK 0.0.122"
);
}
let skimmed_fee_msat = skimmed_fee_msat.unwrap_or(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

And None can happen when we upgrade while there are still unprocessed events on disk?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, though they'd need to upgrade directly from an LDK Node version prior to 0.3.0, which shipped June 2024. I don't think we have any users that would still run such an old version, let alone as a forwarding node. And the LSP functionality was ofc. also introduced later than this.

@tnull tnull merged commit bbca7d2 into lightningdevkit:main Oct 16, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants