Skip to content
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.

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

routing+channeldb: mpp bucket structure #4000

Merged
merged 16 commits into from
Mar 10, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
6 changes: 6 additions & 0 deletions channeldb/db.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/coreos/bbolt"
"github.com/go-errors/errors"
"github.com/lightningnetwork/lnd/channeldb/migration12"
"github.com/lightningnetwork/lnd/channeldb/migration13"
"github.com/lightningnetwork/lnd/channeldb/migration_01_to_11"
"github.com/lightningnetwork/lnd/clock"
"github.com/lightningnetwork/lnd/lnwire"
Expand Down Expand Up @@ -124,6 +125,11 @@ var (
number: 12,
migration: migration12.MigrateInvoiceTLV,
},
{
// Migrate to multi-path payments.
number: 13,
migration: migration13.MigrateMPP,
},
}

// Big endian is the preferred byte order, due to cursor scans over
Expand Down
2 changes: 2 additions & 0 deletions channeldb/log.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"github.com/btcsuite/btclog"
"github.com/lightningnetwork/lnd/build"
"github.com/lightningnetwork/lnd/channeldb/migration12"
"github.com/lightningnetwork/lnd/channeldb/migration13"
"github.com/lightningnetwork/lnd/channeldb/migration_01_to_11"
)

Expand All @@ -29,4 +30,5 @@ func UseLogger(logger btclog.Logger) {
log = logger
migration_01_to_11.UseLogger(logger)
migration12.UseLogger(logger)
migration13.UseLogger(logger)
}
14 changes: 14 additions & 0 deletions channeldb/migration13/log.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package migration13

import (
"github.com/btcsuite/btclog"
)

// log is a logger that is initialized as disabled. This means the package will
// not perform any logging by default until a logger is set.
var log = btclog.Disabled

// UseLogger uses a specified Logger to output package logging info.
func UseLogger(logger btclog.Logger) {
log = logger
}
202 changes: 202 additions & 0 deletions channeldb/migration13/migration.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,202 @@
package migration13
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be okay having the migration in a separate commit. No one is gonna run the commit pre migration anyway, and it eases review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think commits should work on their own also with pre-existing data.

Copy link
Member

Choose a reason for hiding this comment

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

There really is a lot in this commit: al the modifications for the new format, as well as a migration for the logic. As reviewer looking at the commits sequentially, I prefer to have the changes first as then I can update my working mental model of the code, then use that to verify that the migration does what I expect. Larger commits like this also reduce the efficacy of bisection.

Copy link
Member

Choose a reason for hiding this comment

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

I'd also prefer additional commentary on the migration commit as well (also for the base commit). This commit as is touches nearly a dozen files (in a non-trivial manner), yet has just two sentences of a commit body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved migration to separate commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved full failure reason addition to separate commit, now that migration is on its own anyway. Extended commit messages


import (
"encoding/binary"
"fmt"

"github.com/coreos/bbolt"
)

var (
paymentsRootBucket = []byte("payments-root-bucket")

// paymentCreationInfoKey is a key used in the payment's sub-bucket to
// store the creation info of the payment.
paymentCreationInfoKey = []byte("payment-creation-info")

// paymentFailInfoKey is a key used in the payment's sub-bucket to
// store information about the reason a payment failed.
paymentFailInfoKey = []byte("payment-fail-info")

// paymentAttemptInfoKey is a key used in the payment's sub-bucket to
// store the info about the latest attempt that was done for the
// payment in question.
paymentAttemptInfoKey = []byte("payment-attempt-info")

// paymentSettleInfoKey is a key used in the payment's sub-bucket to
// store the settle info of the payment.
paymentSettleInfoKey = []byte("payment-settle-info")

// paymentHtlcsBucket is a bucket where we'll store the information
// about the HTLCs that were attempted for a payment.
paymentHtlcsBucket = []byte("payment-htlcs-bucket")

// htlcAttemptInfoKey is a key used in a HTLC's sub-bucket to store the
// info about the attempt that was done for the HTLC in question.
htlcAttemptInfoKey = []byte("htlc-attempt-info")

// htlcSettleInfoKey is a key used in a HTLC's sub-bucket to store the
// settle info, if any.
htlcSettleInfoKey = []byte("htlc-settle-info")

// htlcFailInfoKey is a key used in a HTLC's sub-bucket to store
// failure information, if any.
htlcFailInfoKey = []byte("htlc-fail-info")

byteOrder = binary.BigEndian
)

// MigrateMPP migrates the payments to a new structure that accommodates for mpp
// payments.
func MigrateMPP(tx *bbolt.Tx) error {
joostjager marked this conversation as resolved.
Show resolved Hide resolved
log.Infof("Migrating payments to mpp structure")

// Iterate over all payments and store their indexing keys. This is
// needed, because no modifications are allowed inside a Bucket.ForEach
// loop.
paymentsBucket := tx.Bucket(paymentsRootBucket)
if paymentsBucket == nil {
return nil
}

var paymentKeys [][]byte
err := paymentsBucket.ForEach(func(k, v []byte) error {
paymentKeys = append(paymentKeys, k)
return nil
})
if err != nil {
return err
}

// With all keys retrieved, start the migration.
for _, k := range paymentKeys {
bucket := paymentsBucket.Bucket(k)

// We only expect sub-buckets to be found in
// this top-level bucket.
if bucket == nil {
return fmt.Errorf("non bucket element in " +
"payments bucket")
}

// Fetch old format creation info.
creationInfo := bucket.Get(paymentCreationInfoKey)
if creationInfo == nil {
return fmt.Errorf("creation info not found")
}

// Make a copy because bbolt doesn't allow this value to be
// changed in-place.
newCreationInfo := make([]byte, len(creationInfo))
copy(newCreationInfo, creationInfo)

// Convert to nano seconds.
timeBytes := newCreationInfo[32+8 : 32+8+8]
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking but this section could be made more readable by getting ride of the magic numbers and using types from the time package.

time := byteOrder.Uint64(timeBytes)
timeNs := time * 1000000000
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably easier to create a time struct from the bytes, then write out as UnixNano.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is that easier? Conversion from secs to nanosecs isn't more than this multiplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe you commented on this because of the stray timeBytes[0]=0 statement?

byteOrder.PutUint64(timeBytes, timeNs)

// Write back new format creation info.
err := bucket.Put(paymentCreationInfoKey, newCreationInfo)
if err != nil {
return err
}

// No migration needed if there is no attempt stored.
attemptInfo := bucket.Get(paymentAttemptInfoKey)
if attemptInfo == nil {
continue
}

// Delete attempt info on the payment level.
if err := bucket.Delete(paymentAttemptInfoKey); err != nil {
return err
}

// Save attempt id for later use.
attemptID := attemptInfo[:8]

// Discard attempt id. It will become a bucket key in the new
// structure.
attemptInfo = attemptInfo[8:]

// Append unknown (zero) attempt time.
var zero [8]byte
attemptInfo = append(attemptInfo, zero[:]...)

// Create bucket that contains all htlcs.
htlcsBucket, err := bucket.CreateBucket(paymentHtlcsBucket)
if err != nil {
return err
}

// Create an htlc for this attempt.
htlcBucket, err := htlcsBucket.CreateBucket(attemptID)
if err != nil {
return err
}

// Save migrated attempt info.
err = htlcBucket.Put(htlcAttemptInfoKey, attemptInfo)
if err != nil {
return err
}

// Migrate settle info.
settleInfo := bucket.Get(paymentSettleInfoKey)
if settleInfo != nil {
// Payment-level settle info can be deleted.
err := bucket.Delete(paymentSettleInfoKey)
if err != nil {
return err
}

// Append unknown (zero) settle time.
settleInfo = append(settleInfo, zero[:]...)

// Save settle info.
err = htlcBucket.Put(htlcSettleInfoKey, settleInfo)
if err != nil {
return err
}

// Migration for settled htlc completed.
continue
}

// If there is no payment-level failure reason, the payment is
// still in flight and nothing else needs to be migrated.
// Otherwise the payment-level failure reason can remain
// unchanged.
inFlight := bucket.Get(paymentFailInfoKey) == nil
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we also keep the payment level failure?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, it just stays where it already is. Maybe add a comment to make that clear.

Didn't we have some problems before by migrating values in-loop?. Could be worth both for that reason and readability to read out data and put back into bucket in separate loops.

Reference:
b7a3772#diff-99f3013528876f55a0e46ff7014e8a19
974ec02#diff-99f3013528876f55a0e46ff7014e8a19

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to fetch keys in separate loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think storing the key slice is safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added about keeping payment level failure

if inFlight {
continue
}

// The htlc failed. Add htlc fail info with reason unknown. We
// don't have access to the original failure reason anymore.
failInfo := []byte{
// Fail time unknown.
0, 0, 0, 0, 0, 0, 0, 0,

// Zero length wire message.
0,
joostjager marked this conversation as resolved.
Show resolved Hide resolved

// Failure reason unknown.
0,

// Failure source index zero.
0, 0, 0, 0,
}

// Save fail info.
err = htlcBucket.Put(htlcFailInfoKey, failInfo)
if err != nil {
return err
}
}

log.Infof("Migration of payments to mpp structure complete!")

return nil
}
123 changes: 123 additions & 0 deletions channeldb/migration13/migration_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
package migration13

import (
"testing"

"github.com/coreos/bbolt"
"github.com/lightningnetwork/lnd/channeldb/migtest"
)

var (
hex = migtest.Hex

zeroTime = hex("0000000000000000")
noFailureMessage = hex("00")
failureReasonUnknown = hex("00")
zeroFailureSourceIdx = hex("00000000")

hash1 = hex("02acee76ebd53d00824410cf6adecad4f50334dac702bd5a2d3ba01b91709f0e")
creationInfoAmt1 = hex("00000000004c4b40")
creationInfoTime1 = hex("000000005e4fb7ab") // 1582282667 (decimal)
creationInfoTimeNano1 = hex("15f565b3cccaee00") // 1582282667000000000 (decimal)
creationInfoPayReq1 = hex("00000000")
attemptInfo1 = hex("2997a72e129fc9d638ef2fa4e233567d808d4f18a4f087637582427962eb3bf800005ce600000000004c4b402102ec12e83eafe27ce6d03bbe0c0de4b79fe2b9934615c8aa7693f73d2e41b089700000000121028c2dd128c7a6c1a0fceb3e3eb5ed55e0a0ae1a939eb786b097322d830d47db75005ca4000001000000005ce600000000004c4b400000000000")
attemptID1 = hex("0000000000000001")
paymentID1 = hex("0000000000000001")

hash2 = hex("62eb3f0a48f954e495d0c14ac63df04a67cefa59dafdbcd3d5046d1f5647840c")
preimage2 = hex("479593b7d3cbb45beb22d448451a2f3619b2095adfb38f4d92e9886e96534368")
attemptID2 = hex("00000000000003e8")
paymentID2 = hex("0000000000000002")
attemptInfo2 = hex("8de663f9bb4b8d1ebdb496d22dc1cb657a346215607308549f41b01e2adf2ce900005ce600000000005b8d802102ec12e83eafe27ce6d03bbe0c0de4b79fe2b9934615c8aa7693f73d2e41b089700000000121028c2dd128c7a6c1a0fceb3e3eb5ed55e0a0ae1a939eb786b097322d830d47db75005ca4000001000000005ce600000000005b8d8000000000010000000000000008233d281e2cbe01f0b82dd6750967c9233426b98ae6549c696365f57f86f942a3795b8d80")
creationInfoAmt2 = hex("00000000005b8d80")
creationInfoTime2 = hex("000000005e4fb97f") // 1582283135 (decimal)
creationInfoTimeNano2 = hex("15F56620C3C43600") // 1582283135000000000 (decimal)
creationInfoPayReq2 = hex("000000fc6c6e62637274363075317030796c7774367070357674346e377a6a676c39327766397773633939767630307366666e7561376a656d74376d6535373471336b3337346a387373787164717163717a70677370353835357075743937713863747374776b7735796b306a667278736e746e7a6878326a77786a636d3937346c636437327a3564757339717939717371653872336b3578733379367868667366366d6a6e706d717172306661797a677a63336a6b663571787a6c376866787a6666763578667a7679647564327275767974706571787072376868796830726a747574373033333274737774686661616e303773766b6667716b7174667275")

hash3 = hex("62eb3f0a48f954e495d0c14ac63df04a67cefa59dafdbcd3d5046d1f5647840d")
attemptInfo3 = hex("53ce0a4c1507cc5ea00ec88b76bd43a3978ac13605497030b821af6ce9c110f300005ce600000000006acfc02102ec12e83eafe27ce6d03bbe0c0de4b79fe2b9934615c8aa7693f73d2e41b089700000000121028c2dd128c7a6c1a0fceb3e3eb5ed55e0a0ae1a939eb786b097322d830d47db75005ca4000001000000005ce600000000006acfc000000000010000000000000008233044f235354472318b381fad3e21eb5a58f5099918868b0610e7b7bcb7a4adc96acfc0")
attemptID3 = hex("00000000000003e9")
paymentID3 = hex("0000000000000003")
creationInfoAmt3 = hex("00000000006acfc0")
creationInfoTime3 = hex("000000005e4fb98d") // 1582283149
creationInfoTimeNano3 = hex("15F56624063B4200") // 1582283149000000000 (decimal)
creationInfoPayReq3 = hex("000000fc6c6e62637274373075317030796c7776327070357674346e377a6a676c39327766397773633939767630307366666e7561376a656d74376d6535373471336b3337346a387373787364717163717a706773703578707a307964663467336572727a656372376b6e7567307474667630327a7665727a72676b70737375376d6d6564617934687973397179397173717774656479336e666c323534787a36787a75763974746767757a647473356e617a7461616a6735667772686438396b336d70753971726d7a6c3779637a306e30666e6e763077753032726632706e64636c393761646c667636376a7a6e7063677477356434366771323571326e32")

// pre is the data in the payments root bucket in database version 12 format.
pre = map[string]interface{}{
// A failed payment.
hash1: map[string]interface{}{
"payment-attempt-info": attemptID1 + attemptInfo1,
"payment-creation-info": hash1 + creationInfoAmt1 + creationInfoTime1 + creationInfoPayReq1,
"payment-fail-info": hex("03"),
"payment-sequence-key": paymentID1,
},

// A settled payment.
hash2: map[string]interface{}{
"payment-attempt-info": attemptID2 + attemptInfo2,
"payment-creation-info": hash2 + creationInfoAmt2 + creationInfoTime2 + creationInfoPayReq2,
"payment-sequence-key": paymentID2,
"payment-settle-info": preimage2,
},

// An in-flight payment.
hash3: map[string]interface{}{
"payment-attempt-info": attemptID3 + attemptInfo3,
"payment-creation-info": hash3 + creationInfoAmt3 + creationInfoTime3 + creationInfoPayReq3,
"payment-sequence-key": paymentID3,
},
}

// post is the expected data after migration.
post = map[string]interface{}{
hash1: map[string]interface{}{
"payment-creation-info": hash1 + creationInfoAmt1 + creationInfoTimeNano1 + creationInfoPayReq1,
"payment-fail-info": hex("03"),
"payment-htlcs-bucket": map[string]interface{}{
attemptID1: map[string]interface{}{
"htlc-attempt-info": attemptInfo1 + zeroTime,
"htlc-fail-info": zeroTime + noFailureMessage + failureReasonUnknown + zeroFailureSourceIdx,
},
},
"payment-sequence-key": paymentID1,
},
hash2: map[string]interface{}{
"payment-creation-info": hash2 + creationInfoAmt2 + creationInfoTimeNano2 + creationInfoPayReq2,
"payment-htlcs-bucket": map[string]interface{}{
attemptID2: map[string]interface{}{
"htlc-attempt-info": attemptInfo2 + zeroTime,
"htlc-settle-info": preimage2 + zeroTime,
},
},
"payment-sequence-key": paymentID2,
},
hash3: map[string]interface{}{
"payment-creation-info": hash3 + creationInfoAmt3 + creationInfoTimeNano3 + creationInfoPayReq3,
"payment-htlcs-bucket": map[string]interface{}{
attemptID3: map[string]interface{}{
"htlc-attempt-info": attemptInfo3 + zeroTime,
},
},
"payment-sequence-key": paymentID3,
},
}
)

// TestMigrateMpp asserts that the database is properly migrated to the mpp
// payment structure.
func TestMigrateMpp(t *testing.T) {
var paymentsRootBucket = []byte("payments-root-bucket")

migtest.ApplyMigration(
t,
func(tx *bbolt.Tx) error {
return migtest.RestoreDB(tx, paymentsRootBucket, pre)
},
func(tx *bbolt.Tx) error {
return migtest.VerifyDB(tx, paymentsRootBucket, post)
},
MigrateMPP,
false,
)
}