Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
21 changes: 11 additions & 10 deletions src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ use lightning::util::persist::{
use lightning::util::ser::ReadableArgs;
use lightning::util::sweep::OutputSweeper;
use lightning_dns_resolver::OMDomainResolver;
use lightning_persister::fs_store::v1::FilesystemStore;
use vss_client::headers::VssHeaderProvider;

use crate::chain::ChainSource;
Expand All @@ -59,8 +58,9 @@ use crate::fee_estimator::OnchainFeeEstimator;
use crate::gossip::GossipSource;
use crate::io::sqlite_store::SqliteStore;
use crate::io::utils::{
read_all_objects, read_event_queue, read_external_pathfinding_scores_from_cache,
read_network_graph, read_node_metrics, read_output_sweeper, read_peer_info, read_scorer,
open_or_migrate_fs_store, read_all_objects, read_event_queue,
read_external_pathfinding_scores_from_cache, read_network_graph, read_node_metrics,
read_output_sweeper, read_peer_info, read_scorer,
};
use crate::io::vss_store::VssStoreBuilder;
use crate::io::{
Expand Down Expand Up @@ -644,18 +644,19 @@ impl NodeBuilder {
self.build_with_store_and_logger(node_entropy, kv_store, logger)
}

/// Builds a [`Node`] instance with a [`FilesystemStore`] backend and according to the options
/// Builds a [`Node`] instance with a [`FilesystemStoreV2`] backend and according to the options
/// previously configured.
///
/// If the storage directory contains data from a v1 filesystem store, it will be
/// automatically migrated to the v2 format.
///
/// [`FilesystemStoreV2`]: lightning_persister::fs_store::v2::FilesystemStoreV2
pub fn build_with_fs_store(&self, node_entropy: NodeEntropy) -> Result<Node, BuildError> {
let logger = setup_logger(&self.log_writer_config, &self.config)?;
let mut storage_dir_path: PathBuf = self.config.storage_dir_path.clone().into();
storage_dir_path.push("fs_store");

fs::create_dir_all(storage_dir_path.clone()).map_err(|e| {
log_error!(logger, "Failed to setup Filesystem store: {}", e);
BuildError::StoragePathAccessFailed
})?;
let kv_store = FilesystemStore::new(storage_dir_path);
let kv_store = open_or_migrate_fs_store(storage_dir_path)?;
self.build_with_store_and_logger(node_entropy, kv_store, logger)
}

Expand Down Expand Up @@ -1115,7 +1116,7 @@ impl ArcedNodeBuilder {
self.inner.read().expect("lock").build(*node_entropy).map(Arc::new)
}

/// Builds a [`Node`] instance with a [`FilesystemStore`] backend and according to the options
/// Builds a [`Node`] instance with a [`FilesystemStoreV2`] backend and according to the options
/// previously configured.
pub fn build_with_fs_store(
&self, node_entropy: Arc<NodeEntropy>,
Expand Down
267 changes: 258 additions & 9 deletions src/io/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use std::io::Write;
use std::ops::Deref;
#[cfg(unix)]
use std::os::unix::fs::OpenOptionsExt;
use std::path::Path;
use std::path::{Path, PathBuf};
use std::sync::{Arc, RwLock};

use bdk_chain::indexer::keychain_txout::ChangeSet as BdkIndexerChangeSet;
Expand All @@ -26,14 +26,16 @@ use lightning::routing::scoring::{
ChannelLiquidities, ProbabilisticScorer, ProbabilisticScoringDecayParameters,
};
use lightning::util::persist::{
KVStore, KVStoreSync, KVSTORE_NAMESPACE_KEY_ALPHABET, KVSTORE_NAMESPACE_KEY_MAX_LEN,
NETWORK_GRAPH_PERSISTENCE_KEY, NETWORK_GRAPH_PERSISTENCE_PRIMARY_NAMESPACE,
NETWORK_GRAPH_PERSISTENCE_SECONDARY_NAMESPACE, OUTPUT_SWEEPER_PERSISTENCE_KEY,
OUTPUT_SWEEPER_PERSISTENCE_PRIMARY_NAMESPACE, OUTPUT_SWEEPER_PERSISTENCE_SECONDARY_NAMESPACE,
SCORER_PERSISTENCE_KEY, SCORER_PERSISTENCE_PRIMARY_NAMESPACE,
SCORER_PERSISTENCE_SECONDARY_NAMESPACE,
migrate_kv_store_data, KVStore, KVStoreSync, KVSTORE_NAMESPACE_KEY_ALPHABET,
KVSTORE_NAMESPACE_KEY_MAX_LEN, NETWORK_GRAPH_PERSISTENCE_KEY,
NETWORK_GRAPH_PERSISTENCE_PRIMARY_NAMESPACE, NETWORK_GRAPH_PERSISTENCE_SECONDARY_NAMESPACE,
OUTPUT_SWEEPER_PERSISTENCE_KEY, OUTPUT_SWEEPER_PERSISTENCE_PRIMARY_NAMESPACE,
OUTPUT_SWEEPER_PERSISTENCE_SECONDARY_NAMESPACE, SCORER_PERSISTENCE_KEY,
SCORER_PERSISTENCE_PRIMARY_NAMESPACE, SCORER_PERSISTENCE_SECONDARY_NAMESPACE,
};
use lightning::util::ser::{Readable, ReadableArgs, Writeable};
use lightning_persister::fs_store::v1::FilesystemStore;
use lightning_persister::fs_store::v2::{FilesystemStoreV2, FilesystemStoreV2Error};
use lightning_types::string::PrintableString;

use super::*;
Expand All @@ -47,7 +49,7 @@ use crate::logger::{log_error, LdkLogger, Logger};
use crate::peer_store::PeerStore;
use crate::types::{Broadcaster, DynStore, KeysManager, Sweeper};
use crate::wallet::ser::{ChangeSetDeserWrapper, ChangeSetSerWrapper};
use crate::{Error, EventQueue, NodeMetrics};
use crate::{BuildError, Error, EventQueue, NodeMetrics};

pub const EXTERNAL_PATHFINDING_SCORES_CACHE_KEY: &str = "external_pathfinding_scores_cache";

Expand Down Expand Up @@ -619,10 +621,103 @@ pub(crate) fn read_bdk_wallet_change_set(
Ok(Some(change_set))
}

/// Opens a [`FilesystemStoreV2`], automatically migrating from v1 format if necessary.
///
/// If the directory contains v1 data (files at the top level), the data is migrated to v2 format
/// in a temporary directory, the original is renamed to `fs_store_v1_backup`, and the migrated
/// directory is moved into place.
pub(crate) fn open_or_migrate_fs_store(
storage_dir_path: PathBuf,
) -> Result<FilesystemStoreV2, BuildError> {
let parent_dir = storage_dir_path.parent().ok_or(BuildError::StoragePathAccessFailed)?;
fs::create_dir_all(parent_dir).map_err(|_| BuildError::StoragePathAccessFailed)?;
recover_incomplete_fs_store_migration(&storage_dir_path)?;
if !storage_dir_path.exists() {
fs::create_dir_all(storage_dir_path.clone())
.map_err(|_| BuildError::StoragePathAccessFailed)?;
}

match FilesystemStoreV2::new(storage_dir_path.clone()) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  • [P2] Detect v1 empty-secondary namespace data — /home/tnull/workspace/ldk-node/src/io/utils.rs:640-641
    When an old fs store only has v1 entries under a non-empty primary namespace with an empty secondary namespace, such as fs_store/bdk_wallet/descriptor before any top-level LDK files have been persisted, FilesystemStoreV2::new succeeds because it only rejects top-level files. Startup then
    skips migration and V2 looks under bdk_wallet/[empty]/descriptor, silently ignoring the existing wallet state and potentially missing historical funds; please detect these direct namespace files and migrate them too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

migrate_kv_store_data does this already

Ok(store) => Ok(store),
Err(FilesystemStoreV2Error::V1DataDetected(_)) => {
// The directory contains v1 data, migrate to v2.
let mut v1_store = FilesystemStore::new(storage_dir_path.clone());

let v2_dir = fs_store_sibling_path(&storage_dir_path, "fs_store_v2_migrating");
fs::create_dir_all(v2_dir.clone()).map_err(|_| BuildError::StoragePathAccessFailed)?;
let mut v2_store = FilesystemStoreV2::new(v2_dir.clone())
.map_err(|_| BuildError::KVStoreSetupFailed)?;

migrate_kv_store_data(&mut v1_store, &mut v2_store)
Comment thread
tnull marked this conversation as resolved.
.map_err(|_| BuildError::KVStoreSetupFailed)?;

// Swap directories: rename v1 out of the way, move v2 into place.
let backup_dir = fs_store_sibling_path(&storage_dir_path, "fs_store_v1_backup");
fs::rename(&storage_dir_path, &backup_dir)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Codex:

  • P1 /tmp/ldk-node-pr872/src/io/utils.rs:650, /tmp/ldk-node-pr872/src/builder.rs:659: the v1 to v2 migration is not crash-recoverable. After fs_store is renamed to fs_store_v1_backup, a crash or error before fs_store_v2_migrating is renamed into place leaves no fs_store. On the next startup,
    build_with_fs_store recreates an empty fs_store, FilesystemStoreV2::new succeeds, and the node can start against empty storage while the real data sits in backup/temp dirs. The migration helper should detect and recover incomplete states before creating a fresh store.

Seems we still might want to double-check we survive an ill-timed crash?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added migration defense and tests for all scenarios

.map_err(|_| BuildError::KVStoreSetupFailed)?;
fs::rename(&v2_dir, &storage_dir_path).map_err(|_| BuildError::KVStoreSetupFailed)?;

FilesystemStoreV2::new(storage_dir_path).map_err(|_| BuildError::KVStoreSetupFailed)
},
Err(_) => Err(BuildError::KVStoreSetupFailed),
}
}

fn fs_store_sibling_path(storage_dir_path: &Path, file_name: &str) -> PathBuf {
let mut sibling_path = storage_dir_path.to_path_buf();
sibling_path.set_file_name(file_name);
sibling_path
}

fn recover_incomplete_fs_store_migration(storage_dir_path: &Path) -> Result<(), BuildError> {
let v2_dir = fs_store_sibling_path(storage_dir_path, "fs_store_v2_migrating");
let backup_dir = fs_store_sibling_path(storage_dir_path, "fs_store_v1_backup");

if storage_dir_path.exists() {
if v2_dir.exists() {
// The original store is still in place, so a temp migration dir is from a crash before
// the rename step and can be discarded before retrying migration.
fs::remove_dir_all(&v2_dir).map_err(|_| BuildError::KVStoreSetupFailed)?;
}
return Ok(());
}

if backup_dir.exists() {
if v2_dir.exists() {
// Prefer retrying from the v1 backup instead of deciding here whether the temp v2 dir is
// usable. open_or_migrate_fs_store owns the actual v1-to-v2 migration.
fs::remove_dir_all(&v2_dir).map_err(|_| BuildError::KVStoreSetupFailed)?;
}
// The crash happened after moving v1 aside; restore it so normal startup can migrate it.
fs::rename(&backup_dir, storage_dir_path).map_err(|_| BuildError::KVStoreSetupFailed)?;
return Ok(());
}

if v2_dir.exists() {
// There is no v1 backup to retry from. Move the temp dir into place and let
// open_or_migrate_fs_store decide whether it is a valid v2 store.
fs::rename(&v2_dir, storage_dir_path).map_err(|_| BuildError::KVStoreSetupFailed)?;
}

Ok(())
}

#[cfg(test)]
mod tests {
use super::read_or_generate_seed_file;
use std::fs;
use std::path::{Path, PathBuf};

use lightning::util::persist::{migrate_kv_store_data, KVStoreSync};
use lightning_persister::fs_store::v1::FilesystemStore;
use lightning_persister::fs_store::v2::FilesystemStoreV2;

use super::test_utils::random_storage_path;
use super::{open_or_migrate_fs_store, read_or_generate_seed_file};

const TEST_PRIMARY_NAMESPACE: &str = "test_primary_namespace";
const TEST_SECONDARY_NAMESPACE: &str = "test_secondary_namespace";
const TEST_KEY: &str = "test_key";
const TEST_VALUE: &[u8] = b"test_value";

#[test]
fn generated_seed_is_readable() {
Expand All @@ -632,4 +727,158 @@ mod tests {
let read_seed_bytes = read_or_generate_seed_file(&rand_path.to_str().unwrap()).unwrap();
assert_eq!(expected_seed_bytes, read_seed_bytes);
}

#[test]
fn fs_store_migration_recovers_before_v1_backup_rename() {
let fs_store_path = fs_store_path();
let mut v1_store = write_v1_test_data(&fs_store_path);
let v2_migrating_path = sibling_path(&fs_store_path, "fs_store_v2_migrating");
let mut v2_store = FilesystemStoreV2::new(v2_migrating_path.clone()).unwrap();
migrate_kv_store_data(&mut v1_store, &mut v2_store).unwrap();

let migrated_store = open_or_migrate_fs_store(fs_store_path.clone()).unwrap();
assert_eq!(
KVStoreSync::read(
&migrated_store,
TEST_PRIMARY_NAMESPACE,
TEST_SECONDARY_NAMESPACE,
TEST_KEY
)
.unwrap(),
TEST_VALUE
);
assert!(fs_store_path.exists());
assert!(!v2_migrating_path.exists());
}

#[test]
fn fs_store_migration_recovers_after_v1_backup_rename() {
let fs_store_path = fs_store_path();
let mut v1_store = write_v1_test_data(&fs_store_path);
let v2_migrating_path = sibling_path(&fs_store_path, "fs_store_v2_migrating");
let mut v2_store = FilesystemStoreV2::new(v2_migrating_path.clone()).unwrap();
migrate_kv_store_data(&mut v1_store, &mut v2_store).unwrap();

let backup_path = sibling_path(&fs_store_path, "fs_store_v1_backup");
fs::rename(&fs_store_path, backup_path).unwrap();

let migrated_store = open_or_migrate_fs_store(fs_store_path.clone()).unwrap();
assert_eq!(
KVStoreSync::read(
&migrated_store,
TEST_PRIMARY_NAMESPACE,
TEST_SECONDARY_NAMESPACE,
TEST_KEY
)
.unwrap(),
TEST_VALUE
);
assert!(fs_store_path.exists());
assert!(!v2_migrating_path.exists());
}

#[test]
fn fs_store_migration_recovers_after_v2_rename() {
let fs_store_path = fs_store_path();
let mut v1_store = write_v1_test_data(&fs_store_path);
let v2_migrating_path = sibling_path(&fs_store_path, "fs_store_v2_migrating");
let mut v2_store = FilesystemStoreV2::new(v2_migrating_path.clone()).unwrap();
migrate_kv_store_data(&mut v1_store, &mut v2_store).unwrap();

let backup_path = sibling_path(&fs_store_path, "fs_store_v1_backup");
fs::rename(&fs_store_path, &backup_path).unwrap();
fs::rename(&v2_migrating_path, &fs_store_path).unwrap();

let migrated_store = open_or_migrate_fs_store(fs_store_path.clone()).unwrap();
assert_eq!(
KVStoreSync::read(
&migrated_store,
TEST_PRIMARY_NAMESPACE,
TEST_SECONDARY_NAMESPACE,
TEST_KEY
)
.unwrap(),
TEST_VALUE
);
assert!(fs_store_path.exists());
assert!(backup_path.exists());
assert!(!v2_migrating_path.exists());
}

#[test]
fn fs_store_migration_recovers_backup_without_migrating_dir() {
let fs_store_path = fs_store_path();
write_v1_test_data(&fs_store_path);

let backup_path = sibling_path(&fs_store_path, "fs_store_v1_backup");
fs::rename(&fs_store_path, backup_path).unwrap();

let migrated_store = open_or_migrate_fs_store(fs_store_path.clone()).unwrap();
assert_eq!(
KVStoreSync::read(
&migrated_store,
TEST_PRIMARY_NAMESPACE,
TEST_SECONDARY_NAMESPACE,
TEST_KEY
)
.unwrap(),
TEST_VALUE
);
assert!(fs_store_path.exists());
assert!(!sibling_path(&fs_store_path, "fs_store_v1_backup").exists());
}

#[test]
fn fs_store_migration_recovers_unexpected_migrating_dir_without_backup() {
let fs_store_path = fs_store_path();
let v2_migrating_path = sibling_path(&fs_store_path, "fs_store_v2_migrating");
let v2_store = FilesystemStoreV2::new(v2_migrating_path.clone()).unwrap();
KVStoreSync::write(
&v2_store,
TEST_PRIMARY_NAMESPACE,
TEST_SECONDARY_NAMESPACE,
TEST_KEY,
TEST_VALUE.to_vec(),
)
.unwrap();

let migrated_store = open_or_migrate_fs_store(fs_store_path.clone()).unwrap();
assert_eq!(
KVStoreSync::read(
&migrated_store,
TEST_PRIMARY_NAMESPACE,
TEST_SECONDARY_NAMESPACE,
TEST_KEY
)
.unwrap(),
TEST_VALUE
);
assert!(fs_store_path.exists());
assert!(!v2_migrating_path.exists());
}

fn fs_store_path() -> PathBuf {
let mut fs_store_path = random_storage_path();
fs_store_path.push("fs_store");
fs_store_path
}

fn sibling_path(path: &Path, file_name: &str) -> PathBuf {
let mut sibling_path = path.to_path_buf();
sibling_path.set_file_name(file_name);
sibling_path
}

fn write_v1_test_data(fs_store_path: &Path) -> FilesystemStore {
let v1_store = FilesystemStore::new(fs_store_path.to_path_buf());
KVStoreSync::write(
&v1_store,
TEST_PRIMARY_NAMESPACE,
TEST_SECONDARY_NAMESPACE,
TEST_KEY,
TEST_VALUE.to_vec(),
)
.unwrap();
v1_store
}
}
4 changes: 4 additions & 0 deletions tests/common/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,7 @@ pub(crate) enum TestChainSource<'a> {
pub(crate) enum TestStoreType {
TestSyncStore,
Sqlite,
FilesystemStore,
}

impl Default for TestStoreType {
Expand Down Expand Up @@ -592,6 +593,9 @@ pub(crate) fn setup_node(chain_source: &TestChainSource, config: TestConfig) ->
builder.build_with_store(config.node_entropy.into(), kv_store).unwrap()
},
TestStoreType::Sqlite => builder.build(config.node_entropy.into()).unwrap(),
TestStoreType::FilesystemStore => {
builder.build_with_fs_store(config.node_entropy.into()).unwrap()
},
};

if config.recovery_mode {
Expand Down
Loading
Loading