0FC tests round 1 #4089

tankyleo · 2025-09-18T16:47:43Z

Repurpose existing keyed anchor tests to p2a anchors, more to follow either in or after this PR

ldk-reviews-bot · 2025-09-18T16:47:45Z

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

codecov · 2025-09-18T17:00:49Z

Codecov Report

❌ Patch coverage is 97.72727% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.56%. Comparing base (677eec6) to head (1618c77).
⚠️ Report is 115 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/monitor_tests.rs	97.05%	7 Missing ⚠️
lightning/src/ln/functional_test_utils.rs	86.36%	3 Missing ⚠️
lightning/src/events/bump_transaction/mod.rs	95.23%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4089      +/-   ##
==========================================
- Coverage   88.69%   88.56%   -0.13%     
==========================================
  Files         176      180       +4     
  Lines      132839   134549    +1710     
  Branches   132839   134549    +1710     
==========================================
+ Hits       117816   119168    +1352     
- Misses      12330    12630     +300     
- Partials     2693     2751      +58

Flag	Coverage Δ
fuzzing	`21.23% <4.44%> (-0.32%)`	⬇️
tests	`88.40% <97.72%> (-0.13%)`	⬇️

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

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

🚀 New features to boost your workflow:

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

ldk-reviews-bot · 2025-09-20T16:57:05Z

🔔 1st Reminder

Hey @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-09-22T16:57:16Z

🔔 2nd Reminder

Hey @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

lightning/src/ln/monitor_tests.rs

carlaKC

Nice update to the tests! Consider most comments non-blocking given the Friday deadline to get this in.

Only question I have is whether we need to worry about V3 tx size constraints.

carlaKC · 2025-09-23T14:01:34Z

lightning/src/events/bump_transaction/mod.rs

 		let mut htlc_tx = Transaction {
-			version: Version::TWO,
+			version: if channel_type.supports_anchor_zero_fee_commitments() {
+				Version::non_standard(3)


This seems pretty unlikely, but is it possible that we go over the 10,000 vB rule 4 restriction on TRUC transactions if we aggregate a bunch of second stage txns together?

We don't need to worry about package limits because we're still waiting for the commitment to confirm (like we would with CSV 1 regular anchors), but this limit applies to individual V3 transactions IIUC.

If we're still waiting on the commitment to confirm I'm not sure we need to worry to much - the anchor spend on the commit should eventually get bumped enough to confirm it. What worries me more is if we aggregate HTLC claims into one transaction.

Good points thank you ! It seems the only reason why this is version 3 now is so that these HTLC tx's can be aggregated with the anchor spend. We are not doing this here, so we could revert the HTLC tx version to 2.

lightning/src/ln/chan_utils.rs

carlaKC · 2025-09-23T15:12:26Z

lightning/src/ln/functional_test_utils.rs

-			"Spending tx output didn't meet dust limit"
-		);
+		if outp.value < outp.script_pubkey.minimal_non_dust() {
+			if single_output_below_dust {


Wouldn't we also want to checkshared_anchor_script_pubkey like below?

thanks I initially had the general case in mind, but we certainly don't expect any other non-p2a outputs to be below dust. Will fix.

carlaKC · 2025-09-23T17:11:11Z

lightning/src/ln/chan_utils.rs

 				   "0020215d61bba56b19e9eadb6107f5a85d7f99c40f65992443f69229c290165bc00d");

-		// Generate broadcaster output and received and offered HTLC outputs,  with anchors
+		// Generate broadcaster output and received and offered HTLC outputs,  with keyed anchors


nit: double space after comma and on 2361

thanks ! I was keeping it consistent with existing formatting, but don't see a need, will delete the extra space everywhere.

lightning/src/ln/chan_utils.rs

carlaKC · 2025-09-23T17:33:15Z

lightning/src/ln/monitor_tests.rs

+	if keyed_anchors {
 		user_config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true;
 		user_config.manually_accept_inbound_channels = true;
+	} else if p2a_anchor {
+		user_config.channel_handshake_config.negotiate_anchor_zero_fee_commitments = true;
+		user_config.manually_accept_inbound_channels = true;


could save a few lines in a few spots:

user_config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = keyed_anchors; user_config.channel_handshake_config.negotiate_anchor_zero_fee_commitments = p2a_anchor; user_config.manually_accept_inbound_channels = keyed_anchors || p2a_anchor;

carlaKC · 2025-09-23T17:37:28Z

lightning/src/ln/monitor_tests.rs

+	do_test_lost_timeout_monitor_events(CommitmentType::RevokedCounterparty, false, false);
+	do_test_lost_timeout_monitor_events(CommitmentType::RevokedCounterparty, true, false);
+	do_test_lost_timeout_monitor_events(CommitmentType::PreviousCounterparty, false, false);
+	do_test_lost_timeout_monitor_events(CommitmentType::PreviousCounterparty, true, false);
+	do_test_lost_timeout_monitor_events(CommitmentType::LatestCounterparty, false, false);
+	do_test_lost_timeout_monitor_events(CommitmentType::LatestCounterparty, true, false);
+	do_test_lost_timeout_monitor_events(CommitmentType::LocalWithoutLastHTLC, false, false);
+	do_test_lost_timeout_monitor_events(CommitmentType::LocalWithoutLastHTLC, true, false);
+	do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, false, false);
+	do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, true, false);
+
+	do_test_lost_timeout_monitor_events(CommitmentType::RevokedCounterparty, false, true);
+	do_test_lost_timeout_monitor_events(CommitmentType::RevokedCounterparty, true, true);
+	do_test_lost_timeout_monitor_events(CommitmentType::PreviousCounterparty, false, true);
+	do_test_lost_timeout_monitor_events(CommitmentType::PreviousCounterparty, true, true);
+	do_test_lost_timeout_monitor_events(CommitmentType::LatestCounterparty, false, true);
+	do_test_lost_timeout_monitor_events(CommitmentType::LatestCounterparty, true, true);
+	do_test_lost_timeout_monitor_events(CommitmentType::LocalWithoutLastHTLC, false, true);
+	do_test_lost_timeout_monitor_events(CommitmentType::LocalWithoutLastHTLC, true, true);
+	do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, false, true);
+	do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, true, true);


DRY up a bit?

for p2a in [true, false] { do_test_lost_timeout_monitor_events(CommitmentType::RevokedCounterparty, false, p2a); ...

let me see if one of these errors fails, the backtrace would not tell us whether we failed with p2a true or false ?

carlaKC · 2025-09-23T18:21:38Z

lightning/src/ln/reorg_tests.rs

+		mine_transactions(&nodes[0], &[&commitment_tx, &anchor_tx]);
+		check_closed_broadcast(&nodes[0], 1, false);
+		let reason = ClosureReason::CommitmentTxConfirmed;
+		check_closed_event(&nodes[0], 1, reason, false, &[node_b_id], 10_000_000);
+		check_added_monitors(&nodes[0], 1);
+
+		mine_transactions(&nodes[1], &[&commitment_tx, &anchor_tx]);
+		handle_bump_events(&nodes[1], nodes[1].connect_style.borrow().updates_best_block_first(), 1);


This can be DRY-ed up a bit - both branches are the same except the list of transactions we need to mine.

carlaKC · 2025-09-23T18:47:53Z

lightning/src/ln/zero_fee_commitment_tests.rs

+	}
+
+	p2a_value_test!(1,, 1);
+	p2a_value_test!(238_000,, 238);


Perhaps we could change the macro to make invocation a little more readable - this is my best shot at it diff.

TheBlueMatt

A few assorted notes, otherwise basically lgtm.

TheBlueMatt · 2025-09-23T19:20:56Z

lightning/src/events/bump_transaction/mod.rs

+			.channel_type_features;
 		let mut htlc_tx = Transaction {
-			version: Version::TWO,
+			version: if channel_type.supports_anchor_zero_fee_commitments() {


Is this required by the counterparty's signature? Ideally we wouldn't want to do this I think, but I'm not sure if the sig covers it.

The counterparty sig does cover it, that's how I found this here :) See above comment from carla, as long as we don't aggregate HTLCs into the anchor tx, we could revert HTLC txs to V2.

TheBlueMatt · 2025-09-23T19:23:47Z

lightning/src/chain/channelmonitor.rs

 							} else if _script_pubkey.is_p2wpkh() {
 								assert_eq!(&bitcoin::Address::p2wpkh(&bitcoin::CompressedPublicKey(bitcoin::PublicKey::from_slice(&input.witness.last().unwrap()).unwrap().inner), bitcoin::Network::Bitcoin).script_pubkey(), _script_pubkey);
+							} else if _script_pubkey == &chan_utils::shared_anchor_script_pubkey() {
+								assert!(input.witness.is_empty());


Might also make sense to do this in check_spends.

TheBlueMatt · 2025-09-23T19:25:11Z

lightning/src/ln/functional_test_utils.rs

-			"Spending tx output didn't meet dust limit"
-		);
+		if outp.value < outp.script_pubkey.minimal_non_dust() {
+			if single_output_below_dust {


Should we check that the output is P2A here?

TheBlueMatt · 2025-09-23T19:25:39Z

lightning/src/ln/functional_test_utils.rs

-	assert!(total_value_out + min_fee <= total_value_in);
+	if single_output_below_dust {
+		assert_eq!(
+			total_value_in, total_value_out,


This should fail, right? The P2A output can drift above dust if we have some dust HTLCs.

this should be true; we are allowed a single output below dust as long as the transaction is zero fee.

at 240 sats, single_output_below_dust is false - now I realize i need to relax the branch below to allow for sub 1sat/vb txs if they have this 240sat output.

TheBlueMatt · 2025-09-23T19:32:05Z

lightning/src/ln/monitor_tests.rs

 		assert_eq!(timeout_htlc_txn[0].input.len(), 3);
-		assert_eq!(timeout_htlc_txn[0].input[0].witness.last().unwrap().len(), chan_utils::OFFERED_HTLC_SCRIPT_WEIGHT_ANCHORS);
-		assert_eq!(timeout_htlc_txn[0].input[1].witness.last().unwrap().len(), chan_utils::OFFERED_HTLC_SCRIPT_WEIGHT_ANCHORS);
+		assert_eq!(timeout_htlc_txn[0].input[0].witness.last().unwrap().len(), if p2a_anchor { chan_utils::OFFERED_HTLC_SCRIPT_WEIGHT } else { chan_utils::OFFERED_HTLC_SCRIPT_WEIGHT_ANCHORS });


Oh, can you update the docs on OFFERED_HTLC_SCRIPT_WEIGHT to mention 0FC?

TheBlueMatt · 2025-09-23T19:37:09Z

lightning/src/ln/zero_fee_commitment_tests.rs

+	p2a_value_test!(240_000,, 240);
+	p2a_value_test!(240_001,, 240);
+	p2a_value_test!(353_000,, 240);
+	p2a_value_test!(353_999,, 240);


Would be nice to have a test with two 353-sat HTLCs to demonstrate we never go over 240 even with multiple HTLCs.

TheBlueMatt · 2025-09-23T19:39:23Z

lightning/src/ln/chan_utils.rs

+/// The upper bound weight of an HTLC timeout input from a commitment transaction with keyed anchor outputs.
+pub const HTLC_TIMEOUT_INPUT_KEYED_ANCHOR_WITNESS_WEIGHT: u64 = 288;
+/// The upper bound weight of an HTLC timeout input from a commitment transaction with a p2a anchor output.
+pub const HTLC_TIMEOUT_INPUT_P2A_ANCHOR_WITNESS_WEIGHT: u64 = 285;


There is no coverage for this change. Tests pass if this is 288.

These are used to pass estimates of the size of witness to coin selection in BumpTransactionEventHandler::handle_htlc_resolution - they both break the 1% max overestimate there if you push them high enough.

lightning/src/events/bump_transaction/mod.rs

tankyleo · 2025-09-24T01:08:19Z

In the very last commit, I revert HTLC transaction version to Version::TWO.

git range-diff main 3692f83 9e2a61a

 1:  67ed89dd42 =  1:  67ed89dd42 Set HTLC tx version 3 on handling `BumpTransactionEvent::HTLCResolution`
 2:  a7cdd28b50 =  2:  a7cdd28b50 Adjust the weight of htlc success and timeout witnesses in 0FC channels
 -:  ---------- >  3:  259b426589 fixup: htlc witnesses in 0FC channels are smaller due to no CSV lock
 3:  b8008ba389 =  4:  85783e7369 Assert in tests that the witness spending a P2A anchor is empty
 4:  85b241734b =  5:  458d2945e2 Allow a single P2A output to be below dust in `check_spends!`
 -:  ---------- >  6:  934075e90f fixup: don't allow non-p2a outputs to be below dust
 -:  ---------- >  7:  2f5c861630 fixup: assert that p2a spends have empty witnesses in check_spends
 -:  ---------- >  8:  4d7d5d4df2 fixup: allow 0-fee commitments to have non-zero fees below 1sat/vb
 5:  b377b46612 =  9:  33d1daeba4 Add 0FC to the set of supported features in non-test builds
 6:  81a88e899b = 10:  d52348921a Include 0FC channels in `chan_utils::test_anchors`
 -:  ---------- > 11:  8aec2a312c fixup: remove unneeded spaces
 -:  ---------- > 12:  29dd2aa21e fixup: update `test_anchors`
 7:  4ce3aafa89 = 13:  8e2970c928 Run `async_signer_tests::test_async_holder_signatures` with P2A anchors
 8:  c46141e8ed = 14:  1a02b5e0ab Add coverage for 0FC channels in `monitor_tests`
 -:  ---------- > 15:  e1d573bfc7 fixup: adjust comment for OFFERED_HTLC_SCRIPT_WEIGHT to mention 0FC
 -:  ---------- > 16:  377bb50c3e fixup: underscore the bigger htlc script weight is for keyed anchors only
 9:  6482d1c12f = 17:  02e7a0cad6 Add coverage for 0FC channels in `reorg_tests`
 -:  ---------- > 18:  6b49f95f40 fixup: only branch when necessary
10:  d3f12ebe0d = 19:  1207393809 Add 0FC case to `functional_tests::test_multiple_package_conflicts`
11:  489865e8f9 = 20:  73b9239ec7 Add `insane_zero_fee_channel_open` test to exported tests
12:  3692f83838 = 21:  68f46178ee Add `test_p2a_anchor_values_under_trims_and_rounds`
 -:  ---------- > 22:  8f69055aa1 fixup: beautify macro
 -:  ---------- > 23:  77e28f7120 fixup: Add test to assert we never go over 240sats on p2a anchor
 -:  ---------- > 24:  a0e25b6092 fixup: cleanup awkward if else
 -:  ---------- > 25:  9e2a61aff7 Revert HTLC transactions in 0FC channels back to `Version::TWO`

TheBlueMatt · 2025-09-24T19:18:01Z

Feel free to squash, imo

tankyleo · 2025-09-24T21:50:18Z

Squashed with no further changes

TheBlueMatt · 2025-09-25T13:49:42Z

lightning/src/ln/chan_utils.rs

-
 	Transaction {
-		version,
+		version: Version::TWO,


Grrrrrrrrrr discussed this further with @carlaKC and @instagibbs and we have to leave them v3, at least for the "consensus"/local commitment tx/SIGHASH_SINGLE case -

in theory someone can do a pinning attack by holding the HTLC preimage until the timeout is confirmable. Its kinda a weird attack and maybe doesn't matter that much but we should still keep it. In theory we can leave v3 for HTLCs claimed from a counterparty commitment tx cause they dont have a counterparty signature, but not sure if we should bother.

In any case we need to split up batches if they get too big. Probably worth just dropping the last commit here and landing this then fixing the too-big batch separately.

tankyleo · 2025-09-25T14:02:50Z

Dropped the last commit: HTLC transactions remain V3

TheBlueMatt · 2025-09-25T14:17:56Z

lightning/src/ln/functional_test_utils.rs

+			if p2a_output_below_dust || !is_p2a {
+				panic!("Spending tx output didn't meet dust limit");
+			}
+			p2a_output_below_dust = true;


Oh do we also want to check that the tx is v3 in this case? Its not required by policy (I think?) but in general we should only ever generate a tx that has a P2A dust output if the tx is v3 (or maybe has any P2A output, not just dust). Can be in a followup too.

thank you will add to the follow-up

ran the test suite with that check added, passes

tankyleo mentioned this pull request Sep 18, 2025

End-to-end 0FC tests #4088

Open

tankyleo requested a review from carlaKC September 18, 2025 16:56

TheBlueMatt added this to the 0.2 milestone Sep 18, 2025

tankyleo force-pushed the 2025-09-round-1-0fc-tests branch 2 times, most recently from b5fbb0b to a80c26a Compare September 18, 2025 22:42

tankyleo force-pushed the 2025-09-round-1-0fc-tests branch 2 times, most recently from 8a20d8d to 795a0a6 Compare September 21, 2025 20:17

Set HTLC tx version 3 on handling BumpTransactionEvent::HTLCResolution

67ed89d

tankyleo force-pushed the 2025-09-round-1-0fc-tests branch from 795a0a6 to 3856b51 Compare September 22, 2025 17:16

graphite-app bot reviewed Sep 23, 2025

View reviewed changes

lightning/src/ln/monitor_tests.rs Outdated Show resolved Hide resolved

tankyleo force-pushed the 2025-09-round-1-0fc-tests branch 2 times, most recently from f7ca7fe to 3692f83 Compare September 23, 2025 16:03

tankyleo requested a review from TheBlueMatt September 23, 2025 16:26

carlaKC reviewed Sep 23, 2025

View reviewed changes

TheBlueMatt reviewed Sep 23, 2025

View reviewed changes

tankyleo force-pushed the 2025-09-round-1-0fc-tests branch from 3692f83 to 9e2a61a Compare September 24, 2025 00:56

graphite-app bot reviewed Sep 24, 2025

View reviewed changes

lightning/src/events/bump_transaction/mod.rs Show resolved Hide resolved

tankyleo requested review from TheBlueMatt and carlaKC September 24, 2025 01:13

tankyleo added 5 commits September 24, 2025 21:27

Adjust the weight of htlc success and timeout witnesses in 0FC channels

bb3eeed

Assert in tests that the witness spending a P2A anchor is empty

74c11a8

Allow a single P2A output to be below dust in check_spends!

7a9d713

Add 0FC to the set of supported features in non-test builds

df0df2f

Include 0FC channels in chan_utils::test_anchors

1ebc270

tankyleo and others added 6 commits September 24, 2025 21:29

Run async_signer_tests::test_async_holder_signatures with P2A anchors

2eb7d31

Add coverage for 0FC channels in monitor_tests

175a3e3

Add coverage for 0FC channels in reorg_tests

25fce0a

Add 0FC case to functional_tests::test_multiple_package_conflicts

0c54c51

Add insane_zero_fee_channel_open test to exported tests

6a697b1

Add test_p2a_anchor_values_under_trims_and_rounds

1618c77

tankyleo force-pushed the 2025-09-round-1-0fc-tests branch from 9e2a61a to d90bfc6 Compare September 24, 2025 21:38

TheBlueMatt reviewed Sep 25, 2025

View reviewed changes

tankyleo force-pushed the 2025-09-round-1-0fc-tests branch from d90bfc6 to 1618c77 Compare September 25, 2025 14:00

TheBlueMatt approved these changes Sep 25, 2025

View reviewed changes

TheBlueMatt reviewed Sep 25, 2025

View reviewed changes

carlaKC approved these changes Sep 26, 2025

View reviewed changes

TheBlueMatt merged commit 0d64c86 into lightningdevkit:main Sep 26, 2025
25 checks passed

0FC tests round 1 #4089

0FC tests round 1 #4089

Conversation

tankyleo commented Sep 18, 2025

Uh oh!

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

Uh oh!

Uh oh!

codecov bot commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ldk-reviews-bot commented Sep 20, 2025

Uh oh!

ldk-reviews-bot commented Sep 22, 2025

Uh oh!

Uh oh!

carlaKC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tankyleo commented Sep 24, 2025

Uh oh!

TheBlueMatt commented Sep 24, 2025

Uh oh!

tankyleo commented Sep 24, 2025

Uh oh!

ldk-reviews-bot commented Sep 18, 2025 •

edited

Loading

codecov bot commented Sep 18, 2025 •

edited

Loading

tankyleo Sep 23, 2025 •

edited

Loading

tankyleo commented Sep 25, 2025 •

edited

Loading