fuzz: add explicit chanmon manager persistence commands

[joostjager]

Add chanmon_consistency commands to persist each node's ChannelManager state explicitly. This lets the fuzz target exercise delayed manager persistence instead of checkpointing it after every command.

[ldk-reviews-bot]

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.67%. Comparing base (ee456a8) to head (c18fab5).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4631 +/- ##
===========================================
+ Coverage 28.02% 86.67% +58.65% 
===========================================
Files 126 159 +33 Lines 69960 110568 +40608 Branches 69960 110568 +40608 ===========================================
+ Hits 19606 95836 +76230 + Misses 49020 12213 -36807 - Partials 1334 2519 +1185 

Flag	Coverage Δ
fuzzing-fake-hashes	7.02% <ø> (+0.40%)	⬆️
fuzzing-real-hashes	23.28% <ø> (+0.03%)	⬆️
tests	86.23% <ø> (?)

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.

[joostjager]

Is this a bit fake? Perhaps we should just restart from the latest persisted state, but that also means that payment tracking may be off a bit.

[TheBlueMatt]

Yea, I mean this is kinda emulating the behavior we had before where we persist on every iteration, except now we only make sure we persist every time before we read it...Indeed, we'd have to handle the payment tracking parts, though :/.

[joostjager]

I don't think it was quite the same, because we were potentially stacking up monitor writes a bit more. But I added code to handle the payment tracking to make it more realistic.

I really hope we can improve the current persistence model situation soon. Keeping the separate monitor and manager persistence model coherent is such a time sink.

[ldk-claude-review-bot]

No issues found.

I reviewed every hunk in the diff, tracing through:

1. Generation tracking correctness: next_manager_persistence_generation() returns current + 1, payments are tagged with this value, and checkpoints increment the counter — the invariant that a payment tagged with generation G is present in all snapshots >= G holds.

2. restart_node flow: Non-deferred mode checkpoints before reload (ensuring all in-flight payments are captured), while deferred mode intentionally skips it to exercise stale-state reload. The loaded_manager_generation is correctly captured before the post-reload force_checkpoint_manager_persistence() bump.

3. sync_pending_with_manager_generation: Correctly rolls back only payments from pending (not resolved) whose generation exceeds the loaded snapshot, and cleans up claimed_payment_hashes for those rolled-back payments.

4. Pre-existing edge case preservation: The refactored NodePayments methods (mark_sent, mark_resolved_without_hash, mark_successful_probe) preserve the exact semantics of the old PaymentTracker methods, just operating on PendingPayment structs instead of bare PaymentIds.

5. No off-by-one: The comparison first_persisted_manager_generation > loaded_manager_generation correctly keeps payments at the boundary (gen == loaded) and rolls back those strictly above.

6. Command byte collision: 0x90-0x92 don't overlap with any existing commands.

7. process_all_events/settle_all still calls checkpoint_manager_persistences(), so settlement paths remain correct.

[TheBlueMatt]

Thanks!

One thing I noticed is that we add to claimed_payment_hashes immediately upon calling node.claim_funds and then expect that to result in a payment resolution on the sending side. This should lead to failures (and since it apparently doesn't we should expand coverage so that it does!) in cases where we call claim and then immediately restart without persisting manager or monitors. Instead, we should presumably be adding to claimed_payment_hashes after a PaymentClaimed event.

[ldk-reviews-bot]

👋 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]

One thing I noticed is that we add to claimed_payment_hashes immediately upon calling node.claim_funds and then expect that to result in a payment resolution on the sending side. This should lead to failures (and since it apparently doesn't we should expand coverage so that it does!) in cases where we call claim and then immediately restart without persisting manager or monitors. Instead, we should presumably be adding to claimed_payment_hashes after a PaymentClaimed event.

I tried to repro that failure, but for the cases I tested, the HTLC is then emitted as PaymentClaimable again after reload, the harness claims it again, and makes the tracking check out?

It is cleaner though to mark claimed after PaymentClaimed: #4635

[joostjager]

Add chanmon_consistency commands to persist each node's ChannelManager state explicitly. This lets the fuzz target exercise delayed manager persistence instead of checkpointing it after every command.

[ldk-reviews-bot]

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.67%. Comparing base (ee456a8) to head (c18fab5).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4631 +/- ##
===========================================
+ Coverage 28.02% 86.67% +58.65% 
===========================================
Files 126 159 +33 Lines 69960 110568 +40608 Branches 69960 110568 +40608 ===========================================
+ Hits 19606 95836 +76230 + Misses 49020 12213 -36807 - Partials 1334 2519 +1185 

Flag	Coverage Δ
fuzzing-fake-hashes	7.02% <ø> (+0.40%)	⬆️
fuzzing-real-hashes	23.28% <ø> (+0.03%)	⬆️
tests	86.23% <ø> (?)

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.

[joostjager]

Is this a bit fake? Perhaps we should just restart from the latest persisted state, but that also means that payment tracking may be off a bit.

[TheBlueMatt]

Yea, I mean this is kinda emulating the behavior we had before where we persist on every iteration, except now we only make sure we persist every time before we read it...Indeed, we'd have to handle the payment tracking parts, though :/.

[joostjager]

I don't think it was quite the same, because we were potentially stacking up monitor writes a bit more. But I added code to handle the payment tracking to make it more realistic.

I really hope we can improve the current persistence model situation soon. Keeping the separate monitor and manager persistence model coherent is such a time sink.

[ldk-claude-review-bot]

No issues found.

I reviewed every hunk in the diff, tracing through:

1. Generation tracking correctness: next_manager_persistence_generation() returns current + 1, payments are tagged with this value, and checkpoints increment the counter — the invariant that a payment tagged with generation G is present in all snapshots >= G holds.

2. restart_node flow: Non-deferred mode checkpoints before reload (ensuring all in-flight payments are captured), while deferred mode intentionally skips it to exercise stale-state reload. The loaded_manager_generation is correctly captured before the post-reload force_checkpoint_manager_persistence() bump.

3. sync_pending_with_manager_generation: Correctly rolls back only payments from pending (not resolved) whose generation exceeds the loaded snapshot, and cleans up claimed_payment_hashes for those rolled-back payments.

4. Pre-existing edge case preservation: The refactored NodePayments methods (mark_sent, mark_resolved_without_hash, mark_successful_probe) preserve the exact semantics of the old PaymentTracker methods, just operating on PendingPayment structs instead of bare PaymentIds.

5. No off-by-one: The comparison first_persisted_manager_generation > loaded_manager_generation correctly keeps payments at the boundary (gen == loaded) and rolls back those strictly above.

6. Command byte collision: 0x90-0x92 don't overlap with any existing commands.

7. process_all_events/settle_all still calls checkpoint_manager_persistences(), so settlement paths remain correct.

[TheBlueMatt]

Thanks!

One thing I noticed is that we add to claimed_payment_hashes immediately upon calling node.claim_funds and then expect that to result in a payment resolution on the sending side. This should lead to failures (and since it apparently doesn't we should expand coverage so that it does!) in cases where we call claim and then immediately restart without persisting manager or monitors. Instead, we should presumably be adding to claimed_payment_hashes after a PaymentClaimed event.

[ldk-reviews-bot]

👋 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]

One thing I noticed is that we add to claimed_payment_hashes immediately upon calling node.claim_funds and then expect that to result in a payment resolution on the sending side. This should lead to failures (and since it apparently doesn't we should expand coverage so that it does!) in cases where we call claim and then immediately restart without persisting manager or monitors. Instead, we should presumably be adding to claimed_payment_hashes after a PaymentClaimed event.

I tried to repro that failure, but for the cases I tested, the HTLC is then emitted as PaymentClaimable again after reload, the harness claims it again, and makes the tracking check out?

It is cleaner though to mark claimed after PaymentClaimed: #4635

[joostjager]

Add chanmon_consistency commands to persist each node's ChannelManager state explicitly. This lets the fuzz target exercise delayed manager persistence instead of checkpointing it after every command.

[ldk-reviews-bot]

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.67%. Comparing base (ee456a8) to head (c18fab5).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4631 +/- ##
===========================================
+ Coverage 28.02% 86.67% +58.65% 
===========================================
Files 126 159 +33 Lines 69960 110568 +40608 Branches 69960 110568 +40608 ===========================================
+ Hits 19606 95836 +76230 + Misses 49020 12213 -36807 - Partials 1334 2519 +1185 

Flag	Coverage Δ
fuzzing-fake-hashes	7.02% <ø> (+0.40%)	⬆️
fuzzing-real-hashes	23.28% <ø> (+0.03%)	⬆️
tests	86.23% <ø> (?)

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.

[joostjager]

Is this a bit fake? Perhaps we should just restart from the latest persisted state, but that also means that payment tracking may be off a bit.

[TheBlueMatt]

Yea, I mean this is kinda emulating the behavior we had before where we persist on every iteration, except now we only make sure we persist every time before we read it...Indeed, we'd have to handle the payment tracking parts, though :/.

[joostjager]

I don't think it was quite the same, because we were potentially stacking up monitor writes a bit more. But I added code to handle the payment tracking to make it more realistic.

I really hope we can improve the current persistence model situation soon. Keeping the separate monitor and manager persistence model coherent is such a time sink.

[ldk-claude-review-bot]

No issues found.

I reviewed every hunk in the diff, tracing through:

1. Generation tracking correctness: next_manager_persistence_generation() returns current + 1, payments are tagged with this value, and checkpoints increment the counter — the invariant that a payment tagged with generation G is present in all snapshots >= G holds.

2. restart_node flow: Non-deferred mode checkpoints before reload (ensuring all in-flight payments are captured), while deferred mode intentionally skips it to exercise stale-state reload. The loaded_manager_generation is correctly captured before the post-reload force_checkpoint_manager_persistence() bump.

3. sync_pending_with_manager_generation: Correctly rolls back only payments from pending (not resolved) whose generation exceeds the loaded snapshot, and cleans up claimed_payment_hashes for those rolled-back payments.

4. Pre-existing edge case preservation: The refactored NodePayments methods (mark_sent, mark_resolved_without_hash, mark_successful_probe) preserve the exact semantics of the old PaymentTracker methods, just operating on PendingPayment structs instead of bare PaymentIds.

5. No off-by-one: The comparison first_persisted_manager_generation > loaded_manager_generation correctly keeps payments at the boundary (gen == loaded) and rolls back those strictly above.

6. Command byte collision: 0x90-0x92 don't overlap with any existing commands.

7. process_all_events/settle_all still calls checkpoint_manager_persistences(), so settlement paths remain correct.

[TheBlueMatt]

Thanks!

One thing I noticed is that we add to claimed_payment_hashes immediately upon calling node.claim_funds and then expect that to result in a payment resolution on the sending side. This should lead to failures (and since it apparently doesn't we should expand coverage so that it does!) in cases where we call claim and then immediately restart without persisting manager or monitors. Instead, we should presumably be adding to claimed_payment_hashes after a PaymentClaimed event.

[ldk-reviews-bot]

👋 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]

One thing I noticed is that we add to claimed_payment_hashes immediately upon calling node.claim_funds and then expect that to result in a payment resolution on the sending side. This should lead to failures (and since it apparently doesn't we should expand coverage so that it does!) in cases where we call claim and then immediately restart without persisting manager or monitors. Instead, we should presumably be adding to claimed_payment_hashes after a PaymentClaimed event.

I tried to repro that failure, but for the cases I tested, the HTLC is then emitted as PaymentClaimable again after reload, the harness claims it again, and makes the tracking check out?

It is cleaner though to mark claimed after PaymentClaimed: #4635

[joostjager]

Add chanmon_consistency commands to persist each node's ChannelManager state explicitly. This lets the fuzz target exercise delayed manager persistence instead of checkpointing it after every command.

[ldk-reviews-bot]

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.67%. Comparing base (ee456a8) to head (c18fab5).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4631 +/- ##
===========================================
+ Coverage 28.02% 86.67% +58.65% 
===========================================
Files 126 159 +33 Lines 69960 110568 +40608 Branches 69960 110568 +40608 ===========================================
+ Hits 19606 95836 +76230 + Misses 49020 12213 -36807 - Partials 1334 2519 +1185 

Flag	Coverage Δ
fuzzing-fake-hashes	7.02% <ø> (+0.40%)	⬆️
fuzzing-real-hashes	23.28% <ø> (+0.03%)	⬆️
tests	86.23% <ø> (?)

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.

[joostjager]

Is this a bit fake? Perhaps we should just restart from the latest persisted state, but that also means that payment tracking may be off a bit.

[TheBlueMatt]

Yea, I mean this is kinda emulating the behavior we had before where we persist on every iteration, except now we only make sure we persist every time before we read it...Indeed, we'd have to handle the payment tracking parts, though :/.

[joostjager]

I don't think it was quite the same, because we were potentially stacking up monitor writes a bit more. But I added code to handle the payment tracking to make it more realistic.

I really hope we can improve the current persistence model situation soon. Keeping the separate monitor and manager persistence model coherent is such a time sink.

[ldk-claude-review-bot]

No issues found.

I reviewed every hunk in the diff, tracing through:

1. Generation tracking correctness: next_manager_persistence_generation() returns current + 1, payments are tagged with this value, and checkpoints increment the counter — the invariant that a payment tagged with generation G is present in all snapshots >= G holds.

2. restart_node flow: Non-deferred mode checkpoints before reload (ensuring all in-flight payments are captured), while deferred mode intentionally skips it to exercise stale-state reload. The loaded_manager_generation is correctly captured before the post-reload force_checkpoint_manager_persistence() bump.

3. sync_pending_with_manager_generation: Correctly rolls back only payments from pending (not resolved) whose generation exceeds the loaded snapshot, and cleans up claimed_payment_hashes for those rolled-back payments.

4. Pre-existing edge case preservation: The refactored NodePayments methods (mark_sent, mark_resolved_without_hash, mark_successful_probe) preserve the exact semantics of the old PaymentTracker methods, just operating on PendingPayment structs instead of bare PaymentIds.

5. No off-by-one: The comparison first_persisted_manager_generation > loaded_manager_generation correctly keeps payments at the boundary (gen == loaded) and rolls back those strictly above.

6. Command byte collision: 0x90-0x92 don't overlap with any existing commands.

7. process_all_events/settle_all still calls checkpoint_manager_persistences(), so settlement paths remain correct.

[TheBlueMatt]

Thanks!

One thing I noticed is that we add to claimed_payment_hashes immediately upon calling node.claim_funds and then expect that to result in a payment resolution on the sending side. This should lead to failures (and since it apparently doesn't we should expand coverage so that it does!) in cases where we call claim and then immediately restart without persisting manager or monitors. Instead, we should presumably be adding to claimed_payment_hashes after a PaymentClaimed event.

[ldk-reviews-bot]

👋 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]

One thing I noticed is that we add to claimed_payment_hashes immediately upon calling node.claim_funds and then expect that to result in a payment resolution on the sending side. This should lead to failures (and since it apparently doesn't we should expand coverage so that it does!) in cases where we call claim and then immediately restart without persisting manager or monitors. Instead, we should presumably be adding to claimed_payment_hashes after a PaymentClaimed event.

I tried to repro that failure, but for the cases I tested, the HTLC is then emitted as PaymentClaimable again after reload, the harness claims it again, and makes the tracking check out?

It is cleaner though to mark claimed after PaymentClaimed: #4635

[joostjager]

Add chanmon_consistency commands to persist each node's ChannelManager state explicitly. This lets the fuzz target exercise delayed manager persistence instead of checkpointing it after every command.

[ldk-reviews-bot]

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.67%. Comparing base (ee456a8) to head (c18fab5).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4631 +/- ##
===========================================
+ Coverage 28.02% 86.67% +58.65% 
===========================================
Files 126 159 +33 Lines 69960 110568 +40608 Branches 69960 110568 +40608 ===========================================
+ Hits 19606 95836 +76230 + Misses 49020 12213 -36807 - Partials 1334 2519 +1185 

Flag	Coverage Δ
fuzzing-fake-hashes	7.02% <ø> (+0.40%)	⬆️
fuzzing-real-hashes	23.28% <ø> (+0.03%)	⬆️
tests	86.23% <ø> (?)

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.

[joostjager]

Is this a bit fake? Perhaps we should just restart from the latest persisted state, but that also means that payment tracking may be off a bit.

[TheBlueMatt]

Yea, I mean this is kinda emulating the behavior we had before where we persist on every iteration, except now we only make sure we persist every time before we read it...Indeed, we'd have to handle the payment tracking parts, though :/.

[joostjager]

I don't think it was quite the same, because we were potentially stacking up monitor writes a bit more. But I added code to handle the payment tracking to make it more realistic.

I really hope we can improve the current persistence model situation soon. Keeping the separate monitor and manager persistence model coherent is such a time sink.

[ldk-claude-review-bot]

No issues found.

I reviewed every hunk in the diff, tracing through:

1. Generation tracking correctness: next_manager_persistence_generation() returns current + 1, payments are tagged with this value, and checkpoints increment the counter — the invariant that a payment tagged with generation G is present in all snapshots >= G holds.

2. restart_node flow: Non-deferred mode checkpoints before reload (ensuring all in-flight payments are captured), while deferred mode intentionally skips it to exercise stale-state reload. The loaded_manager_generation is correctly captured before the post-reload force_checkpoint_manager_persistence() bump.

3. sync_pending_with_manager_generation: Correctly rolls back only payments from pending (not resolved) whose generation exceeds the loaded snapshot, and cleans up claimed_payment_hashes for those rolled-back payments.

4. Pre-existing edge case preservation: The refactored NodePayments methods (mark_sent, mark_resolved_without_hash, mark_successful_probe) preserve the exact semantics of the old PaymentTracker methods, just operating on PendingPayment structs instead of bare PaymentIds.

5. No off-by-one: The comparison first_persisted_manager_generation > loaded_manager_generation correctly keeps payments at the boundary (gen == loaded) and rolls back those strictly above.

6. Command byte collision: 0x90-0x92 don't overlap with any existing commands.

7. process_all_events/settle_all still calls checkpoint_manager_persistences(), so settlement paths remain correct.

[TheBlueMatt]

Thanks!

One thing I noticed is that we add to claimed_payment_hashes immediately upon calling node.claim_funds and then expect that to result in a payment resolution on the sending side. This should lead to failures (and since it apparently doesn't we should expand coverage so that it does!) in cases where we call claim and then immediately restart without persisting manager or monitors. Instead, we should presumably be adding to claimed_payment_hashes after a PaymentClaimed event.

[ldk-reviews-bot]

👋 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]

One thing I noticed is that we add to claimed_payment_hashes immediately upon calling node.claim_funds and then expect that to result in a payment resolution on the sending side. This should lead to failures (and since it apparently doesn't we should expand coverage so that it does!) in cases where we call claim and then immediately restart without persisting manager or monitors. Instead, we should presumably be adding to claimed_payment_hashes after a PaymentClaimed event.

I tried to repro that failure, but for the cases I tested, the HTLC is then emitted as PaymentClaimable again after reload, the harness claims it again, and makes the tracking check out?

It is cleaner though to mark claimed after PaymentClaimed: #4635

[joostjager]

Add chanmon_consistency commands to persist each node's ChannelManager state explicitly. This lets the fuzz target exercise delayed manager persistence instead of checkpointing it after every command.

[ldk-reviews-bot]

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.67%. Comparing base (ee456a8) to head (c18fab5).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4631 +/- ##
===========================================
+ Coverage 28.02% 86.67% +58.65% 
===========================================
Files 126 159 +33 Lines 69960 110568 +40608 Branches 69960 110568 +40608 ===========================================
+ Hits 19606 95836 +76230 + Misses 49020 12213 -36807 - Partials 1334 2519 +1185 

Flag	Coverage Δ
fuzzing-fake-hashes	7.02% <ø> (+0.40%)	⬆️
fuzzing-real-hashes	23.28% <ø> (+0.03%)	⬆️
tests	86.23% <ø> (?)

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.

[joostjager]

Is this a bit fake? Perhaps we should just restart from the latest persisted state, but that also means that payment tracking may be off a bit.

[TheBlueMatt]

Yea, I mean this is kinda emulating the behavior we had before where we persist on every iteration, except now we only make sure we persist every time before we read it...Indeed, we'd have to handle the payment tracking parts, though :/.

[joostjager]

I don't think it was quite the same, because we were potentially stacking up monitor writes a bit more. But I added code to handle the payment tracking to make it more realistic.

I really hope we can improve the current persistence model situation soon. Keeping the separate monitor and manager persistence model coherent is such a time sink.

[ldk-claude-review-bot]

No issues found.

I reviewed every hunk in the diff, tracing through:

1. Generation tracking correctness: next_manager_persistence_generation() returns current + 1, payments are tagged with this value, and checkpoints increment the counter — the invariant that a payment tagged with generation G is present in all snapshots >= G holds.

2. restart_node flow: Non-deferred mode checkpoints before reload (ensuring all in-flight payments are captured), while deferred mode intentionally skips it to exercise stale-state reload. The loaded_manager_generation is correctly captured before the post-reload force_checkpoint_manager_persistence() bump.

3. sync_pending_with_manager_generation: Correctly rolls back only payments from pending (not resolved) whose generation exceeds the loaded snapshot, and cleans up claimed_payment_hashes for those rolled-back payments.

4. Pre-existing edge case preservation: The refactored NodePayments methods (mark_sent, mark_resolved_without_hash, mark_successful_probe) preserve the exact semantics of the old PaymentTracker methods, just operating on PendingPayment structs instead of bare PaymentIds.

5. No off-by-one: The comparison first_persisted_manager_generation > loaded_manager_generation correctly keeps payments at the boundary (gen == loaded) and rolls back those strictly above.

6. Command byte collision: 0x90-0x92 don't overlap with any existing commands.

7. process_all_events/settle_all still calls checkpoint_manager_persistences(), so settlement paths remain correct.

[TheBlueMatt]

Thanks!

One thing I noticed is that we add to claimed_payment_hashes immediately upon calling node.claim_funds and then expect that to result in a payment resolution on the sending side. This should lead to failures (and since it apparently doesn't we should expand coverage so that it does!) in cases where we call claim and then immediately restart without persisting manager or monitors. Instead, we should presumably be adding to claimed_payment_hashes after a PaymentClaimed event.

[ldk-reviews-bot]

👋 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]

One thing I noticed is that we add to claimed_payment_hashes immediately upon calling node.claim_funds and then expect that to result in a payment resolution on the sending side. This should lead to failures (and since it apparently doesn't we should expand coverage so that it does!) in cases where we call claim and then immediately restart without persisting manager or monitors. Instead, we should presumably be adding to claimed_payment_hashes after a PaymentClaimed event.

I tried to repro that failure, but for the cases I tested, the HTLC is then emitted as PaymentClaimable again after reload, the harness claims it again, and makes the tracking check out?

It is cleaner though to mark claimed after PaymentClaimed: #4635

[joostjager]

Add chanmon_consistency commands to persist each node's ChannelManager state explicitly. This lets the fuzz target exercise delayed manager persistence instead of checkpointing it after every command.

[ldk-reviews-bot]

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.67%. Comparing base (ee456a8) to head (c18fab5).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4631 +/- ##
===========================================
+ Coverage 28.02% 86.67% +58.65% 
===========================================
Files 126 159 +33 Lines 69960 110568 +40608 Branches 69960 110568 +40608 ===========================================
+ Hits 19606 95836 +76230 + Misses 49020 12213 -36807 - Partials 1334 2519 +1185 

Flag	Coverage Δ
fuzzing-fake-hashes	7.02% <ø> (+0.40%)	⬆️
fuzzing-real-hashes	23.28% <ø> (+0.03%)	⬆️
tests	86.23% <ø> (?)

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.

[joostjager]

Is this a bit fake? Perhaps we should just restart from the latest persisted state, but that also means that payment tracking may be off a bit.

[TheBlueMatt]

Yea, I mean this is kinda emulating the behavior we had before where we persist on every iteration, except now we only make sure we persist every time before we read it...Indeed, we'd have to handle the payment tracking parts, though :/.

[joostjager]

I don't think it was quite the same, because we were potentially stacking up monitor writes a bit more. But I added code to handle the payment tracking to make it more realistic.

I really hope we can improve the current persistence model situation soon. Keeping the separate monitor and manager persistence model coherent is such a time sink.

[ldk-claude-review-bot]

No issues found.

I reviewed every hunk in the diff, tracing through:

1. Generation tracking correctness: next_manager_persistence_generation() returns current + 1, payments are tagged with this value, and checkpoints increment the counter — the invariant that a payment tagged with generation G is present in all snapshots >= G holds.

2. restart_node flow: Non-deferred mode checkpoints before reload (ensuring all in-flight payments are captured), while deferred mode intentionally skips it to exercise stale-state reload. The loaded_manager_generation is correctly captured before the post-reload force_checkpoint_manager_persistence() bump.

3. sync_pending_with_manager_generation: Correctly rolls back only payments from pending (not resolved) whose generation exceeds the loaded snapshot, and cleans up claimed_payment_hashes for those rolled-back payments.

4. Pre-existing edge case preservation: The refactored NodePayments methods (mark_sent, mark_resolved_without_hash, mark_successful_probe) preserve the exact semantics of the old PaymentTracker methods, just operating on PendingPayment structs instead of bare PaymentIds.

5. No off-by-one: The comparison first_persisted_manager_generation > loaded_manager_generation correctly keeps payments at the boundary (gen == loaded) and rolls back those strictly above.

6. Command byte collision: 0x90-0x92 don't overlap with any existing commands.

7. process_all_events/settle_all still calls checkpoint_manager_persistences(), so settlement paths remain correct.

[TheBlueMatt]

Thanks!

One thing I noticed is that we add to claimed_payment_hashes immediately upon calling node.claim_funds and then expect that to result in a payment resolution on the sending side. This should lead to failures (and since it apparently doesn't we should expand coverage so that it does!) in cases where we call claim and then immediately restart without persisting manager or monitors. Instead, we should presumably be adding to claimed_payment_hashes after a PaymentClaimed event.

[ldk-reviews-bot]

👋 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]

One thing I noticed is that we add to claimed_payment_hashes immediately upon calling node.claim_funds and then expect that to result in a payment resolution on the sending side. This should lead to failures (and since it apparently doesn't we should expand coverage so that it does!) in cases where we call claim and then immediately restart without persisting manager or monitors. Instead, we should presumably be adding to claimed_payment_hashes after a PaymentClaimed event.

I tried to repro that failure, but for the cases I tested, the HTLC is then emitted as PaymentClaimable again after reload, the harness claims it again, and makes the tracking check out?

It is cleaner though to mark claimed after PaymentClaimed: #4635

[joostjager]

Add chanmon_consistency commands to persist each node's ChannelManager state explicitly. This lets the fuzz target exercise delayed manager persistence instead of checkpointing it after every command.

[ldk-reviews-bot]

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.67%. Comparing base (ee456a8) to head (c18fab5).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4631 +/- ##
===========================================
+ Coverage 28.02% 86.67% +58.65% 
===========================================
Files 126 159 +33 Lines 69960 110568 +40608 Branches 69960 110568 +40608 ===========================================
+ Hits 19606 95836 +76230 + Misses 49020 12213 -36807 - Partials 1334 2519 +1185 

Flag	Coverage Δ
fuzzing-fake-hashes	7.02% <ø> (+0.40%)	⬆️
fuzzing-real-hashes	23.28% <ø> (+0.03%)	⬆️
tests	86.23% <ø> (?)

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.

[joostjager]

Is this a bit fake? Perhaps we should just restart from the latest persisted state, but that also means that payment tracking may be off a bit.

[TheBlueMatt]

Yea, I mean this is kinda emulating the behavior we had before where we persist on every iteration, except now we only make sure we persist every time before we read it...Indeed, we'd have to handle the payment tracking parts, though :/.

[joostjager]

I don't think it was quite the same, because we were potentially stacking up monitor writes a bit more. But I added code to handle the payment tracking to make it more realistic.

I really hope we can improve the current persistence model situation soon. Keeping the separate monitor and manager persistence model coherent is such a time sink.

[ldk-claude-review-bot]

No issues found.

I reviewed every hunk in the diff, tracing through:

1. Generation tracking correctness: next_manager_persistence_generation() returns current + 1, payments are tagged with this value, and checkpoints increment the counter — the invariant that a payment tagged with generation G is present in all snapshots >= G holds.

2. restart_node flow: Non-deferred mode checkpoints before reload (ensuring all in-flight payments are captured), while deferred mode intentionally skips it to exercise stale-state reload. The loaded_manager_generation is correctly captured before the post-reload force_checkpoint_manager_persistence() bump.

3. sync_pending_with_manager_generation: Correctly rolls back only payments from pending (not resolved) whose generation exceeds the loaded snapshot, and cleans up claimed_payment_hashes for those rolled-back payments.

4. Pre-existing edge case preservation: The refactored NodePayments methods (mark_sent, mark_resolved_without_hash, mark_successful_probe) preserve the exact semantics of the old PaymentTracker methods, just operating on PendingPayment structs instead of bare PaymentIds.

5. No off-by-one: The comparison first_persisted_manager_generation > loaded_manager_generation correctly keeps payments at the boundary (gen == loaded) and rolls back those strictly above.

6. Command byte collision: 0x90-0x92 don't overlap with any existing commands.

7. process_all_events/settle_all still calls checkpoint_manager_persistences(), so settlement paths remain correct.

[TheBlueMatt]

Thanks!

One thing I noticed is that we add to claimed_payment_hashes immediately upon calling node.claim_funds and then expect that to result in a payment resolution on the sending side. This should lead to failures (and since it apparently doesn't we should expand coverage so that it does!) in cases where we call claim and then immediately restart without persisting manager or monitors. Instead, we should presumably be adding to claimed_payment_hashes after a PaymentClaimed event.

[ldk-reviews-bot]

👋 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]

One thing I noticed is that we add to claimed_payment_hashes immediately upon calling node.claim_funds and then expect that to result in a payment resolution on the sending side. This should lead to failures (and since it apparently doesn't we should expand coverage so that it does!) in cases where we call claim and then immediately restart without persisting manager or monitors. Instead, we should presumably be adding to claimed_payment_hashes after a PaymentClaimed event.

I tried to repro that failure, but for the cases I tested, the HTLC is then emitted as PaymentClaimable again after reload, the harness claims it again, and makes the tracking check out?

It is cleaner though to mark claimed after PaymentClaimed: #4635