Skip to content

routing+migration: fix mission control migration with nil failure msg #9770

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 2, 2025
Merged

routing+migration: fix mission control migration with nil failure msg #9770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e5c5414
routing: make msg and index optional
bitromortac Apr 30, 2025
4498a78
routing: remove paymentFailureInfo
bitromortac Apr 30, 2025
a450d85
routing: remove paymentFailure pointer
bitromortac Apr 30, 2025
aa9abb3
routing: make sure failure message is valid
bitromortac Apr 30, 2025
15f0888
routing: fix mission control migration
bitromortac Apr 28, 2025
5020e1f
docs: update release notes
bitromortac May 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 54 additions & 3 deletions channeldb/migration32/migration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,32 @@ var (
},
}

resultOld3 = paymentResultOld{
id: 3,
timeFwd: time.Unix(0, 4),
timeReply: time.Unix(0, 7),
success: false,
failure: nil,
failureSourceIdx: &failureIndex,
route: &Route{
TotalTimeLock: 101,
TotalAmount: 401,
SourcePubKey: testPub2,
Hops: []*Hop{
{
PubKeyBytes: testPub,
ChannelID: 800,
OutgoingTimeLock: 4,
AmtToForward: 4,
BlindingPoint: pubkey,
EncryptedData: []byte{1, 2, 3},
CustomRecords: customRecord,
TotalAmtMsat: 600,
},
},
},
}

//nolint:ll
resultNew1Hop1 = &mcHop{
channelID: tlv.NewPrimitiveRecord[tlv.TlvType0, uint64](100),
Expand Down Expand Up @@ -182,7 +208,7 @@ var (
),
failure: tlv.SomeRecordT(
tlv.NewRecordT[tlv.TlvType3](
*newPaymentFailure(
newPaymentFailure(
&failureIndex,
&lnwire.FailFeeInsufficient{},
),
Expand Down Expand Up @@ -217,6 +243,31 @@ var (
}),
}),
}

//nolint:ll
resultNew3 = paymentResultNew{
id: 3,
timeFwd: tlv.NewPrimitiveRecord[tlv.TlvType0, uint64](
uint64(time.Unix(0, 4).UnixNano()),
),
timeReply: tlv.NewPrimitiveRecord[tlv.TlvType1, uint64](
uint64(time.Unix(0, 7).UnixNano()),
),
failure: tlv.SomeRecordT(
tlv.NewRecordT[tlv.TlvType3](
newPaymentFailure(
&failureIndex, nil,
),
),
),
route: tlv.NewRecordT[tlv.TlvType2](mcRoute{
sourcePubKey: tlv.NewRecordT[tlv.TlvType0](testPub2),
totalAmount: tlv.NewRecordT[tlv.TlvType1, lnwire.MilliSatoshi](401),
hops: tlv.NewRecordT[tlv.TlvType2](mcHops{
resultNew2Hop1,
}),
}),
}
)

// TestMigrateMCRouteSerialisation tests that the MigrateMCRouteSerialisation
Expand All @@ -225,10 +276,10 @@ var (
func TestMigrateMCRouteSerialisation(t *testing.T) {
var (
resultsOld = []*paymentResultOld{
&resultOld1, &resultOld2,
&resultOld1, &resultOld2, &resultOld3,
}
expectedResultsNew = []*paymentResultNew{
&resultNew1, &resultNew2,
&resultNew1, &resultNew2, &resultNew3,
}
)

Expand Down
156 changes: 67 additions & 89 deletions channeldb/migration32/mission_control_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,12 @@ func deserializeOldResult(k, v []byte) (*paymentResultOld, error) {

// convertPaymentResult converts a paymentResultOld to a paymentResultNew.
func convertPaymentResult(old *paymentResultOld) *paymentResultNew {
var failure *paymentFailure
var failure fn.Option[paymentFailure]
if !old.success {
failure = newPaymentFailure(old.failureSourceIdx, old.failure)
failure = fn.Some(newPaymentFailure(
old.failureSourceIdx,
old.failure,
))
}

return newPaymentResult(
Expand All @@ -106,7 +109,7 @@ func convertPaymentResult(old *paymentResultOld) *paymentResultNew {

// newPaymentResult constructs a new paymentResult.
func newPaymentResult(id uint64, rt *mcRoute, timeFwd, timeReply time.Time,
failure *paymentFailure) *paymentResultNew {
failure fn.Option[paymentFailure]) *paymentResultNew {

result := &paymentResultNew{
id: id,
Expand All @@ -119,11 +122,11 @@ func newPaymentResult(id uint64, rt *mcRoute, timeFwd, timeReply time.Time,
route: tlv.NewRecordT[tlv.TlvType2](*rt),
}

if failure != nil {
failure.WhenSome(func(f paymentFailure) {
result.failure = tlv.SomeRecordT(
tlv.NewRecordT[tlv.TlvType3](*failure),
tlv.NewRecordT[tlv.TlvType3](f),
)
}
})

return result
}
Expand All @@ -142,33 +145,49 @@ type paymentResultNew struct {
failure tlv.OptionalRecordT[tlv.TlvType3, paymentFailure]
}

// paymentFailure represents the presence of a payment failure. It may or may
// not include additional information about said failure.
type paymentFailure struct {
info tlv.OptionalRecordT[tlv.TlvType0, paymentFailureInfo]
}

// newPaymentFailure constructs a new paymentFailure struct. If the source
// index is nil, then an empty paymentFailure is returned. This represents a
// failure with unknown details. Otherwise, the index and failure message are
// used to populate the info field of the paymentFailure.
func newPaymentFailure(sourceIdx *int,
failureMsg lnwire.FailureMessage) *paymentFailure {
Copy link
Collaborator

@ziggie1984 ziggie1984 Apr 30, 2025

Choose a reason for hiding this comment

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

as mentioned before I think we can also get rid of the sourceIdx pointer ?

Copy link
Collaborator Author

@bitromortac bitromortac May 2, 2025

Choose a reason for hiding this comment

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

I'm currently not seeing a lot of advantages in removing the source index pointer and would like to keep further changes in this bug fix PR minimal (also with regards to changing the signature to return an error), since this is not touching the serialization. I can change it though if you feel strongly about it.

Copy link
Collaborator

@ziggie1984 ziggie1984 May 2, 2025

Choose a reason for hiding this comment

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

ok cool let's keep it as is.

failureMsg lnwire.FailureMessage) paymentFailure {

// If we can't identify a failure source, we also won't have a decrypted
// failure message. In this case we return an empty payment failure.
if sourceIdx == nil {
return &paymentFailure{}
return paymentFailure{}
}

info := paymentFailureInfo{
sourceIdx: tlv.NewPrimitiveRecord[tlv.TlvType0](
uint8(*sourceIdx),
info := paymentFailure{
sourceIdx: tlv.SomeRecordT(
tlv.NewPrimitiveRecord[tlv.TlvType0](
uint8(*sourceIdx),
),
),
msg: tlv.NewRecordT[tlv.TlvType1](failureMessage{failureMsg}),
}

return &paymentFailure{
info: tlv.SomeRecordT(tlv.NewRecordT[tlv.TlvType0](info)),
if failureMsg != nil {
Copy link
Collaborator

@ziggie1984 ziggie1984 Apr 28, 2025

Choose a reason for hiding this comment

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

Add a comment why this might happen, decoding error of the error msg.

What about the case where we have a nil source index:

lnd/routing/missioncontrol.go

Lines 843 to 845 in b34afa3

if sourceIdx == nil {
return &paymentFailure{}
}

we should instead return nil here, so we do not create the failure information.

Copy link
Collaborator Author

@bitromortac bitromortac Apr 29, 2025

Choose a reason for hiding this comment

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

we should instead return nil here, so we do not create the failure information.

Ah, I think we should do something about the pointer, I converted to use a fn.Option[paymentFailure] in other places and removed the pointer, because newPaymentFailure always constructs a paymentFailure, it still represents a payment failure if the info fields are absent.

Copy link
Collaborator Author

@bitromortac bitromortac Apr 29, 2025

Choose a reason for hiding this comment

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

Add a comment why this might happen, decoding error of the error msg.

I added comments to the paymentFailure fields.

What about the case where we have a nil source index

The sourceIdx is a prerequisite to be able to decrypt the error via the hop's shared secret, so I think it makes sense to return an empty paymentFailure here, because if we don't know the index, we won't be able to get a message.

Copy link
Collaborator

@guggero guggero Apr 29, 2025

Choose a reason for hiding this comment

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

I more or less had the same question when looking at the code. Perhaps this explanation is worth a comment in both versions of newPaymentFailure where we return an empty paymentFailure.

Copy link
Collaborator Author

@bitromortac bitromortac Apr 30, 2025

Choose a reason for hiding this comment

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

It looks like one level of nesting can be removed, trying that out.

Copy link
Collaborator Author

@bitromortac bitromortac Apr 30, 2025

Choose a reason for hiding this comment

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

It seems to work, I removed paymentFailure, whose role is now taken over by the old paymentFailureInfo. Instead of checking the optional record in paymentFailure, I think we can use the source index instead. I put all mission control migration changes together in the last commit, which was easier for me. Let me know if I should make changes in lockstep.

info.msg = tlv.SomeRecordT(
tlv.NewRecordT[tlv.TlvType1](
failureMessage{failureMsg},
),
)
}

return info
}

// paymentFailure holds additional information about a payment failure.
type paymentFailure struct {
// sourceIdx is the hop the error was reported from. In order to be able
// to decrypt the error message, we need to know the source, which is
// why an error message can only be present if the source is known.
sourceIdx tlv.OptionalRecordT[tlv.TlvType0, uint8]

// msg is the error why a payment failed. If we identify the failure of
// a certain hop at the above index, but aren't able to decode the
// failure message we indicate this by not setting this field.
msg tlv.OptionalRecordT[tlv.TlvType1, failureMessage]
}

// Record returns a TLV record that can be used to encode/decode a
Expand All @@ -194,14 +213,27 @@ func (r *paymentFailure) Record() tlv.Record {
func encodePaymentFailure(w io.Writer, val interface{}, _ *[8]byte) error {
if v, ok := val.(*paymentFailure); ok {
var recordProducers []tlv.RecordProducer
v.info.WhenSome(
func(r tlv.RecordT[tlv.TlvType0, paymentFailureInfo]) {
recordProducers = append(recordProducers, &r)

v.sourceIdx.WhenSome(
func(r tlv.RecordT[tlv.TlvType0, uint8]) {
recordProducers = append(
recordProducers, &r,
)
},
)

v.msg.WhenSome(
func(r tlv.RecordT[tlv.TlvType1, failureMessage]) {
recordProducers = append(
recordProducers, &r,
)
},
)

return lnwire.EncodeRecordsTo(
w, lnwire.ProduceRecordsSorted(recordProducers...),
w, lnwire.ProduceRecordsSorted(
recordProducers...,
),
)
}

Expand All @@ -210,81 +242,27 @@ func encodePaymentFailure(w io.Writer, val interface{}, _ *[8]byte) error {

func decodePaymentFailure(r io.Reader, val interface{}, _ *[8]byte,
l uint64) error {

if v, ok := val.(*paymentFailure); ok {
var h paymentFailure

info := tlv.ZeroRecordT[tlv.TlvType0, paymentFailureInfo]()
sourceIdx := tlv.ZeroRecordT[tlv.TlvType0, uint8]()
msg := tlv.ZeroRecordT[tlv.TlvType1, failureMessage]()

typeMap, err := lnwire.DecodeRecords(
r, lnwire.ProduceRecordsSorted(&info)...,
r,
lnwire.ProduceRecordsSorted(&sourceIdx, &msg)...,
)

if err != nil {
return err
}

if _, ok := typeMap[h.info.TlvType()]; ok {
h.info = tlv.SomeRecordT(info)
if _, ok := typeMap[h.sourceIdx.TlvType()]; ok {
h.sourceIdx = tlv.SomeRecordT(sourceIdx)
}

*v = h

return nil
}

return tlv.NewTypeForDecodingErr(val, "routing.paymentFailure", l, l)
}

// paymentFailureInfo holds additional information about a payment failure.
type paymentFailureInfo struct {
sourceIdx tlv.RecordT[tlv.TlvType0, uint8]
msg tlv.RecordT[tlv.TlvType1, failureMessage]
}

// Record returns a TLV record that can be used to encode/decode a
// paymentFailureInfo to/from a TLV stream.
func (r *paymentFailureInfo) Record() tlv.Record {
recordSize := func() uint64 {
var (
b bytes.Buffer
buf [8]byte
)
if err := encodePaymentFailureInfo(&b, r, &buf); err != nil {
panic(err)
}

return uint64(len(b.Bytes()))
}

return tlv.MakeDynamicRecord(
0, r, recordSize, encodePaymentFailureInfo,
decodePaymentFailureInfo,
)
}

func encodePaymentFailureInfo(w io.Writer, val interface{}, _ *[8]byte) error {
if v, ok := val.(*paymentFailureInfo); ok {
return lnwire.EncodeRecordsTo(
w, lnwire.ProduceRecordsSorted(
&v.sourceIdx, &v.msg,
),
)
}

return tlv.NewTypeForEncodingErr(val, "routing.paymentFailureInfo")
}

func decodePaymentFailureInfo(r io.Reader, val interface{}, _ *[8]byte,
l uint64) error {

if v, ok := val.(*paymentFailureInfo); ok {
var h paymentFailureInfo

_, err := lnwire.DecodeRecords(
r,
lnwire.ProduceRecordsSorted(&h.sourceIdx, &h.msg)...,
)
if err != nil {
return err
if _, ok := typeMap[h.msg.TlvType()]; ok {
h.msg = tlv.SomeRecordT(msg)
}

*v = h
Expand All @@ -293,7 +271,7 @@ func decodePaymentFailureInfo(r io.Reader, val interface{}, _ *[8]byte,
}

return tlv.NewTypeForDecodingErr(
val, "routing.paymentFailureInfo", l, l,
val, "routing.paymentFailure", l, l,
)
}

Expand Down
10 changes: 6 additions & 4 deletions docs/release-notes/release-notes-0.19.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,10 +459,12 @@ the on going rate we'll permit.

## Database

* [Migrate the mission control
store](https://github.com/lightningnetwork/lnd/pull/8911) to use a more
minimal encoding for payment attempt routes as well as use [pure TLV
encoding](https://github.com/lightningnetwork/lnd/pull/9167).
* [Migrate the mission control
Copy link
Collaborator

@ziggie1984 ziggie1984 Apr 30, 2025

Choose a reason for hiding this comment

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

Release notes in a separate commit please.

store](https://github.com/lightningnetwork/lnd/pull/8911) to use a more
minimal encoding for payment attempt routes as well as use [pure TLV
encoding](https://github.com/lightningnetwork/lnd/pull/9167). [A
fix](https://github.com/lightningnetwork/lnd/pull/9770) was added to handle
nil routing failure messages and the serialization was optimized.

* [Migrate the mission control
store](https://github.com/lightningnetwork/lnd/pull/9001) so that results are
Expand Down
Loading
Loading