Implement `lazy` deletes for `VssStore` by tnull · Pull Request #689 · lightningdevkit/ldk-node

tnull · 2025-10-31T10:52:05Z

Now based on #691

We bump the LDK depedencies to the just-released 2.0 release candidate and account for minor last-minute API changes.

Furthermore, we implement lazy deletion for VssStore by tracking pending lazy deletes and supplying them as delete_items on the next put operation.

ldk-reviews-bot · 2025-10-31T10:52:08Z

👋 Thanks for assigning @joostjager as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

tnull · 2025-10-31T10:56:23Z

+ .pending_lazy_deletes
+ .try_lock()
+ .ok()
+ .and_then(|mut guard| guard.take())


Hmm, we might lose some lazy deletes if the write below would fail. I do wonder if we should go out of our way to restore the pending items in such a case, or if we're fine just leaning into the 'may or may not succeed' API contract here.
@joostjager Any opinion?
Similiarly, I do wonder if we should spawn-and-forget some tasks on Drop to attempt cleaning up the pending deletes on shutdown?

Good question how loose we can get away with. Those keys are then never deleted anymore, which isn't great? I am not sure.

What do you both think about re-adding the delete_items back to pending_lazy_deletes if write fails? We get to retry deleting them on a subsequent write attempt.

What do you both think about re-adding the delete_items back to pending_lazy_deletes if write fails? We get to retry deleting them on a subsequent write attempt.
Yeah, that's the straightforward approach. Technically, we wouldn't even need to do this according to the API contract, but it's probably best to keep it best-effort. Blocking on re-acquiring the pending_lazy_deletes mutex in write isn't too great as it suddenly creates yet another interdependency between remove and write, but it should be safe here.
Went ahead and did that now.

joostjager

I wonder if lazy delete is needed for VSS? Maybe we can just ignore the flag and delete sync always.

tnull · 2025-10-31T12:03:53Z

I wonder if lazy delete is needed for VSS? Maybe we can just ignore the flag and delete sync always.

Well, esp. now that we're on MonitorUpdatingPersister, it should make a difference in performance on cleanup.

joostjager · 2025-10-31T13:10:48Z

I just don't know if it is worth it for end-user nodes to worry about this. Even persisting full monitors always apparently was good enough performance?

In this PR the delete semantics are again different, because a write is needed before deletes are executed. It doesn't make it easier to understand.

tnull · 2025-10-31T13:12:28Z

I just don't know if it is worth it for end-user nodes to worry about this. Even persisting full monitors always apparently was good enough performance?
In this PR the delete semantics are again different, because a write is needed before deletes are executed. It doesn't make it easier to understand.

I'm not sure the 'end user' would ever need to worry about, as there are very little lazy deletes to begin with?

As you know I was open to dropping the lazy flag, but now that we have it (again), we should also implement it.

joostjager

Code looks good. Just not sure about the implications of lingering data if something goes wrong / a write never happens.

In particular for the incremental channel updates, it would be good if that is always followed by a write somehow without much delay. Idk if that's indeed the case?

joostjager · 2025-10-31T13:50:30Z

+ .pending_lazy_deletes
+ .try_lock()
+ .ok()
+ .and_then(|mut guard| guard.take())


Good question how loose we can get away with. Those keys are then never deleted anymore, which isn't great? I am not sure.

joostjager · 2025-10-31T13:51:38Z

+ let obfuscated_key =
+ self.build_obfuscated_key(&primary_namespace, &secondary_namespace, &key);
+
+ let key_value = KeyValue { key: obfuscated_key, version: -1, value: vec![] };


Can't we store just keys here?

Currently we might get away with it, but going forward we might utilize the version field there, so I'd prefer to do the future-safer thing here.

You say 'might'. Do you mean that just using keys can already be a problem without the version field?
Even if we use the version field in the future, value still isn't needed?
In general, I would avoid dead code (or fields in this case). It just raises questions with devs. At least it did with me. Or otherwise clearly document.
Final question - is a version needed for lazy deletes?

You say 'might'. Do you mean that just using keys can already be a problem without the version field?
No, but it's something we could easily overlook when making use of the version going forward.
Even if we use the version field in the future, value still isn't needed?
Well, whether the API design of KeyValue is good or not is debatable (I didn't invent it), but it is the type that we use to reference entries in the store. I.e., it is the thing used in remove/DeleteObjectRequest.
Final question - is a version needed for lazy deletes?
Well, if we issue a delete on a certain version, the service could in the future ignore a delete if it already has a higher version, no? I.e., it would thereby detect that the value has been updated since the delete was issued.

We could map the key to KeyValue at a later point, and not use it internally? Not a blocker.
Well, if we issue a delete on a certain version, the service could in the future ignore a delete if it already has a higher version, no? I.e., it would thereby detect that the value has been updated since the delete was issued.
I thought we didn't want rewrites of the same key after it has been deleted lazily, but not 100% sure anymore. There's no version currently anyway, so a worry for later.

tnull · 2025-11-17T09:18:22Z

Excuse the delay here. I now added a fixup that has us restore any delete_items on failed write. I think that should be sufficient for now.

We implement `lazy` deletion for `VssStore` by tracking pending lazy deletes and supplying them as `delete_items` on the next `put` operation.

We add a testcase that ensures we only delete a lazily-deleted key after the next write operation succeeds. Co-authored by Claude AI

joostjager · 2025-11-17T11:57:45Z

+ let obfuscated_key =
+ self.build_obfuscated_key(&primary_namespace, &secondary_namespace, &key);
+
+ let key_value = KeyValue { key: obfuscated_key, version: -1, value: vec![] };


You say 'might'. Do you mean that just using keys can already be a problem without the version field?
Even if we use the version field in the future, value still isn't needed?
In general, I would avoid dead code (or fields in this case). It just raises questions with devs. At least it did with me. Or otherwise clearly document.
Final question - is a version needed for lazy deletes?

tnull requested a review from joostjager October 31, 2025 10:52

tnull force-pushed the 2025-10-bump-to-ldk-2.0rc1 branch from a64be1d to 0a26053Compare October 31, 2025 11:05

tnull commented Oct 31, 2025
View reviewed changes

tnull added this to the 0.7 milestone Oct 31, 2025

joostjager reviewed Oct 31, 2025
View reviewed changes

tnull mentioned this pull request Oct 31, 2025
Add simple workflow to generate Swift bindings in CI #688
Merged

tnull mentioned this pull request Oct 31, 2025
Fix bitcoind shutdown hang #682
Merged

chuksys mentioned this pull request Oct 31, 2025
Implement Display for UserChannelId #690
Merged

joostjager reviewed Oct 31, 2025
View reviewed changes

tnull changed the title ~~Bump to LDK 2.0.0-rc1 and implement lazy deletes for VssStore~~Implement lazy deletes for VssStoreOct 31, 2025

tnull mentioned this pull request Nov 10, 2025
LDK Node v0.7 Tracking issue #443
Closed
8 tasks

tnull added this to Weekly Goals Nov 13, 2025

tnull self-assigned this Nov 13, 2025

tnull moved this to Goal: Merge in Weekly Goals Nov 13, 2025

tnull requested a review from joostjager November 17, 2025 09:18

tnull added 2 commits November 17, 2025 10:19

Implement lazy deletes for VssStore
20ffc9c
We implement `lazy` deletion for `VssStore` by tracking pending lazy deletes and supplying them as `delete_items` on the next `put` operation.

f Restore delete_items on failed write
db246fc

tnull force-pushed the 2025-10-bump-to-ldk-2.0rc1 branch from eb42617 to 0dbec9eCompare November 17, 2025 09:19

Add test for lazy deletion behavior
c2b6b18
We add a testcase that ensures we only delete a lazily-deleted key after the next write operation succeeds. Co-authored by Claude AI

tnull force-pushed the 2025-10-bump-to-ldk-2.0rc1 branch from 0dbec9e to c2b6b18Compare November 17, 2025 09:20

joostjager reviewed Nov 17, 2025
View reviewed changes

tnull requested a review from joostjager November 17, 2025 14:43

joostjager approved these changes Nov 17, 2025
View reviewed changes

tnull merged commit be3cb48 into lightningdevkit:mainNov 17, 2025
17 checks passed

github-project-automationBot moved this from Goal: Merge to Done in Weekly Goals Nov 17, 2025

tnull mentioned this pull request Nov 17, 2025
Upgrade VSS encryption and obfuscation scheme #627
Merged

Uh oh!

Conversation

tnull commented Oct 31, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Oct 31, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

tnull commented Oct 31, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joostjager commented Oct 31, 2025

Uh oh!

tnull commented Oct 31, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Nov 17, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tnull commented Oct 31, 2025•
edited
Loading

ldk-reviews-bot commented Oct 31, 2025•
edited
Loading

tnull commented Oct 31, 2025•
edited
Loading

tnull commented Oct 31, 2025•
edited
Loading