Peer Storage Feature – Part 2 #3623

adi2011 · 2025-02-26T15:00:46Z

This is the second PR in the peer storage feature series.

Key Changes

Enable ChainMonitor to send peer storage to all channel partners whenever a new block is mined.
Add get_peer_storage_key() to NodeSigner.
Implement decryption logic for peer storage in handle_peer_storage_retrieval().

In the next one, I will add serialisation logic for ChannelMonitors inside peer storage.

tnull · 2025-02-27T08:52:04Z

Seems this isn't properly rebased, hence CI is failing?

adi2011 · 2025-02-27T11:45:11Z

Seems this isn't properly rebased, hence CI is failing?

I think #3626 will fix this.

tnull · 2025-02-27T11:58:21Z

Seems this isn't properly rebased, hence CI is failing?

I think #3626 will fix this.

No, it fails with:

src/ln/our_peer_storage.rs - ln::our_peer_storage::OurPeerStorage (line 48) stdout ----
error[E0433]: failed to resolve: use of undeclared type `OurPeerStorage`
 --> src/ln/our_peer_storage.rs:49:28
  |
3 | let mut our_peer_storage = OurPeerStorage::new();
  |                            ^^^^^^^^^^^^^^ not found in this scope
  |
help: consider importing this struct
  |
2 | use lightning::ln::our_peer_storage::OurPeerStorage;
  |

error[E0433]: failed to resolve: use of undeclared type `OurPeerStorage`
 --> src/ln/our_peer_storage.rs:54:1
  |
8 | OurPeerStorage::decrypt_our_peer_storage(&mut decrypted, &encrypted).unwrap();
  | ^^^^^^^^^^^^^^ not found in this scope
  |
help: consider importing this struct
  |
2 | use lightning::ln::our_peer_storage::OurPeerStorage;
  |

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0433`.
Couldn't compile the test.

failures:
    src/ln/our_peer_storage.rs - ln::our_peer_storage::OurPeerStorage (line 48)

test result: FAILED. 28 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 3.00s

error: test failed, to rerun pass '--doc'

adi2011 · 2025-02-27T12:06:38Z

That is because it’s trying to compile the commented code in the documentation, I will fix it in a bit.

tnull

Unfortunately CI is still failing as the code doesn't work in no_std.

lightning/src/ln/our_peer_storage.rs

tnull · 2025-02-28T08:43:27Z

lightning/src/ln/our_peer_storage.rs

+impl OurPeerStorage {
+	/// Returns a [`OurPeerStorage`] with version 1 and current timestamp.
+	pub fn new() -> Self {
+		let duration_since_epoch = std::time::SystemTime::now()


As LDK also supports no_std, we need to make this code work in no_std environments. As accessing time is only available in std, it means we either need to find a way to have the user provide the timestamp themselves, or feature-gate the peer storage to only work on std.

That said, are we positive that we need a timestamp in the first place?

No, I have added a timestamp so that if we receive multiple peer storage instances, we can select the latest one. I was thinking of replacing the timestamp with block height...

Not sure why we need to select the latest one specifically? Can't we just decode all of them and see if any have new data for our ChannelMonitors?

lightning/src/ln/our_peer_storage.rs

adi2011 · 2025-03-01T07:09:52Z

Thanks for the review @tnull.

Fixed the CI :)

codecov · 2025-03-01T07:54:17Z

Codecov Report

Attention: Patch coverage is 83.91960% with 32 lines in your changes missing coverage. Please review.

Project coverage is 89.37%. Comparing base (c355ea4) to head (7d56ef7).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	74.00%	12 Missing and 1 partial ⚠️
lightning/src/chain/chainmonitor.rs	80.95%	8 Missing ⚠️
lightning/src/ln/peer_handler.rs	60.00%	5 Missing and 1 partial ⚠️
lightning/src/ln/our_peer_storage.rs	95.55%	2 Missing ⚠️
lightning/src/util/test_utils.rs	66.66%	2 Missing ⚠️
lightning/src/ln/blinded_payment_tests.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3623      +/-   ##
==========================================
+ Coverage   89.20%   89.37%   +0.16%     
==========================================
  Files         155      156       +1     
  Lines      119377   121060    +1683     
  Branches   119377   121060    +1683     
==========================================
+ Hits       106496   108193    +1697     
+ Misses      10266    10262       -4     
+ Partials     2615     2605      -10

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

TheBlueMatt · 2025-03-03T20:55:48Z

lightning/src/sign/mod.rs

+	/// This function derives an encryption key for peer storage by using the HKDF
+	/// (HMAC-based Key Derivation Function) with a specific label and the node
+	/// secret key. The derived key is used for encrypting or decrypting peer storage
+	/// data.
+	///
+	/// The process involves the following steps:
+	/// 1. Retrieves the node secret key.
+	/// 2. Uses the node secret key and the label `"Peer Storage Encryption Key"`
+	///    to perform HKDF extraction and expansion.
+	/// 3. Returns the first part of the derived key, which is a 32-byte array.


No, this is a trait method, it doesn't, itself, do anything. Documentation on a trait method should describe what the method should do as well as additional context for when the method is called and possibly a possible implementation, but it what the method "does" do.

Yes, makes sense. Fixed it.

TheBlueMatt · 2025-03-03T20:59:39Z

lightning/src/sign/mod.rs

@@ -2201,6 +2230,14 @@ impl NodeSigner for KeysManager {
 		self.inbound_payment_key.clone()
 	}

+	fn get_peer_storage_key(&self) -> [u8; 32] {
+		let (t1, _) = hkdf_extract_expand_twice(


Rather than HKDF'ing from the node secret, let's derive a new key from the next hardened idx off the seed in KeysManager::new?

Sure, I will update the PR to derive the key from the next hardened index. Could you share a bit more about the reasoning behind this approach?

TheBlueMatt · 2025-03-03T21:01:40Z

lightning/src/ln/our_peer_storage.rs

+/// # Fields
+/// - `version`: Defines the structure's version for backward compatibility.
+/// - `timestamp`: UNIX timestamp indicating the creation or modification time of the instance.
+/// - `ser_channels`: Serialized channel data.


Seems weird to document private things?

Yes. I initially implemented this with public fields for simplicity, but after further consideration, I refactored the code to use getter and setter methods instead.

TheBlueMatt · 2025-03-03T21:02:50Z

lightning/src/ln/our_peer_storage.rs

+impl OurPeerStorage {
+	/// Returns a [`OurPeerStorage`] with version 1 and current timestamp.
+	pub fn new() -> Self {
+		let duration_since_epoch = std::time::SystemTime::now()


Not sure why we need to select the latest one specifically? Can't we just decode all of them and see if any have new data for our ChannelMonitors?

TheBlueMatt · 2025-03-03T21:04:19Z

lightning/src/ln/our_peer_storage.rs

+		}
+	}
+
+	/// Stubs a channel inside [`OurPeerStorage`]


I have no idea from the docs in this module what it means to "stub a channel" nor what this method does without looking at the code. Maybe just say that it "sets the serialized data to be included in the storage"?

Sorry for being less descriptive. Fixed it.

TheBlueMatt · 2025-03-03T21:06:25Z

lightning/src/ln/our_peer_storage.rs

+
+impl OurPeerStorage {
+	/// Returns a [`OurPeerStorage`] with version 1 and current timestamp.
+	pub fn new() -> Self {


This flow is fairly brittle. We currently have to create a dummy OurPeerStorage with new, then store the data we need with stub_channels after calling encrypt_our_peer_storage on the data. But at no point does any of this change the type. Instead, why not just have a method to create_from_data that takes the key and does the encryption, and a decrypt(&self) -> Vec<u8> data that returns the data decrypted?

Agreed. In the future PR we might need setter for ser_channels but not now. Thanks!

TheBlueMatt · 2025-03-03T21:10:02Z

lightning/src/ln/msgs.rs

+/// This trait extends [`MessageSendEventsProvider`], meaning it is capable of generating 
+/// message send events, which can be processed using
+/// [`MessageSendEventsProvider::get_and_clear_pending_msg_events`].
+pub trait SendingOnlyMessageHandler: MessageSendEventsProvider {


Its not clear to me why this needs a trait? MessageSendEventsProvider already lets us send messages, and send_peer_storage is only called internally in chainmonitor.rs so it doesn't seem like something we need to expose to the world (let along via a trait rather than letting ChainMonitor have the method directly)

Yes, that's what I initially thought as well. However, since we need to process events through PeerManager::process_events, I considered adding a new message handler to allow calling get_and_clear_pending_msg_events on ChainMonitor.

What could be an alternative structure that would still allow access to ChainMonitor::get_and_clear_pending_msg_events through PeerManager::process_events?

TheBlueMatt · 2025-03-03T21:18:17Z

lightning/src/chain/chainmonitor.rs

@@ -388,7 +392,7 @@ where C::Target: chain::Filter,
 	/// pre-filter blocks or only fetch blocks matching a compact filter. Otherwise, clients may
 	/// always need to fetch full blocks absent another means for determining which blocks contain
 	/// transactions relevant to the watched channels.
-	pub fn new(chain_source: Option<C>, broadcaster: T, logger: L, feeest: F, persister: P) -> Self {
+	pub fn new(chain_source: Option<C>, broadcaster: T, logger: L, feeest: F, persister: P, our_peerstorage_encryption_key: [u8; 32]) -> Self {


We should explicitly document that this should be from the KeysManager's method to match what ChannelManager does.

Thanks for pointing this out. Fixed!

Hmm, if it's ~a requirement to use the key retrieved via NodeSigner::get_peer_storage_key, why not actually introduce a PeerStorageKey (new)type rather than using a generic [u8; 32] that could be anything?

I agree PeerStorageKey would be better than [u8; 32], what do you think @TheBlueMatt?

TheBlueMatt · 2025-03-03T21:19:10Z

lightning/src/chain/chainmonitor.rs

@@ -255,6 +257,8 @@ pub struct ChainMonitor<ChannelSigner: EcdsaChannelSigner, C: Deref, T: Deref, F
 	/// it to give to users (or [`MonitorEvent`]s for `ChannelManager` to process).
 	event_notifier: Notifier,
 	pending_send_only_events: Mutex<Vec<MessageSendEvent>>,
+	our_peer_storage: Mutex<OurPeerStorage>,


Not sure I understand the point of storing this in the ChainMonitor all the time. Rather, isn't it simpler to just build the OurPeerStorage that we need when send_peer_storage is called, encrypt it, and directly enqueue it as a message?

Agreed. Thanks for pointing out.

TheBlueMatt · 2025-03-03T21:20:37Z

lightning/src/ln/channelmanager.rs

+ 		let mut res = vec![0; msg.data.len() - 16];
+		let our_peerstorage_encryption_key = self.node_signer.get_peer_storage_key();
+		let mut cyphertext_with_key = Vec::with_capacity(msg.data.len() + our_peerstorage_encryption_key.len());
+		cyphertext_with_key.extend(msg.data.clone());


Please pass the key and ciphertext as separate arguments rather than concatenating them.

Implemented this change based on the feedback from #2943 (comment).

tnull · 2025-03-07T14:53:53Z

This unfortunately needs a rebase now.

fuzz/src/full_stack.rs

lightning/src/sign/mod.rs

lightning/src/ln/our_peer_storage.rs

tnull · 2025-03-10T10:56:23Z

lightning/src/ln/our_peer_storage.rs

+
+impl OurPeerStorage {
+	/// Get `ser_channels` field from [`OurPeerStorage`]
+	pub fn get_ser_channels(&self) -> Vec<u8> {


nit: According to the Rust API guidlines, this should just be called ser_channels. Also, maybe it would be worth spelling/renaming out ser_channels as the name doesn't communicate super clearly what it holds.

Also, can we have this return a reference and leave it up to the callsite whether they want/need to clone or not?

lightning/src/ln/channelmanager.rs

lightning/src/ln/blinded_payment_tests.rs

adi2011 · 2025-03-11T11:56:16Z

Thank you so much, @tnull and @TheBlueMatt, for taking the time to review this. I’ve done my best to address all the comments. Please let me know if there’s anything I might have missed or if further changes are needed.

TheBlueMatt · 2025-03-11T14:05:19Z

lightning/src/ln/channelmanager.rs

+
+		if msg.data.len() < MIN_CYPHERTEXT_LEN {
+			log_debug!(logger, "Invalid YourPeerStorage received from {}", log_pubkey!(counterparty_node_id));
+			return Err(MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn(


Not sure we need to warn our peer if they send us bogus data.

This won’t lead to a unilateral close and is also mentioned in the spec.

It's not compulsory, but it won’t make much of a difference, I guess?

The receiver of peer_storage_retrieval: when it receives peer_storage_retrieval with an outdated or irrelevant data: MAY send a warning.

I guess it doesn't matter much, but if a peer is corrupting our data they probably know it cause they're corrupting state in other ways.

TheBlueMatt · 2025-03-11T14:06:39Z

lightning/src/ln/our_peer_storage.rs

+	}
+}
+
+impl Writeable for OurPeerStorage {


We dont need to implement readable/writeable now since we use just a vec for the encrypted data, no?

Writeable is used internally inside create_from_data, and Readable is used inside decrypt_our_peer_storage. I agree we currently don’t need them very much, but in the next one, we are going to use them for serialisation and deserialisation. Wouldn’t it be better to have it in this one instead of implementing serialisation and deserialisation logic directly within those functions?

TheBlueMatt · 2025-03-11T14:10:16Z

lightning/src/sign/mod.rs

+	/// This method is invoked when encrypting or decrypting peer storage data.
+	/// It must return the same key every time it is called, ensuring consistency
+	/// for encryption and decryption operations.
+	fn get_peer_storage_key(&self) -> SecretKey;


I don't think this needs to be a SecretKey? SecretKey implies secp256k1 operations. We really want a [u8; 32], I think.

Well, that was my fault as I had requested it above. No strong opinion though, excuse the noise!

TheBlueMatt · 2025-03-11T14:10:26Z

lightning/src/sign/mod.rs

+	/// - The derived key must be exactly **32 bytes** and suitable for symmetric
+	///   encryption algorithms.


This is given by the return type, we don't have to specify it.

Yes, I wrote it just as a note to implementations. But, yeah, the return type specifies it. Removing it.

TheBlueMatt · 2025-03-11T14:10:37Z

lightning/src/sign/mod.rs

+	///
+	/// # Returns
+	///
+	/// A [`SecretKey`] representing the encryption key for peer storage.


What does this communicate that the function signature does not?

TheBlueMatt · 2025-03-11T14:30:14Z

lightning/src/sign/mod.rs

+	///
+	/// # Implementation Details
+	///
+	/// - The key must be derived from a node-specific secret to ensure uniqueness.


I'm not sure what a "node-specific secret" means. Honestly, I'm not sure any of the sections below the first paragraph are needed.

Yes, on second thought, I agree it’s unnecessary.

lightning/src/sign/mod.rs

TheBlueMatt

Feel free to squash!

TheBlueMatt · 2025-03-12T19:54:06Z

lightning/src/ln/our_peer_storage.rs

+	pub fn create_from_data(key: [u8; 32], ser_channels: Vec<u8>) -> OurPeerStorage {
+		let n = 0u64;
+
+		let mut res = vec![0; ser_channels.len() + 16];


There's no reason to allocate a new vec, just resize ser_channels to add 16 bytes to it and encrypt-in-place into it.

Fixed, Thanks for the suggestions.

TheBlueMatt · 2025-03-12T19:59:39Z

lightning/src/ln/our_peer_storage.rs

+		const MIN_CYPHERTEXT_LEN: usize = 16;
+		let cyphertext = &self.encrypted_data[..];
+
+		let mut res = vec![0; cyphertext.len() - 16];


Same here, we can decrypt in-place.

Ensure ChannelManager properly handles peer_storage_retrieval. - Write internal_peer_storage_retreival to verify if we recv correct peer storage. - Send error if we get invalid peer_storage data.

Ensure that we correctly handle the sendpeerstorage message event from chainmonitor and process it through channelmonitor. Key Changes: - Retrieve sendpeerstorage message event from chainmonitor for both nodes. - Handle peer storage messages exchanged between nodes and verify correct decryption.

This commit replaces magic numbers with descriptive constant names for the indices used in key derivation paths within the `new` function. - Added constants: - `NODE_SECRET_INDEX` - `DESTINATION_SCRIPT_INDEX` - `SHUTDOWN_PUBKEY_INDEX` - `CHANNEL_MASTER_KEY_INDEX` - `INBOUND_PAYMENT_KEY_INDEX` - `PEER_STORAGE_KEY_INDEX`

adi2011 · 2025-04-04T14:41:56Z

@tnull We're calling send_peer_storage from ChainMonitor, which doesn't have access to entropy_source to derive random_bytes. Is there any other way to generate random bytes?

TheBlueMatt

Got a way into this but realized it looks like we still need to change the encryption logic?

TheBlueMatt · 2025-04-04T17:21:59Z

lightning/src/ln/our_peer_storage.rs

+/// - `create_from_data`: Returns an encrypted [`OurPeerStorage`] instance created from the provided data.
+/// - `decrypt_our_peer_storage`: Decrypts the [`OurPeerStorage::encrypted_data`] using the key and returns decrypted data.


nit make the functions links so that we get a compilation error if we change them without updating the docs here.

TheBlueMatt · 2025-04-04T17:22:24Z

lightning/src/ln/our_peer_storage.rs

+/// # Usage
+/// This structure can be used for securely managing and exchanging peer storage backups. It
+/// includes methods for encryption and decryption using `ChaCha20Poly1305RFC`, making it
+/// suitable for on-the-wire transmission.


What does this communicate that the first paragraph did not?

Agreed, modifying it. Thanks!

TheBlueMatt · 2025-04-04T17:46:54Z

lightning/src/ln/our_peer_storage.rs

+
+	/// Get encrypted data stored inside [`OurPeerStorage`].
+	pub fn encrypted_data(&self) -> Vec<u8> {
+		self.encrypted_data.clone()


Generally we try to avoid methods surprisingly cloning - we should either return a &Vec<u8>, or, really, in this case, change the method to a into_vec(self) -> Vec<u8> which consumes the object and is what the current only callsite wants.

TheBlueMatt · 2025-04-04T17:51:54Z

lightning/src/ln/our_peer_storage.rs

+
+impl OurPeerStorage {
+	/// Creates a new [`OurPeerStorage`] with given encrypted_data.
+	pub fn new(encrypted_data: Vec<u8>) -> Self {


nit: Seems a bit weird to have a constructor that can build a peer storage with invalid data. Its fine and maybe not worth changing, but in general this API feels really awkward. Its just a wrapper around a Vec<u8> but has no guarantees about what is actually in that vec, implying it should maybe instead just be two freestanding utility functions. If we want to actually have a useful struct, we should have some guarantees, eg check the hmac here. Again, probably not worth changing, just general advice.

Ik this does not make any sense right now but that is because we haven't added channelmonitor serialisation logic which I will put up in the next PR, but if you feel we should check the HMAC here, we can do that but that would require us to add the key to this as well...

I will shift the min_ciphertext_len check to this new() function.

TheBlueMatt · 2025-04-04T17:56:18Z

We're calling send_peer_storage from ChainMonitor, which doesn't have access to entropy_source to derive random_bytes. Is there any other way to generate random bytes?

We'll need to provide ChainMonitor with an EntropySource, sadly.

tnull

Thanks for adding nonce derivation! CI is unfortunately failing currently, but seems a ~unrelated flake: #3716

tnull · 2025-04-08T08:36:41Z

lightning/src/chain/chainmonitor.rs

@@ -261,12 +263,13 @@ pub struct ChainMonitor<ChannelSigner: EcdsaChannelSigner, C: Deref, T: Deref, F
 	our_peerstorage_encryption_key: PeerStorageKey,
 }

-impl<ChannelSigner: EcdsaChannelSigner, C: Deref, T: Deref, F: Deref, L: Deref, P: Deref> ChainMonitor<ChannelSigner, C, T, F, L, P>
+impl<ChannelSigner: EcdsaChannelSigner, C: Deref, T: Deref, F: Deref, L: Deref, P: Deref, ES: Deref> ChainMonitor<ChannelSigner, C, T, F, L, P, ES>
 where C::Target: chain::Filter,
 	    T::Target: BroadcasterInterface,
 	    F::Target: FeeEstimator,
 	    L::Target: Logger,
 	    P::Target: Persist<ChannelSigner>,


nit: While we're here, let's reindent the other params here so they align with the new one.

tnull · 2025-04-08T08:37:29Z

lightning/src/chain/chainmonitor.rs

@@ -697,19 +701,20 @@ where C::Target: chain::Filter,
 	fn send_peer_storage(&self, their_node_id: PublicKey) {
 		// TODO: Serialize `ChannelMonitor`s inside `our_peer_storage`.

-		let our_peer_storage = OurPeerStorage::create_from_data(self.our_peerstorage_encryption_key.clone(), Vec::new());
+		let our_peer_storage = OurPeerStorage::create_from_data(self.our_peerstorage_encryption_key.clone(), Vec::new(), self.entropy_source.get_secure_random_bytes());


Let's avoid this overly long line: please move the arguments to variables before giving them to create_from_data

Yes, that'd be cleaner, Thanks!

tnull · 2025-04-08T08:38:03Z

lightning/src/chain/chainmonitor.rs

-impl<ChannelSigner: EcdsaChannelSigner, C: Deref , T: Deref , F: Deref , L: Deref , P: Deref >
-chain::Watch<ChannelSigner> for ChainMonitor<ChannelSigner, C, T, F, L, P>
+impl<ChannelSigner: EcdsaChannelSigner, C: Deref , T: Deref , F: Deref , L: Deref , P: Deref, ES: Deref>
+chain::Watch<ChannelSigner> for ChainMonitor<ChannelSigner, C, T, F, L, P, ES>
 where C::Target: chain::Filter,
 	    T::Target: BroadcasterInterface,
 	    F::Target: FeeEstimator,
 	    L::Target: Logger,
 	    P::Target: Persist<ChannelSigner>,


Same here and below: let's reindent the pre-existing args to align with the new one.

tnull · 2025-04-08T08:53:20Z

lightning/src/ln/our_peer_storage.rs

+		// Compute Sha256(Sha256(key) + random_bytes).
+		let mut sha = Sha256::engine();
+		sha.input(&key_hash.to_byte_array());
+		sha.input(&random_bytes);
+
+		let mut nonce = [0u8; 12];
+		nonce[4..].copy_from_slice(&Sha256::from_engine(sha).to_byte_array()[0..8]);

 		let mut chacha = ChaCha20Poly1305RFC::new(&key.inner, &nonce, b"");
 		let mut tag = [0; 16];
 		chacha.encrypt_full_message_in_place(&mut ser_channels[0..plaintext_len], &mut tag);

 		ser_channels.extend_from_slice(&tag);


As mentioned above, let's use an Hmac for this:

diff --git a/lightning/src/ln/our_peer_storage.rs b/lightning/src/ln/our_peer_storage.rs index 1dc44ceea..349e29cb4 100644 --- a/lightning/src/ln/our_peer_storage.rs +++ b/lightning/src/ln/our_peer_storage.rs @@ -12,6 +12,7 @@ //! transmission. //! use bitcoin::hashes::sha256::Hash as Sha256; +use bitcoin::hashes::{Hmac, HmacEngine}; use bitcoin::hashes::{Hash, HashEngine}; use crate::sign::PeerStorageKey; @@ -68,13 +69,12 @@ impl OurPeerStorage { let plaintext_len = ser_channels.len(); - // Compute Sha256(Sha256(key) + random_bytes). - let mut sha = Sha256::engine(); - sha.input(&key_hash.to_byte_array()); - sha.input(&random_bytes); + // Compute Hmac(Sha256(key) + random_bytes). + let mut hmac = HmacEngine::<Sha256>::new(key_hash.as_byte_array()); + hmac.input(&random_bytes); let mut nonce = [0u8; 12]; - nonce[4..].copy_from_slice(&Sha256::from_engine(sha).to_byte_array()[0..8]); + nonce.copy_from_slice(&Hmac::from_engine(hmac).to_byte_array()); let mut chacha = ChaCha20Poly1305RFC::new(&key.inner, &nonce, b""); let mut tag = [0; 16];

I think it would make sense to split nonce generation from random bytes and the key out to a helper method that you can simply reuse in decrypt_our_peer_storage.

Also, please add some unit test(s) below to ensure decrypting the encrypted messages actually works as expected.

Thanks for the suggestion! I added the unit test and moved the nonce derivation into a helper function.

tnull · 2025-04-10T12:13:42Z

lightning-liquidity/tests/common/mod.rs

@@ -465,6 +469,7 @@ pub(crate) fn create_liquidity_node(
 		chan_handler: Arc::new(test_utils::TestChannelMessageHandler::new(
 			ChainHash::using_genesis_block(Network::Testnet),
 		)),
+		send_only_message_handler: Arc::clone(&chain_monitor),


nit: Lets add this at the end of the constructor, just as the definition.

Sure, thanks!

tnull · 2025-04-10T12:14:48Z

lightning/src/chain/chainmonitor.rs

@@ -253,14 +258,18 @@ pub struct ChainMonitor<ChannelSigner: EcdsaChannelSigner, C: Deref, T: Deref, F
 	/// A [`Notifier`] used to wake up the background processor in case we have any [`Event`]s for
 	/// it to give to users (or [`MonitorEvent`]s for `ChannelManager` to process).
 	event_notifier: Notifier,
+	pending_send_only_events: Mutex<Vec<MessageSendEvent>>,


Might be worth leaving a comment here describing what 'send-only events' are.

Added a short description to this.

tnull · 2025-04-10T12:15:53Z

lightning/src/chain/chainmonitor.rs

-	pub fn new(chain_source: Option<C>, broadcaster: T, logger: L, feeest: F, persister: P) -> Self {
+	///
+	/// # Note
+	/// `our_peerstorage_encryption_key` must be obtained from [`crate::sign::NodeSigner::get_peer_storage_key()`].


nit: Let's link to NodeSigner::get_peer_storage_key here and include a link at the bottom, just like:

[`NodeSigner::get_peer_storage_key`]: crate::sign::NodeSigner::get_peer_storage_key

(also note the absence of ())

Thanks for pointing this out.

tnull · 2025-04-10T12:17:10Z

lightning/src/chain/chainmonitor.rs

+	/// This key is used to encrypt peer storage backups.
+	///
+	/// **Important**: This key should not be set arbitrarily or changed after initialization. The same key
+	/// is obtained by the `ChannelManager` through `KeyMananger` to decrypt peer backups.


nit: KeysManager, not KeyManager. Please also link these objects, which would have cargo doc complain about the mispelling, fwiw.

Fixed, thanks!

tnull · 2025-04-10T12:19:41Z

lightning/src/chain/chainmonitor.rs

+		let random_bytes = self.entropy_source.get_secure_random_bytes();
+		let peer_storage_key = self.our_peerstorage_encryption_key;
+		let serialised_channels = Vec::new();
+		let our_peer_storage = OurPeerStorage::create_from_data(peer_storage_key, serialised_channels, random_bytes);


I have to say I still find the OurPeerStorage::create_from_data API pretty confusing at this point, especially since OurPeerStorage is not expected to hold any state really and it could just be helper methods at this point. But I hope this will clear up in one of the upcoming PR(s).

You can have a brief idea of how this would go: https://github.com/lightningdevkit/rust-lightning/pull/2943/files#diff-bc3c3cde415648f49633e54b572a14448e7611ea2b389796d732bc042218a4df

Although it could look completely different based on the reviews.

tnull · 2025-04-10T12:33:26Z

lightning/src/ln/our_peer_storage.rs

+	}
+
+	/// Nonce for encryption and decryption: Hmac(Sha256(key) + random_bytes).
+	fn derive_nonce(key: &PeerStorageKey, random_bytes: &[u8]) -> [u8; 12] {


Thanks for adding this helper, though I don't think it should live on the OurPeerStorage struct as it doesn't require &self. Let's move it down and add it as a freestanding util method.

tnull · 2025-04-10T12:34:09Z

lightning/src/ln/our_peer_storage.rs

+		let mut hmac = HmacEngine::<Sha256>::new(key_hash.as_byte_array());
+		hmac.input(&random_bytes);
+		let mut nonce = [0u8; 12];
+		// First 4 bytes of the nonce should be 0.


Huh, why? Why are we not filling the full 12 bytes of the nonce here?

This: https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/crypto/chacha20poly1305rfc.rs#L40

Hmm, right, I keep forgetting this limitation and keep getting confused by it. So fine to leave as is here, sorry for the noise.

That said, @TheBlueMatt could you shed some light on that (not very verbose) comment here? IIUC, at the time ChaCha20 couldn't handle 12-byte nonces? Or what is the diff to the RFC exactly? If there is no easy way to align the implementation, wouldn't it be safer if our ChaCha20Poly1305RFC implementation would simply accept a nonce: [u8; 8] and do the expected 0-padding instead of panicing on this undocumented requirement?

tnull · 2025-04-10T12:39:34Z

lightning/src/ln/our_peer_storage.rs

+	///
+	/// This function takes a `key` (for encryption), `ser_channels` data
+	/// (serialised channel information) and random_bytes (to derive nonce for encryption) and returns a serialised
+	/// [`OurPeerStorage`] as a `Vec<u8>`.


Seems this outdated as it's not returning a Vec<u8>?

tnull · 2025-04-10T12:40:40Z

lightning/src/ln/our_peer_storage.rs

+
+		ser_channels.extend_from_slice(&tag);
+
+		// Append `random_bytes` in front of the encrypted_blob.


s/Append/Prepend/

tnull · 2025-04-10T12:42:06Z

lightning/src/ln/our_peer_storage.rs

+		let our_peer_storage =
+			OurPeerStorage::create_from_data(key.clone(), vec![42u8; 32], [200; 32]);
+		let decrypted_data = our_peer_storage.decrypt_our_peer_storage(key).unwrap();
+		assert_eq!(decrypted_data, vec![42u8; 32]);


Please also test that failure cases results in errors, e.g., incorrect key, modifying the nonce, etc.

Thanks for the suggestion, covered nonce derivation and key changes in the same test.

adi2011 · 2025-04-14T03:56:21Z

@tnull Thank you so much for your patience and reviews on this.

If it looks good now, can I squash the fixups?

tnull

Looks good from my side, at least for now. Feel free to squash fixups!

I'm still not super happy with the OurPeerStorage API, tbh., but I'll suspend my doubts assuming that it will become clearer in the upcoming PRs.

tnull · 2025-04-15T11:07:42Z

lightning/src/chain/chainmonitor.rs

@@ -735,6 +745,12 @@ where
 			monitor.block_connected(
 				header, txdata, height, &*self.broadcaster, &*self.fee_estimator, &self.logger)
 		});
+
+		// Send peer storage everytime a new block arrives.


@adi2011 Gentle ping here? We don't necessarily need to do this in this PR, but would love to hear your thoughts on it?

tnull · 2025-04-15T11:09:46Z

lightning/src/ln/channelmanager.rs

-			"Invalid peer_storage_retrieval message received.".into(),
-		), ChannelId([0; 32])))
+		let decrypted_data = match OurPeerStorage::new(msg.data)
+			.and_then(|storage| storage.decrypt_our_peer_storage(our_peerstorage_encryption_key)) {


I still wonder if it wouldn't be easier/simpler to have decrypt_our_peer_storage be a constructor and just drop new? Or, we could even consider introducing two separate objects representing the decrypted/encrypted state, i.e., EncryptedPeerStorage and DecryptedPeerStorage and have respective method transition between them? But I guess it will become clear why that's not possible in the upcoming PRs, IIUC?

tnull · 2025-04-15T11:13:47Z

lightning/src/ln/our_peer_storage.rs

+		// Length of tag + Length of random_bytes
+		const MIN_CYPHERTEXT_LEN: usize = 16 + 32;
+
+		if encrypted_data.len() < MIN_CYPHERTEXT_LEN {


Hmm, not sure I'm fully following the API. Couldn't we have two separate objects, EncryptedPeerStorage and DecryptedPeerStorage that represent the two states that this could be in? Then, we could just transition between them? Alternatively, we could consider making OurPeerStorage an enum with Encrypted/Decrypted variants?

tnull · 2025-04-15T11:14:49Z

lightning/src/sign/mod.rs

+	/// The key is used to encrypt or decrypt backups of our state stored with our peers.
+	///
+	/// Thus, if you wish to rely on recovery using this method, you should use a key which
+	/// can be re-derived from data which would be available after state loss (eg the wallet seed)


nit: End in .

adi2011 force-pushed the peer-storage/encrypt-decrypt branch 3 times, most recently from d8df7cf to 361738d Compare February 27, 2025 03:31

adi2011 marked this pull request as ready for review February 27, 2025 07:46

adi2011 force-pushed the peer-storage/encrypt-decrypt branch from 361738d to a1006e0 Compare February 27, 2025 11:25

adi2011 force-pushed the peer-storage/encrypt-decrypt branch 2 times, most recently from ff7fec7 to 465da4a Compare February 27, 2025 12:09

jkczyz added the weekly goal Someone wants to land this this week label Feb 27, 2025

adi2011 force-pushed the peer-storage/encrypt-decrypt branch 2 times, most recently from 4f29813 to 2113034 Compare February 28, 2025 04:51

tnull reviewed Feb 28, 2025

View reviewed changes

adi2011 requested a review from tnull March 1, 2025 07:08

TheBlueMatt reviewed Mar 3, 2025

View reviewed changes

adi2011 requested a review from TheBlueMatt March 6, 2025 17:49

adi2011 force-pushed the peer-storage/encrypt-decrypt branch 2 times, most recently from 22fe4b2 to 35863ce Compare March 7, 2025 21:41

tnull reviewed Mar 10, 2025

View reviewed changes

TheBlueMatt reviewed Mar 11, 2025

View reviewed changes

adi2011 requested review from TheBlueMatt and tnull March 12, 2025 04:22

adi2011 force-pushed the peer-storage/encrypt-decrypt branch from 10d7a4d to 4adeeee Compare March 12, 2025 04:25

TheBlueMatt reviewed Mar 12, 2025

View reviewed changes

Aditya Sharma and others added 2 commits April 4, 2025 20:03

Handle PeerStorageRetrieval in ChannelManager

99b0572

Ensure ChannelManager properly handles peer_storage_retrieval. - Write internal_peer_storage_retreival to verify if we recv correct peer storage. - Send error if we get invalid peer_storage data.

adi2011 force-pushed the peer-storage/encrypt-decrypt branch from 3dbf97c to 306943e Compare April 4, 2025 14:36

adi2011 requested a review from TheBlueMatt April 4, 2025 14:45

TheBlueMatt reviewed Apr 4, 2025

View reviewed changes

adi2011 added 2 commits April 7, 2025 23:07

fixup: Enable ChainMonitor to distribute PeerStorage

680731c

fixup: Add OurPeerStorage for serialized Peer Storage backups

4e4c4ae

tnull reviewed Apr 8, 2025

View reviewed changes

tnull mentioned this pull request Apr 8, 2025

Fix flaky test_trampoline_single_hop_receive test #3716

Open

adi2011 added 5 commits April 9, 2025 16:50

fixup: Enable ChainMonitor to distribute PeerStorage

c0e5008

fixup: Add method to derive Peer Storage encryption key

5643873

fixup: Handle PeerStorageRetrieval in ChannelManager

351df68

fixup: Add OurPeerStorage for serialized Peer Storage backups

2fbb5e5

fixup: Enable ChainMonitor to distribute PeerStorage

28aff94

adi2011 requested review from tnull and TheBlueMatt April 10, 2025 05:53

tnull reviewed Apr 10, 2025

View reviewed changes

dunxen removed their request for review April 11, 2025 10:23

adi2011 added 5 commits April 14, 2025 08:42

fixup: Add OurPeerStorage for serialized Peer Storage backups

7abc134

fixup: Enable ChainMonitor to distribute PeerStorage

7a6306b

fixup: Distribute PeerStorage from ChainMonitor

34e97f7

fixup: Handle PeerStorageRetrieval in ChannelManager

8a0dd68

fixup: Enable ChainMonitor to distribute PeerStorage

9cae0a2

adi2011 force-pushed the peer-storage/encrypt-decrypt branch from 7fc10ba to 9cae0a2 Compare April 14, 2025 03:25

adi2011 requested a review from tnull April 14, 2025 03:54

tnull reviewed Apr 15, 2025

View reviewed changes

		/// - The derived key must be exactly 32 bytes and suitable for symmetric
		/// encryption algorithms.

		/// - `create_from_data`: Returns an encrypted [`OurPeerStorage`] instance created from the provided data.
		/// - `decrypt_our_peer_storage`: Decrypts the [`OurPeerStorage::encrypted_data`] using the key and returns decrypted data.


		ser_channels.extend_from_slice(&tag);

		// Append `random_bytes` in front of the encrypted_blob.

Peer Storage Feature – Part 2 #3623

Are you sure you want to change the base?

Peer Storage Feature – Part 2 #3623

Conversation

adi2011 commented Feb 26, 2025

tnull commented Feb 27, 2025

adi2011 commented Feb 27, 2025

tnull commented Feb 27, 2025

adi2011 commented Feb 27, 2025

tnull 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

adi2011 commented Mar 1, 2025

codecov bot commented Mar 1, 2025 • edited Loading

Codecov Report

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Mar 7, 2025

Choose a reason for hiding this comment

adi2011 commented Mar 11, 2025

Choose a reason for hiding this comment

adi2011 Mar 11, 2025 • edited Loading

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

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

TheBlueMatt 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

adi2011 commented Apr 4, 2025

TheBlueMatt 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

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

TheBlueMatt commented Apr 4, 2025

tnull left a comment • edited Loading

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

codecov bot commented Mar 1, 2025 •

edited

Loading

adi2011 Mar 11, 2025 •

edited

Loading

tnull left a comment •

edited

Loading