Add onion message blinded control tlv payload padding #1771

ViktorTigerstrom · 2022-10-14T19:30:31Z

Adds padding to blinded control tlv payloads in blinded routes. This addresses one of the follow-ups in #1607.

TODO:
This also needs to update fuzz tests.

ViktorTigerstrom · 2022-10-14T19:35:57Z

lightning/src/onion_message/functional_tests.rs

+	// the same length, and that the payload for every blinded payload matches the length of the
+	// largest unblinded payload length.


The spec is a little unclear in regarding this, but the test vectors seems to indicate that this should be the case.

ViktorTigerstrom · 2022-10-15T17:03:01Z

lightning/src/onion_message/blinded_route.rs

@@ -13,9 +13,11 @@ use bitcoin::secp256k1::{self, PublicKey, Secp256k1, SecretKey};

 use chain::keysinterface::KeysInterface;
 use super::utils;
+use ::get_control_tlv_length;


This seems bad. Will check why #[macro_export] places the get_control_tlv_length in the root of the lightning crate for onion_message::utils, but not for in ln::functional_test_utils and similar.

codecov-commenter · 2022-10-15T17:05:16Z

Codecov Report

Base: 90.79% // Head: 90.79% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (7863790) compared to base (6772609).
Patch coverage: 98.54% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #1771    +/-   ##
========================================
  Coverage   90.79%   90.79%            
========================================
  Files          87       87            
  Lines       46969    47090   +121     
  Branches    46969    47090   +121     
========================================
+ Hits        42646    42757   +111     
- Misses       4323     4333    +10

Impacted Files	Coverage Δ
lightning/src/onion_message/utils.rs	`100.00% <ø> (ø)`
lightning/src/onion_message/blinded_route.rs	`96.94% <96.92%> (-0.12%)`	⬇️
lightning/src/onion_message/functional_tests.rs	`97.05% <100.00%> (+0.72%)`	⬆️
lightning/src/onion_message/messenger.rs	`89.60% <100.00%> (+0.10%)`	⬆️
lightning/src/onion_message/packet.rs	`84.16% <100.00%> (+11.63%)`	⬆️
lightning/src/chain/onchaintx.rs	`94.71% <0.00%> (-0.69%)`	⬇️
lightning/src/util/events.rs	`38.13% <0.00%> (-0.27%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.87% <0.00%> (-0.14%)`	⬇️
lightning/src/chain/channelmonitor.rs	`91.16% <0.00%> (-0.06%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.14% <0.00%> (-0.03%)`	⬇️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

valentinewallace

In terms of overall approach, I think we can simplify it by hardcoding the padding length used in ReceiveControlTlvs, and never including padding in ForwardTlvs for now.

We'll need consts for the max expected encrypted payload size (basically the sizeof(next_node_id + tlv type/len)) and the receive TLV padding size, and a test that fails if either of those consts changes (which should fit well into the equal-length-payloads test you wrote, IIUC)

However, since the spec test vectors include padding for every hop and I'm not sure why, feel free to wait on Rusty getting back to me on the question I linked below

valentinewallace · 2022-10-17T16:23:42Z

lightning/src/onion_message/blinded_route.rs

+	//  TODO: Add dummy hops
+	pub fn new<K: KeysInterface, T: secp256k1::Signing + secp256k1::Verification> (
+		node_pks: &[PublicKey], keys_manager: &K, secp_ctx: &Secp256k1<T>,
+		include_next_blinding_override_padding: bool


Think we can get rid of this parameter, we should only need to make the blinded hops equal length for the sake of the sender not being able to tell which hop is the last

But based on the spec test vectors I can see why you added this. Asked Rusty to clarify: https://github.com/lightning/bolts/pull/759/files#r997283120

Nice, thanks for asking :)!

IIUC another case where we also need to support padding for next blinding override in blinded ForwardTlvs is for "concatenation of two blinded routes", included in the route blinding test vectors bolt04/route-blinding-test.json and detailed in 04-onion-routing.md.
Though perhaps it's too early to start preparing for support of that 😅.

"concatenation of two blinded routes", included in the route blinding test vectors bolt04/route-blinding-test.json and detailed in 04-onion-routing.md.
Though perhaps it's too early to start preparing for support of that 😅.

I've always disagreed with that spec wording, bc that just means sending from a list of unblinded hops to a blinded route (i.e. send_onion_message(vec![intermed_node_1, intermed_node_2], Destination::BlindedRoute(..)) in the current codebase)

valentinewallace · 2022-10-17T17:00:16Z

lightning/src/onion_message/packet.rs

+			let read_bytes_len = reader.read(&mut buf[..])?;
+			if read_bytes_len == 0 { break; }
+			for n in 0..read_bytes_len {
+				if buf[n] != 0 as u8 { return Err(DecodeError::InvalidValue); }


I don't think the padding is required to be 0s, or at least I can't find it in the spec ?

Oh yeah, my bad! Good catch :)

ViktorTigerstrom · 2022-10-17T21:33:02Z

Thanks a lot for the review @valentinewallace! Definitely agree that it'd be great if we can remove the need for support for next_blinding_override padding in BlindedRoute, to simplify things!

In terms of overall approach, I think we can simplify it by hardcoding the padding length used in ReceiveControlTlvs, and never including padding in ForwardTlvs for now.

Hmmm sadly even if the next_blinding_override padding is removed from the onion message test vectors it'll be hard to support them if we aim to create the entire route using our ControlTlvs if we also effectively hard code no padding for ForwardTlvs and a set max size for ReceiveControlTlvs, as Bob's payload contains a custom tag, and all the other payloads would need to add padding to account for that.
Or do we for test vector coverage aim to skip the step of creating the packet using our ControlTlvs and verifying that the created packet matches the test vector onion_message_packet, and instead opt for just the second step of decoding the raw test vector onion_message_packet, and verifying that each hop is decoded correctly?

However, since the spec test vectors include padding for every hop and I'm not sure why, feel free to wait on Rusty getting back to me on the question I linked below

Sounds good, thanks for asking!

TheBlueMatt · 2022-10-20T23:57:02Z

lightning/src/onion_message/utils.rs

+
+/// Calculates the length of a Control TLV payload, based on the payload content.
+#[macro_export]
+macro_rules! get_control_tlv_length {


Bleh. Is there some way to construct the tlvs first and then call serialized_length() on the appropriate Writeable-implementing object rather than redo the size calculations here?

TheBlueMatt · 2022-11-28T23:58:27Z

Needs rebase and some comments addressed.

ViktorTigerstrom · 2022-11-29T00:07:40Z

Needs rebase and some comments addressed.

Yes, will pick this up again :). Though we're still waiting for the spec clarifications Val requested (#1771 (comment)), but I'll address the comments I can address meanwhile.

TheBlueMatt · 2022-11-30T20:01:33Z

Ah, if its blocked on spec that's fine. I added the tag.

ViktorTigerstrom commented Oct 14, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-08-add-tlv-padding branch from 2c19dea to bcc0ab5 Compare October 15, 2022 13:10

ViktorTigerstrom added 2 commits October 15, 2022 18:43

Add BlindedRoute Control Tlvs Padding

a2c59c3

Add BlindedRoute Control Tlvs Padding tests

7863790

ViktorTigerstrom force-pushed the 2022-08-add-tlv-padding branch from bcc0ab5 to 7863790 Compare October 15, 2022 16:43

ViktorTigerstrom commented Oct 15, 2022

View reviewed changes

valentinewallace reviewed Oct 17, 2022

View reviewed changes

TheBlueMatt reviewed Oct 20, 2022

View reviewed changes

TheBlueMatt added the blocked on spec label Nov 30, 2022

ViktorTigerstrom mentioned this pull request Apr 26, 2023

Forward over substitute channels #1578

Closed

TheBlueMatt mentioned this pull request Jan 11, 2024

Onion message follow-ups #1607

Open

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add onion message blinded control tlv payload padding #1771

Add onion message blinded control tlv payload padding #1771

ViktorTigerstrom commented Oct 14, 2022

ViktorTigerstrom Oct 14, 2022

ViktorTigerstrom Oct 15, 2022

codecov-commenter commented Oct 15, 2022

valentinewallace left a comment

valentinewallace Oct 17, 2022

ViktorTigerstrom Oct 17, 2022

valentinewallace Nov 1, 2022

valentinewallace Oct 17, 2022

ViktorTigerstrom Oct 17, 2022

ViktorTigerstrom commented Oct 17, 2022 •

edited

Loading

TheBlueMatt Oct 20, 2022

TheBlueMatt commented Nov 28, 2022

ViktorTigerstrom commented Nov 29, 2022 •

edited

Loading

TheBlueMatt commented Nov 30, 2022

		// the same length, and that the payload for every blinded payload matches the length of the
		// largest unblinded payload length.

Add onion message blinded control tlv payload padding #1771

Are you sure you want to change the base?

Add onion message blinded control tlv payload padding #1771

Conversation

ViktorTigerstrom commented Oct 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 15, 2022

Codecov Report

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom commented Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt commented Nov 28, 2022

ViktorTigerstrom commented Nov 29, 2022 • edited Loading

TheBlueMatt commented Nov 30, 2022

ViktorTigerstrom commented Oct 17, 2022 •

edited

Loading

ViktorTigerstrom commented Nov 29, 2022 •

edited

Loading