Prune locktimed packages when inputs are spent

[whfuyn]

We have to prune lockedtime packages when their inputs are spent, otherwise the notification of the watched outputs might be missed. This can lead to lockedtime packages with spent inputs being added back to the pending claim requests in the future, and never being cleaned up.

Resolves: #3859

[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.

[wpaulino]

Thanks! This could really use some test coverage exercising the bug you ran into, and also handling the reorg case where the locktimed package gets restored.

[wpaulino]

Since we only aggregate locktimed_packages with the same locktime, I think the original package would still have the same one, otherwise we'd have to reinsert it. Let's add a debug_assert that it's true.

[whfuyn]

Do you mean the locktime in the key of locktime_packages equal to the locktime calculated from the package?
The calculated locktime may depend on cur_height. I'm not sure if it's still the same here.

[whfuyn]

Hi @wpaulino , I've added a test to reproduce the problem.
Without this patch, the node will try to timeout an HTLC that has already been claimed by the counterparty. And since this HTLC is in the same package as another unclaimed HTLC, it also prevents that HTLC from being timed out.
The explanation in the issue seems over-complicated. As long as HTLCs are in the locktimed packages, they will miss their notification.

The failed assertion in the CI looks like a mistake. I'm not sure if changing the assertion will still preserve its original test intent.

[whfuyn]

Without this patch, the node will try to timeout an HTLC that has already been claimed by the counterparty. And since this HTLC is in the same package as another unclaimed HTLC, it also prevents that HTLC from being timed out.

This is also the root cause of the repeated fee bump failures in #3831.

[whfuyn]

This is the logs for test_claim_value_force_close (without this patch). We can see that the claiming tx for vout 1 (f7be78d..) is confirmed on node 0, but node 0 still tries to timeout vout 1.

node 1 DEBUG [lightning::chain::onchaintx:761] Updating claims view at height 11 with 2 claim requests
node 1 DEBUG [lightning::chain::package:1294] Adding claiming input for outpoint 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1
node 1 INFO [lightning::chain::onchaintx:853] Broadcasting onchain HTLC-success tx with txid f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258
node 1 INFO [lightning::chain::onchaintx:890] Registering claiming request for 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1
node 1 DEBUG [lightning::chain::package:1294] Adding claiming input for outpoint 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:2
node 1 INFO [lightning::chain::onchaintx:853] Broadcasting onchain HTLC-success tx with txid 0d6d40442723ff9a55b3c17cedac0a3047cc243eb69822aa132bb4a1e0cdc1de
node 1 INFO [lightning::chain::onchaintx:890] Registering claiming request for 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:2
node 0 TRACE [lightning::chain::channelmonitor:4658] Transaction f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258 confirmed in block d6b71222ff0b4fa0099f7e3bf09f3f1ba3c7d7da3661513d16a37feb29d80b7f
node 0 INFO [lightning::chain::channelmonitor:5284] Input spending counterparty commitment tx (1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1) in f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258 resolves outbound HTLC with payment hash 66687aadf862bd776c8fc18b8e9f8e20089714856ee233b3902a591d0d5f2925 with preimage
[lightning/src/ln/monitor_tests.rs:755:2] &a_broadcast_txn[0].input = [
TxIn {
previous_output: OutPoint {
txid: 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177,
vout: 1,
},
...
},
TxIn {
previous_output: OutPoint {
txid: 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177,
vout: 2,
},
...
},
]

[wpaulino]

@whfuyn the test added doesn't fail when removing the fix. To test this accurately, wouldn't we need node A to send two HTLCs with the same expiry, node B then claims one of them, but node A force closes before the claim is settled. Node B claims the HTLC onchain, and then we connect enough blocks to node A such that it reaches the HTLC expiry and can try to time out both HTLCs.

The failed assertion in the CI looks like a mistake. I'm not sure if changing the assertion will still preserve its original test intent.

It looks like these tests are actually exercising the new logic you've added, so they need to updated to no longer expect claiming HTLCs that have already been claimed by the counterparty. We may just be able to update these and have the coverage we need without needing to add a new test.

[whfuyn]

the test added doesn't fail when removing the fix.

thread 'ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests' panicked at lightning/src/ln/functional_tests.rs:11901:5:
HTLC already claimed by counterparty's HTLC-Success transaction
stack backtrace:
0: __rustc::rust_begin_unwind
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/std/src/panicking.rs:697:5
1: core::panicking::panic_fmt
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/panicking.rs:75:14
2: lightning::ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests
at ./src/ln/functional_tests.rs:11901:2
3: lightning::ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests::{{closure}}
at ./src/ln/functional_tests.rs:11770:58
4: core::ops::function::FnOnce::call_once
at /home/dev/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
5: core::ops::function::FnOnce::call_once
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
failures:
ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1092 filtered out; finished in 0.05s
error: test failed, to rerun pass `-p lightning --lib`

This test always fail when the fix is commented out. Are we using the same code? I've uploaded a branch.

cargo test --package lightning --lib --features _externalize_tests -- ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests --exact --show-output

node B then claims one of them,

I made a mistake in the comment. But it's the case in the code.

but node A force closes before the claim is settled

I broadcast B's commitment tx to force close. I guess this should work?

It looks like these tests are actually exercising the new logic you've added, so they need to updated to no longer expect claiming HTLCs that have already been claimed by the counterparty. We may just be able to update these and have the coverage we need without needing to add a new test.

Yes. If the existing test covers the new test, we can remove the new one.

[wpaulino]

Sorry, not sure what went wrong when I tried this yesterday. You can remove the added test and apply this diff to fix the existing ones:

diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index c862d23e1..d6227bb2b 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -1961,45 +1961,9 @@ pub fn test_htlc_on_chain_success() {
_ => panic!("Unexpected event"),
}
-	macro_rules! check_tx_local_broadcast {
- ($node: expr, $htlc_offered: expr, $commitment_tx: expr) => {{
- let mut node_txn = $node.tx_broadcaster.txn_broadcasted.lock().unwrap();
- // HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
- // remote commitment transaction.
- if $htlc_offered {
- assert_eq!(node_txn.len(), 2);
- for tx in node_txn.iter() {
- check_spends!(tx, $commitment_tx);
- assert_ne!(tx.lock_time, LockTime::ZERO);
- assert_eq!(
- tx.input[0].witness.last().unwrap().len(),
- OFFERED_HTLC_SCRIPT_WEIGHT
- );
- assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
- }
- assert_ne!(
- node_txn[0].input[0].previous_output,
- node_txn[1].input[0].previous_output
- );
- } else {
- assert_eq!(node_txn.len(), 1);
- check_spends!(node_txn[0], $commitment_tx);
- assert_ne!(node_txn[0].lock_time, LockTime::ZERO);
- assert_eq!(
- node_txn[0].input[0].witness.last().unwrap().len(),
- ACCEPTED_HTLC_SCRIPT_WEIGHT
- );
- assert!(node_txn[0].output[0].script_pubkey.is_p2wpkh()); // direct payment
- assert_ne!(
- node_txn[0].input[0].previous_output,
- node_txn[0].input[1].previous_output
- );
- }
- node_txn.clear();
- }};
-	}
-	// nodes[1] now broadcasts its own timeout-claim of the output that nodes[2] just claimed via success.
-	check_tx_local_broadcast!(nodes[1], false, commitment_tx[0]);
+	// nodes[1] does not broadcast its own timeout-claim of the output as nodes[2] just claimed it
+	// via success.
+	assert!(nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty());
// Broadcast legit commitment tx from A on B's chain
// Broadcast preimage tx by B on offered output from A commitment tx on A's chain
@@ -2061,7 +2025,17 @@ pub fn test_htlc_on_chain_success() {
_ => panic!("Unexpected event"),
}
}
-	check_tx_local_broadcast!(nodes[0], true, node_a_commitment_tx[0]);
+	// HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
+	// remote commitment transaction.
+	let mut node_txn = nodes[0].tx_broadcaster.txn_broadcast();
+	assert_eq!(node_txn.len(), 2);
+	for tx in node_txn.iter() {
+ check_spends!(tx, node_a_commitment_tx[0]);
+ assert_ne!(tx.lock_time, LockTime::ZERO);
+ assert_eq!(tx.input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
+ assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
+	}
+	assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
}
fn do_test_htlc_on_chain_timeout(connect_style: ConnectStyle) {
diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs
index cdcb570af..5382fba26 100644
--- a/lightning/src/ln/monitor_tests.rs
+++ b/lightning/src/ln/monitor_tests.rs
@@ -732,8 +732,9 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
test_spendable_output(&nodes[0], &remote_txn[0], false);
assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());
-	// After broadcasting the HTLC claim transaction, node A will still consider the HTLC
-	// possibly-claimable up to ANTI_REORG_DELAY, at which point it will drop it.
+	// After confirming the HTLC claim transaction, node A will no longer attempt to claim said
+	// HTLC, unless the transaction is reorged. However, we'll still report a
+	// `MaybeTimeoutClaimableHTLC` balance for it until we reach `ANTI_REORG_DELAY` confirmations.
mine_transaction(&nodes[0], &b_broadcast_txn[0]);
if prev_commitment_tx {
expect_payment_path_successful!(nodes[0]);
@@ -749,18 +750,10 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
// When the HTLC timeout output is spendable in the next block, A should broadcast it
connect_blocks(&nodes[0], htlc_cltv_timeout - nodes[0].best_block_info().1);
let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
-	// Aggregated claim transaction.
assert_eq!(a_broadcast_txn.len(), 1);
check_spends!(a_broadcast_txn[0], remote_txn[0]);
-	assert_eq!(a_broadcast_txn[0].input.len(), 2);
-	assert_ne!(a_broadcast_txn[0].input[0].previous_output.vout, a_broadcast_txn[0].input[1].previous_output.vout);
-	// a_broadcast_txn [0] and [1] should spend the HTLC outputs of the commitment tx
-	assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 3_000));
+	assert_eq!(a_broadcast_txn[0].input.len(), 1);
assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 4_000));
-
-	// Confirm node B's claim for node A to remove that claim from the aggregated claim transaction.
-	mine_transaction(&nodes[0], &b_broadcast_txn[0]);
-	let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
let a_htlc_timeout_tx = a_broadcast_txn.into_iter().next_back().unwrap();
// Once the HTLC-Timeout transaction confirms, A will no longer consider the HTLC

[whfuyn]

@wpaulino I've updated the commit.

[codecov]

Codecov Report

Attention: Patch coverage is 94.02985% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.81%. Comparing base (c48e0a8) to head (56834a1).
Report is 105 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/reorg_tests.rs	92.59%	3 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3860 +/- ##
==========================================
- Coverage 89.65% 88.81% -0.85% 
==========================================
Files 164 166 +2 Lines 134658 119612 -15046 Branches 134658 119612 -15046 ==========================================
- Hits 120734 106231 -14503 + Misses 11246 11045 -201 + Partials 2678 2336 -342 

☔ 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.

[whfuyn]

Uh, another bug found by the reorg test.
Because package_locktime is lower-bounded by the given height, if we pass the current height to it,
the package will be added to locktimed_packages no matter what actual locktime it requires.
That's why we need test coverage. :(

[TheBlueMatt]

Ha, nice. I jumped the gun and was also writing a test, which led to an unrelated bug in reorg handling. I hadn't gotten as far as figuring out why the <= change wasn't passing all tests, though, thanks for handling that!

The test I had written I think covers all the cases yours does plus a few more, so I opened #3923 with your commit here with the test removed, and added a separate test at the end. Let me know what you think of that PR.

[whfuyn]

Great! Feel free to close this one when ready.

[TheBlueMatt]

Supersceded by #3923

[whfuyn]

We have to prune lockedtime packages when their inputs are spent, otherwise the notification of the watched outputs might be missed. This can lead to lockedtime packages with spent inputs being added back to the pending claim requests in the future, and never being cleaned up.

Resolves: #3859

[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.

[wpaulino]

Thanks! This could really use some test coverage exercising the bug you ran into, and also handling the reorg case where the locktimed package gets restored.

[wpaulino]

Since we only aggregate locktimed_packages with the same locktime, I think the original package would still have the same one, otherwise we'd have to reinsert it. Let's add a debug_assert that it's true.

[whfuyn]

Do you mean the locktime in the key of locktime_packages equal to the locktime calculated from the package?
The calculated locktime may depend on cur_height. I'm not sure if it's still the same here.

[whfuyn]

Hi @wpaulino , I've added a test to reproduce the problem.
Without this patch, the node will try to timeout an HTLC that has already been claimed by the counterparty. And since this HTLC is in the same package as another unclaimed HTLC, it also prevents that HTLC from being timed out.
The explanation in the issue seems over-complicated. As long as HTLCs are in the locktimed packages, they will miss their notification.

The failed assertion in the CI looks like a mistake. I'm not sure if changing the assertion will still preserve its original test intent.

[whfuyn]

Without this patch, the node will try to timeout an HTLC that has already been claimed by the counterparty. And since this HTLC is in the same package as another unclaimed HTLC, it also prevents that HTLC from being timed out.

This is also the root cause of the repeated fee bump failures in #3831.

[whfuyn]

This is the logs for test_claim_value_force_close (without this patch). We can see that the claiming tx for vout 1 (f7be78d..) is confirmed on node 0, but node 0 still tries to timeout vout 1.

node 1 DEBUG [lightning::chain::onchaintx:761] Updating claims view at height 11 with 2 claim requests
node 1 DEBUG [lightning::chain::package:1294] Adding claiming input for outpoint 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1
node 1 INFO [lightning::chain::onchaintx:853] Broadcasting onchain HTLC-success tx with txid f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258
node 1 INFO [lightning::chain::onchaintx:890] Registering claiming request for 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1
node 1 DEBUG [lightning::chain::package:1294] Adding claiming input for outpoint 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:2
node 1 INFO [lightning::chain::onchaintx:853] Broadcasting onchain HTLC-success tx with txid 0d6d40442723ff9a55b3c17cedac0a3047cc243eb69822aa132bb4a1e0cdc1de
node 1 INFO [lightning::chain::onchaintx:890] Registering claiming request for 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:2
node 0 TRACE [lightning::chain::channelmonitor:4658] Transaction f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258 confirmed in block d6b71222ff0b4fa0099f7e3bf09f3f1ba3c7d7da3661513d16a37feb29d80b7f
node 0 INFO [lightning::chain::channelmonitor:5284] Input spending counterparty commitment tx (1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1) in f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258 resolves outbound HTLC with payment hash 66687aadf862bd776c8fc18b8e9f8e20089714856ee233b3902a591d0d5f2925 with preimage
[lightning/src/ln/monitor_tests.rs:755:2] &a_broadcast_txn[0].input = [
TxIn {
previous_output: OutPoint {
txid: 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177,
vout: 1,
},
...
},
TxIn {
previous_output: OutPoint {
txid: 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177,
vout: 2,
},
...
},
]

[wpaulino]

@whfuyn the test added doesn't fail when removing the fix. To test this accurately, wouldn't we need node A to send two HTLCs with the same expiry, node B then claims one of them, but node A force closes before the claim is settled. Node B claims the HTLC onchain, and then we connect enough blocks to node A such that it reaches the HTLC expiry and can try to time out both HTLCs.

The failed assertion in the CI looks like a mistake. I'm not sure if changing the assertion will still preserve its original test intent.

It looks like these tests are actually exercising the new logic you've added, so they need to updated to no longer expect claiming HTLCs that have already been claimed by the counterparty. We may just be able to update these and have the coverage we need without needing to add a new test.

[whfuyn]

the test added doesn't fail when removing the fix.

thread 'ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests' panicked at lightning/src/ln/functional_tests.rs:11901:5:
HTLC already claimed by counterparty's HTLC-Success transaction
stack backtrace:
0: __rustc::rust_begin_unwind
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/std/src/panicking.rs:697:5
1: core::panicking::panic_fmt
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/panicking.rs:75:14
2: lightning::ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests
at ./src/ln/functional_tests.rs:11901:2
3: lightning::ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests::{{closure}}
at ./src/ln/functional_tests.rs:11770:58
4: core::ops::function::FnOnce::call_once
at /home/dev/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
5: core::ops::function::FnOnce::call_once
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
failures:
ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1092 filtered out; finished in 0.05s
error: test failed, to rerun pass `-p lightning --lib`

This test always fail when the fix is commented out. Are we using the same code? I've uploaded a branch.

cargo test --package lightning --lib --features _externalize_tests -- ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests --exact --show-output

node B then claims one of them,

I made a mistake in the comment. But it's the case in the code.

but node A force closes before the claim is settled

I broadcast B's commitment tx to force close. I guess this should work?

It looks like these tests are actually exercising the new logic you've added, so they need to updated to no longer expect claiming HTLCs that have already been claimed by the counterparty. We may just be able to update these and have the coverage we need without needing to add a new test.

Yes. If the existing test covers the new test, we can remove the new one.

[wpaulino]

Sorry, not sure what went wrong when I tried this yesterday. You can remove the added test and apply this diff to fix the existing ones:

diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index c862d23e1..d6227bb2b 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -1961,45 +1961,9 @@ pub fn test_htlc_on_chain_success() {
_ => panic!("Unexpected event"),
}
-	macro_rules! check_tx_local_broadcast {
- ($node: expr, $htlc_offered: expr, $commitment_tx: expr) => {{
- let mut node_txn = $node.tx_broadcaster.txn_broadcasted.lock().unwrap();
- // HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
- // remote commitment transaction.
- if $htlc_offered {
- assert_eq!(node_txn.len(), 2);
- for tx in node_txn.iter() {
- check_spends!(tx, $commitment_tx);
- assert_ne!(tx.lock_time, LockTime::ZERO);
- assert_eq!(
- tx.input[0].witness.last().unwrap().len(),
- OFFERED_HTLC_SCRIPT_WEIGHT
- );
- assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
- }
- assert_ne!(
- node_txn[0].input[0].previous_output,
- node_txn[1].input[0].previous_output
- );
- } else {
- assert_eq!(node_txn.len(), 1);
- check_spends!(node_txn[0], $commitment_tx);
- assert_ne!(node_txn[0].lock_time, LockTime::ZERO);
- assert_eq!(
- node_txn[0].input[0].witness.last().unwrap().len(),
- ACCEPTED_HTLC_SCRIPT_WEIGHT
- );
- assert!(node_txn[0].output[0].script_pubkey.is_p2wpkh()); // direct payment
- assert_ne!(
- node_txn[0].input[0].previous_output,
- node_txn[0].input[1].previous_output
- );
- }
- node_txn.clear();
- }};
-	}
-	// nodes[1] now broadcasts its own timeout-claim of the output that nodes[2] just claimed via success.
-	check_tx_local_broadcast!(nodes[1], false, commitment_tx[0]);
+	// nodes[1] does not broadcast its own timeout-claim of the output as nodes[2] just claimed it
+	// via success.
+	assert!(nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty());
// Broadcast legit commitment tx from A on B's chain
// Broadcast preimage tx by B on offered output from A commitment tx on A's chain
@@ -2061,7 +2025,17 @@ pub fn test_htlc_on_chain_success() {
_ => panic!("Unexpected event"),
}
}
-	check_tx_local_broadcast!(nodes[0], true, node_a_commitment_tx[0]);
+	// HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
+	// remote commitment transaction.
+	let mut node_txn = nodes[0].tx_broadcaster.txn_broadcast();
+	assert_eq!(node_txn.len(), 2);
+	for tx in node_txn.iter() {
+ check_spends!(tx, node_a_commitment_tx[0]);
+ assert_ne!(tx.lock_time, LockTime::ZERO);
+ assert_eq!(tx.input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
+ assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
+	}
+	assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
}
fn do_test_htlc_on_chain_timeout(connect_style: ConnectStyle) {
diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs
index cdcb570af..5382fba26 100644
--- a/lightning/src/ln/monitor_tests.rs
+++ b/lightning/src/ln/monitor_tests.rs
@@ -732,8 +732,9 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
test_spendable_output(&nodes[0], &remote_txn[0], false);
assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());
-	// After broadcasting the HTLC claim transaction, node A will still consider the HTLC
-	// possibly-claimable up to ANTI_REORG_DELAY, at which point it will drop it.
+	// After confirming the HTLC claim transaction, node A will no longer attempt to claim said
+	// HTLC, unless the transaction is reorged. However, we'll still report a
+	// `MaybeTimeoutClaimableHTLC` balance for it until we reach `ANTI_REORG_DELAY` confirmations.
mine_transaction(&nodes[0], &b_broadcast_txn[0]);
if prev_commitment_tx {
expect_payment_path_successful!(nodes[0]);
@@ -749,18 +750,10 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
// When the HTLC timeout output is spendable in the next block, A should broadcast it
connect_blocks(&nodes[0], htlc_cltv_timeout - nodes[0].best_block_info().1);
let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
-	// Aggregated claim transaction.
assert_eq!(a_broadcast_txn.len(), 1);
check_spends!(a_broadcast_txn[0], remote_txn[0]);
-	assert_eq!(a_broadcast_txn[0].input.len(), 2);
-	assert_ne!(a_broadcast_txn[0].input[0].previous_output.vout, a_broadcast_txn[0].input[1].previous_output.vout);
-	// a_broadcast_txn [0] and [1] should spend the HTLC outputs of the commitment tx
-	assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 3_000));
+	assert_eq!(a_broadcast_txn[0].input.len(), 1);
assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 4_000));
-
-	// Confirm node B's claim for node A to remove that claim from the aggregated claim transaction.
-	mine_transaction(&nodes[0], &b_broadcast_txn[0]);
-	let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
let a_htlc_timeout_tx = a_broadcast_txn.into_iter().next_back().unwrap();
// Once the HTLC-Timeout transaction confirms, A will no longer consider the HTLC

[whfuyn]

@wpaulino I've updated the commit.

[codecov]

Codecov Report

Attention: Patch coverage is 94.02985% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.81%. Comparing base (c48e0a8) to head (56834a1).
Report is 105 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/reorg_tests.rs	92.59%	3 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3860 +/- ##
==========================================
- Coverage 89.65% 88.81% -0.85% 
==========================================
Files 164 166 +2 Lines 134658 119612 -15046 Branches 134658 119612 -15046 ==========================================
- Hits 120734 106231 -14503 + Misses 11246 11045 -201 + Partials 2678 2336 -342 

☔ 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.

[whfuyn]

Uh, another bug found by the reorg test.
Because package_locktime is lower-bounded by the given height, if we pass the current height to it,
the package will be added to locktimed_packages no matter what actual locktime it requires.
That's why we need test coverage. :(

[TheBlueMatt]

Ha, nice. I jumped the gun and was also writing a test, which led to an unrelated bug in reorg handling. I hadn't gotten as far as figuring out why the <= change wasn't passing all tests, though, thanks for handling that!

The test I had written I think covers all the cases yours does plus a few more, so I opened #3923 with your commit here with the test removed, and added a separate test at the end. Let me know what you think of that PR.

[whfuyn]

Great! Feel free to close this one when ready.

[TheBlueMatt]

Supersceded by #3923

[whfuyn]

We have to prune lockedtime packages when their inputs are spent, otherwise the notification of the watched outputs might be missed. This can lead to lockedtime packages with spent inputs being added back to the pending claim requests in the future, and never being cleaned up.

Resolves: #3859

[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.

[wpaulino]

Thanks! This could really use some test coverage exercising the bug you ran into, and also handling the reorg case where the locktimed package gets restored.

[wpaulino]

Since we only aggregate locktimed_packages with the same locktime, I think the original package would still have the same one, otherwise we'd have to reinsert it. Let's add a debug_assert that it's true.

[whfuyn]

Do you mean the locktime in the key of locktime_packages equal to the locktime calculated from the package?
The calculated locktime may depend on cur_height. I'm not sure if it's still the same here.

[whfuyn]

Hi @wpaulino , I've added a test to reproduce the problem.
Without this patch, the node will try to timeout an HTLC that has already been claimed by the counterparty. And since this HTLC is in the same package as another unclaimed HTLC, it also prevents that HTLC from being timed out.
The explanation in the issue seems over-complicated. As long as HTLCs are in the locktimed packages, they will miss their notification.

The failed assertion in the CI looks like a mistake. I'm not sure if changing the assertion will still preserve its original test intent.

[whfuyn]

Without this patch, the node will try to timeout an HTLC that has already been claimed by the counterparty. And since this HTLC is in the same package as another unclaimed HTLC, it also prevents that HTLC from being timed out.

This is also the root cause of the repeated fee bump failures in #3831.

[whfuyn]

This is the logs for test_claim_value_force_close (without this patch). We can see that the claiming tx for vout 1 (f7be78d..) is confirmed on node 0, but node 0 still tries to timeout vout 1.

node 1 DEBUG [lightning::chain::onchaintx:761] Updating claims view at height 11 with 2 claim requests
node 1 DEBUG [lightning::chain::package:1294] Adding claiming input for outpoint 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1
node 1 INFO [lightning::chain::onchaintx:853] Broadcasting onchain HTLC-success tx with txid f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258
node 1 INFO [lightning::chain::onchaintx:890] Registering claiming request for 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1
node 1 DEBUG [lightning::chain::package:1294] Adding claiming input for outpoint 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:2
node 1 INFO [lightning::chain::onchaintx:853] Broadcasting onchain HTLC-success tx with txid 0d6d40442723ff9a55b3c17cedac0a3047cc243eb69822aa132bb4a1e0cdc1de
node 1 INFO [lightning::chain::onchaintx:890] Registering claiming request for 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:2
node 0 TRACE [lightning::chain::channelmonitor:4658] Transaction f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258 confirmed in block d6b71222ff0b4fa0099f7e3bf09f3f1ba3c7d7da3661513d16a37feb29d80b7f
node 0 INFO [lightning::chain::channelmonitor:5284] Input spending counterparty commitment tx (1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1) in f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258 resolves outbound HTLC with payment hash 66687aadf862bd776c8fc18b8e9f8e20089714856ee233b3902a591d0d5f2925 with preimage
[lightning/src/ln/monitor_tests.rs:755:2] &a_broadcast_txn[0].input = [
TxIn {
previous_output: OutPoint {
txid: 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177,
vout: 1,
},
...
},
TxIn {
previous_output: OutPoint {
txid: 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177,
vout: 2,
},
...
},
]

[wpaulino]

@whfuyn the test added doesn't fail when removing the fix. To test this accurately, wouldn't we need node A to send two HTLCs with the same expiry, node B then claims one of them, but node A force closes before the claim is settled. Node B claims the HTLC onchain, and then we connect enough blocks to node A such that it reaches the HTLC expiry and can try to time out both HTLCs.

The failed assertion in the CI looks like a mistake. I'm not sure if changing the assertion will still preserve its original test intent.

It looks like these tests are actually exercising the new logic you've added, so they need to updated to no longer expect claiming HTLCs that have already been claimed by the counterparty. We may just be able to update these and have the coverage we need without needing to add a new test.

[whfuyn]

the test added doesn't fail when removing the fix.

thread 'ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests' panicked at lightning/src/ln/functional_tests.rs:11901:5:
HTLC already claimed by counterparty's HTLC-Success transaction
stack backtrace:
0: __rustc::rust_begin_unwind
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/std/src/panicking.rs:697:5
1: core::panicking::panic_fmt
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/panicking.rs:75:14
2: lightning::ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests
at ./src/ln/functional_tests.rs:11901:2
3: lightning::ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests::{{closure}}
at ./src/ln/functional_tests.rs:11770:58
4: core::ops::function::FnOnce::call_once
at /home/dev/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
5: core::ops::function::FnOnce::call_once
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
failures:
ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1092 filtered out; finished in 0.05s
error: test failed, to rerun pass `-p lightning --lib`

This test always fail when the fix is commented out. Are we using the same code? I've uploaded a branch.

cargo test --package lightning --lib --features _externalize_tests -- ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests --exact --show-output

node B then claims one of them,

I made a mistake in the comment. But it's the case in the code.

but node A force closes before the claim is settled

I broadcast B's commitment tx to force close. I guess this should work?

It looks like these tests are actually exercising the new logic you've added, so they need to updated to no longer expect claiming HTLCs that have already been claimed by the counterparty. We may just be able to update these and have the coverage we need without needing to add a new test.

Yes. If the existing test covers the new test, we can remove the new one.

[wpaulino]

Sorry, not sure what went wrong when I tried this yesterday. You can remove the added test and apply this diff to fix the existing ones:

diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index c862d23e1..d6227bb2b 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -1961,45 +1961,9 @@ pub fn test_htlc_on_chain_success() {
_ => panic!("Unexpected event"),
}
-	macro_rules! check_tx_local_broadcast {
- ($node: expr, $htlc_offered: expr, $commitment_tx: expr) => {{
- let mut node_txn = $node.tx_broadcaster.txn_broadcasted.lock().unwrap();
- // HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
- // remote commitment transaction.
- if $htlc_offered {
- assert_eq!(node_txn.len(), 2);
- for tx in node_txn.iter() {
- check_spends!(tx, $commitment_tx);
- assert_ne!(tx.lock_time, LockTime::ZERO);
- assert_eq!(
- tx.input[0].witness.last().unwrap().len(),
- OFFERED_HTLC_SCRIPT_WEIGHT
- );
- assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
- }
- assert_ne!(
- node_txn[0].input[0].previous_output,
- node_txn[1].input[0].previous_output
- );
- } else {
- assert_eq!(node_txn.len(), 1);
- check_spends!(node_txn[0], $commitment_tx);
- assert_ne!(node_txn[0].lock_time, LockTime::ZERO);
- assert_eq!(
- node_txn[0].input[0].witness.last().unwrap().len(),
- ACCEPTED_HTLC_SCRIPT_WEIGHT
- );
- assert!(node_txn[0].output[0].script_pubkey.is_p2wpkh()); // direct payment
- assert_ne!(
- node_txn[0].input[0].previous_output,
- node_txn[0].input[1].previous_output
- );
- }
- node_txn.clear();
- }};
-	}
-	// nodes[1] now broadcasts its own timeout-claim of the output that nodes[2] just claimed via success.
-	check_tx_local_broadcast!(nodes[1], false, commitment_tx[0]);
+	// nodes[1] does not broadcast its own timeout-claim of the output as nodes[2] just claimed it
+	// via success.
+	assert!(nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty());
// Broadcast legit commitment tx from A on B's chain
// Broadcast preimage tx by B on offered output from A commitment tx on A's chain
@@ -2061,7 +2025,17 @@ pub fn test_htlc_on_chain_success() {
_ => panic!("Unexpected event"),
}
}
-	check_tx_local_broadcast!(nodes[0], true, node_a_commitment_tx[0]);
+	// HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
+	// remote commitment transaction.
+	let mut node_txn = nodes[0].tx_broadcaster.txn_broadcast();
+	assert_eq!(node_txn.len(), 2);
+	for tx in node_txn.iter() {
+ check_spends!(tx, node_a_commitment_tx[0]);
+ assert_ne!(tx.lock_time, LockTime::ZERO);
+ assert_eq!(tx.input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
+ assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
+	}
+	assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
}
fn do_test_htlc_on_chain_timeout(connect_style: ConnectStyle) {
diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs
index cdcb570af..5382fba26 100644
--- a/lightning/src/ln/monitor_tests.rs
+++ b/lightning/src/ln/monitor_tests.rs
@@ -732,8 +732,9 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
test_spendable_output(&nodes[0], &remote_txn[0], false);
assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());
-	// After broadcasting the HTLC claim transaction, node A will still consider the HTLC
-	// possibly-claimable up to ANTI_REORG_DELAY, at which point it will drop it.
+	// After confirming the HTLC claim transaction, node A will no longer attempt to claim said
+	// HTLC, unless the transaction is reorged. However, we'll still report a
+	// `MaybeTimeoutClaimableHTLC` balance for it until we reach `ANTI_REORG_DELAY` confirmations.
mine_transaction(&nodes[0], &b_broadcast_txn[0]);
if prev_commitment_tx {
expect_payment_path_successful!(nodes[0]);
@@ -749,18 +750,10 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
// When the HTLC timeout output is spendable in the next block, A should broadcast it
connect_blocks(&nodes[0], htlc_cltv_timeout - nodes[0].best_block_info().1);
let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
-	// Aggregated claim transaction.
assert_eq!(a_broadcast_txn.len(), 1);
check_spends!(a_broadcast_txn[0], remote_txn[0]);
-	assert_eq!(a_broadcast_txn[0].input.len(), 2);
-	assert_ne!(a_broadcast_txn[0].input[0].previous_output.vout, a_broadcast_txn[0].input[1].previous_output.vout);
-	// a_broadcast_txn [0] and [1] should spend the HTLC outputs of the commitment tx
-	assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 3_000));
+	assert_eq!(a_broadcast_txn[0].input.len(), 1);
assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 4_000));
-
-	// Confirm node B's claim for node A to remove that claim from the aggregated claim transaction.
-	mine_transaction(&nodes[0], &b_broadcast_txn[0]);
-	let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
let a_htlc_timeout_tx = a_broadcast_txn.into_iter().next_back().unwrap();
// Once the HTLC-Timeout transaction confirms, A will no longer consider the HTLC

[whfuyn]

@wpaulino I've updated the commit.

[codecov]

Codecov Report

Attention: Patch coverage is 94.02985% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.81%. Comparing base (c48e0a8) to head (56834a1).
Report is 105 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/reorg_tests.rs	92.59%	3 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3860 +/- ##
==========================================
- Coverage 89.65% 88.81% -0.85% 
==========================================
Files 164 166 +2 Lines 134658 119612 -15046 Branches 134658 119612 -15046 ==========================================
- Hits 120734 106231 -14503 + Misses 11246 11045 -201 + Partials 2678 2336 -342 

☔ 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.

[whfuyn]

Uh, another bug found by the reorg test.
Because package_locktime is lower-bounded by the given height, if we pass the current height to it,
the package will be added to locktimed_packages no matter what actual locktime it requires.
That's why we need test coverage. :(

[TheBlueMatt]

Ha, nice. I jumped the gun and was also writing a test, which led to an unrelated bug in reorg handling. I hadn't gotten as far as figuring out why the <= change wasn't passing all tests, though, thanks for handling that!

The test I had written I think covers all the cases yours does plus a few more, so I opened #3923 with your commit here with the test removed, and added a separate test at the end. Let me know what you think of that PR.

[whfuyn]

Great! Feel free to close this one when ready.

[TheBlueMatt]

Supersceded by #3923

[whfuyn]

We have to prune lockedtime packages when their inputs are spent, otherwise the notification of the watched outputs might be missed. This can lead to lockedtime packages with spent inputs being added back to the pending claim requests in the future, and never being cleaned up.

Resolves: #3859

[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.

[wpaulino]

Thanks! This could really use some test coverage exercising the bug you ran into, and also handling the reorg case where the locktimed package gets restored.

[wpaulino]

Since we only aggregate locktimed_packages with the same locktime, I think the original package would still have the same one, otherwise we'd have to reinsert it. Let's add a debug_assert that it's true.

[whfuyn]

Do you mean the locktime in the key of locktime_packages equal to the locktime calculated from the package?
The calculated locktime may depend on cur_height. I'm not sure if it's still the same here.

[whfuyn]

Hi @wpaulino , I've added a test to reproduce the problem.
Without this patch, the node will try to timeout an HTLC that has already been claimed by the counterparty. And since this HTLC is in the same package as another unclaimed HTLC, it also prevents that HTLC from being timed out.
The explanation in the issue seems over-complicated. As long as HTLCs are in the locktimed packages, they will miss their notification.

The failed assertion in the CI looks like a mistake. I'm not sure if changing the assertion will still preserve its original test intent.

[whfuyn]

Without this patch, the node will try to timeout an HTLC that has already been claimed by the counterparty. And since this HTLC is in the same package as another unclaimed HTLC, it also prevents that HTLC from being timed out.

This is also the root cause of the repeated fee bump failures in #3831.

[whfuyn]

This is the logs for test_claim_value_force_close (without this patch). We can see that the claiming tx for vout 1 (f7be78d..) is confirmed on node 0, but node 0 still tries to timeout vout 1.

node 1 DEBUG [lightning::chain::onchaintx:761] Updating claims view at height 11 with 2 claim requests
node 1 DEBUG [lightning::chain::package:1294] Adding claiming input for outpoint 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1
node 1 INFO [lightning::chain::onchaintx:853] Broadcasting onchain HTLC-success tx with txid f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258
node 1 INFO [lightning::chain::onchaintx:890] Registering claiming request for 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1
node 1 DEBUG [lightning::chain::package:1294] Adding claiming input for outpoint 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:2
node 1 INFO [lightning::chain::onchaintx:853] Broadcasting onchain HTLC-success tx with txid 0d6d40442723ff9a55b3c17cedac0a3047cc243eb69822aa132bb4a1e0cdc1de
node 1 INFO [lightning::chain::onchaintx:890] Registering claiming request for 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:2
node 0 TRACE [lightning::chain::channelmonitor:4658] Transaction f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258 confirmed in block d6b71222ff0b4fa0099f7e3bf09f3f1ba3c7d7da3661513d16a37feb29d80b7f
node 0 INFO [lightning::chain::channelmonitor:5284] Input spending counterparty commitment tx (1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1) in f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258 resolves outbound HTLC with payment hash 66687aadf862bd776c8fc18b8e9f8e20089714856ee233b3902a591d0d5f2925 with preimage
[lightning/src/ln/monitor_tests.rs:755:2] &a_broadcast_txn[0].input = [
TxIn {
previous_output: OutPoint {
txid: 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177,
vout: 1,
},
...
},
TxIn {
previous_output: OutPoint {
txid: 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177,
vout: 2,
},
...
},
]

[wpaulino]

@whfuyn the test added doesn't fail when removing the fix. To test this accurately, wouldn't we need node A to send two HTLCs with the same expiry, node B then claims one of them, but node A force closes before the claim is settled. Node B claims the HTLC onchain, and then we connect enough blocks to node A such that it reaches the HTLC expiry and can try to time out both HTLCs.

The failed assertion in the CI looks like a mistake. I'm not sure if changing the assertion will still preserve its original test intent.

It looks like these tests are actually exercising the new logic you've added, so they need to updated to no longer expect claiming HTLCs that have already been claimed by the counterparty. We may just be able to update these and have the coverage we need without needing to add a new test.

[whfuyn]

the test added doesn't fail when removing the fix.

thread 'ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests' panicked at lightning/src/ln/functional_tests.rs:11901:5:
HTLC already claimed by counterparty's HTLC-Success transaction
stack backtrace:
0: __rustc::rust_begin_unwind
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/std/src/panicking.rs:697:5
1: core::panicking::panic_fmt
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/panicking.rs:75:14
2: lightning::ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests
at ./src/ln/functional_tests.rs:11901:2
3: lightning::ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests::{{closure}}
at ./src/ln/functional_tests.rs:11770:58
4: core::ops::function::FnOnce::call_once
at /home/dev/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
5: core::ops::function::FnOnce::call_once
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
failures:
ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1092 filtered out; finished in 0.05s
error: test failed, to rerun pass `-p lightning --lib`

This test always fail when the fix is commented out. Are we using the same code? I've uploaded a branch.

cargo test --package lightning --lib --features _externalize_tests -- ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests --exact --show-output

node B then claims one of them,

I made a mistake in the comment. But it's the case in the code.

but node A force closes before the claim is settled

I broadcast B's commitment tx to force close. I guess this should work?

It looks like these tests are actually exercising the new logic you've added, so they need to updated to no longer expect claiming HTLCs that have already been claimed by the counterparty. We may just be able to update these and have the coverage we need without needing to add a new test.

Yes. If the existing test covers the new test, we can remove the new one.

[wpaulino]

Sorry, not sure what went wrong when I tried this yesterday. You can remove the added test and apply this diff to fix the existing ones:

diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index c862d23e1..d6227bb2b 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -1961,45 +1961,9 @@ pub fn test_htlc_on_chain_success() {
_ => panic!("Unexpected event"),
}
-	macro_rules! check_tx_local_broadcast {
- ($node: expr, $htlc_offered: expr, $commitment_tx: expr) => {{
- let mut node_txn = $node.tx_broadcaster.txn_broadcasted.lock().unwrap();
- // HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
- // remote commitment transaction.
- if $htlc_offered {
- assert_eq!(node_txn.len(), 2);
- for tx in node_txn.iter() {
- check_spends!(tx, $commitment_tx);
- assert_ne!(tx.lock_time, LockTime::ZERO);
- assert_eq!(
- tx.input[0].witness.last().unwrap().len(),
- OFFERED_HTLC_SCRIPT_WEIGHT
- );
- assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
- }
- assert_ne!(
- node_txn[0].input[0].previous_output,
- node_txn[1].input[0].previous_output
- );
- } else {
- assert_eq!(node_txn.len(), 1);
- check_spends!(node_txn[0], $commitment_tx);
- assert_ne!(node_txn[0].lock_time, LockTime::ZERO);
- assert_eq!(
- node_txn[0].input[0].witness.last().unwrap().len(),
- ACCEPTED_HTLC_SCRIPT_WEIGHT
- );
- assert!(node_txn[0].output[0].script_pubkey.is_p2wpkh()); // direct payment
- assert_ne!(
- node_txn[0].input[0].previous_output,
- node_txn[0].input[1].previous_output
- );
- }
- node_txn.clear();
- }};
-	}
-	// nodes[1] now broadcasts its own timeout-claim of the output that nodes[2] just claimed via success.
-	check_tx_local_broadcast!(nodes[1], false, commitment_tx[0]);
+	// nodes[1] does not broadcast its own timeout-claim of the output as nodes[2] just claimed it
+	// via success.
+	assert!(nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty());
// Broadcast legit commitment tx from A on B's chain
// Broadcast preimage tx by B on offered output from A commitment tx on A's chain
@@ -2061,7 +2025,17 @@ pub fn test_htlc_on_chain_success() {
_ => panic!("Unexpected event"),
}
}
-	check_tx_local_broadcast!(nodes[0], true, node_a_commitment_tx[0]);
+	// HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
+	// remote commitment transaction.
+	let mut node_txn = nodes[0].tx_broadcaster.txn_broadcast();
+	assert_eq!(node_txn.len(), 2);
+	for tx in node_txn.iter() {
+ check_spends!(tx, node_a_commitment_tx[0]);
+ assert_ne!(tx.lock_time, LockTime::ZERO);
+ assert_eq!(tx.input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
+ assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
+	}
+	assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
}
fn do_test_htlc_on_chain_timeout(connect_style: ConnectStyle) {
diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs
index cdcb570af..5382fba26 100644
--- a/lightning/src/ln/monitor_tests.rs
+++ b/lightning/src/ln/monitor_tests.rs
@@ -732,8 +732,9 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
test_spendable_output(&nodes[0], &remote_txn[0], false);
assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());
-	// After broadcasting the HTLC claim transaction, node A will still consider the HTLC
-	// possibly-claimable up to ANTI_REORG_DELAY, at which point it will drop it.
+	// After confirming the HTLC claim transaction, node A will no longer attempt to claim said
+	// HTLC, unless the transaction is reorged. However, we'll still report a
+	// `MaybeTimeoutClaimableHTLC` balance for it until we reach `ANTI_REORG_DELAY` confirmations.
mine_transaction(&nodes[0], &b_broadcast_txn[0]);
if prev_commitment_tx {
expect_payment_path_successful!(nodes[0]);
@@ -749,18 +750,10 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
// When the HTLC timeout output is spendable in the next block, A should broadcast it
connect_blocks(&nodes[0], htlc_cltv_timeout - nodes[0].best_block_info().1);
let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
-	// Aggregated claim transaction.
assert_eq!(a_broadcast_txn.len(), 1);
check_spends!(a_broadcast_txn[0], remote_txn[0]);
-	assert_eq!(a_broadcast_txn[0].input.len(), 2);
-	assert_ne!(a_broadcast_txn[0].input[0].previous_output.vout, a_broadcast_txn[0].input[1].previous_output.vout);
-	// a_broadcast_txn [0] and [1] should spend the HTLC outputs of the commitment tx
-	assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 3_000));
+	assert_eq!(a_broadcast_txn[0].input.len(), 1);
assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 4_000));
-
-	// Confirm node B's claim for node A to remove that claim from the aggregated claim transaction.
-	mine_transaction(&nodes[0], &b_broadcast_txn[0]);
-	let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
let a_htlc_timeout_tx = a_broadcast_txn.into_iter().next_back().unwrap();
// Once the HTLC-Timeout transaction confirms, A will no longer consider the HTLC

[whfuyn]

@wpaulino I've updated the commit.

[codecov]

Codecov Report

Attention: Patch coverage is 94.02985% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.81%. Comparing base (c48e0a8) to head (56834a1).
Report is 105 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/reorg_tests.rs	92.59%	3 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3860 +/- ##
==========================================
- Coverage 89.65% 88.81% -0.85% 
==========================================
Files 164 166 +2 Lines 134658 119612 -15046 Branches 134658 119612 -15046 ==========================================
- Hits 120734 106231 -14503 + Misses 11246 11045 -201 + Partials 2678 2336 -342 

☔ 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.

[whfuyn]

Uh, another bug found by the reorg test.
Because package_locktime is lower-bounded by the given height, if we pass the current height to it,
the package will be added to locktimed_packages no matter what actual locktime it requires.
That's why we need test coverage. :(

[TheBlueMatt]

Ha, nice. I jumped the gun and was also writing a test, which led to an unrelated bug in reorg handling. I hadn't gotten as far as figuring out why the <= change wasn't passing all tests, though, thanks for handling that!

The test I had written I think covers all the cases yours does plus a few more, so I opened #3923 with your commit here with the test removed, and added a separate test at the end. Let me know what you think of that PR.

[whfuyn]

Great! Feel free to close this one when ready.

[TheBlueMatt]

Supersceded by #3923

[whfuyn]

We have to prune lockedtime packages when their inputs are spent, otherwise the notification of the watched outputs might be missed. This can lead to lockedtime packages with spent inputs being added back to the pending claim requests in the future, and never being cleaned up.

Resolves: #3859

[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.

[wpaulino]

Thanks! This could really use some test coverage exercising the bug you ran into, and also handling the reorg case where the locktimed package gets restored.

[wpaulino]

Since we only aggregate locktimed_packages with the same locktime, I think the original package would still have the same one, otherwise we'd have to reinsert it. Let's add a debug_assert that it's true.

[whfuyn]

Do you mean the locktime in the key of locktime_packages equal to the locktime calculated from the package?
The calculated locktime may depend on cur_height. I'm not sure if it's still the same here.

[whfuyn]

Hi @wpaulino , I've added a test to reproduce the problem.
Without this patch, the node will try to timeout an HTLC that has already been claimed by the counterparty. And since this HTLC is in the same package as another unclaimed HTLC, it also prevents that HTLC from being timed out.
The explanation in the issue seems over-complicated. As long as HTLCs are in the locktimed packages, they will miss their notification.

The failed assertion in the CI looks like a mistake. I'm not sure if changing the assertion will still preserve its original test intent.

[whfuyn]

Without this patch, the node will try to timeout an HTLC that has already been claimed by the counterparty. And since this HTLC is in the same package as another unclaimed HTLC, it also prevents that HTLC from being timed out.

This is also the root cause of the repeated fee bump failures in #3831.

[whfuyn]

This is the logs for test_claim_value_force_close (without this patch). We can see that the claiming tx for vout 1 (f7be78d..) is confirmed on node 0, but node 0 still tries to timeout vout 1.

node 1 DEBUG [lightning::chain::onchaintx:761] Updating claims view at height 11 with 2 claim requests
node 1 DEBUG [lightning::chain::package:1294] Adding claiming input for outpoint 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1
node 1 INFO [lightning::chain::onchaintx:853] Broadcasting onchain HTLC-success tx with txid f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258
node 1 INFO [lightning::chain::onchaintx:890] Registering claiming request for 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1
node 1 DEBUG [lightning::chain::package:1294] Adding claiming input for outpoint 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:2
node 1 INFO [lightning::chain::onchaintx:853] Broadcasting onchain HTLC-success tx with txid 0d6d40442723ff9a55b3c17cedac0a3047cc243eb69822aa132bb4a1e0cdc1de
node 1 INFO [lightning::chain::onchaintx:890] Registering claiming request for 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:2
node 0 TRACE [lightning::chain::channelmonitor:4658] Transaction f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258 confirmed in block d6b71222ff0b4fa0099f7e3bf09f3f1ba3c7d7da3661513d16a37feb29d80b7f
node 0 INFO [lightning::chain::channelmonitor:5284] Input spending counterparty commitment tx (1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1) in f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258 resolves outbound HTLC with payment hash 66687aadf862bd776c8fc18b8e9f8e20089714856ee233b3902a591d0d5f2925 with preimage
[lightning/src/ln/monitor_tests.rs:755:2] &a_broadcast_txn[0].input = [
TxIn {
previous_output: OutPoint {
txid: 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177,
vout: 1,
},
...
},
TxIn {
previous_output: OutPoint {
txid: 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177,
vout: 2,
},
...
},
]

[wpaulino]

@whfuyn the test added doesn't fail when removing the fix. To test this accurately, wouldn't we need node A to send two HTLCs with the same expiry, node B then claims one of them, but node A force closes before the claim is settled. Node B claims the HTLC onchain, and then we connect enough blocks to node A such that it reaches the HTLC expiry and can try to time out both HTLCs.

The failed assertion in the CI looks like a mistake. I'm not sure if changing the assertion will still preserve its original test intent.

It looks like these tests are actually exercising the new logic you've added, so they need to updated to no longer expect claiming HTLCs that have already been claimed by the counterparty. We may just be able to update these and have the coverage we need without needing to add a new test.

[whfuyn]

the test added doesn't fail when removing the fix.

thread 'ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests' panicked at lightning/src/ln/functional_tests.rs:11901:5:
HTLC already claimed by counterparty's HTLC-Success transaction
stack backtrace:
0: __rustc::rust_begin_unwind
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/std/src/panicking.rs:697:5
1: core::panicking::panic_fmt
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/panicking.rs:75:14
2: lightning::ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests
at ./src/ln/functional_tests.rs:11901:2
3: lightning::ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests::{{closure}}
at ./src/ln/functional_tests.rs:11770:58
4: core::ops::function::FnOnce::call_once
at /home/dev/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
5: core::ops::function::FnOnce::call_once
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
failures:
ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1092 filtered out; finished in 0.05s
error: test failed, to rerun pass `-p lightning --lib`

This test always fail when the fix is commented out. Are we using the same code? I've uploaded a branch.

cargo test --package lightning --lib --features _externalize_tests -- ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests --exact --show-output

node B then claims one of them,

I made a mistake in the comment. But it's the case in the code.

but node A force closes before the claim is settled

I broadcast B's commitment tx to force close. I guess this should work?

It looks like these tests are actually exercising the new logic you've added, so they need to updated to no longer expect claiming HTLCs that have already been claimed by the counterparty. We may just be able to update these and have the coverage we need without needing to add a new test.

Yes. If the existing test covers the new test, we can remove the new one.

[wpaulino]

Sorry, not sure what went wrong when I tried this yesterday. You can remove the added test and apply this diff to fix the existing ones:

diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index c862d23e1..d6227bb2b 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -1961,45 +1961,9 @@ pub fn test_htlc_on_chain_success() {
_ => panic!("Unexpected event"),
}
-	macro_rules! check_tx_local_broadcast {
- ($node: expr, $htlc_offered: expr, $commitment_tx: expr) => {{
- let mut node_txn = $node.tx_broadcaster.txn_broadcasted.lock().unwrap();
- // HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
- // remote commitment transaction.
- if $htlc_offered {
- assert_eq!(node_txn.len(), 2);
- for tx in node_txn.iter() {
- check_spends!(tx, $commitment_tx);
- assert_ne!(tx.lock_time, LockTime::ZERO);
- assert_eq!(
- tx.input[0].witness.last().unwrap().len(),
- OFFERED_HTLC_SCRIPT_WEIGHT
- );
- assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
- }
- assert_ne!(
- node_txn[0].input[0].previous_output,
- node_txn[1].input[0].previous_output
- );
- } else {
- assert_eq!(node_txn.len(), 1);
- check_spends!(node_txn[0], $commitment_tx);
- assert_ne!(node_txn[0].lock_time, LockTime::ZERO);
- assert_eq!(
- node_txn[0].input[0].witness.last().unwrap().len(),
- ACCEPTED_HTLC_SCRIPT_WEIGHT
- );
- assert!(node_txn[0].output[0].script_pubkey.is_p2wpkh()); // direct payment
- assert_ne!(
- node_txn[0].input[0].previous_output,
- node_txn[0].input[1].previous_output
- );
- }
- node_txn.clear();
- }};
-	}
-	// nodes[1] now broadcasts its own timeout-claim of the output that nodes[2] just claimed via success.
-	check_tx_local_broadcast!(nodes[1], false, commitment_tx[0]);
+	// nodes[1] does not broadcast its own timeout-claim of the output as nodes[2] just claimed it
+	// via success.
+	assert!(nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty());
// Broadcast legit commitment tx from A on B's chain
// Broadcast preimage tx by B on offered output from A commitment tx on A's chain
@@ -2061,7 +2025,17 @@ pub fn test_htlc_on_chain_success() {
_ => panic!("Unexpected event"),
}
}
-	check_tx_local_broadcast!(nodes[0], true, node_a_commitment_tx[0]);
+	// HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
+	// remote commitment transaction.
+	let mut node_txn = nodes[0].tx_broadcaster.txn_broadcast();
+	assert_eq!(node_txn.len(), 2);
+	for tx in node_txn.iter() {
+ check_spends!(tx, node_a_commitment_tx[0]);
+ assert_ne!(tx.lock_time, LockTime::ZERO);
+ assert_eq!(tx.input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
+ assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
+	}
+	assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
}
fn do_test_htlc_on_chain_timeout(connect_style: ConnectStyle) {
diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs
index cdcb570af..5382fba26 100644
--- a/lightning/src/ln/monitor_tests.rs
+++ b/lightning/src/ln/monitor_tests.rs
@@ -732,8 +732,9 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
test_spendable_output(&nodes[0], &remote_txn[0], false);
assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());
-	// After broadcasting the HTLC claim transaction, node A will still consider the HTLC
-	// possibly-claimable up to ANTI_REORG_DELAY, at which point it will drop it.
+	// After confirming the HTLC claim transaction, node A will no longer attempt to claim said
+	// HTLC, unless the transaction is reorged. However, we'll still report a
+	// `MaybeTimeoutClaimableHTLC` balance for it until we reach `ANTI_REORG_DELAY` confirmations.
mine_transaction(&nodes[0], &b_broadcast_txn[0]);
if prev_commitment_tx {
expect_payment_path_successful!(nodes[0]);
@@ -749,18 +750,10 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
// When the HTLC timeout output is spendable in the next block, A should broadcast it
connect_blocks(&nodes[0], htlc_cltv_timeout - nodes[0].best_block_info().1);
let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
-	// Aggregated claim transaction.
assert_eq!(a_broadcast_txn.len(), 1);
check_spends!(a_broadcast_txn[0], remote_txn[0]);
-	assert_eq!(a_broadcast_txn[0].input.len(), 2);
-	assert_ne!(a_broadcast_txn[0].input[0].previous_output.vout, a_broadcast_txn[0].input[1].previous_output.vout);
-	// a_broadcast_txn [0] and [1] should spend the HTLC outputs of the commitment tx
-	assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 3_000));
+	assert_eq!(a_broadcast_txn[0].input.len(), 1);
assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 4_000));
-
-	// Confirm node B's claim for node A to remove that claim from the aggregated claim transaction.
-	mine_transaction(&nodes[0], &b_broadcast_txn[0]);
-	let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
let a_htlc_timeout_tx = a_broadcast_txn.into_iter().next_back().unwrap();
// Once the HTLC-Timeout transaction confirms, A will no longer consider the HTLC

[whfuyn]

@wpaulino I've updated the commit.

[codecov]

Codecov Report

Attention: Patch coverage is 94.02985% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.81%. Comparing base (c48e0a8) to head (56834a1).
Report is 105 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/reorg_tests.rs	92.59%	3 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3860 +/- ##
==========================================
- Coverage 89.65% 88.81% -0.85% 
==========================================
Files 164 166 +2 Lines 134658 119612 -15046 Branches 134658 119612 -15046 ==========================================
- Hits 120734 106231 -14503 + Misses 11246 11045 -201 + Partials 2678 2336 -342 

☔ 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.

[whfuyn]

Uh, another bug found by the reorg test.
Because package_locktime is lower-bounded by the given height, if we pass the current height to it,
the package will be added to locktimed_packages no matter what actual locktime it requires.
That's why we need test coverage. :(

[TheBlueMatt]

Ha, nice. I jumped the gun and was also writing a test, which led to an unrelated bug in reorg handling. I hadn't gotten as far as figuring out why the <= change wasn't passing all tests, though, thanks for handling that!

The test I had written I think covers all the cases yours does plus a few more, so I opened #3923 with your commit here with the test removed, and added a separate test at the end. Let me know what you think of that PR.

[whfuyn]

Great! Feel free to close this one when ready.

[TheBlueMatt]

Supersceded by #3923

[whfuyn]

We have to prune lockedtime packages when their inputs are spent, otherwise the notification of the watched outputs might be missed. This can lead to lockedtime packages with spent inputs being added back to the pending claim requests in the future, and never being cleaned up.

Resolves: #3859

[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.

[wpaulino]

Thanks! This could really use some test coverage exercising the bug you ran into, and also handling the reorg case where the locktimed package gets restored.

[wpaulino]

Since we only aggregate locktimed_packages with the same locktime, I think the original package would still have the same one, otherwise we'd have to reinsert it. Let's add a debug_assert that it's true.

[whfuyn]

Do you mean the locktime in the key of locktime_packages equal to the locktime calculated from the package?
The calculated locktime may depend on cur_height. I'm not sure if it's still the same here.

[whfuyn]

Hi @wpaulino , I've added a test to reproduce the problem.
Without this patch, the node will try to timeout an HTLC that has already been claimed by the counterparty. And since this HTLC is in the same package as another unclaimed HTLC, it also prevents that HTLC from being timed out.
The explanation in the issue seems over-complicated. As long as HTLCs are in the locktimed packages, they will miss their notification.

The failed assertion in the CI looks like a mistake. I'm not sure if changing the assertion will still preserve its original test intent.

[whfuyn]

Without this patch, the node will try to timeout an HTLC that has already been claimed by the counterparty. And since this HTLC is in the same package as another unclaimed HTLC, it also prevents that HTLC from being timed out.

This is also the root cause of the repeated fee bump failures in #3831.

[whfuyn]

This is the logs for test_claim_value_force_close (without this patch). We can see that the claiming tx for vout 1 (f7be78d..) is confirmed on node 0, but node 0 still tries to timeout vout 1.

node 1 DEBUG [lightning::chain::onchaintx:761] Updating claims view at height 11 with 2 claim requests
node 1 DEBUG [lightning::chain::package:1294] Adding claiming input for outpoint 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1
node 1 INFO [lightning::chain::onchaintx:853] Broadcasting onchain HTLC-success tx with txid f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258
node 1 INFO [lightning::chain::onchaintx:890] Registering claiming request for 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1
node 1 DEBUG [lightning::chain::package:1294] Adding claiming input for outpoint 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:2
node 1 INFO [lightning::chain::onchaintx:853] Broadcasting onchain HTLC-success tx with txid 0d6d40442723ff9a55b3c17cedac0a3047cc243eb69822aa132bb4a1e0cdc1de
node 1 INFO [lightning::chain::onchaintx:890] Registering claiming request for 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:2
node 0 TRACE [lightning::chain::channelmonitor:4658] Transaction f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258 confirmed in block d6b71222ff0b4fa0099f7e3bf09f3f1ba3c7d7da3661513d16a37feb29d80b7f
node 0 INFO [lightning::chain::channelmonitor:5284] Input spending counterparty commitment tx (1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1) in f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258 resolves outbound HTLC with payment hash 66687aadf862bd776c8fc18b8e9f8e20089714856ee233b3902a591d0d5f2925 with preimage
[lightning/src/ln/monitor_tests.rs:755:2] &a_broadcast_txn[0].input = [
TxIn {
previous_output: OutPoint {
txid: 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177,
vout: 1,
},
...
},
TxIn {
previous_output: OutPoint {
txid: 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177,
vout: 2,
},
...
},
]

[wpaulino]

@whfuyn the test added doesn't fail when removing the fix. To test this accurately, wouldn't we need node A to send two HTLCs with the same expiry, node B then claims one of them, but node A force closes before the claim is settled. Node B claims the HTLC onchain, and then we connect enough blocks to node A such that it reaches the HTLC expiry and can try to time out both HTLCs.

The failed assertion in the CI looks like a mistake. I'm not sure if changing the assertion will still preserve its original test intent.

It looks like these tests are actually exercising the new logic you've added, so they need to updated to no longer expect claiming HTLCs that have already been claimed by the counterparty. We may just be able to update these and have the coverage we need without needing to add a new test.

[whfuyn]

the test added doesn't fail when removing the fix.

thread 'ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests' panicked at lightning/src/ln/functional_tests.rs:11901:5:
HTLC already claimed by counterparty's HTLC-Success transaction
stack backtrace:
0: __rustc::rust_begin_unwind
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/std/src/panicking.rs:697:5
1: core::panicking::panic_fmt
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/panicking.rs:75:14
2: lightning::ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests
at ./src/ln/functional_tests.rs:11901:2
3: lightning::ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests::{{closure}}
at ./src/ln/functional_tests.rs:11770:58
4: core::ops::function::FnOnce::call_once
at /home/dev/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
5: core::ops::function::FnOnce::call_once
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
failures:
ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1092 filtered out; finished in 0.05s
error: test failed, to rerun pass `-p lightning --lib`

This test always fail when the fix is commented out. Are we using the same code? I've uploaded a branch.

cargo test --package lightning --lib --features _externalize_tests -- ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests --exact --show-output

node B then claims one of them,

I made a mistake in the comment. But it's the case in the code.

but node A force closes before the claim is settled

I broadcast B's commitment tx to force close. I guess this should work?

It looks like these tests are actually exercising the new logic you've added, so they need to updated to no longer expect claiming HTLCs that have already been claimed by the counterparty. We may just be able to update these and have the coverage we need without needing to add a new test.

Yes. If the existing test covers the new test, we can remove the new one.

[wpaulino]

Sorry, not sure what went wrong when I tried this yesterday. You can remove the added test and apply this diff to fix the existing ones:

diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index c862d23e1..d6227bb2b 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -1961,45 +1961,9 @@ pub fn test_htlc_on_chain_success() {
_ => panic!("Unexpected event"),
}
-	macro_rules! check_tx_local_broadcast {
- ($node: expr, $htlc_offered: expr, $commitment_tx: expr) => {{
- let mut node_txn = $node.tx_broadcaster.txn_broadcasted.lock().unwrap();
- // HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
- // remote commitment transaction.
- if $htlc_offered {
- assert_eq!(node_txn.len(), 2);
- for tx in node_txn.iter() {
- check_spends!(tx, $commitment_tx);
- assert_ne!(tx.lock_time, LockTime::ZERO);
- assert_eq!(
- tx.input[0].witness.last().unwrap().len(),
- OFFERED_HTLC_SCRIPT_WEIGHT
- );
- assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
- }
- assert_ne!(
- node_txn[0].input[0].previous_output,
- node_txn[1].input[0].previous_output
- );
- } else {
- assert_eq!(node_txn.len(), 1);
- check_spends!(node_txn[0], $commitment_tx);
- assert_ne!(node_txn[0].lock_time, LockTime::ZERO);
- assert_eq!(
- node_txn[0].input[0].witness.last().unwrap().len(),
- ACCEPTED_HTLC_SCRIPT_WEIGHT
- );
- assert!(node_txn[0].output[0].script_pubkey.is_p2wpkh()); // direct payment
- assert_ne!(
- node_txn[0].input[0].previous_output,
- node_txn[0].input[1].previous_output
- );
- }
- node_txn.clear();
- }};
-	}
-	// nodes[1] now broadcasts its own timeout-claim of the output that nodes[2] just claimed via success.
-	check_tx_local_broadcast!(nodes[1], false, commitment_tx[0]);
+	// nodes[1] does not broadcast its own timeout-claim of the output as nodes[2] just claimed it
+	// via success.
+	assert!(nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty());
// Broadcast legit commitment tx from A on B's chain
// Broadcast preimage tx by B on offered output from A commitment tx on A's chain
@@ -2061,7 +2025,17 @@ pub fn test_htlc_on_chain_success() {
_ => panic!("Unexpected event"),
}
}
-	check_tx_local_broadcast!(nodes[0], true, node_a_commitment_tx[0]);
+	// HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
+	// remote commitment transaction.
+	let mut node_txn = nodes[0].tx_broadcaster.txn_broadcast();
+	assert_eq!(node_txn.len(), 2);
+	for tx in node_txn.iter() {
+ check_spends!(tx, node_a_commitment_tx[0]);
+ assert_ne!(tx.lock_time, LockTime::ZERO);
+ assert_eq!(tx.input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
+ assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
+	}
+	assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
}
fn do_test_htlc_on_chain_timeout(connect_style: ConnectStyle) {
diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs
index cdcb570af..5382fba26 100644
--- a/lightning/src/ln/monitor_tests.rs
+++ b/lightning/src/ln/monitor_tests.rs
@@ -732,8 +732,9 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
test_spendable_output(&nodes[0], &remote_txn[0], false);
assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());
-	// After broadcasting the HTLC claim transaction, node A will still consider the HTLC
-	// possibly-claimable up to ANTI_REORG_DELAY, at which point it will drop it.
+	// After confirming the HTLC claim transaction, node A will no longer attempt to claim said
+	// HTLC, unless the transaction is reorged. However, we'll still report a
+	// `MaybeTimeoutClaimableHTLC` balance for it until we reach `ANTI_REORG_DELAY` confirmations.
mine_transaction(&nodes[0], &b_broadcast_txn[0]);
if prev_commitment_tx {
expect_payment_path_successful!(nodes[0]);
@@ -749,18 +750,10 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
// When the HTLC timeout output is spendable in the next block, A should broadcast it
connect_blocks(&nodes[0], htlc_cltv_timeout - nodes[0].best_block_info().1);
let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
-	// Aggregated claim transaction.
assert_eq!(a_broadcast_txn.len(), 1);
check_spends!(a_broadcast_txn[0], remote_txn[0]);
-	assert_eq!(a_broadcast_txn[0].input.len(), 2);
-	assert_ne!(a_broadcast_txn[0].input[0].previous_output.vout, a_broadcast_txn[0].input[1].previous_output.vout);
-	// a_broadcast_txn [0] and [1] should spend the HTLC outputs of the commitment tx
-	assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 3_000));
+	assert_eq!(a_broadcast_txn[0].input.len(), 1);
assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 4_000));
-
-	// Confirm node B's claim for node A to remove that claim from the aggregated claim transaction.
-	mine_transaction(&nodes[0], &b_broadcast_txn[0]);
-	let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
let a_htlc_timeout_tx = a_broadcast_txn.into_iter().next_back().unwrap();
// Once the HTLC-Timeout transaction confirms, A will no longer consider the HTLC

[whfuyn]

@wpaulino I've updated the commit.

[codecov]

Codecov Report

Attention: Patch coverage is 94.02985% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.81%. Comparing base (c48e0a8) to head (56834a1).
Report is 105 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/reorg_tests.rs	92.59%	3 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3860 +/- ##
==========================================
- Coverage 89.65% 88.81% -0.85% 
==========================================
Files 164 166 +2 Lines 134658 119612 -15046 Branches 134658 119612 -15046 ==========================================
- Hits 120734 106231 -14503 + Misses 11246 11045 -201 + Partials 2678 2336 -342 

☔ 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.

[whfuyn]

Uh, another bug found by the reorg test.
Because package_locktime is lower-bounded by the given height, if we pass the current height to it,
the package will be added to locktimed_packages no matter what actual locktime it requires.
That's why we need test coverage. :(

[TheBlueMatt]

Ha, nice. I jumped the gun and was also writing a test, which led to an unrelated bug in reorg handling. I hadn't gotten as far as figuring out why the <= change wasn't passing all tests, though, thanks for handling that!

The test I had written I think covers all the cases yours does plus a few more, so I opened #3923 with your commit here with the test removed, and added a separate test at the end. Let me know what you think of that PR.

[whfuyn]

Great! Feel free to close this one when ready.

[TheBlueMatt]

Supersceded by #3923

[whfuyn]

We have to prune lockedtime packages when their inputs are spent, otherwise the notification of the watched outputs might be missed. This can lead to lockedtime packages with spent inputs being added back to the pending claim requests in the future, and never being cleaned up.

Resolves: #3859

[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.

[wpaulino]

Thanks! This could really use some test coverage exercising the bug you ran into, and also handling the reorg case where the locktimed package gets restored.

[wpaulino]

Since we only aggregate locktimed_packages with the same locktime, I think the original package would still have the same one, otherwise we'd have to reinsert it. Let's add a debug_assert that it's true.

[whfuyn]

Do you mean the locktime in the key of locktime_packages equal to the locktime calculated from the package?
The calculated locktime may depend on cur_height. I'm not sure if it's still the same here.

[whfuyn]

Hi @wpaulino , I've added a test to reproduce the problem.
Without this patch, the node will try to timeout an HTLC that has already been claimed by the counterparty. And since this HTLC is in the same package as another unclaimed HTLC, it also prevents that HTLC from being timed out.
The explanation in the issue seems over-complicated. As long as HTLCs are in the locktimed packages, they will miss their notification.

The failed assertion in the CI looks like a mistake. I'm not sure if changing the assertion will still preserve its original test intent.

[whfuyn]

Without this patch, the node will try to timeout an HTLC that has already been claimed by the counterparty. And since this HTLC is in the same package as another unclaimed HTLC, it also prevents that HTLC from being timed out.

This is also the root cause of the repeated fee bump failures in #3831.

[whfuyn]

This is the logs for test_claim_value_force_close (without this patch). We can see that the claiming tx for vout 1 (f7be78d..) is confirmed on node 0, but node 0 still tries to timeout vout 1.

node 1 DEBUG [lightning::chain::onchaintx:761] Updating claims view at height 11 with 2 claim requests
node 1 DEBUG [lightning::chain::package:1294] Adding claiming input for outpoint 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1
node 1 INFO [lightning::chain::onchaintx:853] Broadcasting onchain HTLC-success tx with txid f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258
node 1 INFO [lightning::chain::onchaintx:890] Registering claiming request for 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1
node 1 DEBUG [lightning::chain::package:1294] Adding claiming input for outpoint 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:2
node 1 INFO [lightning::chain::onchaintx:853] Broadcasting onchain HTLC-success tx with txid 0d6d40442723ff9a55b3c17cedac0a3047cc243eb69822aa132bb4a1e0cdc1de
node 1 INFO [lightning::chain::onchaintx:890] Registering claiming request for 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:2
node 0 TRACE [lightning::chain::channelmonitor:4658] Transaction f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258 confirmed in block d6b71222ff0b4fa0099f7e3bf09f3f1ba3c7d7da3661513d16a37feb29d80b7f
node 0 INFO [lightning::chain::channelmonitor:5284] Input spending counterparty commitment tx (1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1) in f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258 resolves outbound HTLC with payment hash 66687aadf862bd776c8fc18b8e9f8e20089714856ee233b3902a591d0d5f2925 with preimage
[lightning/src/ln/monitor_tests.rs:755:2] &a_broadcast_txn[0].input = [
TxIn {
previous_output: OutPoint {
txid: 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177,
vout: 1,
},
...
},
TxIn {
previous_output: OutPoint {
txid: 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177,
vout: 2,
},
...
},
]

[wpaulino]

@whfuyn the test added doesn't fail when removing the fix. To test this accurately, wouldn't we need node A to send two HTLCs with the same expiry, node B then claims one of them, but node A force closes before the claim is settled. Node B claims the HTLC onchain, and then we connect enough blocks to node A such that it reaches the HTLC expiry and can try to time out both HTLCs.

The failed assertion in the CI looks like a mistake. I'm not sure if changing the assertion will still preserve its original test intent.

It looks like these tests are actually exercising the new logic you've added, so they need to updated to no longer expect claiming HTLCs that have already been claimed by the counterparty. We may just be able to update these and have the coverage we need without needing to add a new test.

[whfuyn]

the test added doesn't fail when removing the fix.

thread 'ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests' panicked at lightning/src/ln/functional_tests.rs:11901:5:
HTLC already claimed by counterparty's HTLC-Success transaction
stack backtrace:
0: __rustc::rust_begin_unwind
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/std/src/panicking.rs:697:5
1: core::panicking::panic_fmt
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/panicking.rs:75:14
2: lightning::ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests
at ./src/ln/functional_tests.rs:11901:2
3: lightning::ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests::{{closure}}
at ./src/ln/functional_tests.rs:11770:58
4: core::ops::function::FnOnce::call_once
at /home/dev/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
5: core::ops::function::FnOnce::call_once
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
failures:
ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1092 filtered out; finished in 0.05s
error: test failed, to rerun pass `-p lightning --lib`

This test always fail when the fix is commented out. Are we using the same code? I've uploaded a branch.

cargo test --package lightning --lib --features _externalize_tests -- ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests --exact --show-output

node B then claims one of them,

I made a mistake in the comment. But it's the case in the code.

but node A force closes before the claim is settled

I broadcast B's commitment tx to force close. I guess this should work?

It looks like these tests are actually exercising the new logic you've added, so they need to updated to no longer expect claiming HTLCs that have already been claimed by the counterparty. We may just be able to update these and have the coverage we need without needing to add a new test.

Yes. If the existing test covers the new test, we can remove the new one.

[wpaulino]

Sorry, not sure what went wrong when I tried this yesterday. You can remove the added test and apply this diff to fix the existing ones:

diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index c862d23e1..d6227bb2b 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -1961,45 +1961,9 @@ pub fn test_htlc_on_chain_success() {
_ => panic!("Unexpected event"),
}
-	macro_rules! check_tx_local_broadcast {
- ($node: expr, $htlc_offered: expr, $commitment_tx: expr) => {{
- let mut node_txn = $node.tx_broadcaster.txn_broadcasted.lock().unwrap();
- // HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
- // remote commitment transaction.
- if $htlc_offered {
- assert_eq!(node_txn.len(), 2);
- for tx in node_txn.iter() {
- check_spends!(tx, $commitment_tx);
- assert_ne!(tx.lock_time, LockTime::ZERO);
- assert_eq!(
- tx.input[0].witness.last().unwrap().len(),
- OFFERED_HTLC_SCRIPT_WEIGHT
- );
- assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
- }
- assert_ne!(
- node_txn[0].input[0].previous_output,
- node_txn[1].input[0].previous_output
- );
- } else {
- assert_eq!(node_txn.len(), 1);
- check_spends!(node_txn[0], $commitment_tx);
- assert_ne!(node_txn[0].lock_time, LockTime::ZERO);
- assert_eq!(
- node_txn[0].input[0].witness.last().unwrap().len(),
- ACCEPTED_HTLC_SCRIPT_WEIGHT
- );
- assert!(node_txn[0].output[0].script_pubkey.is_p2wpkh()); // direct payment
- assert_ne!(
- node_txn[0].input[0].previous_output,
- node_txn[0].input[1].previous_output
- );
- }
- node_txn.clear();
- }};
-	}
-	// nodes[1] now broadcasts its own timeout-claim of the output that nodes[2] just claimed via success.
-	check_tx_local_broadcast!(nodes[1], false, commitment_tx[0]);
+	// nodes[1] does not broadcast its own timeout-claim of the output as nodes[2] just claimed it
+	// via success.
+	assert!(nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty());
// Broadcast legit commitment tx from A on B's chain
// Broadcast preimage tx by B on offered output from A commitment tx on A's chain
@@ -2061,7 +2025,17 @@ pub fn test_htlc_on_chain_success() {
_ => panic!("Unexpected event"),
}
}
-	check_tx_local_broadcast!(nodes[0], true, node_a_commitment_tx[0]);
+	// HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
+	// remote commitment transaction.
+	let mut node_txn = nodes[0].tx_broadcaster.txn_broadcast();
+	assert_eq!(node_txn.len(), 2);
+	for tx in node_txn.iter() {
+ check_spends!(tx, node_a_commitment_tx[0]);
+ assert_ne!(tx.lock_time, LockTime::ZERO);
+ assert_eq!(tx.input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
+ assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
+	}
+	assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
}
fn do_test_htlc_on_chain_timeout(connect_style: ConnectStyle) {
diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs
index cdcb570af..5382fba26 100644
--- a/lightning/src/ln/monitor_tests.rs
+++ b/lightning/src/ln/monitor_tests.rs
@@ -732,8 +732,9 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
test_spendable_output(&nodes[0], &remote_txn[0], false);
assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());
-	// After broadcasting the HTLC claim transaction, node A will still consider the HTLC
-	// possibly-claimable up to ANTI_REORG_DELAY, at which point it will drop it.
+	// After confirming the HTLC claim transaction, node A will no longer attempt to claim said
+	// HTLC, unless the transaction is reorged. However, we'll still report a
+	// `MaybeTimeoutClaimableHTLC` balance for it until we reach `ANTI_REORG_DELAY` confirmations.
mine_transaction(&nodes[0], &b_broadcast_txn[0]);
if prev_commitment_tx {
expect_payment_path_successful!(nodes[0]);
@@ -749,18 +750,10 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
// When the HTLC timeout output is spendable in the next block, A should broadcast it
connect_blocks(&nodes[0], htlc_cltv_timeout - nodes[0].best_block_info().1);
let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
-	// Aggregated claim transaction.
assert_eq!(a_broadcast_txn.len(), 1);
check_spends!(a_broadcast_txn[0], remote_txn[0]);
-	assert_eq!(a_broadcast_txn[0].input.len(), 2);
-	assert_ne!(a_broadcast_txn[0].input[0].previous_output.vout, a_broadcast_txn[0].input[1].previous_output.vout);
-	// a_broadcast_txn [0] and [1] should spend the HTLC outputs of the commitment tx
-	assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 3_000));
+	assert_eq!(a_broadcast_txn[0].input.len(), 1);
assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 4_000));
-
-	// Confirm node B's claim for node A to remove that claim from the aggregated claim transaction.
-	mine_transaction(&nodes[0], &b_broadcast_txn[0]);
-	let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
let a_htlc_timeout_tx = a_broadcast_txn.into_iter().next_back().unwrap();
// Once the HTLC-Timeout transaction confirms, A will no longer consider the HTLC

[whfuyn]

@wpaulino I've updated the commit.

[codecov]

Codecov Report

Attention: Patch coverage is 94.02985% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.81%. Comparing base (c48e0a8) to head (56834a1).
Report is 105 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/reorg_tests.rs	92.59%	3 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3860 +/- ##
==========================================
- Coverage 89.65% 88.81% -0.85% 
==========================================
Files 164 166 +2 Lines 134658 119612 -15046 Branches 134658 119612 -15046 ==========================================
- Hits 120734 106231 -14503 + Misses 11246 11045 -201 + Partials 2678 2336 -342 

☔ 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.

[whfuyn]

Uh, another bug found by the reorg test.
Because package_locktime is lower-bounded by the given height, if we pass the current height to it,
the package will be added to locktimed_packages no matter what actual locktime it requires.
That's why we need test coverage. :(

[TheBlueMatt]

Ha, nice. I jumped the gun and was also writing a test, which led to an unrelated bug in reorg handling. I hadn't gotten as far as figuring out why the <= change wasn't passing all tests, though, thanks for handling that!

The test I had written I think covers all the cases yours does plus a few more, so I opened #3923 with your commit here with the test removed, and added a separate test at the end. Let me know what you think of that PR.

[whfuyn]

Great! Feel free to close this one when ready.

[TheBlueMatt]

Supersceded by #3923

[whfuyn]

We have to prune lockedtime packages when their inputs are spent, otherwise the notification of the watched outputs might be missed. This can lead to lockedtime packages with spent inputs being added back to the pending claim requests in the future, and never being cleaned up.

Resolves: #3859

[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.

[wpaulino]

Thanks! This could really use some test coverage exercising the bug you ran into, and also handling the reorg case where the locktimed package gets restored.

[wpaulino]

Since we only aggregate locktimed_packages with the same locktime, I think the original package would still have the same one, otherwise we'd have to reinsert it. Let's add a debug_assert that it's true.

[whfuyn]

Do you mean the locktime in the key of locktime_packages equal to the locktime calculated from the package?
The calculated locktime may depend on cur_height. I'm not sure if it's still the same here.

[whfuyn]

Hi @wpaulino , I've added a test to reproduce the problem.
Without this patch, the node will try to timeout an HTLC that has already been claimed by the counterparty. And since this HTLC is in the same package as another unclaimed HTLC, it also prevents that HTLC from being timed out.
The explanation in the issue seems over-complicated. As long as HTLCs are in the locktimed packages, they will miss their notification.

The failed assertion in the CI looks like a mistake. I'm not sure if changing the assertion will still preserve its original test intent.

[whfuyn]

Without this patch, the node will try to timeout an HTLC that has already been claimed by the counterparty. And since this HTLC is in the same package as another unclaimed HTLC, it also prevents that HTLC from being timed out.

This is also the root cause of the repeated fee bump failures in #3831.

[whfuyn]

This is the logs for test_claim_value_force_close (without this patch). We can see that the claiming tx for vout 1 (f7be78d..) is confirmed on node 0, but node 0 still tries to timeout vout 1.

node 1 DEBUG [lightning::chain::onchaintx:761] Updating claims view at height 11 with 2 claim requests
node 1 DEBUG [lightning::chain::package:1294] Adding claiming input for outpoint 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1
node 1 INFO [lightning::chain::onchaintx:853] Broadcasting onchain HTLC-success tx with txid f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258
node 1 INFO [lightning::chain::onchaintx:890] Registering claiming request for 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1
node 1 DEBUG [lightning::chain::package:1294] Adding claiming input for outpoint 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:2
node 1 INFO [lightning::chain::onchaintx:853] Broadcasting onchain HTLC-success tx with txid 0d6d40442723ff9a55b3c17cedac0a3047cc243eb69822aa132bb4a1e0cdc1de
node 1 INFO [lightning::chain::onchaintx:890] Registering claiming request for 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:2
node 0 TRACE [lightning::chain::channelmonitor:4658] Transaction f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258 confirmed in block d6b71222ff0b4fa0099f7e3bf09f3f1ba3c7d7da3661513d16a37feb29d80b7f
node 0 INFO [lightning::chain::channelmonitor:5284] Input spending counterparty commitment tx (1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177:1) in f7be78d5eb3eb0ad6b047eec9dd4b06bd38772ab385961657de5f2dc7a427258 resolves outbound HTLC with payment hash 66687aadf862bd776c8fc18b8e9f8e20089714856ee233b3902a591d0d5f2925 with preimage
[lightning/src/ln/monitor_tests.rs:755:2] &a_broadcast_txn[0].input = [
TxIn {
previous_output: OutPoint {
txid: 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177,
vout: 1,
},
...
},
TxIn {
previous_output: OutPoint {
txid: 1808a46d743c61bf78d7a28347a6175f87a12a2977070590bae2624f87966177,
vout: 2,
},
...
},
]

[wpaulino]

@whfuyn the test added doesn't fail when removing the fix. To test this accurately, wouldn't we need node A to send two HTLCs with the same expiry, node B then claims one of them, but node A force closes before the claim is settled. Node B claims the HTLC onchain, and then we connect enough blocks to node A such that it reaches the HTLC expiry and can try to time out both HTLCs.

The failed assertion in the CI looks like a mistake. I'm not sure if changing the assertion will still preserve its original test intent.

It looks like these tests are actually exercising the new logic you've added, so they need to updated to no longer expect claiming HTLCs that have already been claimed by the counterparty. We may just be able to update these and have the coverage we need without needing to add a new test.

[whfuyn]

the test added doesn't fail when removing the fix.

thread 'ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests' panicked at lightning/src/ln/functional_tests.rs:11901:5:
HTLC already claimed by counterparty's HTLC-Success transaction
stack backtrace:
0: __rustc::rust_begin_unwind
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/std/src/panicking.rs:697:5
1: core::panicking::panic_fmt
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/panicking.rs:75:14
2: lightning::ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests
at ./src/ln/functional_tests.rs:11901:2
3: lightning::ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests::{{closure}}
at ./src/ln/functional_tests.rs:11770:58
4: core::ops::function::FnOnce::call_once
at /home/dev/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
5: core::ops::function::FnOnce::call_once
at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
failures:
ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1092 filtered out; finished in 0.05s
error: test failed, to rerun pass `-p lightning --lib`

This test always fail when the fix is commented out. Are we using the same code? I've uploaded a branch.

cargo test --package lightning --lib --features _externalize_tests -- ln::functional_tests::test_claimed_htlcs_not_in_pending_claim_requests --exact --show-output

node B then claims one of them,

I made a mistake in the comment. But it's the case in the code.

but node A force closes before the claim is settled

I broadcast B's commitment tx to force close. I guess this should work?

It looks like these tests are actually exercising the new logic you've added, so they need to updated to no longer expect claiming HTLCs that have already been claimed by the counterparty. We may just be able to update these and have the coverage we need without needing to add a new test.

Yes. If the existing test covers the new test, we can remove the new one.

[wpaulino]

Sorry, not sure what went wrong when I tried this yesterday. You can remove the added test and apply this diff to fix the existing ones:

diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index c862d23e1..d6227bb2b 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -1961,45 +1961,9 @@ pub fn test_htlc_on_chain_success() {
_ => panic!("Unexpected event"),
}
-	macro_rules! check_tx_local_broadcast {
- ($node: expr, $htlc_offered: expr, $commitment_tx: expr) => {{
- let mut node_txn = $node.tx_broadcaster.txn_broadcasted.lock().unwrap();
- // HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
- // remote commitment transaction.
- if $htlc_offered {
- assert_eq!(node_txn.len(), 2);
- for tx in node_txn.iter() {
- check_spends!(tx, $commitment_tx);
- assert_ne!(tx.lock_time, LockTime::ZERO);
- assert_eq!(
- tx.input[0].witness.last().unwrap().len(),
- OFFERED_HTLC_SCRIPT_WEIGHT
- );
- assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
- }
- assert_ne!(
- node_txn[0].input[0].previous_output,
- node_txn[1].input[0].previous_output
- );
- } else {
- assert_eq!(node_txn.len(), 1);
- check_spends!(node_txn[0], $commitment_tx);
- assert_ne!(node_txn[0].lock_time, LockTime::ZERO);
- assert_eq!(
- node_txn[0].input[0].witness.last().unwrap().len(),
- ACCEPTED_HTLC_SCRIPT_WEIGHT
- );
- assert!(node_txn[0].output[0].script_pubkey.is_p2wpkh()); // direct payment
- assert_ne!(
- node_txn[0].input[0].previous_output,
- node_txn[0].input[1].previous_output
- );
- }
- node_txn.clear();
- }};
-	}
-	// nodes[1] now broadcasts its own timeout-claim of the output that nodes[2] just claimed via success.
-	check_tx_local_broadcast!(nodes[1], false, commitment_tx[0]);
+	// nodes[1] does not broadcast its own timeout-claim of the output as nodes[2] just claimed it
+	// via success.
+	assert!(nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty());
// Broadcast legit commitment tx from A on B's chain
// Broadcast preimage tx by B on offered output from A commitment tx on A's chain
@@ -2061,7 +2025,17 @@ pub fn test_htlc_on_chain_success() {
_ => panic!("Unexpected event"),
}
}
-	check_tx_local_broadcast!(nodes[0], true, node_a_commitment_tx[0]);
+	// HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
+	// remote commitment transaction.
+	let mut node_txn = nodes[0].tx_broadcaster.txn_broadcast();
+	assert_eq!(node_txn.len(), 2);
+	for tx in node_txn.iter() {
+ check_spends!(tx, node_a_commitment_tx[0]);
+ assert_ne!(tx.lock_time, LockTime::ZERO);
+ assert_eq!(tx.input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
+ assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
+	}
+	assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
}
fn do_test_htlc_on_chain_timeout(connect_style: ConnectStyle) {
diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs
index cdcb570af..5382fba26 100644
--- a/lightning/src/ln/monitor_tests.rs
+++ b/lightning/src/ln/monitor_tests.rs
@@ -732,8 +732,9 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
test_spendable_output(&nodes[0], &remote_txn[0], false);
assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());
-	// After broadcasting the HTLC claim transaction, node A will still consider the HTLC
-	// possibly-claimable up to ANTI_REORG_DELAY, at which point it will drop it.
+	// After confirming the HTLC claim transaction, node A will no longer attempt to claim said
+	// HTLC, unless the transaction is reorged. However, we'll still report a
+	// `MaybeTimeoutClaimableHTLC` balance for it until we reach `ANTI_REORG_DELAY` confirmations.
mine_transaction(&nodes[0], &b_broadcast_txn[0]);
if prev_commitment_tx {
expect_payment_path_successful!(nodes[0]);
@@ -749,18 +750,10 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
// When the HTLC timeout output is spendable in the next block, A should broadcast it
connect_blocks(&nodes[0], htlc_cltv_timeout - nodes[0].best_block_info().1);
let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
-	// Aggregated claim transaction.
assert_eq!(a_broadcast_txn.len(), 1);
check_spends!(a_broadcast_txn[0], remote_txn[0]);
-	assert_eq!(a_broadcast_txn[0].input.len(), 2);
-	assert_ne!(a_broadcast_txn[0].input[0].previous_output.vout, a_broadcast_txn[0].input[1].previous_output.vout);
-	// a_broadcast_txn [0] and [1] should spend the HTLC outputs of the commitment tx
-	assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 3_000));
+	assert_eq!(a_broadcast_txn[0].input.len(), 1);
assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 4_000));
-
-	// Confirm node B's claim for node A to remove that claim from the aggregated claim transaction.
-	mine_transaction(&nodes[0], &b_broadcast_txn[0]);
-	let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
let a_htlc_timeout_tx = a_broadcast_txn.into_iter().next_back().unwrap();
// Once the HTLC-Timeout transaction confirms, A will no longer consider the HTLC

[whfuyn]

@wpaulino I've updated the commit.

[codecov]

Codecov Report

Attention: Patch coverage is 94.02985% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.81%. Comparing base (c48e0a8) to head (56834a1).
Report is 105 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/reorg_tests.rs	92.59%	3 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3860 +/- ##
==========================================
- Coverage 89.65% 88.81% -0.85% 
==========================================
Files 164 166 +2 Lines 134658 119612 -15046 Branches 134658 119612 -15046 ==========================================
- Hits 120734 106231 -14503 + Misses 11246 11045 -201 + Partials 2678 2336 -342 

☔ 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.

[whfuyn]

Uh, another bug found by the reorg test.
Because package_locktime is lower-bounded by the given height, if we pass the current height to it,
the package will be added to locktimed_packages no matter what actual locktime it requires.
That's why we need test coverage. :(

[TheBlueMatt]

Ha, nice. I jumped the gun and was also writing a test, which led to an unrelated bug in reorg handling. I hadn't gotten as far as figuring out why the <= change wasn't passing all tests, though, thanks for handling that!

The test I had written I think covers all the cases yours does plus a few more, so I opened #3923 with your commit here with the test removed, and added a separate test at the end. Let me know what you think of that PR.

[whfuyn]

Great! Feel free to close this one when ready.

[TheBlueMatt]

Supersceded by #3923