[tnull]

Closes #1226

TransactionType was serialized with impl_writeable_tlv_based_enum, which fails to read any variant it doesn't know, odd or even. A node reading a payment classified with a later variant would fail to decode the whole PaymentDetails, and with it the payment store.

No release writes tx_type yet, so the reader can still change for free. Serialize TransactionType with impl_writeable_tlv_based_enum_upgradable and read both fields holding one, PaymentKind::Onchain's tx_type and ChannelTxFacts' self_role, as upgradable_option, as we already do for LDK's ClosureReason and PaymentFailureReason. An unknown odd variant now reads as no classification, while an unknown even one is still required. The written bytes are unchanged.

Written with the help of Claude Code.

[ldk-reviews-bot]

👋 Thanks for assigning @jkczyz 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.

[Camillarhi]

Thanks, this LGTM.

I noticed ChannelOutputRole and SpliceKind have the same issue. Both are new this cycle and still use the plain impl_writeable_tlv_based_enum!, so adding a variant to either would break reading the store after a downgrade. Could we fix them here or in a follow-up before the release?

Nit: the docs on PaymentKind::Onchain::tx_type list the reasons it can be None. It would help to add: "the transaction was classified by a newer version of LDK Node using a type this version doesn't recognize."

[tnull]

I noticed ChannelOutputRole and SpliceKind have the same issue. Both are new this cycle and still use the plain impl_writeable_tlv_based_enum!, so adding a variant to either would break reading the store after a downgrade. Could we fix them here or in a follow-up before the release?

Hmm, briefly considered it, but at least for SpliceKind it probably doesn't matter much. ChannelOutputRole maybe more, but it's a bit tedious to break the macro, so to do it properly we'd need to handle Options for these everywhere.

@jkczyz Thoughts? Worth now, or just when something breaks in the future?

[jkczyz]

Hmm, briefly considered it, but at least for SpliceKind it probably doesn't matter much. ChannelOutputRole maybe more, but it's a bit tedious to break the macro, so to do it properly we'd need to handle Options for these everywhere.

@jkczyz Thoughts? Worth now, or just when something breaks in the future?

FWIW, SpliceKind could probably be dropped now that #1080 removed the use of SpliceIntent (which could as well).

Regarding, ChannelOutputRole, could the macro be fixed upstream?

[jkczyz]

FWIW, SpliceKind could probably be dropped now that #1080 removed the use of SpliceIntent (which could as well).

Opened #1232.