Move persist into async part of the sweeper #3819

joostjager · 2025-06-02T12:27:00Z

Prepares for making the kv store async in #3778. Otherwise it might be necessary to use block_on in the sweeper. For block_on, a runtime would be needed.

Prepares for making the kv store async. Otherwise it might be necessary to use block_on in the sweeper. For block_on, a runtime would be needed.

ldk-reviews-bot · 2025-06-02T12:27:03Z

👋 Thanks for assigning @valentinewallace 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

Took a first look and left some comments.

Besides, I still think if we go this way we should just also switch to use a Notifier to wake the background processor to trigger persistence.

tnull · 2025-06-04T11:15:36Z

lightning/src/util/sweep.rs

@@ -783,11 +788,13 @@ where
 struct SweeperState {
 	outputs: Vec<TrackedSpendableOutput>,
 	best_block: BestBlock,
+	dirty: bool,


We had that discussion before: I'd really prefer it if we don't mix in runtime state with the SweeperState, which is precisely the object we use to isolated the persisted state from the non-persisted state, which also avoid having to hand a mutable state to persist_state.

Last time, I created that isolation, but then reverted in favor of an atomic boolean. Which direction would you suggest taking it with the dirty flag? I don't think I'd like another atomic boolean. Already didn't like the first one, but two independent sync primitives is expanding the state space even further.

Yes, I would much prefer to just have another needs_persist: AtomicBool on OutputSweeper directly.

The type of flows that I'd like to avoid is stuff like: update state, unlock state, mark dirty and then concurrently a persist is happening in between unlock and mark dirty, ultimately leading to clean state marked as dirty that will be re-persisted without changes. Ofc the re-persist isn't the biggest problem, but I am cautious of requiring devs to reason through scenarios like the one above.

Try out here: main...joostjager:rust-lightning:sweeper-async-persist-atomicbool

I definitely feel all those cases pop up within me if I use that atomic bool.

I don't know this area of the code well, but I tend to agree with Joost that if we had put both flags inside the SweeperState then it would be easier to reason about -- everything would have to be changed under the same lock so would definitely be no concerns about concurrency. At face value, having a separate lock seems like it asks for a race condition?

So there a two aspects here why I dislike the idea of pushing the runtime flags into SweeperState:

We intentionally had SweeperState hold the 'actual'/persisted state of the sweeper, not any runtime-specific behavior. The (_unused, dirty, (static_value, false)), in the persistence logic really just shows that you unnecessarily broke the separation of data and logic we had here. If we think that all should be locked under a single Mutex, we'd need to create a wrapper struct holding both the SweeperState and the runtime-specific bool to maintain that.

However, secondly, I don't think we should introduce the lock contention and block the background processor that is woken and processing a 'I need persist' notification just to check if it actually still needs to re-persist. We don't have strong guarantees when the BP responds to a notification, so if it's mid-loop already it might take a while until it gets back to actually process the persist. Also note that what we do in this PR is effectively splitting the persistence in two: sync in-line persistence for stuff that really needs to happen before we return (track_spendable_outputs) and 'lazy'/async persistence that will happen some time after block connection. For the latter we have relaxed consistency guarantees anyways, and we basically increase chances of missing a persistence anyways. So I don't quite understand where the concern for race conditions in this 'lazy' case comes from. I don't see why we favor lock contention over (theoretical) relaxed consistency guarantees for a case where we already opt into the latter knowingly.

It might also be noteworthy that post-async KVStore we might need to rework the current pattern anyways, as we wouldn't be able to hold the MutexGuard across the write().await boundary. We'll figure that out when we get there, but it could mean that we need to clone the to-be persisted state before dropping the lock, and actually making the call, which would be another reason to not include ~unrelated fields in the state object.

TLDR: I'd prefer to continue to have the runtime bools live as AtomicBools on OutputSweeper directly, but if you guys really worry about any races for the already-lazy case, we should at the very least solve it by wrapping the two fields and SweeperState in yet another struct, maintaining the data/logic separation.

tnull · 2025-06-04T11:20:35Z

lightning/src/util/sweep.rs

+		let result = {
+			self.regenerate_and_broadcast_spend_if_necessary_internal().await?;
+
+			// If there is still dirty state, we need to persist it.


This is a weird pattern. Why not move persistence out of regenerate_and_broadcast_spend_if_necessary_internal and just set the dirty flag there?

I looked at that, but I think we have to persist before we broadcast? Or is that not necessary?

I looked at that, but I think we have to persist before we broadcast? Or is that not necessary?

Hmm, not sure if necessary, but yes, it's probably cleaner to persist that we broadcasted before we attempt it.

However, I think you can avoid the entire 'if it's still dirty'-pattern if you'd trigger the repersistence via a Notifier rather than through the call to regenerate_and_broadcast_if_necessary, as discussed below.

Discussed offline. Probably still need a dirty flag to prevent unnecessary persists when only sweeps need to be checked.

Discussed offline. Probably still need a dirty flag to prevent unnecessary persists when only sweeps need to be checked.

Well, this was never the question, the question was around whether we need to run the 'if it's still dirty'-pattern after we may have just persisted. And to avoid that, we should just switch to use the notifier, as we intend to do that anyways.

ldk-reviews-bot · 2025-06-04T11:22:51Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

joostjager · 2025-06-04T11:48:03Z

Besides, I still think if we go this way we should just also switch to use a Notifier to wake the background processor to trigger persistence.

You mean as part of this PR? I agree that that would be nicer than a timer, but it seems orthogonal to what we are doing here?

tnull · 2025-06-04T11:54:55Z

You mean as part of this PR? I agree that that would be nicer than a timer, but it seems orthogonal to what we are doing here?

Yes, I presume it would just be another (~ 20 LoC ?) commit that I don't consider orthogonal to changing the persistence scheme of the OutputSweeper, but very much in-line with / related to the effort in this PR.

joostjager · 2025-06-04T12:00:16Z

It is of course related, but it is not necessary to do it in this PR? For unblocking the async kv store, what's in this PR is all I need.

tnull · 2025-06-04T12:02:56Z

It is of course related, but it is not necessary to do it in this PR? For unblocking the async kv store, what's in this PR is all I need.

See #3819 (comment): I think you can avoid that 'double-check' pattern if you have repersistence triggered via a notifier.

valentinewallace · 2025-06-04T17:43:44Z

lightning/src/util/sweep.rs

+			// If there is still dirty state, we need to persist it.
+			let mut sweeper_state = self.sweeper_state.lock().unwrap();
+			if sweeper_state.dirty {
+				self.persist_state(&mut *sweeper_state).map_err(|e| {


Would rather take a non-mutable reference here and update the dirty flag afterwards, since persist_state seems like it should be "read-only" on the state. Or we could rename the method.

Perhaps flush_state is an option? Updating the dirty flag afterwards means some code duplication.

That works for me!

valentinewallace · 2025-06-04T17:45:03Z

lightning/src/util/sweep.rs

@@ -783,11 +788,13 @@ where
 struct SweeperState {
 	outputs: Vec<TrackedSpendableOutput>,
 	best_block: BestBlock,
+	dirty: bool,


I don't know this area of the code well, but I tend to agree with Joost that if we had put both flags inside the SweeperState then it would be easier to reason about -- everything would have to be changed under the same lock so would definitely be no concerns about concurrency. At face value, having a separate lock seems like it asks for a race condition?

Move persist into async part of the sweeper

c80cc36

Prepares for making the kv store async. Otherwise it might be necessary to use block_on in the sweeper. For block_on, a runtime would be needed.

joostjager requested a review from tnull June 2, 2025 13:53

joostjager marked this pull request as ready for review June 4, 2025 09:50

tnull reviewed Jun 4, 2025

View reviewed changes

joostjager requested a review from valentinewallace June 4, 2025 15:53

valentinewallace reviewed Jun 4, 2025

View reviewed changes

Move persist into async part of the sweeper #3819

Are you sure you want to change the base?

Move persist into async part of the sweeper #3819

Uh oh!

Conversation

joostjager commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

tnull left a comment

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!

Choose a reason for hiding this comment

Uh oh!

valentinewallace Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Jun 4, 2025

Uh oh!

joostjager commented Jun 4, 2025

Uh oh!

tnull commented Jun 4, 2025

Uh oh!

joostjager commented Jun 4, 2025

Uh oh!

tnull commented Jun 4, 2025

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!

valentinewallace Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joostjager commented Jun 2, 2025 •

edited

Loading

ldk-reviews-bot commented Jun 2, 2025 •

edited

Loading

valentinewallace Jun 4, 2025 •

edited

Loading

tnull Jun 5, 2025 •

edited

Loading

valentinewallace Jun 4, 2025 •

edited

Loading