Skip to content

fix: prevent channel data overwrite in FS→KV migration - #74

Merged
ovitrif merged 14 commits into
mainfrom
fix/channel-monitor-migration
Mar 16, 2026
Merged

fix: prevent channel data overwrite in FS→KV migration#74
ovitrif merged 14 commits into
mainfrom
fix/channel-monitor-migration

Conversation

@ovitrif

@ovitrifovitrif commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fixed FS→KV channel data migration blindly overwriting newer state (monitors and channel manager).
    The migration now skips writing a channel monitor when the KV store already holds one with a newer or equal update_id, and skips the channel manager when one already exists.
  • Migration read/deserialization failures now fail-closed (ReadFailed) to prevent silent data loss.

Test plan

  • cargo fmt --check — clean
  • cargo build — builds successfully
  • cargo test --lib — all unit tests pass (including new migration tests)
  • Bindings regenerated (Swift/Kotlin/Python)
  • xcframework checksum verified in Package.swift

Release

ovitrifand others added 4 commits March 9, 2026 22:22
During FS→KV store migration, the code now compares update_id before
writing and skips if the KV store already has a newer monitor. This
prevents stale migration data from clobbering current channel state
on repeated restarts.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace incomplete testing docs with comprehensive coverage of unit tests,
integration tests (with macOS ulimit note), and CLN/LND/VSS backend tests
using correct RUSTFLAGS syntax.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
claude[bot]

This comment was marked as resolved.

@ovitrif
ovitrif requested review from ben-kaufman and coreyphillips and removed request for ben-kaufman and coreyphillipsMarch 10, 2026 18:31
ovitrifand others added 2 commits March 10, 2026 21:03
Address PR review: change channel monitor migration error handling from
fail-open (proceed with overwrite) to fail-closed (abort with ReadFailed)
for both deserialization errors and non-NotFound I/O errors. This prevents
silent data loss when the KV store has valid data that can't be read due
to transient errors or format changes.
Also moves CHANGELOG entry to Synonym Fork Additions per project convention.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif
ovitrif marked this pull request as draft March 10, 2026 22:01
@ovitrifovitrif changed the title fix: prevent channel monitor migration from overwriting newer statefix: prevent channel monitor overwrite in migrationsMar 10, 2026
The previous bindgen run used gobley-uniffi-bindgen v0.3.7 (from gobley
main branch) instead of v0.2.0 (from fix-v0.2.0 branch), which added
explicit `public` visibility modifiers and return types throughout the
generated Kotlin code. Regenerated with the correct version.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif
ovitrif marked this pull request as ready for review March 11, 2026 00:46
Extract the channel data migration block from build_with_store_internal
into a standalone apply_channel_data_migration function, making it
generic over the keys manager type to enable direct unit testing.
Add 7 unit tests covering all code paths:
- Invalid monitor data returns ReadFailed
- Empty monitors list succeeds
- Fresh write to empty store
- Equal update_id (existing == migrated) still writes
- Existing data with same update_id gets overwritten
- Corrupt existing data triggers fail-closed (ReadFailed)
- Store IO error triggers fail-closed (ReadFailed)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ben-kaufman

Copy link
Copy Markdown

Looks good, but needs to add some tests ideally.

@coreyphillips

coreyphillips commented Mar 11, 2026

Copy link
Copy Markdown
Collaborator

One nit. Line 1666 uses > so when existing_update_id == migrated_update_id, we still write. Could use >= to skip the redundant write. Unless I'm missing something.

Use >= instead of > when comparing existing vs migrated update_id.
When they are equal, the write is redundant — the store already has
equivalent state.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif

ovitrif commented Mar 11, 2026

Copy link
Copy Markdown
CollaboratorAuthor

One nit. Line 1666 uses > so when existing_update_id == migrated_update_id, we still write. Could use >= to skip the redundant write. Unless I'm missing something.

Good catch — changed to >= so equal update_id skips the redundant write. Updated tests accordingly.

See cde6037

@ovitrif

ovitrif commented Mar 11, 2026

Copy link
Copy Markdown
CollaboratorAuthor

Looks good, but needs to add some tests ideally.

Added in 630b364 — 6 unit tests covering all migration code paths (invalid data, empty list, fresh write, skip on equal/newer update_id, corrupt existing data, store IO error). Run with cargo test --lib test_migration.

Comment threadsrc/builder.rs
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>
@ovitrif
ovitrif marked this pull request as draft March 11, 2026 23:27
ovitrifand others added 2 commits March 12, 2026 00:29
…t-data test
Correct the comment explaining why the channel manager guard uses
existence-only checks: the real blocker is that ChannelManagerReadArgs
requires already-deserialized channel monitors (loaded later in build),
not that infrastructure is unavailable.
Add test documenting intentional behavior when corrupt data exists in
the store: migration is skipped (existence-only), preserving corrupt
data. This is the correct trade-off — overwriting a potentially valid
channel manager with stale FS data risks fund loss.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts:
#	CHANGELOG.md
#	Package.swift
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/arm64-v8a/libldk_node.so
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/armeabi-v7a/libldk_node.so
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/x86_64/libldk_node.so
#	bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-aarch64/libldk_node.dylib
#	bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-x86-64/libldk_node.dylib
@ovitrif
ovitrif marked this pull request as ready for review March 12, 2026 01:17
@ben-kaufman

Copy link
Copy Markdown

@ovitrif new bindings and version bump missing?

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif

Copy link
Copy Markdown
CollaboratorAuthor

@ovitrif new bindings and version bump missing?

Yes, was planned, now it's done 🫡

@ovitrif

ovitrif commented Mar 13, 2026

Copy link
Copy Markdown
CollaboratorAuthor

@ben-kaufman version bump skipped because there was no release of the previous PR (#75), where the artifacts got picked up by a CI job for the apps repos.

Thus there was no reason to bump the version.
Instead, I deleted the former rc.33 packages, release, and tag; rebuilt bindings and release, so now this encapsulates the changes of both PRs, on tip of this branch.

@ovitrifovitrif changed the title fix: prevent channel monitor overwrite in migrationsfix: prevent channel data overwrite in migrationsMar 13, 2026
Comment threadsrc/builder.rs Outdated
@ovitrifovitrif changed the title fix: prevent channel data overwrite in migrationsfix: prevent channel data overwrite in FS→KV migrationMar 16, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ovitrif
ovitrif merged commit 1569477 into mainMar 16, 2026
2 checks passed
@ovitrif
ovitrif deleted the fix/channel-monitor-migration branch March 18, 2026 10:21
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ovitrif@ben-kaufman@coreyphillips
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
fix: prevent channel data overwrite in FS→KV migration by ovitrif · Pull Request #74 · synonymdev/ldk-node · GitHub
Skip to content

fix: prevent channel data overwrite in FS→KV migration - #74

Merged
ovitrif merged 14 commits into
mainfrom
fix/channel-monitor-migration
Mar 16, 2026
Merged

fix: prevent channel data overwrite in FS→KV migration#74
ovitrif merged 14 commits into
mainfrom
fix/channel-monitor-migration

Conversation

@ovitrif

@ovitrifovitrif commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fixed FS→KV channel data migration blindly overwriting newer state (monitors and channel manager).
    The migration now skips writing a channel monitor when the KV store already holds one with a newer or equal update_id, and skips the channel manager when one already exists.
  • Migration read/deserialization failures now fail-closed (ReadFailed) to prevent silent data loss.

Test plan

  • cargo fmt --check — clean
  • cargo build — builds successfully
  • cargo test --lib — all unit tests pass (including new migration tests)
  • Bindings regenerated (Swift/Kotlin/Python)
  • xcframework checksum verified in Package.swift

Release

ovitrifand others added 4 commits March 9, 2026 22:22
During FS→KV store migration, the code now compares update_id before
writing and skips if the KV store already has a newer monitor. This
prevents stale migration data from clobbering current channel state
on repeated restarts.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace incomplete testing docs with comprehensive coverage of unit tests,
integration tests (with macOS ulimit note), and CLN/LND/VSS backend tests
using correct RUSTFLAGS syntax.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
claude[bot]

This comment was marked as resolved.

@ovitrif
ovitrif requested review from ben-kaufman and coreyphillips and removed request for ben-kaufman and coreyphillipsMarch 10, 2026 18:31
ovitrifand others added 2 commits March 10, 2026 21:03
Address PR review: change channel monitor migration error handling from
fail-open (proceed with overwrite) to fail-closed (abort with ReadFailed)
for both deserialization errors and non-NotFound I/O errors. This prevents
silent data loss when the KV store has valid data that can't be read due
to transient errors or format changes.
Also moves CHANGELOG entry to Synonym Fork Additions per project convention.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif
ovitrif marked this pull request as draft March 10, 2026 22:01
@ovitrifovitrif changed the title fix: prevent channel monitor migration from overwriting newer statefix: prevent channel monitor overwrite in migrationsMar 10, 2026
The previous bindgen run used gobley-uniffi-bindgen v0.3.7 (from gobley
main branch) instead of v0.2.0 (from fix-v0.2.0 branch), which added
explicit `public` visibility modifiers and return types throughout the
generated Kotlin code. Regenerated with the correct version.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif
ovitrif marked this pull request as ready for review March 11, 2026 00:46
Extract the channel data migration block from build_with_store_internal
into a standalone apply_channel_data_migration function, making it
generic over the keys manager type to enable direct unit testing.
Add 7 unit tests covering all code paths:
- Invalid monitor data returns ReadFailed
- Empty monitors list succeeds
- Fresh write to empty store
- Equal update_id (existing == migrated) still writes
- Existing data with same update_id gets overwritten
- Corrupt existing data triggers fail-closed (ReadFailed)
- Store IO error triggers fail-closed (ReadFailed)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ben-kaufman

Copy link
Copy Markdown

Looks good, but needs to add some tests ideally.

@coreyphillips

coreyphillips commented Mar 11, 2026

Copy link
Copy Markdown
Collaborator

One nit. Line 1666 uses > so when existing_update_id == migrated_update_id, we still write. Could use >= to skip the redundant write. Unless I'm missing something.

Use >= instead of > when comparing existing vs migrated update_id.
When they are equal, the write is redundant — the store already has
equivalent state.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif

ovitrif commented Mar 11, 2026

Copy link
Copy Markdown
CollaboratorAuthor

One nit. Line 1666 uses > so when existing_update_id == migrated_update_id, we still write. Could use >= to skip the redundant write. Unless I'm missing something.

Good catch — changed to >= so equal update_id skips the redundant write. Updated tests accordingly.

See cde6037

@ovitrif

ovitrif commented Mar 11, 2026

Copy link
Copy Markdown
CollaboratorAuthor

Looks good, but needs to add some tests ideally.

Added in 630b364 — 6 unit tests covering all migration code paths (invalid data, empty list, fresh write, skip on equal/newer update_id, corrupt existing data, store IO error). Run with cargo test --lib test_migration.

Comment threadsrc/builder.rs
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>
@ovitrif
ovitrif marked this pull request as draft March 11, 2026 23:27
ovitrifand others added 2 commits March 12, 2026 00:29
…t-data test
Correct the comment explaining why the channel manager guard uses
existence-only checks: the real blocker is that ChannelManagerReadArgs
requires already-deserialized channel monitors (loaded later in build),
not that infrastructure is unavailable.
Add test documenting intentional behavior when corrupt data exists in
the store: migration is skipped (existence-only), preserving corrupt
data. This is the correct trade-off — overwriting a potentially valid
channel manager with stale FS data risks fund loss.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts:
#	CHANGELOG.md
#	Package.swift
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/arm64-v8a/libldk_node.so
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/armeabi-v7a/libldk_node.so
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/x86_64/libldk_node.so
#	bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-aarch64/libldk_node.dylib
#	bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-x86-64/libldk_node.dylib
@ovitrif
ovitrif marked this pull request as ready for review March 12, 2026 01:17
@ben-kaufman

Copy link
Copy Markdown

@ovitrif new bindings and version bump missing?

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif

Copy link
Copy Markdown
CollaboratorAuthor

@ovitrif new bindings and version bump missing?

Yes, was planned, now it's done 🫡

@ovitrif

ovitrif commented Mar 13, 2026

Copy link
Copy Markdown
CollaboratorAuthor

@ben-kaufman version bump skipped because there was no release of the previous PR (#75), where the artifacts got picked up by a CI job for the apps repos.

Thus there was no reason to bump the version.
Instead, I deleted the former rc.33 packages, release, and tag; rebuilt bindings and release, so now this encapsulates the changes of both PRs, on tip of this branch.

@ovitrifovitrif changed the title fix: prevent channel monitor overwrite in migrationsfix: prevent channel data overwrite in migrationsMar 13, 2026
Comment threadsrc/builder.rs Outdated
@ovitrifovitrif changed the title fix: prevent channel data overwrite in migrationsfix: prevent channel data overwrite in FS→KV migrationMar 16, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ovitrif
ovitrif merged commit 1569477 into mainMar 16, 2026
2 checks passed
@ovitrif
ovitrif deleted the fix/channel-monitor-migration branch March 18, 2026 10:21
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ovitrif@ben-kaufman@coreyphillips
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix: prevent channel data overwrite in FS→KV migration by ovitrif · Pull Request #74 · synonymdev/ldk-node · GitHub
Skip to content

fix: prevent channel data overwrite in FS→KV migration - #74

Merged
ovitrif merged 14 commits into
mainfrom
fix/channel-monitor-migration
Mar 16, 2026
Merged

fix: prevent channel data overwrite in FS→KV migration#74
ovitrif merged 14 commits into
mainfrom
fix/channel-monitor-migration

Conversation

@ovitrif

@ovitrifovitrif commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fixed FS→KV channel data migration blindly overwriting newer state (monitors and channel manager).
    The migration now skips writing a channel monitor when the KV store already holds one with a newer or equal update_id, and skips the channel manager when one already exists.
  • Migration read/deserialization failures now fail-closed (ReadFailed) to prevent silent data loss.

Test plan

  • cargo fmt --check — clean
  • cargo build — builds successfully
  • cargo test --lib — all unit tests pass (including new migration tests)
  • Bindings regenerated (Swift/Kotlin/Python)
  • xcframework checksum verified in Package.swift

Release

ovitrifand others added 4 commits March 9, 2026 22:22
During FS→KV store migration, the code now compares update_id before
writing and skips if the KV store already has a newer monitor. This
prevents stale migration data from clobbering current channel state
on repeated restarts.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace incomplete testing docs with comprehensive coverage of unit tests,
integration tests (with macOS ulimit note), and CLN/LND/VSS backend tests
using correct RUSTFLAGS syntax.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
claude[bot]

This comment was marked as resolved.

@ovitrif
ovitrif requested review from ben-kaufman and coreyphillips and removed request for ben-kaufman and coreyphillipsMarch 10, 2026 18:31
ovitrifand others added 2 commits March 10, 2026 21:03
Address PR review: change channel monitor migration error handling from
fail-open (proceed with overwrite) to fail-closed (abort with ReadFailed)
for both deserialization errors and non-NotFound I/O errors. This prevents
silent data loss when the KV store has valid data that can't be read due
to transient errors or format changes.
Also moves CHANGELOG entry to Synonym Fork Additions per project convention.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif
ovitrif marked this pull request as draft March 10, 2026 22:01
@ovitrifovitrif changed the title fix: prevent channel monitor migration from overwriting newer statefix: prevent channel monitor overwrite in migrationsMar 10, 2026
The previous bindgen run used gobley-uniffi-bindgen v0.3.7 (from gobley
main branch) instead of v0.2.0 (from fix-v0.2.0 branch), which added
explicit `public` visibility modifiers and return types throughout the
generated Kotlin code. Regenerated with the correct version.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif
ovitrif marked this pull request as ready for review March 11, 2026 00:46
Extract the channel data migration block from build_with_store_internal
into a standalone apply_channel_data_migration function, making it
generic over the keys manager type to enable direct unit testing.
Add 7 unit tests covering all code paths:
- Invalid monitor data returns ReadFailed
- Empty monitors list succeeds
- Fresh write to empty store
- Equal update_id (existing == migrated) still writes
- Existing data with same update_id gets overwritten
- Corrupt existing data triggers fail-closed (ReadFailed)
- Store IO error triggers fail-closed (ReadFailed)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ben-kaufman

Copy link
Copy Markdown

Looks good, but needs to add some tests ideally.

@coreyphillips

coreyphillips commented Mar 11, 2026

Copy link
Copy Markdown
Collaborator

One nit. Line 1666 uses > so when existing_update_id == migrated_update_id, we still write. Could use >= to skip the redundant write. Unless I'm missing something.

Use >= instead of > when comparing existing vs migrated update_id.
When they are equal, the write is redundant — the store already has
equivalent state.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif

ovitrif commented Mar 11, 2026

Copy link
Copy Markdown
CollaboratorAuthor

One nit. Line 1666 uses > so when existing_update_id == migrated_update_id, we still write. Could use >= to skip the redundant write. Unless I'm missing something.

Good catch — changed to >= so equal update_id skips the redundant write. Updated tests accordingly.

See cde6037

@ovitrif

ovitrif commented Mar 11, 2026

Copy link
Copy Markdown
CollaboratorAuthor

Looks good, but needs to add some tests ideally.

Added in 630b364 — 6 unit tests covering all migration code paths (invalid data, empty list, fresh write, skip on equal/newer update_id, corrupt existing data, store IO error). Run with cargo test --lib test_migration.

Comment threadsrc/builder.rs
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>
@ovitrif
ovitrif marked this pull request as draft March 11, 2026 23:27
ovitrifand others added 2 commits March 12, 2026 00:29
…t-data test
Correct the comment explaining why the channel manager guard uses
existence-only checks: the real blocker is that ChannelManagerReadArgs
requires already-deserialized channel monitors (loaded later in build),
not that infrastructure is unavailable.
Add test documenting intentional behavior when corrupt data exists in
the store: migration is skipped (existence-only), preserving corrupt
data. This is the correct trade-off — overwriting a potentially valid
channel manager with stale FS data risks fund loss.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts:
#	CHANGELOG.md
#	Package.swift
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/arm64-v8a/libldk_node.so
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/armeabi-v7a/libldk_node.so
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/x86_64/libldk_node.so
#	bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-aarch64/libldk_node.dylib
#	bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-x86-64/libldk_node.dylib
@ovitrif
ovitrif marked this pull request as ready for review March 12, 2026 01:17
@ben-kaufman

Copy link
Copy Markdown

@ovitrif new bindings and version bump missing?

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif

Copy link
Copy Markdown
CollaboratorAuthor

@ovitrif new bindings and version bump missing?

Yes, was planned, now it's done 🫡

@ovitrif

ovitrif commented Mar 13, 2026

Copy link
Copy Markdown
CollaboratorAuthor

@ben-kaufman version bump skipped because there was no release of the previous PR (#75), where the artifacts got picked up by a CI job for the apps repos.

Thus there was no reason to bump the version.
Instead, I deleted the former rc.33 packages, release, and tag; rebuilt bindings and release, so now this encapsulates the changes of both PRs, on tip of this branch.

@ovitrifovitrif changed the title fix: prevent channel monitor overwrite in migrationsfix: prevent channel data overwrite in migrationsMar 13, 2026
Comment threadsrc/builder.rs Outdated
@ovitrifovitrif changed the title fix: prevent channel data overwrite in migrationsfix: prevent channel data overwrite in FS→KV migrationMar 16, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ovitrif
ovitrif merged commit 1569477 into mainMar 16, 2026
2 checks passed
@ovitrif
ovitrif deleted the fix/channel-monitor-migration branch March 18, 2026 10:21
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ovitrif@ben-kaufman@coreyphillips
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix: prevent channel data overwrite in FS→KV migration by ovitrif · Pull Request #74 · synonymdev/ldk-node · GitHub
Skip to content

fix: prevent channel data overwrite in FS→KV migration - #74

Merged
ovitrif merged 14 commits into
mainfrom
fix/channel-monitor-migration
Mar 16, 2026
Merged

fix: prevent channel data overwrite in FS→KV migration#74
ovitrif merged 14 commits into
mainfrom
fix/channel-monitor-migration

Conversation

@ovitrif

@ovitrifovitrif commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fixed FS→KV channel data migration blindly overwriting newer state (monitors and channel manager).
    The migration now skips writing a channel monitor when the KV store already holds one with a newer or equal update_id, and skips the channel manager when one already exists.
  • Migration read/deserialization failures now fail-closed (ReadFailed) to prevent silent data loss.

Test plan

  • cargo fmt --check — clean
  • cargo build — builds successfully
  • cargo test --lib — all unit tests pass (including new migration tests)
  • Bindings regenerated (Swift/Kotlin/Python)
  • xcframework checksum verified in Package.swift

Release

ovitrifand others added 4 commits March 9, 2026 22:22
During FS→KV store migration, the code now compares update_id before
writing and skips if the KV store already has a newer monitor. This
prevents stale migration data from clobbering current channel state
on repeated restarts.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace incomplete testing docs with comprehensive coverage of unit tests,
integration tests (with macOS ulimit note), and CLN/LND/VSS backend tests
using correct RUSTFLAGS syntax.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
claude[bot]

This comment was marked as resolved.

@ovitrif
ovitrif requested review from ben-kaufman and coreyphillips and removed request for ben-kaufman and coreyphillipsMarch 10, 2026 18:31
ovitrifand others added 2 commits March 10, 2026 21:03
Address PR review: change channel monitor migration error handling from
fail-open (proceed with overwrite) to fail-closed (abort with ReadFailed)
for both deserialization errors and non-NotFound I/O errors. This prevents
silent data loss when the KV store has valid data that can't be read due
to transient errors or format changes.
Also moves CHANGELOG entry to Synonym Fork Additions per project convention.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif
ovitrif marked this pull request as draft March 10, 2026 22:01
@ovitrifovitrif changed the title fix: prevent channel monitor migration from overwriting newer statefix: prevent channel monitor overwrite in migrationsMar 10, 2026
The previous bindgen run used gobley-uniffi-bindgen v0.3.7 (from gobley
main branch) instead of v0.2.0 (from fix-v0.2.0 branch), which added
explicit `public` visibility modifiers and return types throughout the
generated Kotlin code. Regenerated with the correct version.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif
ovitrif marked this pull request as ready for review March 11, 2026 00:46
Extract the channel data migration block from build_with_store_internal
into a standalone apply_channel_data_migration function, making it
generic over the keys manager type to enable direct unit testing.
Add 7 unit tests covering all code paths:
- Invalid monitor data returns ReadFailed
- Empty monitors list succeeds
- Fresh write to empty store
- Equal update_id (existing == migrated) still writes
- Existing data with same update_id gets overwritten
- Corrupt existing data triggers fail-closed (ReadFailed)
- Store IO error triggers fail-closed (ReadFailed)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ben-kaufman

Copy link
Copy Markdown

Looks good, but needs to add some tests ideally.

@coreyphillips

coreyphillips commented Mar 11, 2026

Copy link
Copy Markdown
Collaborator

One nit. Line 1666 uses > so when existing_update_id == migrated_update_id, we still write. Could use >= to skip the redundant write. Unless I'm missing something.

Use >= instead of > when comparing existing vs migrated update_id.
When they are equal, the write is redundant — the store already has
equivalent state.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif

ovitrif commented Mar 11, 2026

Copy link
Copy Markdown
CollaboratorAuthor

One nit. Line 1666 uses > so when existing_update_id == migrated_update_id, we still write. Could use >= to skip the redundant write. Unless I'm missing something.

Good catch — changed to >= so equal update_id skips the redundant write. Updated tests accordingly.

See cde6037

@ovitrif

ovitrif commented Mar 11, 2026

Copy link
Copy Markdown
CollaboratorAuthor

Looks good, but needs to add some tests ideally.

Added in 630b364 — 6 unit tests covering all migration code paths (invalid data, empty list, fresh write, skip on equal/newer update_id, corrupt existing data, store IO error). Run with cargo test --lib test_migration.

Comment threadsrc/builder.rs
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>
@ovitrif
ovitrif marked this pull request as draft March 11, 2026 23:27
ovitrifand others added 2 commits March 12, 2026 00:29
…t-data test
Correct the comment explaining why the channel manager guard uses
existence-only checks: the real blocker is that ChannelManagerReadArgs
requires already-deserialized channel monitors (loaded later in build),
not that infrastructure is unavailable.
Add test documenting intentional behavior when corrupt data exists in
the store: migration is skipped (existence-only), preserving corrupt
data. This is the correct trade-off — overwriting a potentially valid
channel manager with stale FS data risks fund loss.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts:
#	CHANGELOG.md
#	Package.swift
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/arm64-v8a/libldk_node.so
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/armeabi-v7a/libldk_node.so
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/x86_64/libldk_node.so
#	bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-aarch64/libldk_node.dylib
#	bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-x86-64/libldk_node.dylib
@ovitrif
ovitrif marked this pull request as ready for review March 12, 2026 01:17
@ben-kaufman

Copy link
Copy Markdown

@ovitrif new bindings and version bump missing?

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif

Copy link
Copy Markdown
CollaboratorAuthor

@ovitrif new bindings and version bump missing?

Yes, was planned, now it's done 🫡

@ovitrif

ovitrif commented Mar 13, 2026

Copy link
Copy Markdown
CollaboratorAuthor

@ben-kaufman version bump skipped because there was no release of the previous PR (#75), where the artifacts got picked up by a CI job for the apps repos.

Thus there was no reason to bump the version.
Instead, I deleted the former rc.33 packages, release, and tag; rebuilt bindings and release, so now this encapsulates the changes of both PRs, on tip of this branch.

@ovitrifovitrif changed the title fix: prevent channel monitor overwrite in migrationsfix: prevent channel data overwrite in migrationsMar 13, 2026
Comment threadsrc/builder.rs Outdated
@ovitrifovitrif changed the title fix: prevent channel data overwrite in migrationsfix: prevent channel data overwrite in FS→KV migrationMar 16, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ovitrif
ovitrif merged commit 1569477 into mainMar 16, 2026
2 checks passed
@ovitrif
ovitrif deleted the fix/channel-monitor-migration branch March 18, 2026 10:21
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ovitrif@ben-kaufman@coreyphillips
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' fix: prevent channel data overwrite in FS→KV migration by ovitrif · Pull Request #74 · synonymdev/ldk-node · GitHub
Skip to content

fix: prevent channel data overwrite in FS→KV migration - #74

Merged
ovitrif merged 14 commits into
mainfrom
fix/channel-monitor-migration
Mar 16, 2026
Merged

fix: prevent channel data overwrite in FS→KV migration#74
ovitrif merged 14 commits into
mainfrom
fix/channel-monitor-migration

Conversation

@ovitrif

@ovitrifovitrif commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fixed FS→KV channel data migration blindly overwriting newer state (monitors and channel manager).
    The migration now skips writing a channel monitor when the KV store already holds one with a newer or equal update_id, and skips the channel manager when one already exists.
  • Migration read/deserialization failures now fail-closed (ReadFailed) to prevent silent data loss.

Test plan

  • cargo fmt --check — clean
  • cargo build — builds successfully
  • cargo test --lib — all unit tests pass (including new migration tests)
  • Bindings regenerated (Swift/Kotlin/Python)
  • xcframework checksum verified in Package.swift

Release

ovitrifand others added 4 commits March 9, 2026 22:22
During FS→KV store migration, the code now compares update_id before
writing and skips if the KV store already has a newer monitor. This
prevents stale migration data from clobbering current channel state
on repeated restarts.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace incomplete testing docs with comprehensive coverage of unit tests,
integration tests (with macOS ulimit note), and CLN/LND/VSS backend tests
using correct RUSTFLAGS syntax.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
claude[bot]

This comment was marked as resolved.

@ovitrif
ovitrif requested review from ben-kaufman and coreyphillips and removed request for ben-kaufman and coreyphillipsMarch 10, 2026 18:31
ovitrifand others added 2 commits March 10, 2026 21:03
Address PR review: change channel monitor migration error handling from
fail-open (proceed with overwrite) to fail-closed (abort with ReadFailed)
for both deserialization errors and non-NotFound I/O errors. This prevents
silent data loss when the KV store has valid data that can't be read due
to transient errors or format changes.
Also moves CHANGELOG entry to Synonym Fork Additions per project convention.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif
ovitrif marked this pull request as draft March 10, 2026 22:01
@ovitrifovitrif changed the title fix: prevent channel monitor migration from overwriting newer statefix: prevent channel monitor overwrite in migrationsMar 10, 2026
The previous bindgen run used gobley-uniffi-bindgen v0.3.7 (from gobley
main branch) instead of v0.2.0 (from fix-v0.2.0 branch), which added
explicit `public` visibility modifiers and return types throughout the
generated Kotlin code. Regenerated with the correct version.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif
ovitrif marked this pull request as ready for review March 11, 2026 00:46
Extract the channel data migration block from build_with_store_internal
into a standalone apply_channel_data_migration function, making it
generic over the keys manager type to enable direct unit testing.
Add 7 unit tests covering all code paths:
- Invalid monitor data returns ReadFailed
- Empty monitors list succeeds
- Fresh write to empty store
- Equal update_id (existing == migrated) still writes
- Existing data with same update_id gets overwritten
- Corrupt existing data triggers fail-closed (ReadFailed)
- Store IO error triggers fail-closed (ReadFailed)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ben-kaufman

Copy link
Copy Markdown

Looks good, but needs to add some tests ideally.

@coreyphillips

coreyphillips commented Mar 11, 2026

Copy link
Copy Markdown
Collaborator

One nit. Line 1666 uses > so when existing_update_id == migrated_update_id, we still write. Could use >= to skip the redundant write. Unless I'm missing something.

Use >= instead of > when comparing existing vs migrated update_id.
When they are equal, the write is redundant — the store already has
equivalent state.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif

ovitrif commented Mar 11, 2026

Copy link
Copy Markdown
CollaboratorAuthor

One nit. Line 1666 uses > so when existing_update_id == migrated_update_id, we still write. Could use >= to skip the redundant write. Unless I'm missing something.

Good catch — changed to >= so equal update_id skips the redundant write. Updated tests accordingly.

See cde6037

@ovitrif

ovitrif commented Mar 11, 2026

Copy link
Copy Markdown
CollaboratorAuthor

Looks good, but needs to add some tests ideally.

Added in 630b364 — 6 unit tests covering all migration code paths (invalid data, empty list, fresh write, skip on equal/newer update_id, corrupt existing data, store IO error). Run with cargo test --lib test_migration.

Comment threadsrc/builder.rs
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>
@ovitrif
ovitrif marked this pull request as draft March 11, 2026 23:27
ovitrifand others added 2 commits March 12, 2026 00:29
…t-data test
Correct the comment explaining why the channel manager guard uses
existence-only checks: the real blocker is that ChannelManagerReadArgs
requires already-deserialized channel monitors (loaded later in build),
not that infrastructure is unavailable.
Add test documenting intentional behavior when corrupt data exists in
the store: migration is skipped (existence-only), preserving corrupt
data. This is the correct trade-off — overwriting a potentially valid
channel manager with stale FS data risks fund loss.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts:
#	CHANGELOG.md
#	Package.swift
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/arm64-v8a/libldk_node.so
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/armeabi-v7a/libldk_node.so
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/x86_64/libldk_node.so
#	bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-aarch64/libldk_node.dylib
#	bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-x86-64/libldk_node.dylib
@ovitrif
ovitrif marked this pull request as ready for review March 12, 2026 01:17
@ben-kaufman

Copy link
Copy Markdown

@ovitrif new bindings and version bump missing?

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif

Copy link
Copy Markdown
CollaboratorAuthor

@ovitrif new bindings and version bump missing?

Yes, was planned, now it's done 🫡

@ovitrif

ovitrif commented Mar 13, 2026

Copy link
Copy Markdown
CollaboratorAuthor

@ben-kaufman version bump skipped because there was no release of the previous PR (#75), where the artifacts got picked up by a CI job for the apps repos.

Thus there was no reason to bump the version.
Instead, I deleted the former rc.33 packages, release, and tag; rebuilt bindings and release, so now this encapsulates the changes of both PRs, on tip of this branch.

@ovitrifovitrif changed the title fix: prevent channel monitor overwrite in migrationsfix: prevent channel data overwrite in migrationsMar 13, 2026
Comment threadsrc/builder.rs Outdated
@ovitrifovitrif changed the title fix: prevent channel data overwrite in migrationsfix: prevent channel data overwrite in FS→KV migrationMar 16, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ovitrif
ovitrif merged commit 1569477 into mainMar 16, 2026
2 checks passed
@ovitrif
ovitrif deleted the fix/channel-monitor-migration branch March 18, 2026 10:21
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ovitrif@ben-kaufman@coreyphillips
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix: prevent channel data overwrite in FS→KV migration by ovitrif · Pull Request #74 · synonymdev/ldk-node · GitHub
Skip to content

fix: prevent channel data overwrite in FS→KV migration - #74

Merged
ovitrif merged 14 commits into
mainfrom
fix/channel-monitor-migration
Mar 16, 2026
Merged

fix: prevent channel data overwrite in FS→KV migration#74
ovitrif merged 14 commits into
mainfrom
fix/channel-monitor-migration

Conversation

@ovitrif

@ovitrifovitrif commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fixed FS→KV channel data migration blindly overwriting newer state (monitors and channel manager).
    The migration now skips writing a channel monitor when the KV store already holds one with a newer or equal update_id, and skips the channel manager when one already exists.
  • Migration read/deserialization failures now fail-closed (ReadFailed) to prevent silent data loss.

Test plan

  • cargo fmt --check — clean
  • cargo build — builds successfully
  • cargo test --lib — all unit tests pass (including new migration tests)
  • Bindings regenerated (Swift/Kotlin/Python)
  • xcframework checksum verified in Package.swift

Release

ovitrifand others added 4 commits March 9, 2026 22:22
During FS→KV store migration, the code now compares update_id before
writing and skips if the KV store already has a newer monitor. This
prevents stale migration data from clobbering current channel state
on repeated restarts.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace incomplete testing docs with comprehensive coverage of unit tests,
integration tests (with macOS ulimit note), and CLN/LND/VSS backend tests
using correct RUSTFLAGS syntax.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
claude[bot]

This comment was marked as resolved.

@ovitrif
ovitrif requested review from ben-kaufman and coreyphillips and removed request for ben-kaufman and coreyphillipsMarch 10, 2026 18:31
ovitrifand others added 2 commits March 10, 2026 21:03
Address PR review: change channel monitor migration error handling from
fail-open (proceed with overwrite) to fail-closed (abort with ReadFailed)
for both deserialization errors and non-NotFound I/O errors. This prevents
silent data loss when the KV store has valid data that can't be read due
to transient errors or format changes.
Also moves CHANGELOG entry to Synonym Fork Additions per project convention.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif
ovitrif marked this pull request as draft March 10, 2026 22:01
@ovitrifovitrif changed the title fix: prevent channel monitor migration from overwriting newer statefix: prevent channel monitor overwrite in migrationsMar 10, 2026
The previous bindgen run used gobley-uniffi-bindgen v0.3.7 (from gobley
main branch) instead of v0.2.0 (from fix-v0.2.0 branch), which added
explicit `public` visibility modifiers and return types throughout the
generated Kotlin code. Regenerated with the correct version.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif
ovitrif marked this pull request as ready for review March 11, 2026 00:46
Extract the channel data migration block from build_with_store_internal
into a standalone apply_channel_data_migration function, making it
generic over the keys manager type to enable direct unit testing.
Add 7 unit tests covering all code paths:
- Invalid monitor data returns ReadFailed
- Empty monitors list succeeds
- Fresh write to empty store
- Equal update_id (existing == migrated) still writes
- Existing data with same update_id gets overwritten
- Corrupt existing data triggers fail-closed (ReadFailed)
- Store IO error triggers fail-closed (ReadFailed)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ben-kaufman

Copy link
Copy Markdown

Looks good, but needs to add some tests ideally.

@coreyphillips

coreyphillips commented Mar 11, 2026

Copy link
Copy Markdown
Collaborator

One nit. Line 1666 uses > so when existing_update_id == migrated_update_id, we still write. Could use >= to skip the redundant write. Unless I'm missing something.

Use >= instead of > when comparing existing vs migrated update_id.
When they are equal, the write is redundant — the store already has
equivalent state.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif

ovitrif commented Mar 11, 2026

Copy link
Copy Markdown
CollaboratorAuthor

One nit. Line 1666 uses > so when existing_update_id == migrated_update_id, we still write. Could use >= to skip the redundant write. Unless I'm missing something.

Good catch — changed to >= so equal update_id skips the redundant write. Updated tests accordingly.

See cde6037

@ovitrif

ovitrif commented Mar 11, 2026

Copy link
Copy Markdown
CollaboratorAuthor

Looks good, but needs to add some tests ideally.

Added in 630b364 — 6 unit tests covering all migration code paths (invalid data, empty list, fresh write, skip on equal/newer update_id, corrupt existing data, store IO error). Run with cargo test --lib test_migration.

Comment threadsrc/builder.rs
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>
@ovitrif
ovitrif marked this pull request as draft March 11, 2026 23:27
ovitrifand others added 2 commits March 12, 2026 00:29
…t-data test
Correct the comment explaining why the channel manager guard uses
existence-only checks: the real blocker is that ChannelManagerReadArgs
requires already-deserialized channel monitors (loaded later in build),
not that infrastructure is unavailable.
Add test documenting intentional behavior when corrupt data exists in
the store: migration is skipped (existence-only), preserving corrupt
data. This is the correct trade-off — overwriting a potentially valid
channel manager with stale FS data risks fund loss.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts:
#	CHANGELOG.md
#	Package.swift
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/arm64-v8a/libldk_node.so
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/armeabi-v7a/libldk_node.so
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/x86_64/libldk_node.so
#	bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-aarch64/libldk_node.dylib
#	bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-x86-64/libldk_node.dylib
@ovitrif
ovitrif marked this pull request as ready for review March 12, 2026 01:17
@ben-kaufman

Copy link
Copy Markdown

@ovitrif new bindings and version bump missing?

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif

Copy link
Copy Markdown
CollaboratorAuthor

@ovitrif new bindings and version bump missing?

Yes, was planned, now it's done 🫡

@ovitrif

ovitrif commented Mar 13, 2026

Copy link
Copy Markdown
CollaboratorAuthor

@ben-kaufman version bump skipped because there was no release of the previous PR (#75), where the artifacts got picked up by a CI job for the apps repos.

Thus there was no reason to bump the version.
Instead, I deleted the former rc.33 packages, release, and tag; rebuilt bindings and release, so now this encapsulates the changes of both PRs, on tip of this branch.

@ovitrifovitrif changed the title fix: prevent channel monitor overwrite in migrationsfix: prevent channel data overwrite in migrationsMar 13, 2026
Comment threadsrc/builder.rs Outdated
@ovitrifovitrif changed the title fix: prevent channel data overwrite in migrationsfix: prevent channel data overwrite in FS→KV migrationMar 16, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ovitrif
ovitrif merged commit 1569477 into mainMar 16, 2026
2 checks passed
@ovitrif
ovitrif deleted the fix/channel-monitor-migration branch March 18, 2026 10:21
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ovitrif@ben-kaufman@coreyphillips
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix: prevent channel data overwrite in FS→KV migration by ovitrif · Pull Request #74 · synonymdev/ldk-node · GitHub
Skip to content

fix: prevent channel data overwrite in FS→KV migration - #74

Merged
ovitrif merged 14 commits into
mainfrom
fix/channel-monitor-migration
Mar 16, 2026
Merged

fix: prevent channel data overwrite in FS→KV migration#74
ovitrif merged 14 commits into
mainfrom
fix/channel-monitor-migration

Conversation

@ovitrif

@ovitrifovitrif commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fixed FS→KV channel data migration blindly overwriting newer state (monitors and channel manager).
    The migration now skips writing a channel monitor when the KV store already holds one with a newer or equal update_id, and skips the channel manager when one already exists.
  • Migration read/deserialization failures now fail-closed (ReadFailed) to prevent silent data loss.

Test plan

  • cargo fmt --check — clean
  • cargo build — builds successfully
  • cargo test --lib — all unit tests pass (including new migration tests)
  • Bindings regenerated (Swift/Kotlin/Python)
  • xcframework checksum verified in Package.swift

Release

ovitrifand others added 4 commits March 9, 2026 22:22
During FS→KV store migration, the code now compares update_id before
writing and skips if the KV store already has a newer monitor. This
prevents stale migration data from clobbering current channel state
on repeated restarts.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace incomplete testing docs with comprehensive coverage of unit tests,
integration tests (with macOS ulimit note), and CLN/LND/VSS backend tests
using correct RUSTFLAGS syntax.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
claude[bot]

This comment was marked as resolved.

@ovitrif
ovitrif requested review from ben-kaufman and coreyphillips and removed request for ben-kaufman and coreyphillipsMarch 10, 2026 18:31
ovitrifand others added 2 commits March 10, 2026 21:03
Address PR review: change channel monitor migration error handling from
fail-open (proceed with overwrite) to fail-closed (abort with ReadFailed)
for both deserialization errors and non-NotFound I/O errors. This prevents
silent data loss when the KV store has valid data that can't be read due
to transient errors or format changes.
Also moves CHANGELOG entry to Synonym Fork Additions per project convention.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif
ovitrif marked this pull request as draft March 10, 2026 22:01
@ovitrifovitrif changed the title fix: prevent channel monitor migration from overwriting newer statefix: prevent channel monitor overwrite in migrationsMar 10, 2026
The previous bindgen run used gobley-uniffi-bindgen v0.3.7 (from gobley
main branch) instead of v0.2.0 (from fix-v0.2.0 branch), which added
explicit `public` visibility modifiers and return types throughout the
generated Kotlin code. Regenerated with the correct version.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif
ovitrif marked this pull request as ready for review March 11, 2026 00:46
Extract the channel data migration block from build_with_store_internal
into a standalone apply_channel_data_migration function, making it
generic over the keys manager type to enable direct unit testing.
Add 7 unit tests covering all code paths:
- Invalid monitor data returns ReadFailed
- Empty monitors list succeeds
- Fresh write to empty store
- Equal update_id (existing == migrated) still writes
- Existing data with same update_id gets overwritten
- Corrupt existing data triggers fail-closed (ReadFailed)
- Store IO error triggers fail-closed (ReadFailed)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ben-kaufman

Copy link
Copy Markdown

Looks good, but needs to add some tests ideally.

@coreyphillips

coreyphillips commented Mar 11, 2026

Copy link
Copy Markdown
Collaborator

One nit. Line 1666 uses > so when existing_update_id == migrated_update_id, we still write. Could use >= to skip the redundant write. Unless I'm missing something.

Use >= instead of > when comparing existing vs migrated update_id.
When they are equal, the write is redundant — the store already has
equivalent state.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif

ovitrif commented Mar 11, 2026

Copy link
Copy Markdown
CollaboratorAuthor

One nit. Line 1666 uses > so when existing_update_id == migrated_update_id, we still write. Could use >= to skip the redundant write. Unless I'm missing something.

Good catch — changed to >= so equal update_id skips the redundant write. Updated tests accordingly.

See cde6037

@ovitrif

ovitrif commented Mar 11, 2026

Copy link
Copy Markdown
CollaboratorAuthor

Looks good, but needs to add some tests ideally.

Added in 630b364 — 6 unit tests covering all migration code paths (invalid data, empty list, fresh write, skip on equal/newer update_id, corrupt existing data, store IO error). Run with cargo test --lib test_migration.

Comment threadsrc/builder.rs
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>
@ovitrif
ovitrif marked this pull request as draft March 11, 2026 23:27
ovitrifand others added 2 commits March 12, 2026 00:29
…t-data test
Correct the comment explaining why the channel manager guard uses
existence-only checks: the real blocker is that ChannelManagerReadArgs
requires already-deserialized channel monitors (loaded later in build),
not that infrastructure is unavailable.
Add test documenting intentional behavior when corrupt data exists in
the store: migration is skipped (existence-only), preserving corrupt
data. This is the correct trade-off — overwriting a potentially valid
channel manager with stale FS data risks fund loss.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts:
#	CHANGELOG.md
#	Package.swift
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/arm64-v8a/libldk_node.so
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/armeabi-v7a/libldk_node.so
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/x86_64/libldk_node.so
#	bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-aarch64/libldk_node.dylib
#	bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-x86-64/libldk_node.dylib
@ovitrif
ovitrif marked this pull request as ready for review March 12, 2026 01:17
@ben-kaufman

Copy link
Copy Markdown

@ovitrif new bindings and version bump missing?

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif

Copy link
Copy Markdown
CollaboratorAuthor

@ovitrif new bindings and version bump missing?

Yes, was planned, now it's done 🫡

@ovitrif

ovitrif commented Mar 13, 2026

Copy link
Copy Markdown
CollaboratorAuthor

@ben-kaufman version bump skipped because there was no release of the previous PR (#75), where the artifacts got picked up by a CI job for the apps repos.

Thus there was no reason to bump the version.
Instead, I deleted the former rc.33 packages, release, and tag; rebuilt bindings and release, so now this encapsulates the changes of both PRs, on tip of this branch.

@ovitrifovitrif changed the title fix: prevent channel monitor overwrite in migrationsfix: prevent channel data overwrite in migrationsMar 13, 2026
Comment threadsrc/builder.rs Outdated
@ovitrifovitrif changed the title fix: prevent channel data overwrite in migrationsfix: prevent channel data overwrite in FS→KV migrationMar 16, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ovitrif
ovitrif merged commit 1569477 into mainMar 16, 2026
2 checks passed
@ovitrif
ovitrif deleted the fix/channel-monitor-migration branch March 18, 2026 10:21
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ovitrif@ben-kaufman@coreyphillips
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); fix: prevent channel data overwrite in FS→KV migration by ovitrif · Pull Request #74 · synonymdev/ldk-node · GitHub
Skip to content

fix: prevent channel data overwrite in FS→KV migration - #74

Merged
ovitrif merged 14 commits into
mainfrom
fix/channel-monitor-migration
Mar 16, 2026
Merged

fix: prevent channel data overwrite in FS→KV migration#74
ovitrif merged 14 commits into
mainfrom
fix/channel-monitor-migration

Conversation

@ovitrif

@ovitrifovitrif commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fixed FS→KV channel data migration blindly overwriting newer state (monitors and channel manager).
    The migration now skips writing a channel monitor when the KV store already holds one with a newer or equal update_id, and skips the channel manager when one already exists.
  • Migration read/deserialization failures now fail-closed (ReadFailed) to prevent silent data loss.

Test plan

  • cargo fmt --check — clean
  • cargo build — builds successfully
  • cargo test --lib — all unit tests pass (including new migration tests)
  • Bindings regenerated (Swift/Kotlin/Python)
  • xcframework checksum verified in Package.swift

Release

ovitrifand others added 4 commits March 9, 2026 22:22
During FS→KV store migration, the code now compares update_id before
writing and skips if the KV store already has a newer monitor. This
prevents stale migration data from clobbering current channel state
on repeated restarts.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace incomplete testing docs with comprehensive coverage of unit tests,
integration tests (with macOS ulimit note), and CLN/LND/VSS backend tests
using correct RUSTFLAGS syntax.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
claude[bot]

This comment was marked as resolved.

@ovitrif
ovitrif requested review from ben-kaufman and coreyphillips and removed request for ben-kaufman and coreyphillipsMarch 10, 2026 18:31
ovitrifand others added 2 commits March 10, 2026 21:03
Address PR review: change channel monitor migration error handling from
fail-open (proceed with overwrite) to fail-closed (abort with ReadFailed)
for both deserialization errors and non-NotFound I/O errors. This prevents
silent data loss when the KV store has valid data that can't be read due
to transient errors or format changes.
Also moves CHANGELOG entry to Synonym Fork Additions per project convention.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif
ovitrif marked this pull request as draft March 10, 2026 22:01
@ovitrifovitrif changed the title fix: prevent channel monitor migration from overwriting newer statefix: prevent channel monitor overwrite in migrationsMar 10, 2026
The previous bindgen run used gobley-uniffi-bindgen v0.3.7 (from gobley
main branch) instead of v0.2.0 (from fix-v0.2.0 branch), which added
explicit `public` visibility modifiers and return types throughout the
generated Kotlin code. Regenerated with the correct version.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif
ovitrif marked this pull request as ready for review March 11, 2026 00:46
Extract the channel data migration block from build_with_store_internal
into a standalone apply_channel_data_migration function, making it
generic over the keys manager type to enable direct unit testing.
Add 7 unit tests covering all code paths:
- Invalid monitor data returns ReadFailed
- Empty monitors list succeeds
- Fresh write to empty store
- Equal update_id (existing == migrated) still writes
- Existing data with same update_id gets overwritten
- Corrupt existing data triggers fail-closed (ReadFailed)
- Store IO error triggers fail-closed (ReadFailed)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ben-kaufman

Copy link
Copy Markdown

Looks good, but needs to add some tests ideally.

@coreyphillips

coreyphillips commented Mar 11, 2026

Copy link
Copy Markdown
Collaborator

One nit. Line 1666 uses > so when existing_update_id == migrated_update_id, we still write. Could use >= to skip the redundant write. Unless I'm missing something.

Use >= instead of > when comparing existing vs migrated update_id.
When they are equal, the write is redundant — the store already has
equivalent state.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif

ovitrif commented Mar 11, 2026

Copy link
Copy Markdown
CollaboratorAuthor

One nit. Line 1666 uses > so when existing_update_id == migrated_update_id, we still write. Could use >= to skip the redundant write. Unless I'm missing something.

Good catch — changed to >= so equal update_id skips the redundant write. Updated tests accordingly.

See cde6037

@ovitrif

ovitrif commented Mar 11, 2026

Copy link
Copy Markdown
CollaboratorAuthor

Looks good, but needs to add some tests ideally.

Added in 630b364 — 6 unit tests covering all migration code paths (invalid data, empty list, fresh write, skip on equal/newer update_id, corrupt existing data, store IO error). Run with cargo test --lib test_migration.

Comment threadsrc/builder.rs
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>
@ovitrif
ovitrif marked this pull request as draft March 11, 2026 23:27
ovitrifand others added 2 commits March 12, 2026 00:29
…t-data test
Correct the comment explaining why the channel manager guard uses
existence-only checks: the real blocker is that ChannelManagerReadArgs
requires already-deserialized channel monitors (loaded later in build),
not that infrastructure is unavailable.
Add test documenting intentional behavior when corrupt data exists in
the store: migration is skipped (existence-only), preserving corrupt
data. This is the correct trade-off — overwriting a potentially valid
channel manager with stale FS data risks fund loss.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts:
#	CHANGELOG.md
#	Package.swift
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/arm64-v8a/libldk_node.so
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/armeabi-v7a/libldk_node.so
#	bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/x86_64/libldk_node.so
#	bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-aarch64/libldk_node.dylib
#	bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-x86-64/libldk_node.dylib
@ovitrif
ovitrif marked this pull request as ready for review March 12, 2026 01:17
@ben-kaufman

Copy link
Copy Markdown

@ovitrif new bindings and version bump missing?

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ovitrif

Copy link
Copy Markdown
CollaboratorAuthor

@ovitrif new bindings and version bump missing?

Yes, was planned, now it's done 🫡

@ovitrif

ovitrif commented Mar 13, 2026

Copy link
Copy Markdown
CollaboratorAuthor

@ben-kaufman version bump skipped because there was no release of the previous PR (#75), where the artifacts got picked up by a CI job for the apps repos.

Thus there was no reason to bump the version.
Instead, I deleted the former rc.33 packages, release, and tag; rebuilt bindings and release, so now this encapsulates the changes of both PRs, on tip of this branch.

@ovitrifovitrif changed the title fix: prevent channel monitor overwrite in migrationsfix: prevent channel data overwrite in migrationsMar 13, 2026
Comment threadsrc/builder.rs Outdated
@ovitrifovitrif changed the title fix: prevent channel data overwrite in migrationsfix: prevent channel data overwrite in FS→KV migrationMar 16, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ovitrif
ovitrif merged commit 1569477 into mainMar 16, 2026
2 checks passed
@ovitrif
ovitrif deleted the fix/channel-monitor-migration branch March 18, 2026 10:21
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ovitrif@ben-kaufman@coreyphillips