From 712d97d3fe313ab5d94b191528c72d005a19d80b Mon Sep 17 00:00:00 2001 From: benthecarman Date: Wed, 27 Mar 2024 13:42:19 -0500 Subject: [PATCH] Add DecodeError::DangerousValue for decoding invalid channel managers This would help distinguish different types of errors when deserialzing a channel manager. InvalidValue was used previously but this could be because it is an old serialization format, whereas DangerousValue is a lot more clear on why the deserialization failed. --- fuzz/src/router.rs | 1 + lightning/src/ln/channelmanager.rs | 4 ++-- lightning/src/ln/msgs.rs | 11 +++++++++++ lightning/src/ln/peer_handler.rs | 1 + lightning/src/ln/reload_tests.rs | 2 +- 5 files changed, 16 insertions(+), 3 deletions(-) diff --git a/fuzz/src/router.rs b/fuzz/src/router.rs index bf94c910..ad4373c4 100644 --- a/fuzz/src/router.rs +++ b/fuzz/src/router.rs @@ -157,6 +157,7 @@ pub fn do_test(data: &[u8], out: Out) { msgs::DecodeError::ShortRead => panic!("We picked the length..."), msgs::DecodeError::Io(e) => panic!("{:?}", e), msgs::DecodeError::UnsupportedCompression => return, + msgs::DecodeError::DangerousValue => return, } } }} diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 546b317e..cd9cfdda 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -10918,7 +10918,7 @@ where } } if chan.get_latest_unblocked_monitor_update_id() > max_in_flight_update_id { - // If the channel is ahead of the monitor, return InvalidValue: + // If the channel is ahead of the monitor, return DangerousValue: log_error!(logger, "A ChannelMonitor is stale compared to the current ChannelManager! This indicates a potentially-critical violation of the chain::Watch API!"); log_error!(logger, " The ChannelMonitor for channel {} is at update_id {} with update_id through {} in-flight", chan.context.channel_id(), monitor.get_latest_update_id(), max_in_flight_update_id); @@ -10927,7 +10927,7 @@ where log_error!(logger, " client applications must ensure that ChannelMonitor data is always available and the latest to avoid funds loss!"); log_error!(logger, " Without the latest ChannelMonitor we cannot continue without risking funds."); log_error!(logger, " Please ensure the chain::Watch API requirements are met and file a bug report at https://github.com/lightningdevkit/rust-lightning"); - return Err(DecodeError::InvalidValue); + return Err(DecodeError::DangerousValue); } } else { // We shouldn't have persisted (or read) any unfunded channel types so none should have been diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 034b2451..6a278046 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -91,6 +91,16 @@ pub enum DecodeError { Io(io::ErrorKind), /// The message included zlib-compressed values, which we don't support. UnsupportedCompression, + /// Value is validly encoded but is dangerous to use. + /// + /// This is used for things like [`ChannelManager`] deserialization where we want to ensure + /// that we don't use a [`ChannelManager`] which is in out of sync with the [`ChannelMonitor`]. + /// This indicates that there is a critical implementation flaw in the storage implementation + /// and it's unsafe to continue. + /// + /// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager + /// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor + DangerousValue, } /// An [`init`] message to be sent to or received from a peer. @@ -1796,6 +1806,7 @@ impl fmt::Display for DecodeError { DecodeError::BadLengthDescriptor => f.write_str("A length descriptor in the packet didn't describe the later data correctly"), DecodeError::Io(ref e) => fmt::Debug::fmt(e, f), DecodeError::UnsupportedCompression => f.write_str("We don't support receiving messages with zlib-compressed fields"), + DecodeError::DangerousValue => f.write_str("Value would be dangerous to continue execution with"), } } } diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index 11cdd906..19f505b2 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -1551,6 +1551,7 @@ impl return Err(PeerHandleError { }), (msgs::DecodeError::Io(_), _) => return Err(PeerHandleError { }), + (msgs::DecodeError::DangerousValue, _) => return Err(PeerHandleError { }), } } }; diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index 4422af1e..fa216bc1 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -412,7 +412,7 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() { } let mut nodes_0_read = &nodes_0_serialized[..]; - if let Err(msgs::DecodeError::InvalidValue) = + if let Err(msgs::DecodeError::DangerousValue) = <(BlockHash, ChannelManager<&test_utils::TestChainMonitor, &test_utils::TestBroadcaster, &test_utils::TestKeysInterface, &test_utils::TestKeysInterface, &test_utils::TestKeysInterface, &test_utils::TestFeeEstimator, &test_utils::TestRouter, &test_utils::TestLogger>)>::read(&mut nodes_0_read, ChannelManagerReadArgs { default_config: UserConfig::default(), entropy_source: keys_manager, -- 2.30.2