From 7cf28773bf52a815d6b4411b3645f384428a6bb2 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Mon, 23 May 2022 03:55:00 +0800 Subject: [PATCH 1/3] multi: export channel status field in migration25 Previously, in `migration25.OpenChannel`, there was a private field `chanStatus` used to keep track of the channel status. The following migrations, `migration26` and `migration27` also have their own `OpenChannel` defined, with `migration26` inherited from `migration25`, and `migration27` inherited from `migration26`. The private field `chanStatus`, however, is NOT inherited and each of the migrations uses its own. This is fine for reading and writing as, under the hood, the `chanStatus` is just a `uint8` value. Because each migration has its own fetcher and putter, it can safely access its private field to read and write it correctly. The issue pops up when we use the method `migration25.FundingTxPresent()`. Because it's evaluating its channel status using its own private field `chanStatus`, this field would always be the default value(`ChanStatusDefault`), leading the statement `!c.hasChanStatus(ChanStatusRestored)` to always be true. Thus a restored channel will be mistakenly considered to have funding tx present, causing failures in reading the channel info in the following migrations. We fix this by exporting the `ChanStatus` field so its value can be set by following migrations. --- channeldb/migration25/channel.go | 16 ++++++++++------ channeldb/migration26/channel.go | 8 ++------ channeldb/migration27/channel.go | 8 ++------ 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/channeldb/migration25/channel.go b/channeldb/migration25/channel.go index f33e87955..f825e83cb 100644 --- a/channeldb/migration25/channel.go +++ b/channeldb/migration25/channel.go @@ -277,9 +277,13 @@ type OpenChannel struct { // ChanType denotes which type of channel this is. ChanType ChannelType - // chanStatus is the current status of this channel. If it is not in + // ChanStatus is the current status of this channel. If it is not in // the state Default, it should not be used for forwarding payments. - chanStatus ChannelStatus + // + // NOTE: In `channeldb.OpenChannel`, this field is private. We choose + // to export this private field such that following migrations can + // access this field directly. + ChanStatus ChannelStatus // InitialLocalBalance is the balance we have during the channel // opening. When we are not the initiator, this value represents the @@ -320,10 +324,10 @@ func (c *OpenChannel) hasChanStatus(status ChannelStatus) bool { // Special case ChanStatusDefualt since it isn't actually flag, but a // particular combination (or lack-there-of) of flags. if status == ChanStatusDefault { - return c.chanStatus == ChanStatusDefault + return c.ChanStatus == ChanStatusDefault } - return c.chanStatus&status == status + return c.ChanStatus&status == status } // FundingTxPresent returns true if expect the funding transcation to be found @@ -361,7 +365,7 @@ func fetchChanInfo(chanBucket kvdb.RBucket, c *OpenChannel, legacy bool) error { } c.ChanType = ChannelType(chanType) - c.chanStatus = ChannelStatus(chanStatus) + c.ChanStatus = ChannelStatus(chanStatus) // If this is not the legacy format, we need to read the extra two new // fields. @@ -432,7 +436,7 @@ func putChanInfo(chanBucket kvdb.RwBucket, c *OpenChannel, legacy bool) error { if err := mig.WriteElements(&w, mig.ChannelType(c.ChanType), c.ChainHash, c.FundingOutpoint, c.ShortChannelID, c.IsPending, c.IsInitiator, - mig.ChannelStatus(c.chanStatus), c.FundingBroadcastHeight, + mig.ChannelStatus(c.ChanStatus), c.FundingBroadcastHeight, c.NumConfsRequired, c.ChannelFlags, c.IdentityPub, c.Capacity, c.TotalMSatSent, c.TotalMSatReceived, diff --git a/channeldb/migration26/channel.go b/channeldb/migration26/channel.go index f569ec3a1..cff193274 100644 --- a/channeldb/migration26/channel.go +++ b/channeldb/migration26/channel.go @@ -70,10 +70,6 @@ var ( // NOTE: doesn't have the Packager field as it's not used in current migration. type OpenChannel struct { mig25.OpenChannel - - // chanStatus is the current status of this channel. If it is not in - // the state Default, it should not be used for forwarding payments. - chanStatus mig25.ChannelStatus } // FetchChanInfo deserializes the channel info based on the legacy boolean. @@ -104,7 +100,7 @@ func FetchChanInfo(chanBucket kvdb.RBucket, c *OpenChannel, legacy bool) error { } c.ChanType = mig25.ChannelType(chanType) - c.chanStatus = mig25.ChannelStatus(chanStatus) + c.ChanStatus = mig25.ChannelStatus(chanStatus) // If this is the legacy format, we need to read the extra two new // fields. @@ -239,7 +235,7 @@ func PutChanInfo(chanBucket kvdb.RwBucket, c *OpenChannel, legacy bool) error { if err := mig.WriteElements(&w, mig.ChannelType(c.ChanType), c.ChainHash, c.FundingOutpoint, c.ShortChannelID, c.IsPending, c.IsInitiator, - mig.ChannelStatus(c.chanStatus), c.FundingBroadcastHeight, + mig.ChannelStatus(c.ChanStatus), c.FundingBroadcastHeight, c.NumConfsRequired, c.ChannelFlags, c.IdentityPub, c.Capacity, c.TotalMSatSent, c.TotalMSatReceived, diff --git a/channeldb/migration27/channel.go b/channeldb/migration27/channel.go index 95a6505cd..7fd87a0b4 100644 --- a/channeldb/migration27/channel.go +++ b/channeldb/migration27/channel.go @@ -59,10 +59,6 @@ var ( // NOTE: doesn't have the Packager field as it's not used in current migration. type OpenChannel struct { mig26.OpenChannel - - // chanStatus is the current status of this channel. If it is not in - // the state Default, it should not be used for forwarding payments. - chanStatus mig25.ChannelStatus } // FetchChanInfo deserializes the channel info based on the legacy boolean. @@ -90,7 +86,7 @@ func FetchChanInfo(chanBucket kvdb.RBucket, c *OpenChannel, legacy bool) error { } c.ChanType = mig25.ChannelType(chanType) - c.chanStatus = mig25.ChannelStatus(chanStatus) + c.ChanStatus = mig25.ChannelStatus(chanStatus) // For single funder channels that we initiated and have the funding // transaction to, read the funding txn. @@ -182,7 +178,7 @@ func PutChanInfo(chanBucket kvdb.RwBucket, c *OpenChannel, legacy bool) error { if err := mig.WriteElements(&w, mig.ChannelType(c.ChanType), c.ChainHash, c.FundingOutpoint, c.ShortChannelID, c.IsPending, c.IsInitiator, - mig.ChannelStatus(c.chanStatus), c.FundingBroadcastHeight, + mig.ChannelStatus(c.ChanStatus), c.FundingBroadcastHeight, c.NumConfsRequired, c.ChannelFlags, c.IdentityPub, c.Capacity, c.TotalMSatSent, c.TotalMSatReceived, From d7bb7cd9a7c193097abc24b907cf68b48f4b4e84 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Mon, 23 May 2022 05:13:56 +0800 Subject: [PATCH 2/3] migration27: add unit test to cover ChanStatusRestored --- channeldb/migration27/migration_test.go | 114 +++++++++++++++++------- 1 file changed, 82 insertions(+), 32 deletions(-) diff --git a/channeldb/migration27/migration_test.go b/channeldb/migration27/migration_test.go index 2f5461f63..98946ac55 100644 --- a/channeldb/migration27/migration_test.go +++ b/channeldb/migration27/migration_test.go @@ -54,42 +54,80 @@ var ( }, } - // testChannel is used to test the balance fields are correctly set. - testChannel = &OpenChannel{ - OpenChannel: mig26.OpenChannel{ - OpenChannel: mig25.OpenChannel{ - OpenChannel: mig.OpenChannel{ - IdentityPub: dummyPubKey, - FundingOutpoint: dummyOp, - FundingTxn: commitTx1, - IsInitiator: true, - }, - }, - }, - } + // testChanStatus specifies the following channel status, + // ChanStatusLocalDataLoss|ChanStatusRestored|ChanStatusRemoteCloseInitiator + testChanStatus = mig25.ChannelStatus(0x4c) ) // TestMigrateHistoricalBalances checks that the initial balances fields are // patched to the historical channel info. func TestMigrateHistoricalBalances(t *testing.T) { - // Test that when the historical channel doesn't have the two new - // fields. - migtest.ApplyMigration( - t, - genBeforeMigration(testChannel, false), - genAfterMigration(testChannel), - MigrateHistoricalBalances, - false, - ) + testCases := []struct { + name string + isAfterMigration25 bool + isRestored bool + }{ + { + // Test that when the restored historical channel + // doesn't have the two new fields. + name: "restored before migration25", + isAfterMigration25: false, + isRestored: true, + }, + { + // Test that when the restored historical channel have + // the two new fields. + name: "restored after migration25", + isAfterMigration25: true, + isRestored: true, + }, + { + // Test that when the historical channel with a default + // channel status flag doesn't have the two new fields. + name: "default before migration25", + isAfterMigration25: false, + isRestored: false, + }, + { + // Test that when the historical channel with a default + // channel status flag have the two new fields. + name: "default after migration25", + isAfterMigration25: true, + isRestored: false, + }, + } - // Test that when the historical channel have the two new fields. - migtest.ApplyMigration( - t, - genBeforeMigration(testChannel, true), - genAfterMigration(testChannel), - MigrateHistoricalBalances, - false, - ) + for _, tc := range testCases { + tc := tc + + // testChannel is used to test the balance fields are correctly + // set. + testChannel := &OpenChannel{} + testChannel.IdentityPub = dummyPubKey + testChannel.FundingOutpoint = dummyOp + testChannel.FundingTxn = commitTx1 + testChannel.IsInitiator = true + + // Set the channel status flag is we are testing the restored + // case. + if tc.isRestored { + testChannel.ChanStatus = testChanStatus + } + + // Create before and after migration functions. + beforeFn := genBeforeMigration( + testChannel, tc.isAfterMigration25, + ) + afterFn := genAfterMigration(testChannel, tc.isRestored) + + // Run the test. + t.Run(tc.name, func(t *testing.T) { + migtest.ApplyMigration( + t, beforeFn, afterFn, + MigrateHistoricalBalances, false, + ) + }) + } } func genBeforeMigration(c *OpenChannel, regression bool) func(kvdb.RwTx) error { @@ -116,7 +154,7 @@ func genBeforeMigration(c *OpenChannel, regression bool) func(kvdb.RwTx) error { } } -func genAfterMigration(c *OpenChannel) func(kvdb.RwTx) error { +func genAfterMigration(c *OpenChannel, restored bool) func(kvdb.RwTx) error { return func(tx kvdb.RwTx) error { chanBucket, err := fetchHistoricalChanBucket(tx, c) if err != nil { @@ -159,6 +197,16 @@ func genAfterMigration(c *OpenChannel) func(kvdb.RwTx) error { if !newChan.IsInitiator { return fmt.Errorf("wrong IsInitiator") } + + // If it's restored, there should be no funding tx. + if restored { + if newChan.FundingTxn != nil { + return fmt.Errorf("expect nil FundingTxn") + } + return nil + } + + // Otherwise check the funding tx is read as expected. if newChan.FundingTxn.TxHash() != commitTx1.TxHash() { return fmt.Errorf("wrong FundingTxn") } @@ -167,7 +215,9 @@ func genAfterMigration(c *OpenChannel) func(kvdb.RwTx) error { } } -func createHistoricalBucket(tx kvdb.RwTx, c *OpenChannel) (kvdb.RwBucket, error) { +func createHistoricalBucket(tx kvdb.RwTx, + c *OpenChannel) (kvdb.RwBucket, error) { + // First fetch the top level bucket which stores all data related to // historical channels. rootBucket, err := tx.CreateTopLevelBucket(historicalChannelBucket) From 36b646496a94bfb26dea5de9b318de16e46d6e89 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Mon, 23 May 2022 05:17:03 +0800 Subject: [PATCH 3/3] docs: update release note for migrations fix --- docs/release-notes/release-notes-0.15.0.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/release-notes/release-notes-0.15.0.md b/docs/release-notes/release-notes-0.15.0.md index b4fac82fe..3deeb7fe7 100644 --- a/docs/release-notes/release-notes-0.15.0.md +++ b/docs/release-notes/release-notes-0.15.0.md @@ -172,6 +172,9 @@ from occurring that would result in an erroneous force close.](https://github.co * [Fixed an intermittent panic that would occur due to a violated assumption with our underlying database.](https://github.com/lightningnetwork/lnd/pull/6547) +* [Fixed a wrong channel status inheritance used in `migration26` and + `migration27`](https://github.com/lightningnetwork/lnd/pull/6563). + ## Routing * [Add a new `time_pref` parameter to the QueryRoutes and SendPayment APIs](https://github.com/lightningnetwork/lnd/pull/6024) that