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

👋 Thanks for assigning @tnull 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 95.20000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.33%. Comparing base (de384ff) to head (2ca7681).

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	90.24%	2 Missing and 2 partials ⚠️
lightning/src/ln/accountable_tests.rs	96.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4232      +/-   ##
==========================================
- Coverage   89.33%   89.33%   -0.01%     
==========================================
  Files         180      181       +1     
  Lines      139042   139159     +117     
  Branches   139042   139159     +117     
==========================================
+ Hits       124219   124314      +95     
- Misses      12196    12215      +19     
- Partials     2627     2630       +3

Flag	Coverage Δ
fuzzing	`35.99% <69.81%> (+0.02%)`	⬆️
tests	`88.70% <95.20%> (-0.01%)`	⬇️

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.

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

Seems worth it for the clean up we get!

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?

carlaKC · 2025-12-02T14:32:08Z

Changes apply across a few commits so added fixups for each one, full diff here.

CI is going to fail for tests that get rebased on main:

There are some V3 test vectors there with OutboundHTLCOutput that will be missing a field
process_pending_update_add_htlcs has been renamed to test_process_pending_update_add_htlcs

Should be fixed on rebase!

tankyleo · 2025-12-02T18:35:29Z

@carlaKC have got this last commit on my machine so I can range-diff against the upcoming rebase, ready to rebase if @elnosh is good too.

To allow us to get rid of Option<bool>, we just persist false by default for accountable signals. Moving away from the Option means that we can't distinguish between no signal being set and a false one, but we accept this for the sake of simplifying the code.

elnosh

diff looks good. Prefer returning the bool directly instead of an Option as well

elnosh · 2025-12-02T20:29:28Z

lightning/src/ln/channel.rs

+				return Err(DecodeError::InvalidValue);
+			}
+		}
+		if let Some(held_htlcs) = pending_outbound_accountable {


*held_htlcs/accountable_htlcs (?)

tankyleo

Second pass, feel free to squash the existing fixups if it helps clean things up.

tankyleo · 2025-12-04T01:51:12Z

lightning/src/ln/msgs.rs

+	pub accountable: ExperimentalAccountable,
+}
+
+/// Represents the value sent on the wire to signal experimental accountability. For historical
+/// reasons the least significant three bits are used to represent the accountable signal's value,
+/// though it is now only interpreted as a binary value.
+pub type ExperimentalAccountable = Option<u8>;
+
+/// Converts a boolean accountable signal to its wire representation.
+pub fn accountable_from_bool(value: bool) -> ExperimentalAccountable {
+	Some(if value { 7 } else { 0 })
+}
+
+/// Converts the accountable signal on the wire to a boolean signal.
+pub fn accountable_into_bool(accountable: ExperimentalAccountable) -> bool {
+	accountable.is_some_and(|v| v == 7)
 }


Should have raised this earlier, this came up on second pass :) It seems to me the mapping from Option<u8> (the wire format) to bool (the logical meaning) and back should be done by our serialization / deserialization routines. For example I don't think channelmanager should be calling something like accountable_into_bool to map from a wire format to the logical meaning. So I think I would set the accountable field of UpdateAddHTLC to a straight bool, delete ExperimentalAccountable and its associated mapping functions, and then unroll the serialization macro of UpdateAddHTLC and map the bool to the wire format in there.

Seems like a shame to have to move away from the nice serialization macros for this.

WDYT of making the field private and adding a constructor (handles bool -> u8) / and is_accountable helper (handles u8 -> bool). Channelmanager does still need to call an is_accountable method, but the detail of the value is better encapsulated?

Definitely this is a solution too ! Would you mind pinging a second maintainer here to see which route they would like best ? @elnosh curious to hear your thoughts too

I think it would be a weird exception to make this field private when mostly all msgs and their fields are public.

Seems like a shame to have to move away from the nice serialization macros for this.

agree with this. In this case of the experimental field as defined in the blip I would keep as is with ExperimentalAccountable. And without getting too ahead, if this experimental field ends up getting deprecated and we end up with the TLV as defined in lightning/bolts#1280 that one wouldn't have this weird 7 == accountable meaning and could just keep using the serialization macro.

tankyleo · 2025-12-04T01:57:16Z

lightning/src/ln/msgs.rs

 	// TODO: currently we may fail to read the `ChannelManager` if we write a new even TLV in this message
 	// and then downgrade. Once this is fixed, update the type here to match BOLTs PR 989.
 	(75537, hold_htlc, option),
+	(106823, accountable, option),


This is the direction I have in mind for the serialization routine. Maybe there's a more concise way of doing this, looked around in the serialization tooling and didn't find one :) As mentioned above this would encapsulate the Option<u8> to bool mapping to just the serialization logic. I also have not tested this yet - I realized while writing this that the tests we have so far for the accountable signals don't cover the wire format.

impl Writeable for UpdateAddHTLC { fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> { self.channel_id.write(w)?; self.htlc_id.write(w)?; self.amount_msat.write(w)?; self.payment_hash.write(w)?; self.cltv_expiry.write(w)?; self.onion_routing_packet.write(w)?; encode_tlv_stream!(w, { (0, self.blinding_point, option), (65537, self.skimmed_fee_msat, option), (75537, self.hold_htlc, option), (106823, if self.accountable { 7 } else { 0 }, required), }); Ok(()) } } impl LengthReadable for UpdateAddHTLC { fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> { let channel_id: ChannelId = Readable::read(r)?; let htlc_id: u64 = Readable::read(r)?; let amount_msat: u64 = Readable::read(r)?; let payment_hash: PaymentHash = Readable::read(r)?; let cltv_expiry: u32 = Readable::read(r)?; let onion_routing_packet: OnionPacket = Readable::read(r)?; let mut blinding_point = None; let mut skimmed_fee_msat = None; let mut hold_htlc = None; let mut accountable = false; decode_tlv_stream_with_custom_tlv_decode!(r, { (0, blinding_point, option), (65537, skimmed_fee_msat, option), // TODO: currently we may fail to read the `ChannelManager` if we write a new even TLV in this message // and then downgrade. Once this is fixed, update the type here to match BOLTs PR 989. (75537, hold_htlc, option), }, |msg_type: u64, msg_reader: &mut FixedLengthReader<_>| -> Result<bool, DecodeError> { if msg_type != 10623 { return Ok(false) } if msg_reader.remaining_bytes() != 1 { return Err(DecodeError::InvalidValue); } let mut value = Vec::new(); msg_reader.read_to_limit(&mut value, 1)?; if value[0] == 7 { accountable = true; } Ok(true) }); Ok(UpdateAddHTLC { channel_id, htlc_id, amount_msat, payment_hash, cltv_expiry, skimmed_fee_msat, onion_routing_packet, blinding_point, hold_htlc, accountable, }) } }

tankyleo · 2025-12-04T02:01:23Z

lightning/src/ln/accountable_tests.rs

+		.unwrap();
+	check_added_monitors(&nodes[0], 1);
+
+	let updates_ab = get_htlc_update_msgs(&nodes[0], &nodes[1].node.get_our_node_id());


Looks like this test does not cover the serialization logic of UpdateAddHTLC with the new accountable field - we pass the struct itself from node to node. Would be cool have a separate test where UpdateAddHTLC is serialized to the wire format, and transmits the accountable signal that way.

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

elnosh reviewed Nov 19, 2025

View reviewed changes

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

tankyleo reviewed Nov 26, 2025

View reviewed changes

carlaKC force-pushed the 4181-experimental-accountable branch from 9e14c82 to 2cf23ce Compare December 2, 2025 14:30

carlaKC requested review from elnosh and tankyleo December 2, 2025 14:32

carlaKC added 11 commits December 2, 2025 14:12

ln: add experimental accountable signal to update_add_htlc

18ed54f

f map accountable straight to bool rather than using option

26f8754

ln: add incoming_accountable to PendingHTLCInfo

918b3f7

ln: add accountable signal to HTLCUpdateAwaitingACK::AddHTLC

30e7353

f store accountable as bool in HTLCUpdateAwaitingACK

8ffe6e8

ln: add accountable signal to OutboundHTLCOutput

0225eb0

f store accountable as bool in OutboundHTLCOutput

85135b5

f store accountable as bool in OutboundHTLCOutput

b959078

ln/test: add test coverage for accountable signal propagation

52a3c21

f add coverage to check incoming accountable for receiving node

2ca7681

carlaKC force-pushed the 4181-experimental-accountable branch from 2cf23ce to 2ca7681 Compare December 2, 2025 19:21

elnosh reviewed Dec 2, 2025

View reviewed changes

tankyleo reviewed Dec 4, 2025

View reviewed changes

tankyleo requested review from ldk-reviews-bot and removed request for ldk-reviews-bot December 5, 2025 00:11

tnull self-requested a review December 5, 2025 09:38

Set and relay experimental accountable signal #4232

Are you sure you want to change the base?

Set and relay experimental accountable signal #4232

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

tankyleo commented Dec 2, 2025

Uh oh!

elnosh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

ldk-reviews-bot commented Nov 18, 2025 •

edited

Loading

elnosh left a comment •

edited

Loading

codecov bot commented Nov 19, 2025 •

edited

Loading

carlaKC commented Dec 2, 2025 •

edited

Loading

tankyleo Dec 4, 2025 •

edited

Loading