Set and relay experimental accountable signal #4232

carlaKC · 2025-11-18T20:10:46Z

Fixes: #4181. This PR adds relay of the experimental accountable signal to LDK, as outlined in blip-04.

After this PR LDK will:

Set zero value accountable signals on its own payments, and for forwards that don't have an incoming accountable signal set.
If an incoming accountable signal is set, it'll copy the incoming value to the outgoing update_add_htlc.

One note here is that we don't keep track of the incoming accountable signal once we've forwarded it on our outgoing link. This isn't a requirement for our existing reputation algorithm, so this seems fine (we could add to HTLCSource if we needed it).

ldk-reviews-bot · 2025-11-18T20:10:50Z

I've assigned @tankyleo 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.

elnosh

LGTM. There's a test making the CI unhappy

elnosh · 2025-11-19T18:33:43Z

lightning/src/ln/accountable_tests.rs

+	nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &htlc_ab);
+	do_commitment_signed_dance(&nodes[1], &nodes[0], &updates_ab.commitment_signed, false, false);
+	expect_and_process_pending_htlcs(&nodes[1], false);
+	check_added_monitors!(nodes[1], 1);


nit: these can use the identically-named function instead of the macro.

elnosh · 2025-11-19T18:35:09Z

lightning/src/ln/accountable_tests.rs

+	nodes[0]
+		.node
+		.send_payment(payment_hash, onion_fields, payment_id, route_params, Retry::Attempts(0))
+		.unwrap();
+	check_added_monitors!(nodes[0], 1);
+
+	let updates_ab = get_htlc_update_msgs(&nodes[0], &nodes[1].node.get_our_node_id());
+	assert_eq!(updates_ab.update_add_htlcs.len(), 1);


would it be good to check that the signal from the sender is set to 0

carlaKC · 2025-11-19T20:42:22Z

Rebased to fix test failure + addressed feedback on test.

codecov · 2025-11-19T22:06:41Z

Codecov Report

❌ Patch coverage is 94.91525% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.32%. Comparing base (1f7b249) to head (9e14c82).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	90.69%	2 Missing and 2 partials ⚠️
lightning/src/ln/accountable_tests.rs	97.87%	1 Missing ⚠️
lightning/src/ln/channelmanager.rs	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4232      +/-   ##
==========================================
- Coverage   89.34%   89.32%   -0.02%     
==========================================
  Files         180      181       +1     
  Lines      138620   138751     +131     
  Branches   138620   138751     +131     
==========================================
+ Hits       123846   123941      +95     
- Misses      12149    12187      +38     
+ Partials     2625     2623       -2

Flag	Coverage Δ
fuzzing	`35.99% <69.09%> (+0.03%)`	⬆️
tests	`88.69% <94.91%> (-0.02%)`	⬇️

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-11-20T20:21:51Z

🔔 1st Reminder

Hey @tankyleo! 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-11-24T00:01:11Z

🔔 2nd Reminder

Hey @tankyleo! 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-11-26T00:01:31Z

🔔 3rd Reminder

Hey @tankyleo! 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.

tankyleo

first pass :)

tankyleo · 2025-11-26T04:08:05Z

lightning/src/ln/msgs.rs

+}
+
+/// Converts the accountable signal on the wire to a boolean signal.
+pub fn accountable_into_bool(accountable: ExperimentalAccountable) -> Option<bool> {


When reading from the wire, would you consider converting directly into a bool here instead of Option<bool>, mapping None to false per the spec ?

Looks like we never really use the None variant of Option<bool>; we always call unwrap_or(false). So we could consolidate all of these calls into a single unwrap_or(false) in this function, get rid of any use of Option<bool> across the patchset, and jump only between Option<u8> the wire format, and bool, the internal LDK format.

Gave this a shot - let me know what you think!

We need to have an Option in PendingHTLCInfo so that we can use impl_writeable_tlv_based for the new field.

If this helper converts straight to a bool, we end up re-wrapping it in Some to be used with PendingHTLCInfo

I have a slight preference for keeping as-is, because then PendingHTLCInfo tells us whether the incoming link actually provided the TLV or not. If we convert all the way to a bool and then re-wrap in Some, we don't know whether Some(false) means (a) the TLV was not sent or (b) the TLV was sent but set to zero.

get rid of any use of Option across the patchset, and jump only between Option the wire format, and bool, the internal LDK format.

This is nice though, so happy to make the change if you like it!

We need to have an Option in PendingHTLCInfo so that we can use impl_writeable_tlv_based for the new field.

For that field, I used (11, incoming_accountable, (default_value, false)), does this work better ?

I have a slight preference for keeping as-is, because then PendingHTLCInfo tells us whether the incoming link actually provided the TLV or not.

I agree this is information we would lose if we convert to a straight bool.

tankyleo · 2025-11-26T04:19:01Z

lightning/src/ln/accountable_tests.rs

+	do_commitment_signed_dance(&nodes[2], &nodes[1], &updates_bc.commitment_signed, false, false);
+	expect_and_process_pending_htlcs(&nodes[2], false);
+	check_added_monitors(&nodes[2], 0);
+	expect_payment_claimable!(nodes[2], payment_hash, payment_secret, 100_000);


didn't give it a shot mysef, could dig into the PendingHTLCInfo of node 2 here to cover the create_recv_pending_htlc_info case ?

Will do - should be possible to grab it out of the recipient's forward_htlcs and check it there.

tankyleo · 2025-11-26T04:20:10Z

lightning/src/ln/onion_payment.rs

 		outgoing_amt_msat: onion_amt_msat,
 		outgoing_cltv_value: onion_cltv_expiry,
 		skimmed_fee_msat: counterparty_skimmed_fee_msat,
+		incoming_accountable,


Mentioned in the test, can we easily get test coverage for this spot here?

ldk-reviews-bot requested a review from tankyleo November 18, 2025 20:21

carlaKC added 4 commits November 19, 2025 09:58

ln: add experimental accountable signal to update_add_htlc

6bcd1b6

ln: add incoming_accountable to PendingHTLCInfo

b657945

ln: add accountable signal to HTLCUpdateAwaitingACK::AddHTLC

e60486e

ln: add accountable signal to OutboundHTLCOutput

c4d6406

elnosh reviewed Nov 19, 2025

View reviewed changes

carlaKC force-pushed the 4181-experimental-accountable branch from aa7abc3 to 5accf65 Compare November 19, 2025 20:40

ln/test: add test coverage for accountable signal propagation

9e14c82

carlaKC force-pushed the 4181-experimental-accountable branch from 5accf65 to 9e14c82 Compare November 19, 2025 20:41

tankyleo reviewed Nov 26, 2025

View reviewed changes

Set and relay experimental accountable signal #4232

Are you sure you want to change the base?

Set and relay experimental accountable signal #4232

Conversation

carlaKC commented Nov 18, 2025

Uh oh!

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

Uh oh!

Uh oh!

elnosh left a comment • 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!

carlaKC commented Nov 19, 2025

Uh oh!

codecov bot commented Nov 19, 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 Nov 20, 2025

Uh oh!

ldk-reviews-bot commented Nov 24, 2025

Uh oh!

ldk-reviews-bot commented Nov 26, 2025

Uh oh!

tankyleo 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ldk-reviews-bot commented Nov 18, 2025 •

edited

Loading

elnosh left a comment •

edited

Loading

codecov bot commented Nov 19, 2025 •

edited

Loading