Skip to content

Commit 4f91c8c

Browse files
ovitrifclaude
andcommitted
fix: guard channel manager migration against overwriting newer state
Add existence-check guard to channel manager migration (same fail-closed pattern as monitors). If the KV store already has a channel manager, the migration is skipped to prevent rolling back HTLC states, commitment indices, and pending payments with stale FS-sourced data. Also adds AI rule requiring unit tests for new business logic, and 3 unit tests covering the channel manager guard (fresh write, skip when existing, fail on read error). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent cde6037 commit 4f91c8c

2 files changed

Lines changed: 128 additions & 16 deletions

File tree

AGENTS.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,7 @@ marked as skipped for CI), you must fix it before declaring success.
236236
- NEVER suggest manually adding @Serializable annotations to generated Kotlin bindings
237237
- ALWAYS run `cargo fmt` before committing to ensure consistent code formatting
238238
- ALWAYS move imports to the top of the file when applicable (no inline imports in functions)
239+
- ALWAYS add unit tests for new business logic — untested code will be flagged in review
239240
- Run `./bindgen.sh` in the background when bindings need regeneration (it is long-running)
240241

241242
## Bindings Generation Command

src/builder.rs

Lines changed: 127 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1347,21 +1347,48 @@ where
13471347
K: EntropySource + SignerProvider<EcdsaSigner = InMemorySigner>,
13481348
{
13491349
if let Some(manager_bytes) = &migration.channel_manager {
1350-
runtime
1351-
.block_on(async {
1352-
KVStore::write(
1353-
&**kv_store,
1354-
CHANNEL_MANAGER_PERSISTENCE_PRIMARY_NAMESPACE,
1355-
CHANNEL_MANAGER_PERSISTENCE_SECONDARY_NAMESPACE,
1356-
CHANNEL_MANAGER_PERSISTENCE_KEY,
1357-
manager_bytes.clone(),
1358-
)
1359-
.await
1360-
})
1361-
.map_err(|e| {
1362-
log_error!(logger, "Failed to write migrated channel_manager: {}", e);
1363-
BuildError::WriteFailed
1364-
})?;
1350+
let should_write = match runtime.block_on(KVStore::read(
1351+
&**kv_store,
1352+
CHANNEL_MANAGER_PERSISTENCE_PRIMARY_NAMESPACE,
1353+
CHANNEL_MANAGER_PERSISTENCE_SECONDARY_NAMESPACE,
1354+
CHANNEL_MANAGER_PERSISTENCE_KEY,
1355+
)) {
1356+
Ok(_existing_data) => {
1357+
log_warn!(
1358+
logger,
1359+
"Skipping channel manager migration: store already contains a channel manager"
1360+
);
1361+
false
1362+
},
1363+
Err(e) if e.kind() == lightning::io::ErrorKind::NotFound => true,
1364+
Err(e) => {
1365+
log_error!(
1366+
logger,
1367+
"Failed to read existing channel manager, refusing migration write \
1368+
to avoid overwriting potentially newer state: {}",
1369+
e
1370+
);
1371+
return Err(BuildError::ReadFailed);
1372+
},
1373+
};
1374+
1375+
if should_write {
1376+
runtime
1377+
.block_on(async {
1378+
KVStore::write(
1379+
&**kv_store,
1380+
CHANNEL_MANAGER_PERSISTENCE_PRIMARY_NAMESPACE,
1381+
CHANNEL_MANAGER_PERSISTENCE_SECONDARY_NAMESPACE,
1382+
CHANNEL_MANAGER_PERSISTENCE_KEY,
1383+
manager_bytes.clone(),
1384+
)
1385+
.await
1386+
})
1387+
.map_err(|e| {
1388+
log_error!(logger, "Failed to write migrated channel_manager: {}", e);
1389+
BuildError::WriteFailed
1390+
})?;
1391+
}
13651392
}
13661393

13671394
for monitor_data in &migration.channel_monitors {
@@ -2437,7 +2464,10 @@ mod tests {
24372464
};
24382465
use lightning::sign::KeysManager as LdkKeysManager;
24392466
use lightning::util::persist::{
2440-
KVStore, KVStoreSync, CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE,
2467+
KVStore, KVStoreSync, CHANNEL_MANAGER_PERSISTENCE_KEY,
2468+
CHANNEL_MANAGER_PERSISTENCE_PRIMARY_NAMESPACE,
2469+
CHANNEL_MANAGER_PERSISTENCE_SECONDARY_NAMESPACE,
2470+
CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE,
24412471
CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE,
24422472
};
24432473
use lightning::util::ser::Writeable;
@@ -2708,4 +2738,85 @@ mod tests {
27082738
apply_channel_data_migration(&migration, &store, &keys_manager, &logger, &runtime);
27092739
assert_eq!(result, Err(BuildError::ReadFailed));
27102740
}
2741+
2742+
#[test]
2743+
fn test_channel_manager_migration_fresh_write_to_empty_store() {
2744+
let (store, keys_manager, logger, runtime) = make_test_deps(&[42u8; 32]);
2745+
let manager_bytes = vec![0x01, 0x02, 0x03, 0x04];
2746+
2747+
let migration = ChannelDataMigration {
2748+
channel_manager: Some(manager_bytes.clone()),
2749+
channel_monitors: Vec::new(),
2750+
};
2751+
2752+
let result =
2753+
apply_channel_data_migration(&migration, &store, &keys_manager, &logger, &runtime);
2754+
assert!(result.is_ok());
2755+
2756+
// Verify the channel manager was written to the store.
2757+
let stored = runtime.block_on(KVStore::read(
2758+
&*store,
2759+
CHANNEL_MANAGER_PERSISTENCE_PRIMARY_NAMESPACE,
2760+
CHANNEL_MANAGER_PERSISTENCE_SECONDARY_NAMESPACE,
2761+
CHANNEL_MANAGER_PERSISTENCE_KEY,
2762+
));
2763+
assert!(stored.is_ok());
2764+
assert_eq!(stored.unwrap(), manager_bytes);
2765+
}
2766+
2767+
#[test]
2768+
fn test_channel_manager_migration_skips_when_existing_data_present() {
2769+
let (store, keys_manager, logger, runtime) = make_test_deps(&[42u8; 32]);
2770+
2771+
// Pre-populate the store with existing channel manager data.
2772+
let existing_data = vec![0xAA, 0xBB, 0xCC];
2773+
runtime
2774+
.block_on(KVStore::write(
2775+
&*store,
2776+
CHANNEL_MANAGER_PERSISTENCE_PRIMARY_NAMESPACE,
2777+
CHANNEL_MANAGER_PERSISTENCE_SECONDARY_NAMESPACE,
2778+
CHANNEL_MANAGER_PERSISTENCE_KEY,
2779+
existing_data.clone(),
2780+
))
2781+
.unwrap();
2782+
2783+
// Attempt migration with different data.
2784+
let migration_bytes = vec![0x01, 0x02, 0x03];
2785+
let migration = ChannelDataMigration {
2786+
channel_manager: Some(migration_bytes),
2787+
channel_monitors: Vec::new(),
2788+
};
2789+
2790+
let result =
2791+
apply_channel_data_migration(&migration, &store, &keys_manager, &logger, &runtime);
2792+
assert!(result.is_ok());
2793+
2794+
// Verify the original data is preserved (migration was skipped).
2795+
let stored = runtime
2796+
.block_on(KVStore::read(
2797+
&*store,
2798+
CHANNEL_MANAGER_PERSISTENCE_PRIMARY_NAMESPACE,
2799+
CHANNEL_MANAGER_PERSISTENCE_SECONDARY_NAMESPACE,
2800+
CHANNEL_MANAGER_PERSISTENCE_KEY,
2801+
))
2802+
.unwrap();
2803+
assert_eq!(stored, existing_data);
2804+
}
2805+
2806+
#[test]
2807+
fn test_channel_manager_migration_fails_on_store_read_error() {
2808+
let store: Arc<DynStore> = Arc::new(FailingReadStore);
2809+
let keys_manager = LdkKeysManager::new(&[42u8; 32], 0, 0, false);
2810+
let logger = Arc::new(Logger::new_log_facade());
2811+
let runtime = Arc::new(Runtime::new(Arc::clone(&logger)).unwrap());
2812+
2813+
let migration = ChannelDataMigration {
2814+
channel_manager: Some(vec![0x01, 0x02, 0x03]),
2815+
channel_monitors: Vec::new(),
2816+
};
2817+
2818+
let result =
2819+
apply_channel_data_migration(&migration, &store, &keys_manager, &logger, &runtime);
2820+
assert_eq!(result, Err(BuildError::ReadFailed));
2821+
}
27112822
}

0 commit comments

Comments
 (0)