Upstream VssStoreBuilder and VssStore to lightning-persister

[tnull]

Since the beginning of VSS we intended to upstream the corresponding KVStore implementation to lightning-persister.

Here, we finally do it. I'll put this in draft until lightningdevkit/ldk-node#755 lands so we can directly upstream the version already upgraded for pubkey-auth.

[ldk-reviews-bot]

🎉 This PR is now ready for review!
Please choose at least one reviewer by assigning them on the right bar.
If no reviewers are assigned within 10 minutes, I'll automatically assign one.
Once the first reviewer has submitted a review, a second will be assigned if required.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.17%. Comparing base (d830f10) to head (9686cb9).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4323      +/-   ##
==========================================
- Coverage   86.18%   86.17%   -0.01%     
==========================================
  Files         160      160              
  Lines      107536   107536              
  Branches   107536   107536              
==========================================
- Hits        92680    92670      -10     
- Misses      12231    12237       +6     
- Partials     2625     2629       +4     

Flag	Coverage Δ
tests	86.17% <ø> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

Nice. Maybe should wait on vss-client removing the reqwest and url deps as well? Maybe more importantly base64 which is a solo-maintainer crate and can be implemented in 50 LoC.

[tnull]

Nice. Maybe should wait on vss-client removing the reqwest and url deps as well? Maybe more importantly base64 which is a solo-maintainer crate and can be implemented in 50 LoC.

The former is done here lightningdevkit/vss-client#56, for url I'll have to first spend some more time on my Url implementation and upstream it, so depending how timelines work out I wouldn't want to hold for that.

[ldk-claude-review-bot]

Review Summary

Two new issues found beyond prior review comments:

Inline comments posted

1. lightning-persister/src/vss_store.rs:992-1003 — Test vss_read_write_remove_list_persist will panic: it uses #[test] (no tokio runtime) but VssStore::new and all KVStoreSync methods call tokio::task::block_in_place, which requires a multi-threaded tokio runtime context.

2. lightning-persister/src/vss_store.rs:789-792 — V0 schema version is never persisted, making detection heuristic-based on every startup. If bdk_wallet data is ever deleted from a V0 store while other data remains, the store will be misclassified as V1, causing data loss (wrong key format + wrong AAD for decryption).

Previously flagged issues still open (not re-posted)

• .unwrap() on runtime build (line 110) — should propagate via ?
• Missing tokio sync feature in Cargo.toml — relies on transitive enablement from vss-client-ng
• Drop::drop block_in_place panics outside tokio runtime (line 378)
• Version counter overflow check after fetch_add (line 178)
• Lazy-remove JoinHandle silently dropped (lines 270, 356)
• Unnecessary lightning-macros dependency (Cargo.toml line 20)

[tnull]

Now rebased / updated to incorporate latest VSS changes from LDK Node. Should be good for review.

LDK Node draft PR: lightningdevkit/ldk-node#837

[ldk-claude-review-bot]

Bug: The code uses tokio::sync::Mutex extensively (7+ call sites in vss_store.rs), but the tokio dependency only declares features = ["rt-multi-thread"]. The sync feature is required for tokio::sync::Mutex to be available.

This likely compiles today only because vss-client-ng transitively enables tokio/sync. If that crate ever changes its tokio feature requirements, this will break.

Suggested change

	tokio = { version = "1.35", optional = true, default-features = false, features = ["rt-multi-thread"] }
	tokio = { version = "1.35", optional = true, default-features = false, features = ["rt-multi-thread", "sync"] }

[ldk-claude-review-bot]

Bug: .unwrap() will panic if the OS fails to create the runtime (e.g., resource limits, thread creation failure). Since this function returns io::Result<Self>, the error should be propagated instead:

Suggested change

	.unwrap();
	.build()?;

[ldk-claude-review-bot]

Bug: This test will panic at runtime. VssStore::new (line 128) and all KVStoreSync methods (lines 207, 238, 272, 290) use tokio::task::block_in_place, which panics if called from outside a multi-threaded tokio runtime. This #[test] runs on a plain OS thread with no tokio runtime context.

The second test (vss_read_write_remove_list_persist_in_runtime_context) correctly uses #[tokio::test(flavor = "multi_thread", worker_threads = 1)].

Either make this test #[tokio::test(flavor = "multi_thread")] as well, or handle the non-runtime case in the constructor/sync methods:

fn block_on_internal<F: Future>(runtime: &tokio::runtime::Runtime, fut: F) -> F::Output {
    match tokio::runtime::Handle::try_current() {
        Ok(_) => tokio::task::block_in_place(|| runtime.handle().block_on(fut)),
        Err(_) => runtime.handle().block_on(fut),
    }
}

If the intent is that VssStore always requires a tokio runtime context (even for sync usage), this needs to be documented on the struct/builder, and this test should be updated.

[ldk-claude-review-bot]

Potential data loss: When V0 is detected, the schema version is NOT written to storage. This means every future startup re-runs the detection heuristic (checking for bdk_wallet# prefix). If a V0 store ever loses all its bdk_wallet data (e.g., wallet reset, selective key deletion) while retaining other data (channel monitors, etc.), the next startup will misclassify it as a fresh V1 store. This causes:

1. All existing V0 keys become inaccessible (V1 uses obfuscated namespace prefixes, V0 uses plaintext)
2. Decryption fails (V1 uses the store key as AAD, V0 uses empty AAD)

Consider persisting VssSchemaVersion::V0 to storage (like V1 does) to make the detection durable:

if wallet_data_present {
    // Persist V0 schema version so future startups don't rely on the heuristic
    let schema_version = VssSchemaVersion::V0;
    let encoded_version = schema_version.encode();
    let storable_builder = StorableBuilder::new(VssEntropySource(entropy_source));
    let vss_version = -1;
    let aad = store_key.as_bytes();
    let storable = storable_builder.build(encoded_version, vss_version, &data_encryption_key, aad);
    let request = PutObjectRequest {
        store_id: store_id.clone(),
        global_version: None,
        transaction_items: vec![KeyValue { key: store_key, version: vss_version, value: storable.encode_to_vec() }],
        delete_items: vec![],
    };
    // Best-effort write; if it fails, we'll re-detect next time
    let _ = client.put_object(&request).await;
    Ok(schema_version)
} else {

[TheBlueMatt]

Hmm, does it make sense to upstream this hack or should we go ahead and split the store struct into an async and sync version? I assume pretty soon LDK Node will only need/want the async version anyway?

[TheBlueMatt]

Can we DRY this up with the filesystem store at all?

[TheBlueMatt]

Hmm, its a bit weird to upstream LDK Node-specific migration logic.