From 84f039db4550f2a79593392936887019ca81fa7e Mon Sep 17 00:00:00 2001 From: Abdullahi Yunus Date: Fri, 1 Nov 2024 12:24:04 +0100 Subject: [PATCH] lnd+chanbackup: add lnd config flag In this commit, we add the --no-backup-archive with a default as false. When set to true then previous channel backup file will not be archived but replaced. We also modify TestUpdateAndSwap test to make sure the new behaviour works as expected. --- chanbackup/backupfile.go | 25 ++++++--- chanbackup/backupfile_test.go | 102 ++++++++++++++++++++++++++++++---- config.go | 2 + sample-lnd.conf | 4 ++ server.go | 4 +- 5 files changed, 119 insertions(+), 18 deletions(-) diff --git a/chanbackup/backupfile.go b/chanbackup/backupfile.go index 9f9b7da5e..20ac1e13a 100644 --- a/chanbackup/backupfile.go +++ b/chanbackup/backupfile.go @@ -54,12 +54,15 @@ type MultiFile struct { // archiveDir is the directory where we'll store old channel backups. archiveDir string + + // noBackupArchive indicates whether old backups should be deleted + // rather than archived. + noBackupArchive bool } // NewMultiFile create a new multi-file instance at the target location on the // file system. -func NewMultiFile(fileName string) *MultiFile { - +func NewMultiFile(fileName string, noBackupArchive bool) *MultiFile { // We'll our temporary backup file in the very same directory as the // main backup file. backupFileDir := filepath.Dir(fileName) @@ -71,15 +74,17 @@ func NewMultiFile(fileName string) *MultiFile { ) return &MultiFile{ - fileName: fileName, - tempFileName: tempFileName, - archiveDir: archiveDir, + fileName: fileName, + tempFileName: tempFileName, + archiveDir: archiveDir, + noBackupArchive: noBackupArchive, } } // UpdateAndSwap will attempt write a new temporary backup file to disk with // the newBackup encoded, then atomically swap (via rename) the old file for -// the new file by updating the name of the new file to the old. +// the new file by updating the name of the new file to the old. It also checks +// if the old file should be archived first before swapping it. func (b *MultiFile) UpdateAndSwap(newBackup PackedMulti) error { // If the main backup file isn't set, then we can't proceed. if b.fileName == "" { @@ -172,7 +177,13 @@ func (b *MultiFile) ExtractMulti(keyChain keychain.KeyRing) (*Multi, error) { // specified archive directory, and copies the contents of the main backup file // to the new archive file. func (b *MultiFile) createArchiveFile() error { - // We check for old channel backup file first. + // User can skip archiving of old backup files to save disk space. + if b.noBackupArchive { + log.Debug("Skipping archive of old backup file as configured") + return nil + } + + // Check for old channel backup file. oldFileExists := lnrpc.FileExists(b.fileName) if !oldFileExists { log.Debug("No old channel backup file to archive") diff --git a/chanbackup/backupfile_test.go b/chanbackup/backupfile_test.go index 27804990a..31fcfc721 100644 --- a/chanbackup/backupfile_test.go +++ b/chanbackup/backupfile_test.go @@ -48,7 +48,9 @@ func assertFileDeleted(t *testing.T, filePath string) { // TestUpdateAndSwap test that we're able to properly swap out old backups on // disk with new ones. Additionally, after a swap operation succeeds, then each // time we should only have the main backup file on disk, as the temporary file -// has been removed. +// has been removed. Finally, we check for noBackupArchive to ensure that the +// archive file is created when it's set to false, and not created when it's +// set to true. func TestUpdateAndSwap(t *testing.T) { t.Parallel() @@ -58,7 +60,8 @@ func TestUpdateAndSwap(t *testing.T) { fileName string tempFileName string - oldTempExists bool + oldTempExists bool + noBackupArchive bool valid bool }{ @@ -92,9 +95,37 @@ func TestUpdateAndSwap(t *testing.T) { ), valid: true, }, + + // Test with noBackupArchive set to true - should not create + // archive. + { + fileName: filepath.Join( + tempTestDir, DefaultBackupFileName, + ), + tempFileName: filepath.Join( + tempTestDir, DefaultTempBackupFileName, + ), + noBackupArchive: true, + valid: true, + }, + + // Test with v set to false - should create + // archive. + { + fileName: filepath.Join( + tempTestDir, DefaultBackupFileName, + ), + tempFileName: filepath.Join( + tempTestDir, DefaultTempBackupFileName, + ), + noBackupArchive: false, + valid: true, + }, } for i, testCase := range testCases { - backupFile := NewMultiFile(testCase.fileName) + backupFile := NewMultiFile( + testCase.fileName, testCase.noBackupArchive, + ) // To start with, we'll make a random byte slice that'll pose // as our packed multi backup. @@ -160,6 +191,41 @@ func TestUpdateAndSwap(t *testing.T) { // Additionally, we shouldn't be able to find the temp backup // file on disk, as it should be deleted each time. assertFileDeleted(t, testCase.tempFileName) + + // Now check if archive was created when noBackupArchive is + // false. + archiveDir := filepath.Join( + filepath.Dir(testCase.fileName), + DefaultChanBackupArchiveDirName, + ) + if !testCase.noBackupArchive { + files, err := os.ReadDir(archiveDir) + require.NoError(t, err) + require.Len(t, files, 1) + + // Verify the archive contents match the previous + // backup. + archiveFile := filepath.Join( + archiveDir, files[0].Name(), + ) + // The archived content should match the previous + // backup (newPackedMulti) that was just swapped out. + assertBackupMatches(t, archiveFile, newPackedMulti) + + // Clean up the archive directory. + os.RemoveAll(archiveDir) + + continue + } + + // When noBackupArchive is true, no new archive file should be + // created. Note: In a real environment, the archive directory + // might exist with older backups before the feature is + // disabled, but for test simplicity (since we clean up the + // directory between test cases), we verify the directory + // doesn't exist at all. + require.NoDirExists(t, archiveDir) + } } @@ -238,7 +304,7 @@ func TestExtractMulti(t *testing.T) { } for i, testCase := range testCases { // First, we'll make our backup file with the specified name. - backupFile := NewMultiFile(testCase.fileName) + backupFile := NewMultiFile(testCase.fileName, false) // With our file made, we'll now attempt to read out the // multi-file. @@ -292,12 +358,18 @@ func TestCreateArchiveFile(t *testing.T) { require.NoError(t, err) tests := []struct { - name string - setup func() - wantError bool + name string + setup func() + noBackupArchive bool + wantError bool }{ { - name: "successful archive", + name: "successful archive", + noBackupArchive: false, + }, + { + name: "skip archive when disabled", + noBackupArchive: true, }, { name: "invalid archive directory permissions", @@ -306,7 +378,8 @@ func TestCreateArchiveFile(t *testing.T) { err := os.MkdirAll(archiveDir, 0500) require.NoError(t, err) }, - wantError: true, + noBackupArchive: false, + wantError: true, }, } @@ -318,7 +391,9 @@ func TestCreateArchiveFile(t *testing.T) { tc.setup() } - multiFile := NewMultiFile(backupFile) + multiFile := NewMultiFile( + backupFile, tc.noBackupArchive, + ) err := multiFile.createArchiveFile() if tc.wantError { @@ -328,6 +403,13 @@ func TestCreateArchiveFile(t *testing.T) { require.NoError(t, err) + // If archiving is disabled, verify no archive was + // created. + if tc.noBackupArchive { + require.NoDirExists(t, archiveDir) + return + } + // Verify archive exists and content matches. files, err := os.ReadDir(archiveDir) require.NoError(t, err) diff --git a/config.go b/config.go index 6d6e93d7d..8c3718e95 100644 --- a/config.go +++ b/config.go @@ -360,6 +360,8 @@ type Config struct { MaxPendingChannels int `long:"maxpendingchannels" description:"The maximum number of incoming pending channels permitted per peer."` BackupFilePath string `long:"backupfilepath" description:"The target location of the channel backup file"` + NoBackupArchive bool `long:"no-backup-archive" description:"If set to true, channel backups will be deleted or replaced rather than being archived to a separate location."` + FeeURL string `long:"feeurl" description:"DEPRECATED: Use 'fee.url' option. Optional URL for external fee estimation. If no URL is specified, the method for fee estimation will depend on the chosen backend and network. Must be set for neutrino on mainnet." hidden:"true"` Bitcoin *lncfg.Chain `group:"Bitcoin" namespace:"bitcoin"` diff --git a/sample-lnd.conf b/sample-lnd.conf index 07ccef2fd..b447e6b6f 100644 --- a/sample-lnd.conf +++ b/sample-lnd.conf @@ -309,6 +309,10 @@ ; Example: ; backupfilepath=~/.lnd/data/chain/bitcoin/mainnet/channel.backup +; When false (default), old channel backups are archived to a designated location. +; When true, old backups are simply replaced. +; no-backup-archive=false + ; The maximum capacity of the block cache in bytes. Increasing this will result ; in more blocks being kept in memory but will increase performance when the ; same block is required multiple times. diff --git a/server.go b/server.go index e79b25ac0..d71ebc032 100644 --- a/server.go +++ b/server.go @@ -1648,7 +1648,9 @@ func newServer(cfg *Config, listenAddrs []net.Addr, chanNotifier: s.channelNotifier, addrs: s.addrSource, } - backupFile := chanbackup.NewMultiFile(cfg.BackupFilePath) + backupFile := chanbackup.NewMultiFile( + cfg.BackupFilePath, cfg.NoBackupArchive, + ) startingChans, err := chanbackup.FetchStaticChanBackups( s.chanStateDB, s.addrSource, )