Merge pull request #1403 from jurvis/jurvis/add-paymentforwardingfailed-event
authorJeffrey Czyz <jkczyz@gmail.com>
Tue, 26 Jul 2022 00:23:53 +0000 (19:23 -0500)
committerGitHub <noreply@github.com>
Tue, 26 Jul 2022 00:23:53 +0000 (19:23 -0500)
Add HTLCHandlingFailed event

25 files changed:
.github/workflows/build.yml
fuzz/src/chanmon_consistency.rs
lightning-background-processor/src/lib.rs
lightning-block-sync/src/poll.rs
lightning-net-tokio/src/lib.rs
lightning-rapid-gossip-sync/src/lib.rs
lightning-rapid-gossip-sync/src/processing.rs
lightning/src/chain/chaininterface.rs
lightning/src/chain/chainmonitor.rs
lightning/src/chain/channelmonitor.rs
lightning/src/chain/package.rs
lightning/src/debug_sync.rs
lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_test_utils.rs
lightning/src/ln/functional_tests.rs
lightning/src/ln/msgs.rs
lightning/src/ln/onion_route_tests.rs
lightning/src/ln/peer_handler.rs
lightning/src/ln/priv_short_conf_tests.rs
lightning/src/ln/reorg_tests.rs
lightning/src/routing/gossip.rs
lightning/src/routing/router.rs
lightning/src/routing/scoring.rs
lightning/src/util/test_utils.rs

index 68cf376235f7edcb1e812da6027da9e608e97ed2..cfb1b9c847420dc0cb4ee0edb9188033a738e26f 100644 (file)
@@ -232,14 +232,14 @@ jobs:
           EXPECTED_ROUTING_GRAPH_SNAPSHOT_SHASUM: 05a5361278f68ee2afd086cc04a1f927a63924be451f3221d380533acfacc303
       - name: Fetch rapid graph sync reference input
         run: |
-          curl --verbose -L -o lightning-rapid-gossip-sync/res/full_graph.lngossip https://bitcoin.ninja/ldk-compressed_graph-bc08df7542-2022-05-05.bin
+          curl --verbose -L -o lightning-rapid-gossip-sync/res/full_graph.lngossip https://bitcoin.ninja/ldk-compressed_graph-285cb27df79-2022-07-21.bin
           echo "Sha sum: $(sha256sum lightning-rapid-gossip-sync/res/full_graph.lngossip | awk '{ print $1 }')"
           if [ "$(sha256sum lightning-rapid-gossip-sync/res/full_graph.lngossip | awk '{ print $1 }')" != "${EXPECTED_RAPID_GOSSIP_SHASUM}" ]; then
             echo "Bad hash"
             exit 1
           fi
         env:
-          EXPECTED_RAPID_GOSSIP_SHASUM: 9637b91cea9d64320cf48fc0787c70fe69fc062f90d3512e207044110cadfd7b
+          EXPECTED_RAPID_GOSSIP_SHASUM: e0f5d11641c11896d7af3a2246d3d6c3f1720b7d2d17aab321ecce82e6b7deb8
       - name: Test with Network Graph on Rust ${{ matrix.toolchain }}
         run: |
           cd lightning
index dc9504878f6d3f07fcb2c137579b8dd092688184..f384d21927e91baf377a292e0a7c043ec904d366 100644 (file)
@@ -140,7 +140,7 @@ impl chain::Watch<EnforcingSigner> for TestChainMonitor {
                };
                let deserialized_monitor = <(BlockHash, channelmonitor::ChannelMonitor<EnforcingSigner>)>::
                        read(&mut Cursor::new(&map_entry.get().1), &*self.keys).unwrap().1;
-               deserialized_monitor.update_monitor(&update, &&TestBroadcaster{}, &&FuzzEstimator { ret_val: atomic::AtomicU32::new(253) }, &self.logger).unwrap();
+               deserialized_monitor.update_monitor(&update, &&TestBroadcaster{}, &FuzzEstimator { ret_val: atomic::AtomicU32::new(253) }, &self.logger).unwrap();
                let mut ser = VecWriter(Vec::new());
                deserialized_monitor.write(&mut ser).unwrap();
                map_entry.insert((update.update_id, ser.0));
index 11dca44bbab3d7b5c91585835ddc02ccf4baa393..484439b3907b364dddc3dc2c13bb6c078be2ad0c 100644 (file)
@@ -137,6 +137,55 @@ where A::Target: chain::Access, L::Target: Logger {
        }
 }
 
+/// (C-not exported) as the bindings concretize everything and have constructors for us
+impl<P: Deref<Target = P2PGossipSync<G, A, L>>, G: Deref<Target = NetworkGraph<L>>, A: Deref, L: Deref>
+       GossipSync<P, &RapidGossipSync<G, L>, G, A, L>
+where
+       A::Target: chain::Access,
+       L::Target: Logger,
+{
+       /// Initializes a new [`GossipSync::P2P`] variant.
+       pub fn p2p(gossip_sync: P) -> Self {
+               GossipSync::P2P(gossip_sync)
+       }
+}
+
+/// (C-not exported) as the bindings concretize everything and have constructors for us
+impl<'a, R: Deref<Target = RapidGossipSync<G, L>>, G: Deref<Target = NetworkGraph<L>>, L: Deref>
+       GossipSync<
+               &P2PGossipSync<G, &'a (dyn chain::Access + Send + Sync), L>,
+               R,
+               G,
+               &'a (dyn chain::Access + Send + Sync),
+               L,
+       >
+where
+       L::Target: Logger,
+{
+       /// Initializes a new [`GossipSync::Rapid`] variant.
+       pub fn rapid(gossip_sync: R) -> Self {
+               GossipSync::Rapid(gossip_sync)
+       }
+}
+
+/// (C-not exported) as the bindings concretize everything and have constructors for us
+impl<'a, L: Deref>
+       GossipSync<
+               &P2PGossipSync<&'a NetworkGraph<L>, &'a (dyn chain::Access + Send + Sync), L>,
+               &RapidGossipSync<&'a NetworkGraph<L>, L>,
+               &'a NetworkGraph<L>,
+               &'a (dyn chain::Access + Send + Sync),
+               L,
+       >
+where
+       L::Target: Logger,
+{
+       /// Initializes a new [`GossipSync::None`] variant.
+       pub fn none() -> Self {
+               GossipSync::None
+       }
+}
+
 /// Decorates an [`EventHandler`] with common functionality provided by standard [`EventHandler`]s.
 struct DecoratingEventHandler<
        'a,
index 6e30d2e86d20d268529bd37d942492db810d84b6..4c6cb0c0600725e9cf3552045865e8b5be472199 100644 (file)
@@ -59,12 +59,11 @@ impl Validate for BlockHeaderData {
        type T = ValidatedBlockHeader;
 
        fn validate(self, block_hash: BlockHash) -> BlockSourceResult<Self::T> {
-               self.header
+               let pow_valid_block_hash = self.header
                        .validate_pow(&self.header.target())
                        .or_else(|e| Err(BlockSourceError::persistent(e)))?;
 
-               // TODO: Use the result of validate_pow instead of recomputing the block hash once upstream.
-               if self.header.block_hash() != block_hash {
+               if pow_valid_block_hash != block_hash {
                        return Err(BlockSourceError::persistent("invalid block hash"));
                }
 
@@ -76,12 +75,11 @@ impl Validate for Block {
        type T = ValidatedBlock;
 
        fn validate(self, block_hash: BlockHash) -> BlockSourceResult<Self::T> {
-               self.header
+               let pow_valid_block_hash = self.header
                        .validate_pow(&self.header.target())
                        .or_else(|e| Err(BlockSourceError::persistent(e)))?;
 
-               // TODO: Use the result of validate_pow instead of recomputing the block hash once upstream.
-               if self.block_hash() != block_hash {
+               if pow_valid_block_hash != block_hash {
                        return Err(BlockSourceError::persistent("invalid block hash"));
                }
 
index 2ac10762b04eaf23f602bc3d8dcc987438542cae..645a7434e4536534138c5dd6150fe52bad55ff0c 100644 (file)
@@ -84,6 +84,7 @@ use lightning::ln::peer_handler::CustomMessageHandler;
 use lightning::ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, NetAddress};
 use lightning::util::logger::Logger;
 
+use std::ops::Deref;
 use std::task;
 use std::net::SocketAddr;
 use std::net::TcpStream as StdTcpStream;
@@ -120,11 +121,16 @@ struct Connection {
        id: u64,
 }
 impl Connection {
-       async fn poll_event_process<CMH, RMH, L, UMH>(peer_manager: Arc<peer_handler::PeerManager<SocketDescriptor, Arc<CMH>, Arc<RMH>, Arc<L>, Arc<UMH>>>, mut event_receiver: mpsc::Receiver<()>) where
-                       CMH: ChannelMessageHandler + 'static + Send + Sync,
-                       RMH: RoutingMessageHandler + 'static + Send + Sync,
-                       L: Logger + 'static + ?Sized + Send + Sync,
-                       UMH: CustomMessageHandler + 'static + Send + Sync {
+       async fn poll_event_process<CMH, RMH, L, UMH>(peer_manager: Arc<peer_handler::PeerManager<SocketDescriptor, CMH, RMH, L, UMH>>, mut event_receiver: mpsc::Receiver<()>) where
+                       CMH: Deref + 'static + Send + Sync,
+                       RMH: Deref + 'static + Send + Sync,
+                       L: Deref + 'static + Send + Sync,
+                       UMH: Deref + 'static + Send + Sync,
+                       CMH::Target: ChannelMessageHandler + Send + Sync,
+                       RMH::Target: RoutingMessageHandler + Send + Sync,
+                       L::Target: Logger + Send + Sync,
+                       UMH::Target: CustomMessageHandler + Send + Sync,
+    {
                loop {
                        if event_receiver.recv().await.is_none() {
                                return;
@@ -133,11 +139,16 @@ impl Connection {
                }
        }
 
-       async fn schedule_read<CMH, RMH, L, UMH>(peer_manager: Arc<peer_handler::PeerManager<SocketDescriptor, Arc<CMH>, Arc<RMH>, Arc<L>, Arc<UMH>>>, us: Arc<Mutex<Self>>, mut reader: io::ReadHalf<TcpStream>, mut read_wake_receiver: mpsc::Receiver<()>, mut write_avail_receiver: mpsc::Receiver<()>) where
-                       CMH: ChannelMessageHandler + 'static + Send + Sync,
-                       RMH: RoutingMessageHandler + 'static + Send + Sync,
-                       L: Logger + 'static + ?Sized + Send + Sync,
-                       UMH: CustomMessageHandler + 'static + Send + Sync {
+       async fn schedule_read<CMH, RMH, L, UMH>(peer_manager: Arc<peer_handler::PeerManager<SocketDescriptor, CMH, RMH, L, UMH>>, us: Arc<Mutex<Self>>, mut reader: io::ReadHalf<TcpStream>, mut read_wake_receiver: mpsc::Receiver<()>, mut write_avail_receiver: mpsc::Receiver<()>) where
+                       CMH: Deref + 'static + Send + Sync,
+                       RMH: Deref + 'static + Send + Sync,
+                       L: Deref + 'static + Send + Sync,
+                       UMH: Deref + 'static + Send + Sync,
+                       CMH::Target: ChannelMessageHandler + 'static + Send + Sync,
+                       RMH::Target: RoutingMessageHandler + 'static + Send + Sync,
+                       L::Target: Logger + 'static + Send + Sync,
+                       UMH::Target: CustomMessageHandler + 'static + Send + Sync,
+        {
                // Create a waker to wake up poll_event_process, above
                let (event_waker, event_receiver) = mpsc::channel(1);
                tokio::spawn(Self::poll_event_process(Arc::clone(&peer_manager), event_receiver));
@@ -255,11 +266,16 @@ fn get_addr_from_stream(stream: &StdTcpStream) -> Option<NetAddress> {
 /// The returned future will complete when the peer is disconnected and associated handling
 /// futures are freed, though, because all processing futures are spawned with tokio::spawn, you do
 /// not need to poll the provided future in order to make progress.
-pub fn setup_inbound<CMH, RMH, L, UMH>(peer_manager: Arc<peer_handler::PeerManager<SocketDescriptor, Arc<CMH>, Arc<RMH>, Arc<L>, Arc<UMH>>>, stream: StdTcpStream) -> impl std::future::Future<Output=()> where
-               CMH: ChannelMessageHandler + 'static + Send + Sync,
-               RMH: RoutingMessageHandler + 'static + Send + Sync,
-               L: Logger + 'static + ?Sized + Send + Sync,
-               UMH: CustomMessageHandler + 'static + Send + Sync {
+pub fn setup_inbound<CMH, RMH, L, UMH>(peer_manager: Arc<peer_handler::PeerManager<SocketDescriptor, CMH, RMH, L, UMH>>, stream: StdTcpStream) -> impl std::future::Future<Output=()> where
+               CMH: Deref + 'static + Send + Sync,
+               RMH: Deref + 'static + Send + Sync,
+               L: Deref + 'static + Send + Sync,
+               UMH: Deref + 'static + Send + Sync,
+               CMH::Target: ChannelMessageHandler + Send + Sync,
+               RMH::Target: RoutingMessageHandler + Send + Sync,
+               L::Target: Logger + Send + Sync,
+               UMH::Target: CustomMessageHandler + Send + Sync,
+{
        let remote_addr = get_addr_from_stream(&stream);
        let (reader, write_receiver, read_receiver, us) = Connection::new(stream);
        #[cfg(debug_assertions)]
@@ -297,11 +313,16 @@ pub fn setup_inbound<CMH, RMH, L, UMH>(peer_manager: Arc<peer_handler::PeerManag
 /// The returned future will complete when the peer is disconnected and associated handling
 /// futures are freed, though, because all processing futures are spawned with tokio::spawn, you do
 /// not need to poll the provided future in order to make progress.
-pub fn setup_outbound<CMH, RMH, L, UMH>(peer_manager: Arc<peer_handler::PeerManager<SocketDescriptor, Arc<CMH>, Arc<RMH>, Arc<L>, Arc<UMH>>>, their_node_id: PublicKey, stream: StdTcpStream) -> impl std::future::Future<Output=()> where
-               CMH: ChannelMessageHandler + 'static + Send + Sync,
-               RMH: RoutingMessageHandler + 'static + Send + Sync,
-               L: Logger + 'static + ?Sized + Send + Sync,
-               UMH: CustomMessageHandler + 'static + Send + Sync {
+pub fn setup_outbound<CMH, RMH, L, UMH>(peer_manager: Arc<peer_handler::PeerManager<SocketDescriptor, CMH, RMH, L, UMH>>, their_node_id: PublicKey, stream: StdTcpStream) -> impl std::future::Future<Output=()> where
+               CMH: Deref + 'static + Send + Sync,
+               RMH: Deref + 'static + Send + Sync,
+               L: Deref + 'static + Send + Sync,
+               UMH: Deref + 'static + Send + Sync,
+               CMH::Target: ChannelMessageHandler + Send + Sync,
+               RMH::Target: RoutingMessageHandler + Send + Sync,
+               L::Target: Logger + Send + Sync,
+               UMH::Target: CustomMessageHandler + Send + Sync,
+{
        let remote_addr = get_addr_from_stream(&stream);
        let (reader, mut write_receiver, read_receiver, us) = Connection::new(stream);
        #[cfg(debug_assertions)]
@@ -368,11 +389,16 @@ pub fn setup_outbound<CMH, RMH, L, UMH>(peer_manager: Arc<peer_handler::PeerMana
 /// disconnected and associated handling futures are freed, though, because all processing in said
 /// futures are spawned with tokio::spawn, you do not need to poll the second future in order to
 /// make progress.
-pub async fn connect_outbound<CMH, RMH, L, UMH>(peer_manager: Arc<peer_handler::PeerManager<SocketDescriptor, Arc<CMH>, Arc<RMH>, Arc<L>, Arc<UMH>>>, their_node_id: PublicKey, addr: SocketAddr) -> Option<impl std::future::Future<Output=()>> where
-               CMH: ChannelMessageHandler + 'static + Send + Sync,
-               RMH: RoutingMessageHandler + 'static + Send + Sync,
-               L: Logger + 'static + ?Sized + Send + Sync,
-               UMH: CustomMessageHandler + 'static + Send + Sync {
+pub async fn connect_outbound<CMH, RMH, L, UMH>(peer_manager: Arc<peer_handler::PeerManager<SocketDescriptor, CMH, RMH, L, UMH>>, their_node_id: PublicKey, addr: SocketAddr) -> Option<impl std::future::Future<Output=()>> where
+               CMH: Deref + 'static + Send + Sync,
+               RMH: Deref + 'static + Send + Sync,
+               L: Deref + 'static + Send + Sync,
+               UMH: Deref + 'static + Send + Sync,
+               CMH::Target: ChannelMessageHandler + Send + Sync,
+               RMH::Target: RoutingMessageHandler + Send + Sync,
+               L::Target: Logger + Send + Sync,
+               UMH::Target: CustomMessageHandler + Send + Sync,
+{
        if let Ok(Ok(stream)) = time::timeout(Duration::from_secs(10), async { TcpStream::connect(&addr).await.map(|s| s.into_std().unwrap()) }).await {
                Some(setup_outbound(peer_manager, their_node_id, stream))
        } else { None }
index 7880e11a60ea45c824dfceb3e75d6ee9d48610c8..6e9280f86a3f85909c8c25e49fc254bcc3a06819 100644 (file)
@@ -240,7 +240,7 @@ mod tests {
                let sync_result = rapid_sync
                        .sync_network_graph_with_file_path("./res/full_graph.lngossip");
                if let Err(crate::error::GraphSyncError::DecodeError(DecodeError::Io(io_error))) = &sync_result {
-                       let error_string = format!("Input file lightning-rapid-gossip-sync/res/full_graph.lngossip is missing! Download it from https://bitcoin.ninja/ldk-compressed_graph-bc08df7542-2022-05-05.bin\n\n{:?}", io_error);
+                       let error_string = format!("Input file lightning-rapid-gossip-sync/res/full_graph.lngossip is missing! Download it from https://bitcoin.ninja/ldk-compressed_graph-285cb27df79-2022-07-21.bin\n\n{:?}", io_error);
                        #[cfg(not(require_route_graph_test))]
                        {
                                println!("{}", error_string);
index 09693a76d6f3b4868424d863ddd97399802974f9..818e19fe4b42fa6f12a31597b8ca4798eee2b5cb 100644 (file)
@@ -8,7 +8,7 @@ use bitcoin::BlockHash;
 use bitcoin::secp256k1::PublicKey;
 
 use lightning::ln::msgs::{
-       DecodeError, ErrorAction, LightningError, OptionalField, UnsignedChannelUpdate,
+       DecodeError, ErrorAction, LightningError, UnsignedChannelUpdate,
 };
 use lightning::routing::gossip::NetworkGraph;
 use lightning::util::logger::Logger;
@@ -119,12 +119,7 @@ impl<NG: Deref<Target=NetworkGraph<L>>, L: Deref> RapidGossipSync<NG, L> where L
                let default_htlc_minimum_msat: u64 = Readable::read(&mut read_cursor)?;
                let default_fee_base_msat: u32 = Readable::read(&mut read_cursor)?;
                let default_fee_proportional_millionths: u32 = Readable::read(&mut read_cursor)?;
-               let tentative_default_htlc_maximum_msat: u64 = Readable::read(&mut read_cursor)?;
-               let default_htlc_maximum_msat = if tentative_default_htlc_maximum_msat == u64::max_value() {
-                       OptionalField::Absent
-               } else {
-                       OptionalField::Present(tentative_default_htlc_maximum_msat)
-               };
+               let default_htlc_maximum_msat: u64 = Readable::read(&mut read_cursor)?;
 
                for _ in 0..update_count {
                        let scid_delta: BigSize = Readable::read(read_cursor)?;
@@ -147,7 +142,7 @@ impl<NG: Deref<Target=NetworkGraph<L>>, L: Deref> RapidGossipSync<NG, L> where L
                                        flags: standard_channel_flags,
                                        cltv_expiry_delta: default_cltv_expiry_delta,
                                        htlc_minimum_msat: default_htlc_minimum_msat,
-                                       htlc_maximum_msat: default_htlc_maximum_msat.clone(),
+                                       htlc_maximum_msat: default_htlc_maximum_msat,
                                        fee_base_msat: default_fee_base_msat,
                                        fee_proportional_millionths: default_fee_proportional_millionths,
                                        excess_data: vec![],
@@ -170,13 +165,6 @@ impl<NG: Deref<Target=NetworkGraph<L>>, L: Deref> RapidGossipSync<NG, L> where L
                                                action: ErrorAction::IgnoreError,
                                        })?;
 
-                               let htlc_maximum_msat =
-                                       if let Some(htlc_maximum_msat) = directional_info.htlc_maximum_msat {
-                                               OptionalField::Present(htlc_maximum_msat)
-                                       } else {
-                                               OptionalField::Absent
-                                       };
-
                                UnsignedChannelUpdate {
                                        chain_hash,
                                        short_channel_id,
@@ -184,7 +172,7 @@ impl<NG: Deref<Target=NetworkGraph<L>>, L: Deref> RapidGossipSync<NG, L> where L
                                        flags: standard_channel_flags,
                                        cltv_expiry_delta: directional_info.cltv_expiry_delta,
                                        htlc_minimum_msat: directional_info.htlc_minimum_msat,
-                                       htlc_maximum_msat,
+                                       htlc_maximum_msat: directional_info.htlc_maximum_msat,
                                        fee_base_msat: directional_info.fees.base_msat,
                                        fee_proportional_millionths: directional_info.fees.proportional_millionths,
                                        excess_data: vec![],
@@ -212,13 +200,8 @@ impl<NG: Deref<Target=NetworkGraph<L>>, L: Deref> RapidGossipSync<NG, L> where L
                        }
 
                        if channel_flags & 0b_0000_0100 > 0 {
-                               let tentative_htlc_maximum_msat: u64 = Readable::read(read_cursor)?;
-                               synthetic_update.htlc_maximum_msat = if tentative_htlc_maximum_msat == u64::max_value()
-                               {
-                                       OptionalField::Absent
-                               } else {
-                                       OptionalField::Present(tentative_htlc_maximum_msat)
-                               };
+                               let htlc_maximum_msat: u64 = Readable::read(read_cursor)?;
+                               synthetic_update.htlc_maximum_msat = htlc_maximum_msat;
                        }
 
                        network_graph.update_channel_unsigned(&synthetic_update)?;
index 2ec7f318ff57bb5a3e798a0ecd651cc1fb5f4e1d..cbf609485ce1984800fa991e22d952e33ab5db56 100644 (file)
@@ -52,23 +52,18 @@ pub trait FeeEstimator {
        fn get_est_sat_per_1000_weight(&self, confirmation_target: ConfirmationTarget) -> u32;
 }
 
-// We need `FeeEstimator` implemented so that in some places where we only have a shared
-// reference to a `Deref` to a `FeeEstimator`, we can still wrap it.
-impl<D: Deref> FeeEstimator for D where D::Target: FeeEstimator {
-       fn get_est_sat_per_1000_weight(&self, confirmation_target: ConfirmationTarget) -> u32 {
-               (**self).get_est_sat_per_1000_weight(confirmation_target)
-       }
-}
-
 /// Minimum relay fee as required by bitcoin network mempool policy.
 pub const MIN_RELAY_FEE_SAT_PER_1000_WEIGHT: u64 = 4000;
 /// Minimum feerate that takes a sane approach to bitcoind weight-to-vbytes rounding.
 /// See the following Core Lightning commit for an explanation:
-/// https://github.com/ElementsProject/lightning/commit/2e687b9b352c9092b5e8bd4a688916ac50b44af0
+/// <https://github.com/ElementsProject/lightning/commit/2e687b9b352c9092b5e8bd4a688916ac50b44af0>
 pub const FEERATE_FLOOR_SATS_PER_KW: u32 = 253;
 
 /// Wraps a `Deref` to a `FeeEstimator` so that any fee estimations provided by it
-/// are bounded below by `FEERATE_FLOOR_SATS_PER_KW` (253 sats/KW)
+/// are bounded below by `FEERATE_FLOOR_SATS_PER_KW` (253 sats/KW).
+///
+/// Note that this does *not* implement [`FeeEstimator`] to make it harder to accidentally mix the
+/// two.
 pub(crate) struct LowerBoundedFeeEstimator<F: Deref>(pub F) where F::Target: FeeEstimator;
 
 impl<F: Deref> LowerBoundedFeeEstimator<F> where F::Target: FeeEstimator {
@@ -76,10 +71,8 @@ impl<F: Deref> LowerBoundedFeeEstimator<F> where F::Target: FeeEstimator {
        pub fn new(fee_estimator: F) -> Self {
                LowerBoundedFeeEstimator(fee_estimator)
        }
-}
 
-impl<F: Deref> FeeEstimator for LowerBoundedFeeEstimator<F> where F::Target: FeeEstimator {
-       fn get_est_sat_per_1000_weight(&self, confirmation_target: ConfirmationTarget) -> u32 {
+       pub fn bounded_sat_per_1000_weight(&self, confirmation_target: ConfirmationTarget) -> u32 {
                cmp::max(
                        self.0.get_est_sat_per_1000_weight(confirmation_target),
                        FEERATE_FLOOR_SATS_PER_KW,
@@ -107,7 +100,7 @@ mod tests {
                let test_fee_estimator = &TestFeeEstimator { sat_per_kw };
                let fee_estimator = LowerBoundedFeeEstimator::new(test_fee_estimator);
 
-               assert_eq!(fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background), FEERATE_FLOOR_SATS_PER_KW);
+               assert_eq!(fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Background), FEERATE_FLOOR_SATS_PER_KW);
        }
 
        #[test]
@@ -116,6 +109,6 @@ mod tests {
                let test_fee_estimator = &TestFeeEstimator { sat_per_kw };
                let fee_estimator = LowerBoundedFeeEstimator::new(test_fee_estimator);
 
-               assert_eq!(fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background), sat_per_kw);
+               assert_eq!(fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Background), sat_per_kw);
        }
 }
index eff81f1484a4ed073db3c99bf5c38bf1596afdd6..5c4ede0b16161819a8c7b54f4b8696cb5274912a 100644 (file)
@@ -639,7 +639,7 @@ where C::Target: chain::Filter,
                        Some(monitor_state) => {
                                let monitor = &monitor_state.monitor;
                                log_trace!(self.logger, "Updating ChannelMonitor for channel {}", log_funding_info!(monitor));
-                               let update_res = monitor.update_monitor(&update, &self.broadcaster, &self.fee_estimator, &self.logger);
+                               let update_res = monitor.update_monitor(&update, &self.broadcaster, &*self.fee_estimator, &self.logger);
                                if update_res.is_err() {
                                        log_error!(self.logger, "Failed to update ChannelMonitor for channel {}.", log_funding_info!(monitor));
                                }
index 220951e07f99d19eea145cbb497445658268046d..38dcd6cf0e70daf2457800fc8fc3d7ee68d16247 100644 (file)
@@ -965,6 +965,13 @@ impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {
 }
 
 impl<Signer: Sign> ChannelMonitor<Signer> {
+       /// For lockorder enforcement purposes, we need to have a single site which constructs the
+       /// `inner` mutex, otherwise cases where we lock two monitors at the same time (eg in our
+       /// PartialEq implementation) we may decide a lockorder violation has occurred.
+       fn from_impl(imp: ChannelMonitorImpl<Signer>) -> Self {
+               ChannelMonitor { inner: Mutex::new(imp) }
+       }
+
        pub(crate) fn new(secp_ctx: Secp256k1<secp256k1::All>, keys: Signer, shutdown_script: Option<Script>,
                          on_counterparty_tx_csv: u16, destination_script: &Script, funding_info: (OutPoint, Script),
                          channel_parameters: &ChannelTransactionParameters,
@@ -1012,60 +1019,58 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
                let mut outputs_to_watch = HashMap::new();
                outputs_to_watch.insert(funding_info.0.txid, vec![(funding_info.0.index as u32, funding_info.1.clone())]);
 
-               ChannelMonitor {
-                       inner: Mutex::new(ChannelMonitorImpl {
-                               latest_update_id: 0,
-                               commitment_transaction_number_obscure_factor,
+               Self::from_impl(ChannelMonitorImpl {
+                       latest_update_id: 0,
+                       commitment_transaction_number_obscure_factor,
 
-                               destination_script: destination_script.clone(),
-                               broadcasted_holder_revokable_script: None,
-                               counterparty_payment_script,
-                               shutdown_script,
+                       destination_script: destination_script.clone(),
+                       broadcasted_holder_revokable_script: None,
+                       counterparty_payment_script,
+                       shutdown_script,
 
-                               channel_keys_id,
-                               holder_revocation_basepoint,
-                               funding_info,
-                               current_counterparty_commitment_txid: None,
-                               prev_counterparty_commitment_txid: None,
+                       channel_keys_id,
+                       holder_revocation_basepoint,
+                       funding_info,
+                       current_counterparty_commitment_txid: None,
+                       prev_counterparty_commitment_txid: None,
 
-                               counterparty_commitment_params,
-                               funding_redeemscript,
-                               channel_value_satoshis,
-                               their_cur_per_commitment_points: None,
+                       counterparty_commitment_params,
+                       funding_redeemscript,
+                       channel_value_satoshis,
+                       their_cur_per_commitment_points: None,
 
-                               on_holder_tx_csv: counterparty_channel_parameters.selected_contest_delay,
+                       on_holder_tx_csv: counterparty_channel_parameters.selected_contest_delay,
 
-                               commitment_secrets: CounterpartyCommitmentSecrets::new(),
-                               counterparty_claimable_outpoints: HashMap::new(),
-                               counterparty_commitment_txn_on_chain: HashMap::new(),
-                               counterparty_hash_commitment_number: HashMap::new(),
+                       commitment_secrets: CounterpartyCommitmentSecrets::new(),
+                       counterparty_claimable_outpoints: HashMap::new(),
+                       counterparty_commitment_txn_on_chain: HashMap::new(),
+                       counterparty_hash_commitment_number: HashMap::new(),
 
-                               prev_holder_signed_commitment_tx: None,
-                               current_holder_commitment_tx: holder_commitment_tx,
-                               current_counterparty_commitment_number: 1 << 48,
-                               current_holder_commitment_number,
+                       prev_holder_signed_commitment_tx: None,
+                       current_holder_commitment_tx: holder_commitment_tx,
+                       current_counterparty_commitment_number: 1 << 48,
+                       current_holder_commitment_number,
 
-                               payment_preimages: HashMap::new(),
-                               pending_monitor_events: Vec::new(),
-                               pending_events: Vec::new(),
+                       payment_preimages: HashMap::new(),
+                       pending_monitor_events: Vec::new(),
+                       pending_events: Vec::new(),
 
-                               onchain_events_awaiting_threshold_conf: Vec::new(),
-                               outputs_to_watch,
+                       onchain_events_awaiting_threshold_conf: Vec::new(),
+                       outputs_to_watch,
 
-                               onchain_tx_handler,
+                       onchain_tx_handler,
 
-                               lockdown_from_offchain: false,
-                               holder_tx_signed: false,
-                               funding_spend_seen: false,
-                               funding_spend_confirmed: None,
-                               htlcs_resolved_on_chain: Vec::new(),
+                       lockdown_from_offchain: false,
+                       holder_tx_signed: false,
+                       funding_spend_seen: false,
+                       funding_spend_confirmed: None,
+                       htlcs_resolved_on_chain: Vec::new(),
 
-                               best_block,
-                               counterparty_node_id: Some(counterparty_node_id),
+                       best_block,
+                       counterparty_node_id: Some(counterparty_node_id),
 
-                               secp_ctx,
-                       }),
-               }
+                       secp_ctx,
+               })
        }
 
        #[cfg(test)]
@@ -1134,7 +1139,7 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
                &self,
                updates: &ChannelMonitorUpdate,
                broadcaster: &B,
-               fee_estimator: &F,
+               fee_estimator: F,
                logger: &L,
        ) -> Result<(), ()>
        where
@@ -1948,10 +1953,10 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                self.pending_monitor_events.push(MonitorEvent::CommitmentTxConfirmed(self.funding_info.0));
        }
 
-       pub fn update_monitor<B: Deref, F: Deref, L: Deref>(&mut self, updates: &ChannelMonitorUpdate, broadcaster: &B, fee_estimator: &F, logger: &L) -> Result<(), ()>
+       pub fn update_monitor<B: Deref, F: Deref, L: Deref>(&mut self, updates: &ChannelMonitorUpdate, broadcaster: &B, fee_estimator: F, logger: &L) -> Result<(), ()>
        where B::Target: BroadcasterInterface,
-                   F::Target: FeeEstimator,
-                   L::Target: Logger,
+               F::Target: FeeEstimator,
+               L::Target: Logger,
        {
                log_info!(logger, "Applying update to monitor {}, bringing update_id from {} to {} with {} changes.",
                        log_funding_info!(self), self.latest_update_id, updates.update_id, updates.updates.len());
@@ -1989,7 +1994,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                },
                                ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage } => {
                                        log_trace!(logger, "Updating ChannelMonitor with payment preimage");
-                                       let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator);
+                                       let bounded_fee_estimator = LowerBoundedFeeEstimator::new(&*fee_estimator);
                                        self.provide_payment_preimage(&PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner()), &payment_preimage, broadcaster, &bounded_fee_estimator, logger)
                                },
                                ChannelMonitorUpdateStep::CommitmentSecret { idx, secret } => {
@@ -3365,60 +3370,58 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
                let mut secp_ctx = Secp256k1::new();
                secp_ctx.seeded_randomize(&keys_manager.get_secure_random_bytes());
 
-               Ok((best_block.block_hash(), ChannelMonitor {
-                       inner: Mutex::new(ChannelMonitorImpl {
-                               latest_update_id,
-                               commitment_transaction_number_obscure_factor,
+               Ok((best_block.block_hash(), ChannelMonitor::from_impl(ChannelMonitorImpl {
+                       latest_update_id,
+                       commitment_transaction_number_obscure_factor,
 
-                               destination_script,
-                               broadcasted_holder_revokable_script,
-                               counterparty_payment_script,
-                               shutdown_script,
+                       destination_script,
+                       broadcasted_holder_revokable_script,
+                       counterparty_payment_script,
+                       shutdown_script,
 
-                               channel_keys_id,
-                               holder_revocation_basepoint,
-                               funding_info,
-                               current_counterparty_commitment_txid,
-                               prev_counterparty_commitment_txid,
+                       channel_keys_id,
+                       holder_revocation_basepoint,
+                       funding_info,
+                       current_counterparty_commitment_txid,
+                       prev_counterparty_commitment_txid,
 
-                               counterparty_commitment_params,
-                               funding_redeemscript,
-                               channel_value_satoshis,
-                               their_cur_per_commitment_points,
+                       counterparty_commitment_params,
+                       funding_redeemscript,
+                       channel_value_satoshis,
+                       their_cur_per_commitment_points,
 
-                               on_holder_tx_csv,
+                       on_holder_tx_csv,
 
-                               commitment_secrets,
-                               counterparty_claimable_outpoints,
-                               counterparty_commitment_txn_on_chain,
-                               counterparty_hash_commitment_number,
+                       commitment_secrets,
+                       counterparty_claimable_outpoints,
+                       counterparty_commitment_txn_on_chain,
+                       counterparty_hash_commitment_number,
 
-                               prev_holder_signed_commitment_tx,
-                               current_holder_commitment_tx,
-                               current_counterparty_commitment_number,
-                               current_holder_commitment_number,
+                       prev_holder_signed_commitment_tx,
+                       current_holder_commitment_tx,
+                       current_counterparty_commitment_number,
+                       current_holder_commitment_number,
 
-                               payment_preimages,
-                               pending_monitor_events: pending_monitor_events.unwrap(),
-                               pending_events,
+                       payment_preimages,
+                       pending_monitor_events: pending_monitor_events.unwrap(),
+                       pending_events,
 
-                               onchain_events_awaiting_threshold_conf,
-                               outputs_to_watch,
+                       onchain_events_awaiting_threshold_conf,
+                       outputs_to_watch,
 
-                               onchain_tx_handler,
+                       onchain_tx_handler,
 
-                               lockdown_from_offchain,
-                               holder_tx_signed,
-                               funding_spend_seen: funding_spend_seen.unwrap(),
-                               funding_spend_confirmed,
-                               htlcs_resolved_on_chain: htlcs_resolved_on_chain.unwrap(),
+                       lockdown_from_offchain,
+                       holder_tx_signed,
+                       funding_spend_seen: funding_spend_seen.unwrap(),
+                       funding_spend_confirmed,
+                       htlcs_resolved_on_chain: htlcs_resolved_on_chain.unwrap(),
 
-                               best_block,
-                               counterparty_node_id,
+                       best_block,
+                       counterparty_node_id,
 
-                               secp_ctx,
-                       }),
-               }))
+                       secp_ctx,
+               })))
        }
 }
 
@@ -3538,7 +3541,7 @@ mod tests {
 
                let broadcaster = TestBroadcaster::new(Arc::clone(&nodes[1].blocks));
                assert!(
-                       pre_update_monitor.update_monitor(&replay_update, &&broadcaster, &&chanmon_cfgs[1].fee_estimator, &nodes[1].logger)
+                       pre_update_monitor.update_monitor(&replay_update, &&broadcaster, &chanmon_cfgs[1].fee_estimator, &nodes[1].logger)
                        .is_err());
                // Even though we error'd on the first update, we should still have generated an HTLC claim
                // transaction
index b0cfa9bf000e712f797b3087d7d37b066b0510b7..30530303e59b366239e88c96d846f36da77a49ee 100644 (file)
@@ -778,13 +778,13 @@ fn compute_fee_from_spent_amounts<F: Deref, L: Deref>(input_amounts: u64, predic
        where F::Target: FeeEstimator,
              L::Target: Logger,
 {
-       let mut updated_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64;
+       let mut updated_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64;
        let mut fee = updated_feerate * (predicted_weight as u64) / 1000;
        if input_amounts <= fee {
-               updated_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal) as u64;
+               updated_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal) as u64;
                fee = updated_feerate * (predicted_weight as u64) / 1000;
                if input_amounts <= fee {
-                       updated_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background) as u64;
+                       updated_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Background) as u64;
                        fee = updated_feerate * (predicted_weight as u64) / 1000;
                        if input_amounts <= fee {
                                log_error!(logger, "Failed to generate an on-chain punishment tx as even low priority fee ({} sat) was more than the entire claim balance ({} sat)",
index 6b36682f43233f876dcacaf2e1ae34da0fef5e4e..daec309e9b5e35f81684ef9cdc5ed1df2be9e55c 100644 (file)
@@ -2,11 +2,9 @@ pub use ::alloc::sync::Arc;
 use core::ops::{Deref, DerefMut};
 use core::time::Duration;
 
-use std::collections::HashSet;
 use std::cell::RefCell;
 
 use std::sync::atomic::{AtomicUsize, Ordering};
-
 use std::sync::Mutex as StdMutex;
 use std::sync::MutexGuard as StdMutexGuard;
 use std::sync::RwLock as StdRwLock;
@@ -14,8 +12,15 @@ use std::sync::RwLockReadGuard as StdRwLockReadGuard;
 use std::sync::RwLockWriteGuard as StdRwLockWriteGuard;
 use std::sync::Condvar as StdCondvar;
 
+use prelude::HashMap;
+
 #[cfg(feature = "backtrace")]
-use backtrace::Backtrace;
+use {prelude::hash_map, backtrace::Backtrace, std::sync::Once};
+
+#[cfg(not(feature = "backtrace"))]
+struct Backtrace{}
+#[cfg(not(feature = "backtrace"))]
+impl Backtrace { fn new() -> Backtrace { Backtrace {} } }
 
 pub type LockResult<Guard> = Result<Guard, ()>;
 
@@ -44,34 +49,76 @@ impl Condvar {
 
 thread_local! {
        /// We track the set of locks currently held by a reference to their `LockMetadata`
-       static LOCKS_HELD: RefCell<HashSet<Arc<LockMetadata>>> = RefCell::new(HashSet::new());
+       static LOCKS_HELD: RefCell<HashMap<u64, Arc<LockMetadata>>> = RefCell::new(HashMap::new());
 }
 static LOCK_IDX: AtomicUsize = AtomicUsize::new(0);
 
+#[cfg(feature = "backtrace")]
+static mut LOCKS: Option<StdMutex<HashMap<String, Arc<LockMetadata>>>> = None;
+#[cfg(feature = "backtrace")]
+static LOCKS_INIT: Once = Once::new();
+
 /// Metadata about a single lock, by id, the set of things locked-before it, and the backtrace of
 /// when the Mutex itself was constructed.
 struct LockMetadata {
        lock_idx: u64,
-       locked_before: StdMutex<HashSet<Arc<LockMetadata>>>,
-       #[cfg(feature = "backtrace")]
-       lock_construction_bt: Backtrace,
+       locked_before: StdMutex<HashMap<u64, LockDep>>,
+       _lock_construction_bt: Backtrace,
 }
-impl PartialEq for LockMetadata {
-       fn eq(&self, o: &LockMetadata) -> bool { self.lock_idx == o.lock_idx }
+
+struct LockDep {
+       lock: Arc<LockMetadata>,
+       lockdep_trace: Backtrace,
 }
-impl Eq for LockMetadata {}
-impl std::hash::Hash for LockMetadata {
-       fn hash<H: std::hash::Hasher>(&self, hasher: &mut H) { hasher.write_u64(self.lock_idx); }
+
+#[cfg(feature = "backtrace")]
+fn get_construction_location(backtrace: &Backtrace) -> String {
+       // Find the first frame that is after `debug_sync` (or that is in our tests) and use
+       // that as the mutex construction site. Note that the first few frames may be in
+       // the `backtrace` crate, so we have to ignore those.
+       let sync_mutex_constr_regex = regex::Regex::new(r"lightning.*debug_sync.*new").unwrap();
+       let mut found_debug_sync = false;
+       for frame in backtrace.frames() {
+               for symbol in frame.symbols() {
+                       let symbol_name = symbol.name().unwrap().as_str().unwrap();
+                       if !sync_mutex_constr_regex.is_match(symbol_name) {
+                               if found_debug_sync {
+                                       if let Some(col) = symbol.colno() {
+                                               return format!("{}:{}:{}", symbol.filename().unwrap().display(), symbol.lineno().unwrap(), col);
+                                       } else {
+                                               // Windows debug symbols don't support column numbers, so fall back to
+                                               // line numbers only if no `colno` is available
+                                               return format!("{}:{}", symbol.filename().unwrap().display(), symbol.lineno().unwrap());
+                                       }
+                               }
+                       } else { found_debug_sync = true; }
+               }
+       }
+       panic!("Couldn't find mutex construction callsite");
 }
 
 impl LockMetadata {
-       fn new() -> LockMetadata {
-               LockMetadata {
-                       locked_before: StdMutex::new(HashSet::new()),
-                       lock_idx: LOCK_IDX.fetch_add(1, Ordering::Relaxed) as u64,
-                       #[cfg(feature = "backtrace")]
-                       lock_construction_bt: Backtrace::new(),
+       fn new() -> Arc<LockMetadata> {
+               let backtrace = Backtrace::new();
+               let lock_idx = LOCK_IDX.fetch_add(1, Ordering::Relaxed) as u64;
+
+               let res = Arc::new(LockMetadata {
+                       locked_before: StdMutex::new(HashMap::new()),
+                       lock_idx,
+                       _lock_construction_bt: backtrace,
+               });
+
+               #[cfg(feature = "backtrace")]
+               {
+                       let lock_constr_location = get_construction_location(&res._lock_construction_bt);
+                       LOCKS_INIT.call_once(|| { unsafe { LOCKS = Some(StdMutex::new(HashMap::new())); } });
+                       let mut locks = unsafe { LOCKS.as_ref() }.unwrap().lock().unwrap();
+                       match locks.entry(lock_constr_location) {
+                               hash_map::Entry::Occupied(e) => return Arc::clone(e.get()),
+                               hash_map::Entry::Vacant(e) => { e.insert(Arc::clone(&res)); },
+                       }
                }
+               res
        }
 
        // Returns whether we were a recursive lock (only relevant for read)
@@ -81,28 +128,37 @@ impl LockMetadata {
                        // For each lock which is currently locked, check that no lock's locked-before
                        // set includes the lock we're about to lock, which would imply a lockorder
                        // inversion.
-                       for locked in held.borrow().iter() {
-                               if read && *locked == *this {
+                       for (locked_idx, _locked) in held.borrow().iter() {
+                               if read && *locked_idx == this.lock_idx {
                                        // Recursive read locks are explicitly allowed
                                        return;
                                }
                        }
-                       for locked in held.borrow().iter() {
-                               if !read && *locked == *this {
-                                       panic!("Tried to lock a lock while it was held!");
+                       for (locked_idx, locked) in held.borrow().iter() {
+                               if !read && *locked_idx == this.lock_idx {
+                                       // With `feature = "backtrace"` set, we may be looking at different instances
+                                       // of the same lock.
+                                       debug_assert!(cfg!(feature = "backtrace"), "Tried to acquire a lock while it was held!");
                                }
-                               for locked_dep in locked.locked_before.lock().unwrap().iter() {
-                                       if *locked_dep == *this {
+                               for (locked_dep_idx, locked_dep) in locked.locked_before.lock().unwrap().iter() {
+                                       if *locked_dep_idx == this.lock_idx && *locked_dep_idx != locked.lock_idx {
                                                #[cfg(feature = "backtrace")]
-                                               panic!("Tried to violate existing lockorder.\nMutex that should be locked after the current lock was created at the following backtrace.\nNote that to get a backtrace for the lockorder violation, you should set RUST_BACKTRACE=1\n{:?}", locked.lock_construction_bt);
+                                               panic!("Tried to violate existing lockorder.\nMutex that should be locked after the current lock was created at the following backtrace.\nNote that to get a backtrace for the lockorder violation, you should set RUST_BACKTRACE=1\nLock being taken constructed at: {} ({}):\n{:?}\nLock constructed at: {} ({})\n{:?}\n\nLock dep created at:\n{:?}\n\n",
+                                                       get_construction_location(&this._lock_construction_bt), this.lock_idx, this._lock_construction_bt,
+                                                       get_construction_location(&locked._lock_construction_bt), locked.lock_idx, locked._lock_construction_bt,
+                                                       locked_dep.lockdep_trace);
                                                #[cfg(not(feature = "backtrace"))]
                                                panic!("Tried to violate existing lockorder. Build with the backtrace feature for more info.");
                                        }
                                }
                                // Insert any already-held locks in our locked-before set.
-                               this.locked_before.lock().unwrap().insert(Arc::clone(locked));
+                               let mut locked_before = this.locked_before.lock().unwrap();
+                               if !locked_before.contains_key(&locked.lock_idx) {
+                                       let lockdep = LockDep { lock: Arc::clone(locked), lockdep_trace: Backtrace::new() };
+                                       locked_before.insert(lockdep.lock.lock_idx, lockdep);
+                               }
                        }
-                       held.borrow_mut().insert(Arc::clone(this));
+                       held.borrow_mut().insert(this.lock_idx, Arc::clone(this));
                        inserted = true;
                });
                inserted
@@ -116,10 +172,14 @@ impl LockMetadata {
                        // Since a try-lock will simply fail if the lock is held already, we do not
                        // consider try-locks to ever generate lockorder inversions. However, if a try-lock
                        // succeeds, we do consider it to have created lockorder dependencies.
-                       for locked in held.borrow().iter() {
-                               this.locked_before.lock().unwrap().insert(Arc::clone(locked));
+                       let mut locked_before = this.locked_before.lock().unwrap();
+                       for (locked_idx, locked) in held.borrow().iter() {
+                               if !locked_before.contains_key(locked_idx) {
+                                       let lockdep = LockDep { lock: Arc::clone(locked), lockdep_trace: Backtrace::new() };
+                                       locked_before.insert(*locked_idx, lockdep);
+                               }
                        }
-                       held.borrow_mut().insert(Arc::clone(this));
+                       held.borrow_mut().insert(this.lock_idx, Arc::clone(this));
                });
        }
 }
@@ -149,7 +209,7 @@ impl<'a, T: Sized> MutexGuard<'a, T> {
 impl<T: Sized> Drop for MutexGuard<'_, T> {
        fn drop(&mut self) {
                LOCKS_HELD.with(|held| {
-                       held.borrow_mut().remove(&self.mutex.deps);
+                       held.borrow_mut().remove(&self.mutex.deps.lock_idx);
                });
        }
 }
@@ -170,7 +230,7 @@ impl<T: Sized> DerefMut for MutexGuard<'_, T> {
 
 impl<T> Mutex<T> {
        pub fn new(inner: T) -> Mutex<T> {
-               Mutex { inner: StdMutex::new(inner), deps: Arc::new(LockMetadata::new()) }
+               Mutex { inner: StdMutex::new(inner), deps: LockMetadata::new() }
        }
 
        pub fn lock<'a>(&'a self) -> LockResult<MutexGuard<'a, T>> {
@@ -220,7 +280,7 @@ impl<T: Sized> Drop for RwLockReadGuard<'_, T> {
                        return;
                }
                LOCKS_HELD.with(|held| {
-                       held.borrow_mut().remove(&self.lock.deps);
+                       held.borrow_mut().remove(&self.lock.deps.lock_idx);
                });
        }
 }
@@ -236,7 +296,7 @@ impl<T: Sized> Deref for RwLockWriteGuard<'_, T> {
 impl<T: Sized> Drop for RwLockWriteGuard<'_, T> {
        fn drop(&mut self) {
                LOCKS_HELD.with(|held| {
-                       held.borrow_mut().remove(&self.lock.deps);
+                       held.borrow_mut().remove(&self.lock.deps.lock_idx);
                });
        }
 }
@@ -249,7 +309,7 @@ impl<T: Sized> DerefMut for RwLockWriteGuard<'_, T> {
 
 impl<T> RwLock<T> {
        pub fn new(inner: T) -> RwLock<T> {
-               RwLock { inner: StdRwLock::new(inner), deps: Arc::new(LockMetadata::new()) }
+               RwLock { inner: StdRwLock::new(inner), deps: LockMetadata::new() }
        }
 
        pub fn read<'a>(&'a self) -> LockResult<RwLockReadGuard<'a, T>> {
@@ -271,96 +331,101 @@ impl<T> RwLock<T> {
        }
 }
 
-#[test]
-#[should_panic]
-fn recursive_lock_fail() {
-       let mutex = Mutex::new(());
-       let _a = mutex.lock().unwrap();
-       let _b = mutex.lock().unwrap();
-}
-
-#[test]
-fn recursive_read() {
-       let lock = RwLock::new(());
-       let _a = lock.read().unwrap();
-       let _b = lock.read().unwrap();
-}
+pub type FairRwLock<T> = RwLock<T>;
 
-#[test]
-#[should_panic]
-fn lockorder_fail() {
-       let a = Mutex::new(());
-       let b = Mutex::new(());
-       {
-               let _a = a.lock().unwrap();
-               let _b = b.lock().unwrap();
-       }
-       {
-               let _b = b.lock().unwrap();
-               let _a = a.lock().unwrap();
+mod tests {
+       use super::{RwLock, Mutex};
+
+       #[test]
+       #[should_panic]
+       #[cfg(not(feature = "backtrace"))]
+       fn recursive_lock_fail() {
+               let mutex = Mutex::new(());
+               let _a = mutex.lock().unwrap();
+               let _b = mutex.lock().unwrap();
+       }
+
+       #[test]
+       fn recursive_read() {
+               let lock = RwLock::new(());
+               let _a = lock.read().unwrap();
+               let _b = lock.read().unwrap();
+       }
+
+       #[test]
+       #[should_panic]
+       fn lockorder_fail() {
+               let a = Mutex::new(());
+               let b = Mutex::new(());
+               {
+                       let _a = a.lock().unwrap();
+                       let _b = b.lock().unwrap();
+               }
+               {
+                       let _b = b.lock().unwrap();
+                       let _a = a.lock().unwrap();
+               }
        }
-}
 
-#[test]
-#[should_panic]
-fn write_lockorder_fail() {
-       let a = RwLock::new(());
-       let b = RwLock::new(());
-       {
-               let _a = a.write().unwrap();
-               let _b = b.write().unwrap();
-       }
-       {
-               let _b = b.write().unwrap();
-               let _a = a.write().unwrap();
+       #[test]
+       #[should_panic]
+       fn write_lockorder_fail() {
+               let a = RwLock::new(());
+               let b = RwLock::new(());
+               {
+                       let _a = a.write().unwrap();
+                       let _b = b.write().unwrap();
+               }
+               {
+                       let _b = b.write().unwrap();
+                       let _a = a.write().unwrap();
+               }
        }
-}
 
-#[test]
-#[should_panic]
-fn read_lockorder_fail() {
-       let a = RwLock::new(());
-       let b = RwLock::new(());
-       {
-               let _a = a.read().unwrap();
-               let _b = b.read().unwrap();
-       }
-       {
-               let _b = b.read().unwrap();
-               let _a = a.read().unwrap();
+       #[test]
+       #[should_panic]
+       fn read_lockorder_fail() {
+               let a = RwLock::new(());
+               let b = RwLock::new(());
+               {
+                       let _a = a.read().unwrap();
+                       let _b = b.read().unwrap();
+               }
+               {
+                       let _b = b.read().unwrap();
+                       let _a = a.read().unwrap();
+               }
        }
-}
 
-#[test]
-fn read_recurisve_no_lockorder() {
-       // Like the above, but note that no lockorder is implied when we recursively read-lock a
-       // RwLock, causing this to pass just fine.
-       let a = RwLock::new(());
-       let b = RwLock::new(());
-       let _outer = a.read().unwrap();
-       {
-               let _a = a.read().unwrap();
-               let _b = b.read().unwrap();
-       }
-       {
-               let _b = b.read().unwrap();
-               let _a = a.read().unwrap();
+       #[test]
+       fn read_recursive_no_lockorder() {
+               // Like the above, but note that no lockorder is implied when we recursively read-lock a
+               // RwLock, causing this to pass just fine.
+               let a = RwLock::new(());
+               let b = RwLock::new(());
+               let _outer = a.read().unwrap();
+               {
+                       let _a = a.read().unwrap();
+                       let _b = b.read().unwrap();
+               }
+               {
+                       let _b = b.read().unwrap();
+                       let _a = a.read().unwrap();
+               }
        }
-}
 
-#[test]
-#[should_panic]
-fn read_write_lockorder_fail() {
-       let a = RwLock::new(());
-       let b = RwLock::new(());
-       {
-               let _a = a.write().unwrap();
-               let _b = b.read().unwrap();
-       }
-       {
-               let _b = b.read().unwrap();
-               let _a = a.write().unwrap();
+       #[test]
+       #[should_panic]
+       fn read_write_lockorder_fail() {
+               let a = RwLock::new(());
+               let b = RwLock::new(());
+               {
+                       let _a = a.write().unwrap();
+                       let _b = b.read().unwrap();
+               }
+               {
+                       let _b = b.read().unwrap();
+                       let _a = a.write().unwrap();
+               }
        }
 }
-
-pub type FairRwLock<T> = RwLock<T>;
index ed5bff63d47dd7ce961ebd7cf270e83b150e208e..6da5de04365ee4b1cc20f7501c62605a91d84643 100644 (file)
@@ -917,7 +917,7 @@ impl<Signer: Sign> Channel<Signer> {
                        return Err(APIError::APIMisuseError { err: format!("Holder selected channel  reserve below implemention limit dust_limit_satoshis {}", holder_selected_channel_reserve_satoshis) });
                }
 
-               let feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);
+               let feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal);
 
                let value_to_self_msat = channel_value_satoshis * 1000 - push_msat;
                let commitment_tx_fee = Self::commit_tx_fee_msat(feerate, MIN_AFFORDABLE_HTLC_COUNT, opt_anchors);
@@ -1064,11 +1064,11 @@ impl<Signer: Sign> Channel<Signer> {
                // We generally don't care too much if they set the feerate to something very high, but it
                // could result in the channel being useless due to everything being dust.
                let upper_limit = cmp::max(250 * 25,
-                       fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64 * 10);
+                       fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64 * 10);
                if feerate_per_kw as u64 > upper_limit {
                        return Err(ChannelError::Close(format!("Peer's feerate much too high. Actual: {}. Our expected upper limit: {}", feerate_per_kw, upper_limit)));
                }
-               let lower_limit = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
+               let lower_limit = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Background);
                // Some fee estimators round up to the next full sat/vbyte (ie 250 sats per kw), causing
                // occasional issues with feerate disagreements between an initiator that wants a feerate
                // of 1.1 sat/vbyte and a receiver that wants 1.1 rounded up to 2. Thus, we always add 250
@@ -4022,8 +4022,8 @@ impl<Signer: Sign> Channel<Signer> {
                // Propose a range from our current Background feerate to our Normal feerate plus our
                // force_close_avoidance_max_fee_satoshis.
                // If we fail to come to consensus, we'll have to force-close.
-               let mut proposed_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
-               let normal_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);
+               let mut proposed_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Background);
+               let normal_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal);
                let mut proposed_max_feerate = if self.is_outbound() { normal_feerate } else { u32::max_value() };
 
                // The spec requires that (when the channel does not have anchors) we only send absolute
@@ -6573,7 +6573,7 @@ mod tests {
        use ln::channel::{Channel, InboundHTLCOutput, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator};
        use ln::channel::{MAX_FUNDING_SATOSHIS_NO_WUMBO, TOTAL_BITCOIN_SUPPLY_SATOSHIS};
        use ln::features::{InitFeatures, ChannelTypeFeatures};
-       use ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate};
+       use ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate, MAX_VALUE_MSAT};
        use ln::script::ShutdownScript;
        use ln::chan_utils;
        use ln::chan_utils::{htlc_success_tx_weight, htlc_timeout_tx_weight};
@@ -6989,7 +6989,7 @@ mod tests {
                                flags: 0,
                                cltv_expiry_delta: 100,
                                htlc_minimum_msat: 5,
-                               htlc_maximum_msat: OptionalField::Absent,
+                               htlc_maximum_msat: MAX_VALUE_MSAT,
                                fee_base_msat: 110,
                                fee_proportional_millionths: 11,
                                excess_data: Vec::new(),
index 504ed00ab38b62f0308fa9f3fc64b83b4b4a941b..67c7b58e879cac181b31bd9a42e3775aa84d7ed6 100644 (file)
@@ -48,7 +48,7 @@ use routing::router::{PaymentParameters, Route, RouteHop, RoutePath, RouteParame
 use ln::msgs;
 use ln::msgs::NetAddress;
 use ln::onion_utils;
-use ln::msgs::{ChannelMessageHandler, DecodeError, LightningError, MAX_VALUE_MSAT, OptionalField};
+use ln::msgs::{ChannelMessageHandler, DecodeError, LightningError, MAX_VALUE_MSAT};
 use ln::wire::Encode;
 use chain::keysinterface::{Sign, KeysInterface, KeysManager, InMemorySigner, Recipient};
 use util::config::{UserConfig, ChannelConfig};
@@ -2400,7 +2400,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                        flags: (!were_node_one) as u8 | ((!chan.is_live() as u8) << 1),
                        cltv_expiry_delta: chan.get_cltv_expiry_delta(),
                        htlc_minimum_msat: chan.get_counterparty_htlc_minimum_msat(),
-                       htlc_maximum_msat: OptionalField::Present(chan.get_announced_htlc_max_msat()),
+                       htlc_maximum_msat: chan.get_announced_htlc_max_msat(),
                        fee_base_msat: chan.get_outbound_forwarding_fee_base_msat(),
                        fee_proportional_millionths: chan.get_fee_proportional_millionths(),
                        excess_data: Vec::new(),
@@ -3603,7 +3603,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                PersistenceNotifierGuard::optionally_notify(&self.total_consistency_lock, &self.persistence_notifier, || {
                        let mut should_persist = NotifyOption::SkipPersist;
 
-                       let new_feerate = self.fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);
+                       let new_feerate = self.fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal);
 
                        let mut handle_errors = Vec::new();
                        {
@@ -3642,7 +3642,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                        let mut should_persist = NotifyOption::SkipPersist;
                        if self.process_background_events() { should_persist = NotifyOption::DoPersist; }
 
-                       let new_feerate = self.fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);
+                       let new_feerate = self.fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal);
 
                        let mut handle_errors = Vec::new();
                        let mut timed_out_mpp_htlcs = Vec::new();
@@ -3923,7 +3923,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                        return;
                                }
                                mem::drop(channel_state_lock);
-                               let retry = if let Some(payment_params_data) = payment_params {
+                               let mut retry = if let Some(payment_params_data) = payment_params {
                                        let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
                                        Some(RouteParameters {
                                                payment_params: payment_params_data.clone(),
@@ -3959,6 +3959,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                        // TODO: If we decided to blame ourselves (or one of our channels) in
                                                        // process_onion_failure we should close that channel as it implies our
                                                        // next-hop is needlessly blaming us!
+                                                       if let Some(scid) = short_channel_id {
+                                                               retry.as_mut().map(|r| r.payment_params.previously_failed_channels.push(scid));
+                                                       }
                                                        events::Event::PaymentPathFailed {
                                                                payment_id: Some(payment_id),
                                                                payment_hash: payment_hash.clone(),
@@ -3988,6 +3991,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                // ChannelDetails.
                                                // TODO: For non-temporary failures, we really should be closing the
                                                // channel here as we apparently can't relay through them anyway.
+                                               let scid = path.first().unwrap().short_channel_id;
+                                               retry.as_mut().map(|r| r.payment_params.previously_failed_channels.push(scid));
                                                events::Event::PaymentPathFailed {
                                                        payment_id: Some(payment_id),
                                                        payment_hash: payment_hash.clone(),
@@ -3995,7 +4000,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                        network_update: None,
                                                        all_paths_failed,
                                                        path: path.clone(),
-                                                       short_channel_id: Some(path.first().unwrap().short_channel_id),
+                                                       short_channel_id: Some(scid),
                                                        retry,
 #[cfg(test)]
                                                        error_code: Some(*failure_code),
index 38a593141f4fea7d974635c5c30a0555ecf07828..54d199a26f83f85cec4567baa1b1fae45d9e6faa 100644 (file)
@@ -353,6 +353,11 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
                                }
                        }
 
+                       let broadcaster = test_utils::TestBroadcaster {
+                               txn_broadcasted: Mutex::new(self.tx_broadcaster.txn_broadcasted.lock().unwrap().clone()),
+                               blocks: Arc::new(Mutex::new(self.tx_broadcaster.blocks.lock().unwrap().clone())),
+                       };
+
                        // Before using all the new monitors to check the watch outpoints, use the full set of
                        // them to ensure we can write and reload our ChannelManager.
                        {
@@ -368,20 +373,13 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
                                        keys_manager: self.keys_manager,
                                        fee_estimator: &test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) },
                                        chain_monitor: self.chain_monitor,
-                                       tx_broadcaster: &test_utils::TestBroadcaster {
-                                               txn_broadcasted: Mutex::new(self.tx_broadcaster.txn_broadcasted.lock().unwrap().clone()),
-                                               blocks: Arc::new(Mutex::new(self.tx_broadcaster.blocks.lock().unwrap().clone())),
-                                       },
+                                       tx_broadcaster: &broadcaster,
                                        logger: &self.logger,
                                        channel_monitors,
                                }).unwrap();
                        }
 
                        let persister = test_utils::TestPersister::new();
-                       let broadcaster = test_utils::TestBroadcaster {
-                               txn_broadcasted: Mutex::new(self.tx_broadcaster.txn_broadcasted.lock().unwrap().clone()),
-                               blocks: Arc::new(Mutex::new(self.tx_broadcaster.blocks.lock().unwrap().clone())),
-                       };
                        let chain_source = test_utils::TestChainSource::new(Network::Testnet);
                        let chain_monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &broadcaster, &self.logger, &feeest, &persister, &self.keys_manager);
                        for deserialized_monitor in deserialized_monitors.drain(..) {
@@ -1542,7 +1540,7 @@ pub fn expect_payment_failed_conditions<'a, 'b, 'c, 'd, 'e>(
        let mut events = node.node.get_and_clear_pending_events();
        assert_eq!(events.len(), 1);
        let expected_payment_id = match events.pop().unwrap() {
-               Event::PaymentPathFailed { payment_hash, rejected_by_dest, path, retry, payment_id, network_update,
+               Event::PaymentPathFailed { payment_hash, rejected_by_dest, path, retry, payment_id, network_update, short_channel_id,
                        #[cfg(test)]
                        error_code,
                        #[cfg(test)]
@@ -1552,6 +1550,9 @@ pub fn expect_payment_failed_conditions<'a, 'b, 'c, 'd, 'e>(
                        assert!(retry.is_some(), "expected retry.is_some()");
                        assert_eq!(retry.as_ref().unwrap().final_value_msat, path.last().unwrap().fee_msat, "Retry amount should match last hop in path");
                        assert_eq!(retry.as_ref().unwrap().payment_params.payee_pubkey, path.last().unwrap().pubkey, "Retry payee node_id should match last hop in path");
+                       if let Some(scid) = short_channel_id {
+                               assert!(retry.as_ref().unwrap().payment_params.previously_failed_channels.contains(&scid));
+                       }
 
                        #[cfg(test)]
                        {
index 8e7b3e8fbbd02b8a0697ca57cf29de87ab87100c..be517ea4a7698de94cd9fca02f7b57157a17e99f 100644 (file)
@@ -28,7 +28,7 @@ use routing::gossip::NetworkGraph;
 use routing::router::{PaymentParameters, Route, RouteHop, RouteParameters, find_route, get_route};
 use ln::features::{ChannelFeatures, InitFeatures, InvoiceFeatures, NodeFeatures};
 use ln::msgs;
-use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, OptionalField, ErrorAction};
+use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, ErrorAction};
 use util::enforcing_trait_impls::EnforcingSigner;
 use util::{byte_utils, test_utils};
 use util::events::{Event, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose, ClosureReason, HTLCDestination};
@@ -1060,26 +1060,6 @@ fn fake_network_test() {
        fail_payment(&nodes[1], &vec!(&nodes[3], &nodes[2], &nodes[1])[..], payment_hash_2);
        claim_payment(&nodes[1], &vec!(&nodes[2], &nodes[3], &nodes[1])[..], payment_preimage_1);
 
-       // Add a duplicate new channel from 2 to 4
-       let chan_5 = create_announced_chan_between_nodes(&nodes, 1, 3, InitFeatures::known(), InitFeatures::known());
-
-       // Send some payments across both channels
-       let payment_preimage_3 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], 3000000).0;
-       let payment_preimage_4 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], 3000000).0;
-       let payment_preimage_5 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], 3000000).0;
-
-
-       route_over_limit(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], 3000000);
-       let events = nodes[0].node.get_and_clear_pending_msg_events();
-       assert_eq!(events.len(), 0);
-       nodes[0].logger.assert_log_regex("lightning::ln::channelmanager".to_string(), regex::Regex::new(r"Cannot send value that would put us over the max HTLC value in flight our peer will accept \(\d+\)").unwrap(), 1);
-
-       //TODO: Test that routes work again here as we've been notified that the channel is full
-
-       claim_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], payment_preimage_3);
-       claim_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], payment_preimage_4);
-       claim_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], payment_preimage_5);
-
        // Close down the channels...
        close_channel(&nodes[0], &nodes[1], &chan_1.2, chan_1.3, true);
        check_closed_event!(nodes[0], 1, ClosureReason::CooperativeClosure);
@@ -1093,9 +1073,6 @@ fn fake_network_test() {
        close_channel(&nodes[1], &nodes[3], &chan_4.2, chan_4.3, false);
        check_closed_event!(nodes[1], 1, ClosureReason::CooperativeClosure);
        check_closed_event!(nodes[3], 1, ClosureReason::CooperativeClosure);
-       close_channel(&nodes[1], &nodes[3], &chan_5.2, chan_5.3, false);
-       check_closed_event!(nodes[1], 1, ClosureReason::CooperativeClosure);
-       check_closed_event!(nodes[3], 1, ClosureReason::CooperativeClosure);
 }
 
 #[test]
@@ -3423,7 +3400,7 @@ fn test_htlc_ignore_latest_remote_commitment() {
        check_added_monitors!(nodes[0], 1);
        check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed);
 
-       let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
+       let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
        assert_eq!(node_txn.len(), 3);
        assert_eq!(node_txn[0], node_txn[1]);
 
@@ -4841,7 +4818,7 @@ fn test_claim_on_remote_sizeable_push_msat() {
        check_added_monitors!(nodes[0], 1);
        check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed);
 
-       let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
+       let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
        assert_eq!(node_txn.len(), 1);
        check_spends!(node_txn[0], chan.3);
        assert_eq!(node_txn[0].output.len(), 2); // We can't force trimming of to_remote output as channel_reserve_satoshis block us to do so at channel opening
@@ -5047,7 +5024,7 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() {
        check_closed_event!(nodes[0], 1, ClosureReason::CommitmentTxConfirmed);
        connect_blocks(&nodes[0], TEST_FINAL_CLTV - 1); // Confirm blocks until the HTLC expires
 
-       let revoked_htlc_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
+       let revoked_htlc_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
        assert_eq!(revoked_htlc_txn.len(), 2);
        check_spends!(revoked_htlc_txn[0], chan_1.3);
        assert_eq!(revoked_htlc_txn[1].input.len(), 1);
@@ -7882,7 +7859,7 @@ fn test_bump_penalty_txn_on_revoked_htlcs() {
        check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed);
        connect_blocks(&nodes[1], 49); // Confirm blocks until the HTLC expires (note CLTV was explicitly 50 above)
 
-       let revoked_htlc_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
+       let revoked_htlc_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
        assert_eq!(revoked_htlc_txn.len(), 3);
        check_spends!(revoked_htlc_txn[1], chan.3);
 
@@ -8143,22 +8120,26 @@ fn test_counterparty_raa_skip_no_crash() {
        let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
        let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).2;
 
-       let mut guard = nodes[0].node.channel_state.lock().unwrap();
-       let keys = guard.by_id.get_mut(&channel_id).unwrap().get_signer();
+       let per_commitment_secret;
+       let next_per_commitment_point;
+       {
+               let mut guard = nodes[0].node.channel_state.lock().unwrap();
+               let keys = guard.by_id.get_mut(&channel_id).unwrap().get_signer();
 
-       const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1;
+               const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1;
 
-       // Make signer believe we got a counterparty signature, so that it allows the revocation
-       keys.get_enforcement_state().last_holder_commitment -= 1;
-       let per_commitment_secret = keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER);
+               // Make signer believe we got a counterparty signature, so that it allows the revocation
+               keys.get_enforcement_state().last_holder_commitment -= 1;
+               per_commitment_secret = keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER);
 
-       // Must revoke without gaps
-       keys.get_enforcement_state().last_holder_commitment -= 1;
-       keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 1);
+               // Must revoke without gaps
+               keys.get_enforcement_state().last_holder_commitment -= 1;
+               keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 1);
 
-       keys.get_enforcement_state().last_holder_commitment -= 1;
-       let next_per_commitment_point = PublicKey::from_secret_key(&Secp256k1::new(),
-               &SecretKey::from_slice(&keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)).unwrap());
+               keys.get_enforcement_state().last_holder_commitment -= 1;
+               next_per_commitment_point = PublicKey::from_secret_key(&Secp256k1::new(),
+                       &SecretKey::from_slice(&keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)).unwrap());
+       }
 
        nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(),
                &msgs::RevokeAndACK { channel_id, per_commitment_secret, next_per_commitment_point });
@@ -8366,19 +8347,19 @@ fn test_channel_update_has_correct_htlc_maximum_msat() {
 
        // Assert that `node[0]`'s `ChannelUpdate` is capped at 50 percent of the `channel_value`, as
        // that's the value of `node[1]`'s `holder_max_htlc_value_in_flight_msat`.
-       assert_eq!(node_0_chan_update.contents.htlc_maximum_msat, OptionalField::Present(channel_value_50_percent_msat));
+       assert_eq!(node_0_chan_update.contents.htlc_maximum_msat, channel_value_50_percent_msat);
        // Assert that `node[1]`'s `ChannelUpdate` is capped at 30 percent of the `channel_value`, as
        // that's the value of `node[0]`'s `holder_max_htlc_value_in_flight_msat`.
-       assert_eq!(node_1_chan_update.contents.htlc_maximum_msat, OptionalField::Present(channel_value_30_percent_msat));
+       assert_eq!(node_1_chan_update.contents.htlc_maximum_msat, channel_value_30_percent_msat);
 
        // Assert that `node[2]`'s `ChannelUpdate` is capped at 90 percent of the `channel_value`, as
        // the value of `node[3]`'s `holder_max_htlc_value_in_flight_msat` (100%), exceeds 90% of the
        // `channel_value`.
-       assert_eq!(node_2_chan_update.contents.htlc_maximum_msat, OptionalField::Present(channel_value_90_percent_msat));
+       assert_eq!(node_2_chan_update.contents.htlc_maximum_msat, channel_value_90_percent_msat);
        // Assert that `node[3]`'s `ChannelUpdate` is capped at 90 percent of the `channel_value`, as
        // the value of `node[2]`'s `holder_max_htlc_value_in_flight_msat` (95%), exceeds 90% of the
        // `channel_value`.
-       assert_eq!(node_3_chan_update.contents.htlc_maximum_msat, OptionalField::Present(channel_value_90_percent_msat));
+       assert_eq!(node_3_chan_update.contents.htlc_maximum_msat, channel_value_90_percent_msat);
 }
 
 #[test]
@@ -8500,12 +8481,12 @@ fn test_reject_funding_before_inbound_channel_accepted() {
        // `MessageSendEvent::SendAcceptChannel` event. The message is passed to `nodes[0]`
        // `handle_accept_channel`, which is required in order for `create_funding_transaction` to
        // succeed when `nodes[0]` is passed to it.
-       {
+       let accept_chan_msg = {
                let mut lock;
                let channel = get_channel_ref!(&nodes[1], lock, temp_channel_id);
-               let accept_chan_msg = channel.get_accept_channel_message();
-               nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), InitFeatures::known(), &accept_chan_msg);
-       }
+               channel.get_accept_channel_message()
+       };
+       nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), InitFeatures::known(), &accept_chan_msg);
 
        let (temporary_channel_id, tx, _) = create_funding_transaction(&nodes[0], &nodes[1].node.get_our_node_id(), 100000, 42);
 
index 3e44cfcf024034a7de39a614e50cc5a974cfc632..c2925d7354de4142c109ee79bf7b8c33fb0819ba 100644 (file)
@@ -647,8 +647,8 @@ pub struct UnsignedChannelUpdate {
        pub cltv_expiry_delta: u16,
        /// The minimum HTLC size incoming to sender, in milli-satoshi
        pub htlc_minimum_msat: u64,
-       /// Optionally, the maximum HTLC value incoming to sender, in milli-satoshi
-       pub htlc_maximum_msat: OptionalField<u64>,
+       /// The maximum HTLC value incoming to sender, in milli-satoshi. Used to be optional.
+       pub htlc_maximum_msat: u64,
        /// The base HTLC fee charged by sender, in milli-satoshi
        pub fee_base_msat: u32,
        /// The amount to fee multiplier, in micro-satoshi
@@ -1514,14 +1514,12 @@ impl_writeable!(ChannelAnnouncement, {
 
 impl Writeable for UnsignedChannelUpdate {
        fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
-               let mut message_flags: u8 = 0;
-               if let OptionalField::Present(_) = self.htlc_maximum_msat {
-                       message_flags = 1;
-               }
+               // `message_flags` used to indicate presence of `htlc_maximum_msat`, but was deprecated in the spec.
+               const MESSAGE_FLAGS: u8 = 1;
                self.chain_hash.write(w)?;
                self.short_channel_id.write(w)?;
                self.timestamp.write(w)?;
-               let all_flags = self.flags as u16 | ((message_flags as u16) << 8);
+               let all_flags = self.flags as u16 | ((MESSAGE_FLAGS as u16) << 8);
                all_flags.write(w)?;
                self.cltv_expiry_delta.write(w)?;
                self.htlc_minimum_msat.write(w)?;
@@ -1535,22 +1533,20 @@ impl Writeable for UnsignedChannelUpdate {
 
 impl Readable for UnsignedChannelUpdate {
        fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
-               let has_htlc_maximum_msat;
                Ok(Self {
                        chain_hash: Readable::read(r)?,
                        short_channel_id: Readable::read(r)?,
                        timestamp: Readable::read(r)?,
                        flags: {
                                let flags: u16 = Readable::read(r)?;
-                               let message_flags = flags >> 8;
-                               has_htlc_maximum_msat = (message_flags as i32 & 1) == 1;
+                               // Note: we ignore the `message_flags` for now, since it was deprecated by the spec.
                                flags as u8
                        },
                        cltv_expiry_delta: Readable::read(r)?,
                        htlc_minimum_msat: Readable::read(r)?,
                        fee_base_msat: Readable::read(r)?,
                        fee_proportional_millionths: Readable::read(r)?,
-                       htlc_maximum_msat: if has_htlc_maximum_msat { Readable::read(r)? } else { OptionalField::Absent },
+                       htlc_maximum_msat: Readable::read(r)?,
                        excess_data: read_to_end(r)?,
                })
        }
@@ -2103,7 +2099,7 @@ mod tests {
                do_encoding_node_announcement(false, false, true, false, true, false, false, false);
        }
 
-       fn do_encoding_channel_update(direction: bool, disable: bool, htlc_maximum_msat: bool, excess_data: bool) {
+       fn do_encoding_channel_update(direction: bool, disable: bool, excess_data: bool) {
                let secp_ctx = Secp256k1::new();
                let (privkey_1, _) = get_keys_from!("0101010101010101010101010101010101010101010101010101010101010101", secp_ctx);
                let sig_1 = get_sig_on!(privkey_1, secp_ctx, String::from("01010101010101010101010101010101"));
@@ -2114,7 +2110,7 @@ mod tests {
                        flags: if direction { 1 } else { 0 } | if disable { 1 << 1 } else { 0 },
                        cltv_expiry_delta: 144,
                        htlc_minimum_msat: 1000000,
-                       htlc_maximum_msat: if htlc_maximum_msat { OptionalField::Present(131355275467161) } else { OptionalField::Absent },
+                       htlc_maximum_msat: 131355275467161,
                        fee_base_msat: 10000,
                        fee_proportional_millionths: 20,
                        excess_data: if excess_data { vec![0, 0, 0, 0, 59, 154, 202, 0] } else { Vec::new() }
@@ -2127,11 +2123,7 @@ mod tests {
                let mut target_value = hex::decode("d977cb9b53d93a6ff64bb5f1e158b4094b66e798fb12911168a3ccdf80a83096340a6a95da0ae8d9f776528eecdbb747eb6b545495a4319ed5378e35b21e073a").unwrap();
                target_value.append(&mut hex::decode("000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f").unwrap());
                target_value.append(&mut hex::decode("00083a840000034d013413a7").unwrap());
-               if htlc_maximum_msat {
-                       target_value.append(&mut hex::decode("01").unwrap());
-               } else {
-                       target_value.append(&mut hex::decode("00").unwrap());
-               }
+               target_value.append(&mut hex::decode("01").unwrap());
                target_value.append(&mut hex::decode("00").unwrap());
                if direction {
                        let flag = target_value.last_mut().unwrap();
@@ -2142,9 +2134,7 @@ mod tests {
                        *flag = *flag | 1 << 1;
                }
                target_value.append(&mut hex::decode("009000000000000f42400000271000000014").unwrap());
-               if htlc_maximum_msat {
-                       target_value.append(&mut hex::decode("0000777788889999").unwrap());
-               }
+               target_value.append(&mut hex::decode("0000777788889999").unwrap());
                if excess_data {
                        target_value.append(&mut hex::decode("000000003b9aca00").unwrap());
                }
@@ -2153,16 +2143,14 @@ mod tests {
 
        #[test]
        fn encoding_channel_update() {
-               do_encoding_channel_update(false, false, false, false);
-               do_encoding_channel_update(false, false, false, true);
-               do_encoding_channel_update(true, false, false, false);
-               do_encoding_channel_update(true, false, false, true);
-               do_encoding_channel_update(false, true, false, false);
-               do_encoding_channel_update(false, true, false, true);
-               do_encoding_channel_update(false, false, true, false);
-               do_encoding_channel_update(false, false, true, true);
-               do_encoding_channel_update(true, true, true, false);
-               do_encoding_channel_update(true, true, true, true);
+               do_encoding_channel_update(false, false, false);
+               do_encoding_channel_update(false, false, true);
+               do_encoding_channel_update(true, false, false);
+               do_encoding_channel_update(true, false, true);
+               do_encoding_channel_update(false, true, false);
+               do_encoding_channel_update(false, true, true);
+               do_encoding_channel_update(true, true, false);
+               do_encoding_channel_update(true, true, true);
        }
 
        fn do_encoding_open_channel(random_bit: bool, shutdown: bool, incl_chan_type: bool) {
index b9d86fe3a6f9785868b3d906852efa0a443a10f3..e2c6432337fcadf0e5a5a498132681f26a7f997f 100644 (file)
@@ -21,7 +21,7 @@ use routing::gossip::{NetworkUpdate, RoutingFees, NodeId};
 use routing::router::{get_route, PaymentParameters, Route, RouteHint, RouteHintHop};
 use ln::features::{InitFeatures, InvoiceFeatures, NodeFeatures};
 use ln::msgs;
-use ln::msgs::{ChannelMessageHandler, ChannelUpdate, OptionalField};
+use ln::msgs::{ChannelMessageHandler, ChannelUpdate};
 use ln::wire::Encode;
 use util::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
 use util::ser::{ReadableArgs, Writeable, Writer};
@@ -227,7 +227,7 @@ impl msgs::ChannelUpdate {
                                flags: 0,
                                cltv_expiry_delta: 0,
                                htlc_minimum_msat: 0,
-                               htlc_maximum_msat: OptionalField::Absent,
+                               htlc_maximum_msat: msgs::MAX_VALUE_MSAT,
                                fee_base_msat: 0,
                                fee_proportional_millionths: 0,
                                excess_data: vec![],
index 2a026821f8adf91a9f0d2ba9bfdacbc861e1db48..d34fdfeb52a1ddcb887bfd42c4cfcddac8e7d0e9 100644 (file)
@@ -622,8 +622,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref, CMH: Deref> P
        /// peer using the init message.
        /// The user should pass the remote network address of the host they are connected to.
        ///
-       /// Note that if an Err is returned here you MUST NOT call socket_disconnected for the new
-       /// descriptor but must disconnect the connection immediately.
+       /// If an `Err` is returned here you must disconnect the connection immediately.
        ///
        /// Returns a small number of bytes to send to the remote node (currently always 50).
        ///
@@ -671,9 +670,8 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref, CMH: Deref> P
        /// The user should pass the remote network address of the host they are connected to.
        ///
        /// May refuse the connection by returning an Err, but will never write bytes to the remote end
-       /// (outbound connector always speaks first). Note that if an Err is returned here you MUST NOT
-       /// call socket_disconnected for the new descriptor but must disconnect the connection
-       /// immediately.
+       /// (outbound connector always speaks first). If an `Err` is returned here you must disconnect
+       /// the connection immediately.
        ///
        /// Panics if descriptor is duplicative with some other descriptor which has not yet been
        /// [`socket_disconnected()`].
@@ -1926,11 +1924,18 @@ mod tests {
                peer_a.new_inbound_connection(fd_a.clone(), None).unwrap();
                assert_eq!(peer_a.read_event(&mut fd_a, &initial_data).unwrap(), false);
                peer_a.process_events();
-               assert_eq!(peer_b.read_event(&mut fd_b, &fd_a.outbound_data.lock().unwrap().split_off(0)).unwrap(), false);
+
+               let a_data = fd_a.outbound_data.lock().unwrap().split_off(0);
+               assert_eq!(peer_b.read_event(&mut fd_b, &a_data).unwrap(), false);
+
                peer_b.process_events();
-               assert_eq!(peer_a.read_event(&mut fd_a, &fd_b.outbound_data.lock().unwrap().split_off(0)).unwrap(), false);
+               let b_data = fd_b.outbound_data.lock().unwrap().split_off(0);
+               assert_eq!(peer_a.read_event(&mut fd_a, &b_data).unwrap(), false);
+
                peer_a.process_events();
-               assert_eq!(peer_b.read_event(&mut fd_b, &fd_a.outbound_data.lock().unwrap().split_off(0)).unwrap(), false);
+               let a_data = fd_a.outbound_data.lock().unwrap().split_off(0);
+               assert_eq!(peer_b.read_event(&mut fd_b, &a_data).unwrap(), false);
+
                (fd_a.clone(), fd_b.clone())
        }
 
@@ -2084,14 +2089,16 @@ mod tests {
 
                assert_eq!(peers[0].read_event(&mut fd_a, &initial_data).unwrap(), false);
                peers[0].process_events();
-               assert_eq!(peers[1].read_event(&mut fd_b, &fd_a.outbound_data.lock().unwrap().split_off(0)).unwrap(), false);
+               let a_data = fd_a.outbound_data.lock().unwrap().split_off(0);
+               assert_eq!(peers[1].read_event(&mut fd_b, &a_data).unwrap(), false);
                peers[1].process_events();
 
                // ...but if we get a second timer tick, we should disconnect the peer
                peers[0].timer_tick_occurred();
                assert_eq!(peers[0].peers.read().unwrap().len(), 0);
 
-               assert!(peers[0].read_event(&mut fd_a, &fd_b.outbound_data.lock().unwrap().split_off(0)).is_err());
+               let b_data = fd_b.outbound_data.lock().unwrap().split_off(0);
+               assert!(peers[0].read_event(&mut fd_a, &b_data).is_err());
        }
 
        #[test]
index 5a02d15a9d1e7534caacdaec66aa36c080554c77..ed0311945e6de9d491986906f5be72c0cacb07ae 100644 (file)
@@ -19,7 +19,7 @@ use routing::gossip::RoutingFees;
 use routing::router::{PaymentParameters, RouteHint, RouteHintHop};
 use ln::features::{InitFeatures, InvoiceFeatures, ChannelTypeFeatures};
 use ln::msgs;
-use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, OptionalField, ChannelUpdate, ErrorAction};
+use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, ChannelUpdate, ErrorAction};
 use ln::wire::Encode;
 use util::enforcing_trait_impls::EnforcingSigner;
 use util::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
@@ -523,7 +523,7 @@ fn test_scid_alias_returned() {
                flags: 1,
                cltv_expiry_delta: accept_forward_cfg.channel_config.cltv_expiry_delta,
                htlc_minimum_msat: 1_000,
-               htlc_maximum_msat: OptionalField::Present(1_000_000), // Defaults to 10% of the channel value
+               htlc_maximum_msat: 1_000_000, // Defaults to 10% of the channel value
                fee_base_msat: last_hop[0].counterparty.forwarding_info.as_ref().unwrap().fee_base_msat,
                fee_proportional_millionths: last_hop[0].counterparty.forwarding_info.as_ref().unwrap().fee_proportional_millionths,
                excess_data: Vec::new(),
index f97bdb1bd95943395f15a33b114fa8a60bad1d77..fab6962e514957209b832c786916575be95fed4c 100644 (file)
@@ -82,7 +82,7 @@ fn do_test_onchain_htlc_reorg(local_commitment: bool, claim: bool) {
                check_added_monitors!(nodes[2], 1);
                check_closed_broadcast!(nodes[2], true); // We should get a BroadcastChannelUpdate (and *only* a BroadcstChannelUpdate)
                check_closed_event!(nodes[2], 1, ClosureReason::CommitmentTxConfirmed);
-               let node_2_commitment_txn = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap();
+               let node_2_commitment_txn = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
                assert_eq!(node_2_commitment_txn.len(), 3); // ChannelMonitor: 1 offered HTLC-Claim, ChannelManger: 1 local commitment tx, 1 Received HTLC-Claim
                assert_eq!(node_2_commitment_txn[1].output.len(), 2); // to-remote and Received HTLC (to-self is dust)
                check_spends!(node_2_commitment_txn[1], chan_2.3);
index 10ce74b971e6eebd13054257826aa78cbec94276..37bf1ea5a6f9757d1da5a29f636aa0a853f2de8e 100644 (file)
@@ -25,15 +25,16 @@ use chain;
 use chain::Access;
 use ln::features::{ChannelFeatures, NodeFeatures};
 use ln::msgs::{DecodeError, ErrorAction, Init, LightningError, RoutingMessageHandler, NetAddress, MAX_VALUE_MSAT};
-use ln::msgs::{ChannelAnnouncement, ChannelUpdate, NodeAnnouncement, OptionalField, GossipTimestampFilter};
+use ln::msgs::{ChannelAnnouncement, ChannelUpdate, NodeAnnouncement, GossipTimestampFilter};
 use ln::msgs::{QueryChannelRange, ReplyChannelRange, QueryShortChannelIds, ReplyShortChannelIdsEnd};
 use ln::msgs;
-use util::ser::{Readable, ReadableArgs, Writeable, Writer};
+use util::ser::{Readable, ReadableArgs, Writeable, Writer, MaybeReadable};
 use util::logger::{Logger, Level};
 use util::events::{Event, EventHandler, MessageSendEvent, MessageSendEventsProvider};
 use util::scid_utils::{block_from_scid, scid_from_parts, MAX_SCID_BLOCK};
 
 use io;
+use io_extras::{copy, sink};
 use prelude::*;
 use alloc::collections::{BTreeMap, btree_map::Entry as BtreeEntry};
 use core::{cmp, fmt};
@@ -611,7 +612,7 @@ pub struct ChannelUpdateInfo {
        /// The minimum value, which must be relayed to the next hop via the channel
        pub htlc_minimum_msat: u64,
        /// The maximum value which may be relayed to the next hop via the channel.
-       pub htlc_maximum_msat: Option<u64>,
+       pub htlc_maximum_msat: u64,
        /// Fees charged when the channel is used for routing
        pub fees: RoutingFees,
        /// Most recent update for the channel received from the network
@@ -628,15 +629,58 @@ impl fmt::Display for ChannelUpdateInfo {
        }
 }
 
-impl_writeable_tlv_based!(ChannelUpdateInfo, {
-       (0, last_update, required),
-       (2, enabled, required),
-       (4, cltv_expiry_delta, required),
-       (6, htlc_minimum_msat, required),
-       (8, htlc_maximum_msat, required),
-       (10, fees, required),
-       (12, last_update_message, required),
-});
+impl Writeable for ChannelUpdateInfo {
+       fn write<W: ::util::ser::Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
+               write_tlv_fields!(writer, {
+                       (0, self.last_update, required),
+                       (2, self.enabled, required),
+                       (4, self.cltv_expiry_delta, required),
+                       (6, self.htlc_minimum_msat, required),
+                       // Writing htlc_maximum_msat as an Option<u64> is required to maintain backwards
+                       // compatibility with LDK versions prior to v0.0.110.
+                       (8, Some(self.htlc_maximum_msat), required),
+                       (10, self.fees, required),
+                       (12, self.last_update_message, required),
+               });
+               Ok(())
+       }
+}
+
+impl Readable for ChannelUpdateInfo {
+       fn read<R: io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
+               init_tlv_field_var!(last_update, required);
+               init_tlv_field_var!(enabled, required);
+               init_tlv_field_var!(cltv_expiry_delta, required);
+               init_tlv_field_var!(htlc_minimum_msat, required);
+               init_tlv_field_var!(htlc_maximum_msat, option);
+               init_tlv_field_var!(fees, required);
+               init_tlv_field_var!(last_update_message, required);
+
+               read_tlv_fields!(reader, {
+                       (0, last_update, required),
+                       (2, enabled, required),
+                       (4, cltv_expiry_delta, required),
+                       (6, htlc_minimum_msat, required),
+                       (8, htlc_maximum_msat, required),
+                       (10, fees, required),
+                       (12, last_update_message, required)
+               });
+
+               if let Some(htlc_maximum_msat) = htlc_maximum_msat {
+                       Ok(ChannelUpdateInfo {
+                               last_update: init_tlv_based_struct_field!(last_update, required),
+                               enabled: init_tlv_based_struct_field!(enabled, required),
+                               cltv_expiry_delta: init_tlv_based_struct_field!(cltv_expiry_delta, required),
+                               htlc_minimum_msat: init_tlv_based_struct_field!(htlc_minimum_msat, required),
+                               htlc_maximum_msat,
+                               fees: init_tlv_based_struct_field!(fees, required),
+                               last_update_message: init_tlv_based_struct_field!(last_update_message, required),
+                       })
+               } else {
+                       Err(DecodeError::InvalidValue)
+               }
+       }
+}
 
 #[derive(Clone, Debug, PartialEq)]
 /// Details about a channel (both directions).
@@ -715,16 +759,73 @@ impl fmt::Display for ChannelInfo {
        }
 }
 
-impl_writeable_tlv_based!(ChannelInfo, {
-       (0, features, required),
-       (1, announcement_received_time, (default_value, 0)),
-       (2, node_one, required),
-       (4, one_to_two, required),
-       (6, node_two, required),
-       (8, two_to_one, required),
-       (10, capacity_sats, required),
-       (12, announcement_message, required),
-});
+impl Writeable for ChannelInfo {
+       fn write<W: ::util::ser::Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
+               write_tlv_fields!(writer, {
+                       (0, self.features, required),
+                       (1, self.announcement_received_time, (default_value, 0)),
+                       (2, self.node_one, required),
+                       (4, self.one_to_two, required),
+                       (6, self.node_two, required),
+                       (8, self.two_to_one, required),
+                       (10, self.capacity_sats, required),
+                       (12, self.announcement_message, required),
+               });
+               Ok(())
+       }
+}
+
+// A wrapper allowing for the optional deseralization of ChannelUpdateInfo. Utilizing this is
+// necessary to maintain backwards compatibility with previous serializations of `ChannelUpdateInfo`
+// that may have no `htlc_maximum_msat` field set. In case the field is absent, we simply ignore
+// the error and continue reading the `ChannelInfo`. Hopefully, we'll then eventually receive newer
+// channel updates via the gossip network.
+struct ChannelUpdateInfoDeserWrapper(Option<ChannelUpdateInfo>);
+
+impl MaybeReadable for ChannelUpdateInfoDeserWrapper {
+       fn read<R: io::Read>(reader: &mut R) -> Result<Option<Self>, DecodeError> {
+               match ::util::ser::Readable::read(reader) {
+                       Ok(channel_update_option) => Ok(Some(Self(channel_update_option))),
+                       Err(DecodeError::ShortRead) => Ok(None),
+                       Err(DecodeError::InvalidValue) => Ok(None),
+                       Err(err) => Err(err),
+               }
+       }
+}
+
+impl Readable for ChannelInfo {
+       fn read<R: io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
+               init_tlv_field_var!(features, required);
+               init_tlv_field_var!(announcement_received_time, (default_value, 0));
+               init_tlv_field_var!(node_one, required);
+               let mut one_to_two_wrap: Option<ChannelUpdateInfoDeserWrapper> = None;
+               init_tlv_field_var!(node_two, required);
+               let mut two_to_one_wrap: Option<ChannelUpdateInfoDeserWrapper> = None;
+               init_tlv_field_var!(capacity_sats, required);
+               init_tlv_field_var!(announcement_message, required);
+               read_tlv_fields!(reader, {
+                       (0, features, required),
+                       (1, announcement_received_time, (default_value, 0)),
+                       (2, node_one, required),
+                       (4, one_to_two_wrap, ignorable),
+                       (6, node_two, required),
+                       (8, two_to_one_wrap, ignorable),
+                       (10, capacity_sats, required),
+                       (12, announcement_message, required),
+               });
+
+               Ok(ChannelInfo {
+                       features: init_tlv_based_struct_field!(features, required),
+                       node_one: init_tlv_based_struct_field!(node_one, required),
+                       one_to_two: one_to_two_wrap.map(|w| w.0).unwrap_or(None),
+                       node_two: init_tlv_based_struct_field!(node_two, required),
+                       two_to_one: two_to_one_wrap.map(|w| w.0).unwrap_or(None),
+                       capacity_sats: init_tlv_based_struct_field!(capacity_sats, required),
+                       announcement_message: init_tlv_based_struct_field!(announcement_message, required),
+                       announcement_received_time: init_tlv_based_struct_field!(announcement_received_time, (default_value, 0)),
+               })
+       }
+}
 
 /// A wrapper around [`ChannelInfo`] representing information about the channel as directed from a
 /// source node to a target node.
@@ -739,7 +840,7 @@ pub struct DirectedChannelInfo<'a> {
 impl<'a> DirectedChannelInfo<'a> {
        #[inline]
        fn new(channel: &'a ChannelInfo, direction: Option<&'a ChannelUpdateInfo>) -> Self {
-               let htlc_maximum_msat = direction.and_then(|direction| direction.htlc_maximum_msat);
+               let htlc_maximum_msat = direction.map(|direction| direction.htlc_maximum_msat);
                let capacity_msat = channel.capacity_sats.map(|capacity_sats| capacity_sats * 1000);
 
                let (htlc_maximum_msat, effective_capacity) = match (htlc_maximum_msat, capacity_msat) {
@@ -989,11 +1090,54 @@ impl fmt::Display for NodeInfo {
        }
 }
 
-impl_writeable_tlv_based!(NodeInfo, {
-       (0, lowest_inbound_channel_fees, option),
-       (2, announcement_info, option),
-       (4, channels, vec_type),
-});
+impl Writeable for NodeInfo {
+       fn write<W: ::util::ser::Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
+               write_tlv_fields!(writer, {
+                       (0, self.lowest_inbound_channel_fees, option),
+                       (2, self.announcement_info, option),
+                       (4, self.channels, vec_type),
+               });
+               Ok(())
+       }
+}
+
+// A wrapper allowing for the optional deseralization of `NodeAnnouncementInfo`. Utilizing this is
+// necessary to maintain compatibility with previous serializations of `NetAddress` that have an
+// invalid hostname set. We ignore and eat all errors until we are either able to read a
+// `NodeAnnouncementInfo` or hit a `ShortRead`, i.e., read the TLV field to the end.
+struct NodeAnnouncementInfoDeserWrapper(NodeAnnouncementInfo);
+
+impl MaybeReadable for NodeAnnouncementInfoDeserWrapper {
+       fn read<R: io::Read>(reader: &mut R) -> Result<Option<Self>, DecodeError> {
+               match ::util::ser::Readable::read(reader) {
+                       Ok(node_announcement_info) => return Ok(Some(Self(node_announcement_info))),
+                       Err(_) => {
+                               copy(reader, &mut sink()).unwrap();
+                               return Ok(None)
+                       },
+               };
+       }
+}
+
+impl Readable for NodeInfo {
+       fn read<R: io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
+               init_tlv_field_var!(lowest_inbound_channel_fees, option);
+               let mut announcement_info_wrap: Option<NodeAnnouncementInfoDeserWrapper> = None;
+               init_tlv_field_var!(channels, vec_type);
+
+               read_tlv_fields!(reader, {
+                       (0, lowest_inbound_channel_fees, option),
+                       (2, announcement_info_wrap, ignorable),
+                       (4, channels, vec_type),
+               });
+
+               Ok(NodeInfo {
+                       lowest_inbound_channel_fees: init_tlv_based_struct_field!(lowest_inbound_channel_fees, option),
+                       announcement_info: announcement_info_wrap.map(|w| w.0),
+                       channels: init_tlv_based_struct_field!(channels, vec_type),
+               })
+       }
+}
 
 const SERIALIZATION_VERSION: u8 = 1;
 const MIN_SERIALIZATION_VERSION: u8 = 1;
@@ -1495,17 +1639,19 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
                match channels.get_mut(&msg.short_channel_id) {
                        None => return Err(LightningError{err: "Couldn't find channel for update".to_owned(), action: ErrorAction::IgnoreError}),
                        Some(channel) => {
-                               if let OptionalField::Present(htlc_maximum_msat) = msg.htlc_maximum_msat {
-                                       if htlc_maximum_msat > MAX_VALUE_MSAT {
-                                               return Err(LightningError{err: "htlc_maximum_msat is larger than maximum possible msats".to_owned(), action: ErrorAction::IgnoreError});
-                                       }
+                               if msg.htlc_maximum_msat > MAX_VALUE_MSAT {
+                                       return Err(LightningError{err:
+                                               "htlc_maximum_msat is larger than maximum possible msats".to_owned(),
+                                               action: ErrorAction::IgnoreError});
+                               }
 
-                                       if let Some(capacity_sats) = channel.capacity_sats {
-                                               // It's possible channel capacity is available now, although it wasn't available at announcement (so the field is None).
-                                               // Don't query UTXO set here to reduce DoS risks.
-                                               if capacity_sats > MAX_VALUE_MSAT / 1000 || htlc_maximum_msat > capacity_sats * 1000 {
-                                                       return Err(LightningError{err: "htlc_maximum_msat is larger than channel capacity or capacity is bogus".to_owned(), action: ErrorAction::IgnoreError});
-                                               }
+                               if let Some(capacity_sats) = channel.capacity_sats {
+                                       // It's possible channel capacity is available now, although it wasn't available at announcement (so the field is None).
+                                       // Don't query UTXO set here to reduce DoS risks.
+                                       if capacity_sats > MAX_VALUE_MSAT / 1000 || msg.htlc_maximum_msat > capacity_sats * 1000 {
+                                               return Err(LightningError{err:
+                                                       "htlc_maximum_msat is larger than channel capacity or capacity is bogus".to_owned(),
+                                                       action: ErrorAction::IgnoreError});
                                        }
                                }
                                macro_rules! check_update_latest {
@@ -1539,7 +1685,7 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
                                                        last_update: msg.timestamp,
                                                        cltv_expiry_delta: msg.cltv_expiry_delta,
                                                        htlc_minimum_msat: msg.htlc_minimum_msat,
-                                                       htlc_maximum_msat: if let OptionalField::Present(max_value) = msg.htlc_maximum_msat { Some(max_value) } else { None },
+                                                       htlc_maximum_msat: msg.htlc_maximum_msat,
                                                        fees: RoutingFees {
                                                                base_msat: msg.fee_base_msat,
                                                                proportional_millionths: msg.fee_proportional_millionths,
@@ -1680,8 +1826,8 @@ mod tests {
        use chain;
        use ln::PaymentHash;
        use ln::features::{ChannelFeatures, InitFeatures, NodeFeatures};
-       use routing::gossip::{P2PGossipSync, NetworkGraph, NetworkUpdate, NodeAlias, MAX_EXCESS_BYTES_FOR_RELAY};
-       use ln::msgs::{Init, OptionalField, RoutingMessageHandler, UnsignedNodeAnnouncement, NodeAnnouncement,
+       use routing::gossip::{P2PGossipSync, NetworkGraph, NetworkUpdate, NodeAlias, MAX_EXCESS_BYTES_FOR_RELAY, NodeId, RoutingFees, ChannelUpdateInfo, ChannelInfo, NodeAnnouncementInfo, NodeInfo};
+       use ln::msgs::{Init, RoutingMessageHandler, UnsignedNodeAnnouncement, NodeAnnouncement,
                UnsignedChannelAnnouncement, ChannelAnnouncement, UnsignedChannelUpdate, ChannelUpdate,
                ReplyChannelRange, QueryChannelRange, QueryShortChannelIds, MAX_VALUE_MSAT};
        use util::test_utils;
@@ -1805,7 +1951,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 144,
                        htlc_minimum_msat: 1_000_000,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: 1_000_000,
                        fee_base_msat: 10_000,
                        fee_proportional_millionths: 20,
                        excess_data: Vec::new()
@@ -2026,7 +2172,7 @@ mod tests {
                let valid_channel_update = get_signed_channel_update(|_| {}, node_1_privkey, &secp_ctx);
                match gossip_sync.handle_channel_update(&valid_channel_update) {
                        Ok(res) => assert!(res),
-                       _ => panic!()
+                       _ => panic!(),
                };
 
                {
@@ -2059,7 +2205,7 @@ mod tests {
                };
 
                let valid_channel_update = get_signed_channel_update(|unsigned_channel_update| {
-                       unsigned_channel_update.htlc_maximum_msat = OptionalField::Present(MAX_VALUE_MSAT + 1);
+                       unsigned_channel_update.htlc_maximum_msat = MAX_VALUE_MSAT + 1;
                        unsigned_channel_update.timestamp += 110;
                }, node_1_privkey, &secp_ctx);
                match gossip_sync.handle_channel_update(&valid_channel_update) {
@@ -2068,7 +2214,7 @@ mod tests {
                };
 
                let valid_channel_update = get_signed_channel_update(|unsigned_channel_update| {
-                       unsigned_channel_update.htlc_maximum_msat = OptionalField::Present(amount_sats * 1000 + 1);
+                       unsigned_channel_update.htlc_maximum_msat = amount_sats * 1000 + 1;
                        unsigned_channel_update.timestamp += 110;
                }, node_1_privkey, &secp_ctx);
                match gossip_sync.handle_channel_update(&valid_channel_update) {
@@ -2806,6 +2952,142 @@ mod tests {
                assert_eq!(format_bytes_alias(b"\xFFI <heart>\0LDK!"), "\u{FFFD}I <heart>");
                assert_eq!(format_bytes_alias(b"\xFFI <heart>\tLDK!"), "\u{FFFD}I <heart>\u{FFFD}LDK!");
        }
+
+       #[test]
+       fn channel_info_is_readable() {
+               let chanmon_cfgs = ::ln::functional_test_utils::create_chanmon_cfgs(2);
+               let node_cfgs = ::ln::functional_test_utils::create_node_cfgs(2, &chanmon_cfgs);
+               let node_chanmgrs = ::ln::functional_test_utils::create_node_chanmgrs(2, &node_cfgs, &[None, None, None, None]);
+               let nodes = ::ln::functional_test_utils::create_network(2, &node_cfgs, &node_chanmgrs);
+
+               // 1. Test encoding/decoding of ChannelUpdateInfo
+               let chan_update_info = ChannelUpdateInfo {
+                       last_update: 23,
+                       enabled: true,
+                       cltv_expiry_delta: 42,
+                       htlc_minimum_msat: 1234,
+                       htlc_maximum_msat: 5678,
+                       fees: RoutingFees { base_msat: 9, proportional_millionths: 10 },
+                       last_update_message: None,
+               };
+
+               let mut encoded_chan_update_info: Vec<u8> = Vec::new();
+               assert!(chan_update_info.write(&mut encoded_chan_update_info).is_ok());
+
+               // First make sure we can read ChannelUpdateInfos we just wrote
+               let read_chan_update_info: ChannelUpdateInfo = ::util::ser::Readable::read(&mut encoded_chan_update_info.as_slice()).unwrap();
+               assert_eq!(chan_update_info, read_chan_update_info);
+
+               // Check the serialization hasn't changed.
+               let legacy_chan_update_info_with_some: Vec<u8> = hex::decode("340004000000170201010402002a060800000000000004d2080909000000000000162e0a0d0c00040000000902040000000a0c0100").unwrap();
+               assert_eq!(encoded_chan_update_info, legacy_chan_update_info_with_some);
+
+               // Check we fail if htlc_maximum_msat is not present in either the ChannelUpdateInfo itself
+               // or the ChannelUpdate enclosed with `last_update_message`.
+               let legacy_chan_update_info_with_some_and_fail_update: Vec<u8> = hex::decode("b40004000000170201010402002a060800000000000004d2080909000000000000162e0a0d0c00040000000902040000000a0c8181d977cb9b53d93a6ff64bb5f1e158b4094b66e798fb12911168a3ccdf80a83096340a6a95da0ae8d9f776528eecdbb747eb6b545495a4319ed5378e35b21e073a000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f00083a840000034d013413a70000009000000000000f42400000271000000014").unwrap();
+               let read_chan_update_info_res: Result<ChannelUpdateInfo, ::ln::msgs::DecodeError> = ::util::ser::Readable::read(&mut legacy_chan_update_info_with_some_and_fail_update.as_slice());
+               assert!(read_chan_update_info_res.is_err());
+
+               let legacy_chan_update_info_with_none: Vec<u8> = hex::decode("2c0004000000170201010402002a060800000000000004d20801000a0d0c00040000000902040000000a0c0100").unwrap();
+               let read_chan_update_info_res: Result<ChannelUpdateInfo, ::ln::msgs::DecodeError> = ::util::ser::Readable::read(&mut legacy_chan_update_info_with_none.as_slice());
+               assert!(read_chan_update_info_res.is_err());
+                       
+               // 2. Test encoding/decoding of ChannelInfo
+               // Check we can encode/decode ChannelInfo without ChannelUpdateInfo fields present.
+               let chan_info_none_updates = ChannelInfo {
+                       features: ChannelFeatures::known(),
+                       node_one: NodeId::from_pubkey(&nodes[0].node.get_our_node_id()),
+                       one_to_two: None,
+                       node_two: NodeId::from_pubkey(&nodes[1].node.get_our_node_id()),
+                       two_to_one: None,
+                       capacity_sats: None,
+                       announcement_message: None,
+                       announcement_received_time: 87654,
+               };
+
+               let mut encoded_chan_info: Vec<u8> = Vec::new();
+               assert!(chan_info_none_updates.write(&mut encoded_chan_info).is_ok());
+
+               let read_chan_info: ChannelInfo = ::util::ser::Readable::read(&mut encoded_chan_info.as_slice()).unwrap();
+               assert_eq!(chan_info_none_updates, read_chan_info);
+
+               // Check we can encode/decode ChannelInfo with ChannelUpdateInfo fields present.
+               let chan_info_some_updates = ChannelInfo {
+                       features: ChannelFeatures::known(),
+                       node_one: NodeId::from_pubkey(&nodes[0].node.get_our_node_id()),
+                       one_to_two: Some(chan_update_info.clone()),
+                       node_two: NodeId::from_pubkey(&nodes[1].node.get_our_node_id()),
+                       two_to_one: Some(chan_update_info.clone()),
+                       capacity_sats: None,
+                       announcement_message: None,
+                       announcement_received_time: 87654,
+               };
+
+               let mut encoded_chan_info: Vec<u8> = Vec::new();
+               assert!(chan_info_some_updates.write(&mut encoded_chan_info).is_ok());
+
+               let read_chan_info: ChannelInfo = ::util::ser::Readable::read(&mut encoded_chan_info.as_slice()).unwrap();
+               assert_eq!(chan_info_some_updates, read_chan_info);
+
+               // Check the serialization hasn't changed.
+               let legacy_chan_info_with_some: Vec<u8> = hex::decode("ca00020000010800000000000156660221027f921585f2ac0c7c70e36110adecfd8fd14b8a99bfb3d000a283fcac358fce88043636340004000000170201010402002a060800000000000004d2080909000000000000162e0a0d0c00040000000902040000000a0c010006210355f8d2238a322d16b602bd0ceaad5b01019fb055971eaadcc9b29226a4da6c23083636340004000000170201010402002a060800000000000004d2080909000000000000162e0a0d0c00040000000902040000000a0c01000a01000c0100").unwrap();
+               assert_eq!(encoded_chan_info, legacy_chan_info_with_some);
+
+               // Check we can decode legacy ChannelInfo, even if the `two_to_one` / `one_to_two` /
+               // `last_update_message` fields fail to decode due to missing htlc_maximum_msat.
+               let legacy_chan_info_with_some_and_fail_update = hex::decode("fd01ca00020000010800000000000156660221027f921585f2ac0c7c70e36110adecfd8fd14b8a99bfb3d000a283fcac358fce8804b6b6b40004000000170201010402002a060800000000000004d2080909000000000000162e0a0d0c00040000000902040000000a0c8181d977cb9b53d93a6ff64bb5f1e158b4094b66e798fb12911168a3ccdf80a83096340a6a95da0ae8d9f776528eecdbb747eb6b545495a4319ed5378e35b21e073a000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f00083a840000034d013413a70000009000000000000f4240000027100000001406210355f8d2238a322d16b602bd0ceaad5b01019fb055971eaadcc9b29226a4da6c2308b6b6b40004000000170201010402002a060800000000000004d2080909000000000000162e0a0d0c00040000000902040000000a0c8181d977cb9b53d93a6ff64bb5f1e158b4094b66e798fb12911168a3ccdf80a83096340a6a95da0ae8d9f776528eecdbb747eb6b545495a4319ed5378e35b21e073a000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f00083a840000034d013413a70000009000000000000f424000002710000000140a01000c0100").unwrap();
+               let read_chan_info: ChannelInfo = ::util::ser::Readable::read(&mut legacy_chan_info_with_some_and_fail_update.as_slice()).unwrap();
+               assert_eq!(read_chan_info.announcement_received_time, 87654);
+               assert_eq!(read_chan_info.one_to_two, None);
+               assert_eq!(read_chan_info.two_to_one, None);
+
+               let legacy_chan_info_with_none: Vec<u8> = hex::decode("ba00020000010800000000000156660221027f921585f2ac0c7c70e36110adecfd8fd14b8a99bfb3d000a283fcac358fce88042e2e2c0004000000170201010402002a060800000000000004d20801000a0d0c00040000000902040000000a0c010006210355f8d2238a322d16b602bd0ceaad5b01019fb055971eaadcc9b29226a4da6c23082e2e2c0004000000170201010402002a060800000000000004d20801000a0d0c00040000000902040000000a0c01000a01000c0100").unwrap();
+               let read_chan_info: ChannelInfo = ::util::ser::Readable::read(&mut legacy_chan_info_with_none.as_slice()).unwrap();
+               assert_eq!(read_chan_info.announcement_received_time, 87654);
+               assert_eq!(read_chan_info.one_to_two, None);
+               assert_eq!(read_chan_info.two_to_one, None);
+       }
+
+       #[test]
+       fn node_info_is_readable() {
+               use std::convert::TryFrom;
+
+               // 1. Check we can read a valid NodeAnnouncementInfo and fail on an invalid one
+               let valid_netaddr = ::ln::msgs::NetAddress::Hostname { hostname: ::util::ser::Hostname::try_from("A".to_string()).unwrap(), port: 1234 };
+               let valid_node_ann_info = NodeAnnouncementInfo {
+                       features: NodeFeatures::known(),
+                       last_update: 0,
+                       rgb: [0u8; 3],
+                       alias: NodeAlias([0u8; 32]),
+                       addresses: vec![valid_netaddr],
+                       announcement_message: None,
+               };
+
+               let mut encoded_valid_node_ann_info = Vec::new();
+               assert!(valid_node_ann_info.write(&mut encoded_valid_node_ann_info).is_ok());
+               let read_valid_node_ann_info: NodeAnnouncementInfo = ::util::ser::Readable::read(&mut encoded_valid_node_ann_info.as_slice()).unwrap();
+               assert_eq!(read_valid_node_ann_info, valid_node_ann_info);
+
+               let encoded_invalid_node_ann_info = hex::decode("3f0009000788a000080a51a20204000000000403000000062000000000000000000000000000000000000000000000000000000000000000000a0505014004d2").unwrap();
+               let read_invalid_node_ann_info_res: Result<NodeAnnouncementInfo, ::ln::msgs::DecodeError> = ::util::ser::Readable::read(&mut encoded_invalid_node_ann_info.as_slice());
+               assert!(read_invalid_node_ann_info_res.is_err());
+
+               // 2. Check we can read a NodeInfo anyways, but set the NodeAnnouncementInfo to None if invalid
+               let valid_node_info = NodeInfo {
+                       channels: Vec::new(),
+                       lowest_inbound_channel_fees: None,
+                       announcement_info: Some(valid_node_ann_info),
+               };
+
+               let mut encoded_valid_node_info = Vec::new();
+               assert!(valid_node_info.write(&mut encoded_valid_node_info).is_ok());
+               let read_valid_node_info: NodeInfo = ::util::ser::Readable::read(&mut encoded_valid_node_info.as_slice()).unwrap();
+               assert_eq!(read_valid_node_info, valid_node_info);
+
+               let encoded_invalid_node_info_hex = hex::decode("4402403f0009000788a000080a51a20204000000000403000000062000000000000000000000000000000000000000000000000000000000000000000a0505014004d20400").unwrap();
+               let read_invalid_node_info: NodeInfo = ::util::ser::Readable::read(&mut encoded_invalid_node_info_hex.as_slice()).unwrap();
+               assert_eq!(read_invalid_node_info.announcement_info, None);
+       }
 }
 
 #[cfg(all(test, feature = "_bench_unstable"))]
index 27a21a1899e1014be3c7280f2e1b09a2e8459f60..4c00dc5fcf9327126d0f9bd580823fd1447ea4f7 100644 (file)
@@ -238,8 +238,13 @@ pub struct PaymentParameters {
        /// A value of 0 will allow payments up to and including a channel's total announced usable
        /// capacity, a value of one will only use up to half its capacity, two 1/4, etc.
        ///
-       /// Default value: 1
+       /// Default value: 2
        pub max_channel_saturation_power_of_half: u8,
+
+       /// A list of SCIDs which this payment was previously attempted over and which caused the
+       /// payment to fail. Future attempts for the same payment shouldn't be relayed through any of
+       /// these SCIDs.
+       pub previously_failed_channels: Vec<u64>,
 }
 
 impl_writeable_tlv_based!(PaymentParameters, {
@@ -248,8 +253,9 @@ impl_writeable_tlv_based!(PaymentParameters, {
        (2, features, option),
        (3, max_path_count, (default_value, DEFAULT_MAX_PATH_COUNT)),
        (4, route_hints, vec_type),
-       (5, max_channel_saturation_power_of_half, (default_value, 1)),
+       (5, max_channel_saturation_power_of_half, (default_value, 2)),
        (6, expiry_time, option),
+       (7, previously_failed_channels, vec_type),
 });
 
 impl PaymentParameters {
@@ -262,7 +268,8 @@ impl PaymentParameters {
                        expiry_time: None,
                        max_total_cltv_expiry_delta: DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA,
                        max_path_count: DEFAULT_MAX_PATH_COUNT,
-                       max_channel_saturation_power_of_half: 1,
+                       max_channel_saturation_power_of_half: 2,
+                       previously_failed_channels: Vec::new(),
                }
        }
 
@@ -747,7 +754,7 @@ where L::Target: Logger, GL::Target: Logger {
 pub(crate) fn get_route<L: Deref, S: Score>(
        our_node_pubkey: &PublicKey, payment_params: &PaymentParameters, network_graph: &ReadOnlyNetworkGraph,
        first_hops: Option<&[&ChannelDetails]>, final_value_msat: u64, final_cltv_expiry_delta: u32,
-       logger: L, scorer: &S, random_seed_bytes: &[u8; 32]
+       logger: L, scorer: &S, _random_seed_bytes: &[u8; 32]
 ) -> Result<Route, LightningError>
 where L::Target: Logger {
        let payee_node_id = NodeId::from_pubkey(&payment_params.payee_pubkey);
@@ -789,11 +796,11 @@ where L::Target: Logger {
        // 4. See if we managed to collect paths which aggregately are able to transfer target value
        //    (not recommended value).
        // 5. If yes, proceed. If not, fail routing.
-       // 6. Randomly combine paths into routes having enough to fulfill the payment. (TODO: knapsack)
-       // 7. Of all the found paths, select only those with the lowest total fee.
-       // 8. The last path in every selected route is likely to be more than we need.
-       //    Reduce its value-to-transfer and recompute fees.
-       // 9. Choose the best route by the lowest total fee.
+       // 6. Select the paths which have the lowest cost (fee plus scorer penalty) per amount
+       //    transferred up to the transfer target value.
+       // 7. Reduce the value of the last path until we are sending only the target value.
+       // 8. If our maximum channel saturation limit caused us to pick two identical paths, combine
+       //    them so that we're not sending two HTLCs along the same path.
 
        // As for the actual search algorithm,
        // we do a payee-to-payer pseudo-Dijkstra's sorting by each node's distance from the payee
@@ -1002,7 +1009,7 @@ where L::Target: Logger {
                                        let contributes_sufficient_value = available_value_contribution_msat >= minimal_value_contribution_msat;
                                        // Do not consider candidate hops that would exceed the maximum path length.
                                        let path_length_to_node = $next_hops_path_length + 1;
-                                       let doesnt_exceed_max_path_length = path_length_to_node <= MAX_PATH_LENGTH_ESTIMATE;
+                                       let exceeds_max_path_length = path_length_to_node > MAX_PATH_LENGTH_ESTIMATE;
 
                                        // Do not consider candidates that exceed the maximum total cltv expiry limit.
                                        // In order to already account for some of the privacy enhancing random CLTV
@@ -1013,7 +1020,7 @@ where L::Target: Logger {
                                                .unwrap_or(payment_params.max_total_cltv_expiry_delta - final_cltv_expiry_delta);
                                        let hop_total_cltv_delta = ($next_hops_cltv_delta as u32)
                                                .saturating_add($candidate.cltv_expiry_delta());
-                                       let doesnt_exceed_cltv_delta_limit = hop_total_cltv_delta <= max_total_cltv_expiry_delta;
+                                       let exceeds_cltv_delta_limit = hop_total_cltv_delta > max_total_cltv_expiry_delta;
 
                                        let value_contribution_msat = cmp::min(available_value_contribution_msat, $next_hops_value_contribution);
                                        // Includes paying fees for the use of the following channels.
@@ -1033,15 +1040,19 @@ where L::Target: Logger {
                                                 (amount_to_transfer_over_msat < $next_hops_path_htlc_minimum_msat &&
                                                  recommended_value_msat > $next_hops_path_htlc_minimum_msat));
 
+                                       let payment_failed_on_this_channel =
+                                               payment_params.previously_failed_channels.contains(&short_channel_id);
+
                                        // If HTLC minimum is larger than the amount we're going to transfer, we shouldn't
                                        // bother considering this channel. If retrying with recommended_value_msat may
                                        // allow us to hit the HTLC minimum limit, set htlc_minimum_limit so that we go
                                        // around again with a higher amount.
-                                       if contributes_sufficient_value && doesnt_exceed_max_path_length &&
-                                               doesnt_exceed_cltv_delta_limit && may_overpay_to_meet_path_minimum_msat {
+                                       if !contributes_sufficient_value || exceeds_max_path_length ||
+                                               exceeds_cltv_delta_limit || payment_failed_on_this_channel {
+                                               // Path isn't useful, ignore it and move on.
+                                       } else if may_overpay_to_meet_path_minimum_msat {
                                                hit_minimum_limit = true;
-                                       } else if contributes_sufficient_value && doesnt_exceed_max_path_length &&
-                                               doesnt_exceed_cltv_delta_limit && over_path_minimum_msat {
+                                       } else if over_path_minimum_msat {
                                                // Note that low contribution here (limited by available_liquidity_msat)
                                                // might violate htlc_minimum_msat on the hops which are next along the
                                                // payment path (upstream to the payee). To avoid that, we recompute
@@ -1465,7 +1476,7 @@ where L::Target: Logger {
                // Both these cases (and other cases except reaching recommended_value_msat) mean that
                // paths_collection will be stopped because found_new_path==false.
                // This is not necessarily a routing failure.
-               'path_construction: while let Some(RouteGraphNode { node_id, lowest_fee_to_node, total_cltv_delta, value_contribution_msat, path_htlc_minimum_msat, path_penalty_msat, path_length_to_node, .. }) = targets.pop() {
+               'path_construction: while let Some(RouteGraphNode { node_id, lowest_fee_to_node, total_cltv_delta, mut value_contribution_msat, path_htlc_minimum_msat, path_penalty_msat, path_length_to_node, .. }) = targets.pop() {
 
                        // Since we're going payee-to-payer, hitting our node as a target means we should stop
                        // traversing the graph and arrange the path out of what we found.
@@ -1531,7 +1542,9 @@ where L::Target: Logger {
                                // on some channels we already passed (assuming dest->source direction). Here, we
                                // recompute the fees again, so that if that's the case, we match the currently
                                // underpaid htlc_minimum_msat with fees.
-                               payment_path.update_value_and_recompute_fees(cmp::min(value_contribution_msat, final_value_msat));
+                               debug_assert_eq!(payment_path.get_value_msat(), value_contribution_msat);
+                               value_contribution_msat = cmp::min(value_contribution_msat, final_value_msat);
+                               payment_path.update_value_and_recompute_fees(value_contribution_msat);
 
                                // Since a path allows to transfer as much value as
                                // the smallest channel it has ("bottleneck"), we should recompute
@@ -1642,96 +1655,55 @@ where L::Target: Logger {
                return Err(LightningError{err: "Failed to find a sufficient route to the given destination".to_owned(), action: ErrorAction::IgnoreError});
        }
 
-       // Sort by total fees and take the best paths.
-       payment_paths.sort_unstable_by_key(|path| path.get_total_fee_paid_msat());
-       if payment_paths.len() > 50 {
-               payment_paths.truncate(50);
-       }
+       // Step (6).
+       let mut selected_route = payment_paths;
 
-       // Draw multiple sufficient routes by randomly combining the selected paths.
-       let mut drawn_routes = Vec::new();
-       let mut prng = ChaCha20::new(random_seed_bytes, &[0u8; 12]);
-       let mut random_index_bytes = [0u8; ::core::mem::size_of::<usize>()];
+       debug_assert_eq!(selected_route.iter().map(|p| p.get_value_msat()).sum::<u64>(), already_collected_value_msat);
+       let mut overpaid_value_msat = already_collected_value_msat - final_value_msat;
 
-       let num_permutations = payment_paths.len();
-       for _ in 0..num_permutations {
-               let mut cur_route = Vec::<PaymentPath>::new();
-               let mut aggregate_route_value_msat = 0;
+       // First, sort by the cost-per-value of the path, dropping the paths that cost the most for
+       // the value they contribute towards the payment amount.
+       // We sort in descending order as we will remove from the front in `retain`, next.
+       selected_route.sort_unstable_by(|a, b|
+               (((b.get_cost_msat() as u128) << 64) / (b.get_value_msat() as u128))
+                       .cmp(&(((a.get_cost_msat() as u128) << 64) / (a.get_value_msat() as u128)))
+       );
 
-               // Step (6).
-               // Do a Fisher-Yates shuffle to create a random permutation of the payment paths
-               for cur_index in (1..payment_paths.len()).rev() {
-                       prng.process_in_place(&mut random_index_bytes);
-                       let random_index = usize::from_be_bytes(random_index_bytes).wrapping_rem(cur_index+1);
-                       payment_paths.swap(cur_index, random_index);
+       // We should make sure that at least 1 path left.
+       let mut paths_left = selected_route.len();
+       selected_route.retain(|path| {
+               if paths_left == 1 {
+                       return true
+               }
+               let path_value_msat = path.get_value_msat();
+               if path_value_msat <= overpaid_value_msat {
+                       overpaid_value_msat -= path_value_msat;
+                       paths_left -= 1;
+                       return false;
                }
+               true
+       });
+       debug_assert!(selected_route.len() > 0);
 
+       if overpaid_value_msat != 0 {
                // Step (7).
-               for payment_path in &payment_paths {
-                       cur_route.push(payment_path.clone());
-                       aggregate_route_value_msat += payment_path.get_value_msat();
-                       if aggregate_route_value_msat > final_value_msat {
-                               // Last path likely overpaid. Substract it from the most expensive
-                               // (in terms of proportional fee) path in this route and recompute fees.
-                               // This might be not the most economically efficient way, but fewer paths
-                               // also makes routing more reliable.
-                               let mut overpaid_value_msat = aggregate_route_value_msat - final_value_msat;
-
-                               // First, we drop some expensive low-value paths entirely if possible, since fewer
-                               // paths is better: the payment is less likely to fail. In order to do so, we sort
-                               // by value and fall back to total fees paid, i.e., in case of equal values we
-                               // prefer lower cost paths.
-                               cur_route.sort_unstable_by(|a, b| {
-                                       a.get_value_msat().cmp(&b.get_value_msat())
-                                               // Reverse ordering for cost, so we drop higher-cost paths first
-                                               .then_with(|| b.get_cost_msat().cmp(&a.get_cost_msat()))
-                               });
-
-                               // We should make sure that at least 1 path left.
-                               let mut paths_left = cur_route.len();
-                               cur_route.retain(|path| {
-                                       if paths_left == 1 {
-                                               return true
-                                       }
-                                       let mut keep = true;
-                                       let path_value_msat = path.get_value_msat();
-                                       if path_value_msat <= overpaid_value_msat {
-                                               keep = false;
-                                               overpaid_value_msat -= path_value_msat;
-                                               paths_left -= 1;
-                                       }
-                                       keep
-                               });
-
-                               if overpaid_value_msat == 0 {
-                                       break;
-                               }
+               // Now, subtract the remaining overpaid value from the most-expensive path.
+               // TODO: this could also be optimized by also sorting by feerate_per_sat_routed,
+               // so that the sender pays less fees overall. And also htlc_minimum_msat.
+               selected_route.sort_unstable_by(|a, b| {
+                       let a_f = a.hops.iter().map(|hop| hop.0.candidate.fees().proportional_millionths as u64).sum::<u64>();
+                       let b_f = b.hops.iter().map(|hop| hop.0.candidate.fees().proportional_millionths as u64).sum::<u64>();
+                       a_f.cmp(&b_f).then_with(|| b.get_cost_msat().cmp(&a.get_cost_msat()))
+               });
+               let expensive_payment_path = selected_route.first_mut().unwrap();
 
-                               assert!(cur_route.len() > 0);
-
-                               // Step (8).
-                               // Now, subtract the overpaid value from the most-expensive path.
-                               // TODO: this could also be optimized by also sorting by feerate_per_sat_routed,
-                               // so that the sender pays less fees overall. And also htlc_minimum_msat.
-                               cur_route.sort_unstable_by_key(|path| { path.hops.iter().map(|hop| hop.0.candidate.fees().proportional_millionths as u64).sum::<u64>() });
-                               let expensive_payment_path = cur_route.first_mut().unwrap();
-
-                               // We already dropped all the small value paths above, meaning all the
-                               // remaining paths are larger than remaining overpaid_value_msat.
-                               // Thus, this can't be negative.
-                               let expensive_path_new_value_msat = expensive_payment_path.get_value_msat() - overpaid_value_msat;
-                               expensive_payment_path.update_value_and_recompute_fees(expensive_path_new_value_msat);
-                               break;
-                       }
-               }
-               drawn_routes.push(cur_route);
+               // We already dropped all the paths with value below `overpaid_value_msat` above, thus this
+               // can't go negative.
+               let expensive_path_new_value_msat = expensive_payment_path.get_value_msat() - overpaid_value_msat;
+               expensive_payment_path.update_value_and_recompute_fees(expensive_path_new_value_msat);
        }
 
-       // Step (9).
-       // Select the best route by lowest total cost.
-       drawn_routes.sort_unstable_by_key(|paths| paths.iter().map(|path| path.get_cost_msat()).sum::<u64>());
-       let selected_route = drawn_routes.first_mut().unwrap();
-
+       // Step (8).
        // Sort by the path itself and combine redundant paths.
        // Note that we sort by SCIDs alone as its simpler but when combining we have to ensure we
        // compare both SCIDs and NodeIds as individual nodes may use random aliases causing collisions
@@ -1968,8 +1940,8 @@ mod tests {
        use chain::transaction::OutPoint;
        use chain::keysinterface::KeysInterface;
        use ln::features::{ChannelFeatures, InitFeatures, InvoiceFeatures, NodeFeatures};
-       use ln::msgs::{ErrorAction, LightningError, OptionalField, UnsignedChannelAnnouncement, ChannelAnnouncement, RoutingMessageHandler,
-               NodeAnnouncement, UnsignedNodeAnnouncement, ChannelUpdate, UnsignedChannelUpdate};
+       use ln::msgs::{ErrorAction, LightningError, UnsignedChannelAnnouncement, ChannelAnnouncement, RoutingMessageHandler,
+               NodeAnnouncement, UnsignedNodeAnnouncement, ChannelUpdate, UnsignedChannelUpdate, MAX_VALUE_MSAT};
        use ln::channelmanager;
        use util::test_utils;
        use util::chacha20::ChaCha20;
@@ -1993,6 +1965,8 @@ mod tests {
        use prelude::*;
        use sync::{self, Arc};
 
+       use core::convert::TryInto;
+
        fn get_channel_details(short_channel_id: Option<u64>, node_id: PublicKey,
                        features: InitFeatures, outbound_capacity_msat: u64) -> channelmanager::ChannelDetails {
                channelmanager::ChannelDetails {
@@ -2159,7 +2133,7 @@ mod tests {
                                flags: 0,
                                cltv_expiry_delta: 0,
                                htlc_minimum_msat: 0,
-                               htlc_maximum_msat: OptionalField::Absent,
+                               htlc_maximum_msat: MAX_VALUE_MSAT,
                                fee_base_msat: 0,
                                fee_proportional_millionths: 0,
                                excess_data: Vec::new()
@@ -2171,7 +2145,7 @@ mod tests {
                                flags: 1,
                                cltv_expiry_delta: 0,
                                htlc_minimum_msat: 0,
-                               htlc_maximum_msat: OptionalField::Absent,
+                               htlc_maximum_msat: MAX_VALUE_MSAT,
                                fee_base_msat: 0,
                                fee_proportional_millionths: 0,
                                excess_data: Vec::new()
@@ -2265,7 +2239,7 @@ mod tests {
                        flags: 1,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2281,7 +2255,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (5 << 4) | 3,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: u32::max_value(),
                        fee_proportional_millionths: u32::max_value(),
                        excess_data: Vec::new()
@@ -2293,7 +2267,7 @@ mod tests {
                        flags: 1,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2309,7 +2283,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (5 << 4) | 3,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: u32::max_value(),
                        fee_proportional_millionths: u32::max_value(),
                        excess_data: Vec::new()
@@ -2321,7 +2295,7 @@ mod tests {
                        flags: 1,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2337,7 +2311,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (3 << 4) | 1,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2349,7 +2323,7 @@ mod tests {
                        flags: 1,
                        cltv_expiry_delta: (3 << 4) | 2,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 100,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2363,7 +2337,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (4 << 4) | 1,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 1000000,
                        excess_data: Vec::new()
@@ -2375,7 +2349,7 @@ mod tests {
                        flags: 1,
                        cltv_expiry_delta: (4 << 4) | 2,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2389,7 +2363,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (13 << 4) | 1,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 2000000,
                        excess_data: Vec::new()
@@ -2401,7 +2375,7 @@ mod tests {
                        flags: 1,
                        cltv_expiry_delta: (13 << 4) | 2,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2417,7 +2391,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (6 << 4) | 1,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2429,7 +2403,7 @@ mod tests {
                        flags: 1,
                        cltv_expiry_delta: (6 << 4) | 2,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new(),
@@ -2443,7 +2417,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (11 << 4) | 1,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2455,7 +2429,7 @@ mod tests {
                        flags: 1,
                        cltv_expiry_delta: (11 << 4) | 2,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2473,7 +2447,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (7 << 4) | 1,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 1000000,
                        excess_data: Vec::new()
@@ -2485,7 +2459,7 @@ mod tests {
                        flags: 1,
                        cltv_expiry_delta: (7 << 4) | 2,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2570,7 +2544,7 @@ mod tests {
                        flags: 2, // to disable
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2582,7 +2556,7 @@ mod tests {
                        flags: 2, // to disable
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2594,7 +2568,7 @@ mod tests {
                        flags: 2, // to disable
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2606,7 +2580,7 @@ mod tests {
                        flags: 2, // to disable
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2618,7 +2592,7 @@ mod tests {
                        flags: 2, // to disable
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2633,7 +2607,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 200_000_000,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2648,7 +2622,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(199_999_999),
+                       htlc_maximum_msat: 199_999_999,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2667,7 +2641,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2697,7 +2671,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 35_000,
-                       htlc_maximum_msat: OptionalField::Present(40_000),
+                       htlc_maximum_msat: 40_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2709,7 +2683,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 35_000,
-                       htlc_maximum_msat: OptionalField::Present(40_000),
+                       htlc_maximum_msat: 40_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2723,7 +2697,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2735,7 +2709,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2749,7 +2723,7 @@ mod tests {
                        flags: 2, // to disable
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2770,7 +2744,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 65_000,
-                       htlc_maximum_msat: OptionalField::Present(80_000),
+                       htlc_maximum_msat: 80_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2782,7 +2756,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2794,7 +2768,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 100_000,
                        excess_data: Vec::new()
@@ -2833,7 +2807,7 @@ mod tests {
                        flags: 2, // to disable
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -2845,7 +2819,7 @@ mod tests {
                        flags: 2, // to disable
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3251,7 +3225,7 @@ mod tests {
                        flags: 2, // to disable
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3263,7 +3237,7 @@ mod tests {
                        flags: 2, // to disable
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3323,7 +3297,7 @@ mod tests {
                        flags: 2, // to disable
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3335,7 +3309,7 @@ mod tests {
                        flags: 2, // to disable
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3668,7 +3642,7 @@ mod tests {
                        flags: 2,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3680,7 +3654,7 @@ mod tests {
                        flags: 2,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3695,7 +3669,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(1_000_000_000),
+                       htlc_maximum_msat: 1_000_000_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3710,7 +3684,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: 250_000_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3743,7 +3717,7 @@ mod tests {
                        flags: 2,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(1_000_000_000),
+                       htlc_maximum_msat: 1_000_000_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3778,7 +3752,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(1_000_000_000),
+                       htlc_maximum_msat: 1_000_000_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3793,7 +3767,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(15_000),
+                       htlc_maximum_msat: 15_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3828,7 +3802,7 @@ mod tests {
                        flags: 2,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3851,7 +3825,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (3 << 4) | 1,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: 15_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3863,7 +3837,7 @@ mod tests {
                        flags: 1,
                        cltv_expiry_delta: (3 << 4) | 2,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: 15_000,
                        fee_base_msat: 100,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3895,7 +3869,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(10_000),
+                       htlc_maximum_msat: 10_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3943,7 +3917,7 @@ mod tests {
                        flags: 2,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3955,7 +3929,7 @@ mod tests {
                        flags: 2,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3970,7 +3944,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3982,7 +3956,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -3995,7 +3969,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(50_000),
+                       htlc_maximum_msat: 50_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4007,7 +3981,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4064,7 +4038,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 1_000_000,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4076,7 +4050,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(50_000),
+                       htlc_maximum_msat: 50_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4121,7 +4095,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4133,7 +4107,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(50_000),
+                       htlc_maximum_msat: 50_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4148,7 +4122,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(60_000),
+                       htlc_maximum_msat: 60_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4160,7 +4134,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(60_000),
+                       htlc_maximum_msat: 60_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4175,7 +4149,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(200_000),
+                       htlc_maximum_msat: 200_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4187,7 +4161,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(180_000),
+                       htlc_maximum_msat: 180_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4278,7 +4252,7 @@ mod tests {
                        flags: 2,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4290,7 +4264,7 @@ mod tests {
                        flags: 2,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4304,7 +4278,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4316,7 +4290,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4331,7 +4305,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(200_000),
+                       htlc_maximum_msat: 200_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4347,7 +4321,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(200_000),
+                       htlc_maximum_msat: 200_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4359,7 +4333,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(200_000),
+                       htlc_maximum_msat: 200_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4372,7 +4346,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4384,7 +4358,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4446,7 +4420,7 @@ mod tests {
                        flags: 2,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4458,7 +4432,7 @@ mod tests {
                        flags: 2,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4472,7 +4446,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4484,7 +4458,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4499,7 +4473,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(200_000),
+                       htlc_maximum_msat: 200_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4515,7 +4489,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(200_000),
+                       htlc_maximum_msat: 200_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4527,7 +4501,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(200_000),
+                       htlc_maximum_msat: 200_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4540,7 +4514,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 1_000,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4552,7 +4526,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4613,7 +4587,7 @@ mod tests {
                        flags: 2,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4626,7 +4600,7 @@ mod tests {
                        flags: 2,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4640,7 +4614,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4652,7 +4626,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4666,7 +4640,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4689,7 +4663,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(250_000),
+                       htlc_maximum_msat: 250_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4701,7 +4675,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4714,7 +4688,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 150_000,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4726,7 +4700,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4783,7 +4757,7 @@ mod tests {
                                cltv_expiry_delta: 42,
                                htlc_minimum_msat: None,
                                htlc_maximum_msat: None,
-                       }])]);
+                       }])]).with_max_channel_saturation_power_of_half(0);
 
                // Keep only two paths from us to nodes[2], both with a 99sat HTLC maximum, with one with
                // no fee and one with a 1msat fee. Previously, trying to route 100 sats to nodes[2] here
@@ -4797,7 +4771,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (5 << 4) | 5,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(99_000),
+                       htlc_maximum_msat: 99_000,
                        fee_base_msat: u32::max_value(),
                        fee_proportional_millionths: u32::max_value(),
                        excess_data: Vec::new()
@@ -4809,7 +4783,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (5 << 4) | 3,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(99_000),
+                       htlc_maximum_msat: 99_000,
                        fee_base_msat: u32::max_value(),
                        fee_proportional_millionths: u32::max_value(),
                        excess_data: Vec::new()
@@ -4821,7 +4795,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (4 << 4) | 1,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 1,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4833,7 +4807,7 @@ mod tests {
                        flags: 0|2, // Channel disabled
                        cltv_expiry_delta: (13 << 4) | 1,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 2000000,
                        excess_data: Vec::new()
@@ -4886,7 +4860,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(100_000),
+                       htlc_maximum_msat: 100_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4898,7 +4872,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(50_000),
+                       htlc_maximum_msat: 50_000,
                        fee_base_msat: 100,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4912,7 +4886,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(60_000),
+                       htlc_maximum_msat: 60_000,
                        fee_base_msat: 100,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4924,7 +4898,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(60_000),
+                       htlc_maximum_msat: 60_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4938,7 +4912,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(20_000),
+                       htlc_maximum_msat: 20_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -4950,7 +4924,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(20_000),
+                       htlc_maximum_msat: 20_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -5037,7 +5011,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (6 << 4) | 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -5052,7 +5026,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (5 << 4) | 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 100,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -5067,7 +5041,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (4 << 4) | 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -5082,7 +5056,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (3 << 4) | 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -5097,7 +5071,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (2 << 4) | 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -5111,7 +5085,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (1 << 4) | 0,
                        htlc_minimum_msat: 100,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -5169,7 +5143,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(85_000),
+                       htlc_maximum_msat: 85_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -5182,7 +5156,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (4 << 4) | 1,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(270_000),
+                       htlc_maximum_msat: 270_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 1000000,
                        excess_data: Vec::new()
@@ -5234,7 +5208,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(80_000),
+                       htlc_maximum_msat: 80_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -5246,7 +5220,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (4 << 4) | 1,
                        htlc_minimum_msat: 90_000,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -5573,6 +5547,35 @@ mod tests {
                }
        }
 
+       #[test]
+       fn avoids_recently_failed_paths() {
+               // Ensure that the router always avoids all of the `previously_failed_channels` channels by
+               // randomly inserting channels into it until we can't find a route anymore.
+               let (secp_ctx, network, _, _, logger) = build_graph();
+               let (_, our_id, _, nodes) = get_nodes(&secp_ctx);
+               let network_graph = network.read_only();
+
+               let scorer = test_utils::TestScorer::with_penalty(0);
+               let mut payment_params = PaymentParameters::from_node_id(nodes[6]).with_route_hints(last_hops(&nodes))
+                       .with_max_path_count(1);
+               let keys_manager = test_utils::TestKeysInterface::new(&[0u8; 32], Network::Testnet);
+               let random_seed_bytes = keys_manager.get_secure_random_bytes();
+
+               // We should be able to find a route initially, and then after we fail a few random
+               // channels eventually we won't be able to any longer.
+               assert!(get_route(&our_id, &payment_params, &network_graph, None, 100, 0, Arc::clone(&logger), &scorer, &random_seed_bytes).is_ok());
+               loop {
+                       if let Ok(route) = get_route(&our_id, &payment_params, &network_graph, None, 100, 0, Arc::clone(&logger), &scorer, &random_seed_bytes) {
+                               for chan in route.paths[0].iter() {
+                                       assert!(!payment_params.previously_failed_channels.contains(&chan.short_channel_id));
+                               }
+                               let victim = (u64::from_ne_bytes(random_seed_bytes[0..8].try_into().unwrap()) as usize)
+                                       % route.paths[0].len();
+                               payment_params.previously_failed_channels.push(route.paths[0][victim].short_channel_id);
+                       } else { break; }
+               }
+       }
+
        #[test]
        fn limits_path_length() {
                let (secp_ctx, network, _, _, logger) = build_line_graph();
@@ -5738,7 +5741,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (4 << 4) | 1,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(200_000_000),
+                       htlc_maximum_msat: 250_000_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -5750,7 +5753,7 @@ mod tests {
                        flags: 0,
                        cltv_expiry_delta: (13 << 4) | 1,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(200_000_000),
+                       htlc_maximum_msat: 250_000_000,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new()
@@ -5759,8 +5762,8 @@ mod tests {
                let payment_params = PaymentParameters::from_node_id(nodes[2]).with_features(InvoiceFeatures::known());
                let keys_manager = test_utils::TestKeysInterface::new(&[0u8; 32], Network::Testnet);
                let random_seed_bytes = keys_manager.get_secure_random_bytes();
-               // 150,000 sat is less than the available liquidity on each channel, set above.
-               let route = get_route(&our_id, &payment_params, &network_graph.read_only(), None, 150_000_000, 42, Arc::clone(&logger), &scorer, &random_seed_bytes).unwrap();
+               // 100,000 sats is less than the available liquidity on each channel, set above.
+               let route = get_route(&our_id, &payment_params, &network_graph.read_only(), None, 100_000_000, 42, Arc::clone(&logger), &scorer, &random_seed_bytes).unwrap();
                assert_eq!(route.paths.len(), 2);
                assert!((route.paths[0][1].short_channel_id == 4 && route.paths[1][1].short_channel_id == 13) ||
                        (route.paths[1][1].short_channel_id == 4 && route.paths[0][1].short_channel_id == 13));
index 4922fad78476f56d62e6c5ebe859ce36323c93bf..6ae339eae2bbc01776b840d655c2ef68ce6e83aa 100644 (file)
@@ -324,6 +324,9 @@ where L::Target: Logger {
 ///
 /// Used to configure base, liquidity, and amount penalties, the sum of which comprises the channel
 /// penalty (i.e., the amount in msats willing to be paid to avoid routing through the channel).
+///
+/// The penalty applied to any channel by the [`ProbabilisticScorer`] is the sum of each of the
+/// parameters here.
 #[derive(Clone)]
 pub struct ProbabilisticScoringParameters {
        /// A fixed penalty in msats to apply to each channel.
@@ -331,6 +334,20 @@ pub struct ProbabilisticScoringParameters {
        /// Default value: 500 msat
        pub base_penalty_msat: u64,
 
+       /// A multiplier used with the payment amount to calculate a fixed penalty applied to each
+       /// channel, in excess of the [`base_penalty_msat`].
+       ///
+       /// The purpose of the amount penalty is to avoid having fees dominate the channel cost (i.e.,
+       /// fees plus penalty) for large payments. The penalty is computed as the product of this
+       /// multiplier and `2^30`ths of the payment amount.
+       ///
+       /// ie `base_penalty_amount_multiplier_msat * amount_msat / 2^30`
+       ///
+       /// Default value: 8,192 msat
+       ///
+       /// [`base_penalty_msat`]: Self::base_penalty_msat
+       pub base_penalty_amount_multiplier_msat: u64,
+
        /// A multiplier used in conjunction with the negative `log10` of the channel's success
        /// probability for a payment to determine the liquidity penalty.
        ///
@@ -369,7 +386,7 @@ pub struct ProbabilisticScoringParameters {
        /// multiplier and `2^20`ths of the payment amount, weighted by the negative `log10` of the
        /// success probability.
        ///
-       /// `-log10(success_probability) * amount_penalty_multiplier_msat * amount_msat / 2^20`
+       /// `-log10(success_probability) * liquidity_penalty_amount_multiplier_msat * amount_msat / 2^20`
        ///
        /// In practice, this means for 0.1 success probability (`-log10(0.1) == 1`) each `2^20`th of
        /// the amount will result in a penalty of the multiplier. And, as the success probability
@@ -378,7 +395,7 @@ pub struct ProbabilisticScoringParameters {
        /// fall below `1`.
        ///
        /// Default value: 256 msat
-       pub amount_penalty_multiplier_msat: u64,
+       pub liquidity_penalty_amount_multiplier_msat: u64,
 
        /// Manual penalties used for the given nodes. Allows to set a particular penalty for a given
        /// node. Note that a manual penalty of `u64::max_value()` means the node would not ever be
@@ -394,6 +411,25 @@ pub struct ProbabilisticScoringParameters {
        ///
        /// Default value: 250 msat
        pub anti_probing_penalty_msat: u64,
+
+       /// This penalty is applied when the amount we're attempting to send over a channel exceeds our
+       /// current estimate of the channel's available liquidity.
+       ///
+       /// Note that in this case all other penalties, including the
+       /// [`liquidity_penalty_multiplier_msat`] and [`liquidity_penalty_amount_multiplier_msat`]-based
+       /// penalties, as well as the [`base_penalty_msat`] and the [`anti_probing_penalty_msat`], if
+       /// applicable, are still included in the overall penalty.
+       ///
+       /// If you wish to avoid creating paths with such channels entirely, setting this to a value of
+       /// `u64::max_value()` will guarantee that.
+       ///
+       /// Default value: 1_0000_0000_000 msat (1 Bitcoin)
+       ///
+       /// [`liquidity_penalty_multiplier_msat`]: Self::liquidity_penalty_multiplier_msat
+       /// [`liquidity_penalty_amount_multiplier_msat`]: Self::liquidity_penalty_amount_multiplier_msat
+       /// [`base_penalty_msat`]: Self::base_penalty_msat
+       /// [`anti_probing_penalty_msat`]: Self::anti_probing_penalty_msat
+       pub considered_impossible_penalty_msat: u64,
 }
 
 /// Accounting for channel liquidity balance uncertainty.
@@ -517,11 +553,13 @@ impl ProbabilisticScoringParameters {
        fn zero_penalty() -> Self {
                Self {
                        base_penalty_msat: 0,
+                       base_penalty_amount_multiplier_msat: 0,
                        liquidity_penalty_multiplier_msat: 0,
                        liquidity_offset_half_life: Duration::from_secs(3600),
-                       amount_penalty_multiplier_msat: 0,
+                       liquidity_penalty_amount_multiplier_msat: 0,
                        manual_node_penalties: HashMap::new(),
                        anti_probing_penalty_msat: 0,
+                       considered_impossible_penalty_msat: 0,
                }
        }
 
@@ -538,11 +576,13 @@ impl Default for ProbabilisticScoringParameters {
        fn default() -> Self {
                Self {
                        base_penalty_msat: 500,
+                       base_penalty_amount_multiplier_msat: 8192,
                        liquidity_penalty_multiplier_msat: 40_000,
                        liquidity_offset_half_life: Duration::from_secs(3600),
-                       amount_penalty_multiplier_msat: 256,
+                       liquidity_penalty_amount_multiplier_msat: 256,
                        manual_node_penalties: HashMap::new(),
                        anti_probing_penalty_msat: 250,
+                       considered_impossible_penalty_msat: 1_0000_0000_000,
                }
        }
 }
@@ -610,35 +650,31 @@ const PRECISION_LOWER_BOUND_DENOMINATOR: u64 = approx::LOWER_BITS_BOUND;
 
 /// The divisor used when computing the amount penalty.
 const AMOUNT_PENALTY_DIVISOR: u64 = 1 << 20;
+const BASE_AMOUNT_PENALTY_DIVISOR: u64 = 1 << 30;
 
 impl<L: Deref<Target = u64>, T: Time, U: Deref<Target = T>> DirectedChannelLiquidity<L, T, U> {
-       /// Returns a penalty for routing the given HTLC `amount_msat` through the channel in this
-       /// direction.
+       /// Returns a liquidity penalty for routing the given HTLC `amount_msat` through the channel in
+       /// this direction.
        fn penalty_msat(&self, amount_msat: u64, params: &ProbabilisticScoringParameters) -> u64 {
                let max_liquidity_msat = self.max_liquidity_msat();
                let min_liquidity_msat = core::cmp::min(self.min_liquidity_msat(), max_liquidity_msat);
                if amount_msat <= min_liquidity_msat {
                        0
                } else if amount_msat >= max_liquidity_msat {
-                       if amount_msat > max_liquidity_msat {
-                               u64::max_value()
-                       } else if max_liquidity_msat != self.capacity_msat {
-                               // Avoid using the failed channel on retry.
-                               u64::max_value()
-                       } else {
-                               // Equivalent to hitting the else clause below with the amount equal to the
-                               // effective capacity and without any certainty on the liquidity upper bound.
-                               let negative_log10_times_2048 = NEGATIVE_LOG10_UPPER_BOUND * 2048;
-                               self.combined_penalty_msat(amount_msat, negative_log10_times_2048, params)
-                       }
+                       // Equivalent to hitting the else clause below with the amount equal to the effective
+                       // capacity and without any certainty on the liquidity upper bound, plus the
+                       // impossibility penalty.
+                       let negative_log10_times_2048 = NEGATIVE_LOG10_UPPER_BOUND * 2048;
+                       self.combined_penalty_msat(amount_msat, negative_log10_times_2048, params)
+                               .saturating_add(params.considered_impossible_penalty_msat)
                } else {
                        let numerator = (max_liquidity_msat - amount_msat).saturating_add(1);
                        let denominator = (max_liquidity_msat - min_liquidity_msat).saturating_add(1);
                        if amount_msat - min_liquidity_msat < denominator / PRECISION_LOWER_BOUND_DENOMINATOR {
                                // If the failure probability is < 1.5625% (as 1 - numerator/denominator < 1/64),
                                // don't bother trying to use the log approximation as it gets too noisy to be
-                               // particularly helpful, instead just round down to 0 and return the base penalty.
-                               params.base_penalty_msat
+                               // particularly helpful, instead just round down to 0.
+                               0
                        } else {
                                let negative_log10_times_2048 =
                                        approx::negative_log10_times_2048(numerator, denominator);
@@ -647,7 +683,7 @@ impl<L: Deref<Target = u64>, T: Time, U: Deref<Target = T>> DirectedChannelLiqui
                }
        }
 
-       /// Computes the liquidity and amount penalties and adds them to the base penalty.
+       /// Computes the liquidity penalty from the penalty multipliers.
        #[inline(always)]
        fn combined_penalty_msat(
                &self, amount_msat: u64, negative_log10_times_2048: u64,
@@ -660,12 +696,10 @@ impl<L: Deref<Target = u64>, T: Time, U: Deref<Target = T>> DirectedChannelLiqui
                        (negative_log10_times_2048.saturating_mul(multiplier_msat) / 2048).min(max_penalty_msat)
                };
                let amount_penalty_msat = negative_log10_times_2048
-                       .saturating_mul(params.amount_penalty_multiplier_msat)
+                       .saturating_mul(params.liquidity_penalty_amount_multiplier_msat)
                        .saturating_mul(amount_msat) / 2048 / AMOUNT_PENALTY_DIVISOR;
 
-               params.base_penalty_msat
-                       .saturating_add(liquidity_penalty_msat)
-                       .saturating_add(amount_penalty_msat)
+               liquidity_penalty_msat.saturating_add(amount_penalty_msat)
        }
 
        /// Returns the lower bound of the channel liquidity balance in this direction.
@@ -747,13 +781,17 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref, T: Time> Score for Probabilis
                        return *penalty;
                }
 
+               let base_penalty_msat = self.params.base_penalty_msat.saturating_add(
+                       self.params.base_penalty_amount_multiplier_msat
+                               .saturating_mul(usage.amount_msat) / BASE_AMOUNT_PENALTY_DIVISOR);
+
                let mut anti_probing_penalty_msat = 0;
                match usage.effective_capacity {
                        EffectiveCapacity::ExactLiquidity { liquidity_msat } => {
                                if usage.amount_msat > liquidity_msat {
                                        return u64::max_value();
                                } else {
-                                       return self.params.base_penalty_msat;
+                                       return base_penalty_msat;
                                }
                        },
                        EffectiveCapacity::Total { capacity_msat, htlc_maximum_msat: Some(htlc_maximum_msat) } => {
@@ -774,6 +812,7 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref, T: Time> Score for Probabilis
                        .as_directed(source, target, capacity_msat, liquidity_offset_half_life)
                        .penalty_msat(amount_msat, &self.params)
                        .saturating_add(anti_probing_penalty_msat)
+                       .saturating_add(base_penalty_msat)
        }
 
        fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
@@ -1242,7 +1281,7 @@ mod tests {
        use util::time::tests::SinceEpoch;
 
        use ln::features::{ChannelFeatures, NodeFeatures};
-       use ln::msgs::{ChannelAnnouncement, ChannelUpdate, OptionalField, UnsignedChannelAnnouncement, UnsignedChannelUpdate};
+       use ln::msgs::{ChannelAnnouncement, ChannelUpdate, UnsignedChannelAnnouncement, UnsignedChannelUpdate};
        use routing::gossip::{EffectiveCapacity, NetworkGraph, NodeId};
        use routing::router::RouteHop;
        use routing::scoring::{ChannelUsage, Score};
@@ -1369,7 +1408,7 @@ mod tests {
                        flags,
                        cltv_expiry_delta: 18,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Present(1_000),
+                       htlc_maximum_msat: 1_000,
                        fee_base_msat: 1,
                        fee_proportional_millionths: 0,
                        excess_data: Vec::new(),
@@ -1624,7 +1663,7 @@ mod tests {
                assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 0);
                let usage = ChannelUsage { amount_msat: 102_400, ..usage };
                assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 47);
-               let usage = ChannelUsage { amount_msat: 1_024_000, ..usage };
+               let usage = ChannelUsage { amount_msat: 1_023_999, ..usage };
                assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 2_000);
 
                let usage = ChannelUsage {
@@ -1654,6 +1693,7 @@ mod tests {
                let network_graph = network_graph(&logger);
                let params = ProbabilisticScoringParameters {
                        liquidity_penalty_multiplier_msat: 1_000,
+                       considered_impossible_penalty_msat: u64::max_value(),
                        ..ProbabilisticScoringParameters::zero_penalty()
                };
                let scorer = ProbabilisticScorer::new(params, &network_graph, &logger)
@@ -1745,6 +1785,7 @@ mod tests {
                let network_graph = network_graph(&logger);
                let params = ProbabilisticScoringParameters {
                        liquidity_penalty_multiplier_msat: 1_000,
+                       considered_impossible_penalty_msat: u64::max_value(),
                        ..ProbabilisticScoringParameters::zero_penalty()
                };
                let mut scorer = ProbabilisticScorer::new(params, &network_graph, &logger);
@@ -1811,6 +1852,7 @@ mod tests {
                let params = ProbabilisticScoringParameters {
                        liquidity_penalty_multiplier_msat: 1_000,
                        liquidity_offset_half_life: Duration::from_secs(10),
+                       considered_impossible_penalty_msat: u64::max_value(),
                        ..ProbabilisticScoringParameters::zero_penalty()
                };
                let mut scorer = ProbabilisticScorer::new(params, &network_graph, &logger);
@@ -1820,10 +1862,10 @@ mod tests {
                let usage = ChannelUsage {
                        amount_msat: 0,
                        inflight_htlc_msat: 0,
-                       effective_capacity: EffectiveCapacity::Total { capacity_msat: 1_024, htlc_maximum_msat: Some(1_000) },
+                       effective_capacity: EffectiveCapacity::Total { capacity_msat: 1_024, htlc_maximum_msat: Some(1_024) },
                };
                assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 0);
-               let usage = ChannelUsage { amount_msat: 1_024, ..usage };
+               let usage = ChannelUsage { amount_msat: 1_023, ..usage };
                assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 2_000);
 
                scorer.payment_path_failed(&payment_path_for_amount(768).iter().collect::<Vec<_>>(), 42);
@@ -1867,20 +1909,20 @@ mod tests {
                let usage = ChannelUsage { amount_msat: 1_023, ..usage };
                assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 2_000);
                let usage = ChannelUsage { amount_msat: 1_024, ..usage };
-               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 2_000);
+               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), u64::max_value());
 
                // Fully decay liquidity upper bound.
                SinceEpoch::advance(Duration::from_secs(10));
                let usage = ChannelUsage { amount_msat: 0, ..usage };
                assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 0);
                let usage = ChannelUsage { amount_msat: 1_024, ..usage };
-               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 2_000);
+               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), u64::max_value());
 
                SinceEpoch::advance(Duration::from_secs(10));
                let usage = ChannelUsage { amount_msat: 0, ..usage };
                assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 0);
                let usage = ChannelUsage { amount_msat: 1_024, ..usage };
-               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 2_000);
+               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), u64::max_value());
        }
 
        #[test]
@@ -1965,6 +2007,7 @@ mod tests {
                let params = ProbabilisticScoringParameters {
                        liquidity_penalty_multiplier_msat: 1_000,
                        liquidity_offset_half_life: Duration::from_secs(10),
+                       considered_impossible_penalty_msat: u64::max_value(),
                        ..ProbabilisticScoringParameters::zero_penalty()
                };
                let mut scorer = ProbabilisticScorer::new(params.clone(), &network_graph, &logger);
@@ -2001,6 +2044,7 @@ mod tests {
                let params = ProbabilisticScoringParameters {
                        liquidity_penalty_multiplier_msat: 1_000,
                        liquidity_offset_half_life: Duration::from_secs(10),
+                       considered_impossible_penalty_msat: u64::max_value(),
                        ..ProbabilisticScoringParameters::zero_penalty()
                };
                let mut scorer = ProbabilisticScorer::new(params.clone(), &network_graph, &logger);
@@ -2048,47 +2092,47 @@ mod tests {
                        inflight_htlc_msat: 0,
                        effective_capacity: EffectiveCapacity::Total { capacity_msat: 950_000_000, htlc_maximum_msat: Some(1_000) },
                };
-               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 3613);
+               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 4375);
                let usage = ChannelUsage {
                        effective_capacity: EffectiveCapacity::Total { capacity_msat: 1_950_000_000, htlc_maximum_msat: Some(1_000) }, ..usage
                };
-               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 1977);
+               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 2739);
                let usage = ChannelUsage {
                        effective_capacity: EffectiveCapacity::Total { capacity_msat: 2_950_000_000, htlc_maximum_msat: Some(1_000) }, ..usage
                };
-               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 1474);
+               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 2236);
                let usage = ChannelUsage {
                        effective_capacity: EffectiveCapacity::Total { capacity_msat: 3_950_000_000, htlc_maximum_msat: Some(1_000) }, ..usage
                };
-               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 1223);
+               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 1985);
                let usage = ChannelUsage {
                        effective_capacity: EffectiveCapacity::Total { capacity_msat: 4_950_000_000, htlc_maximum_msat: Some(1_000) }, ..usage
                };
-               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 877);
+               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 1639);
                let usage = ChannelUsage {
                        effective_capacity: EffectiveCapacity::Total { capacity_msat: 5_950_000_000, htlc_maximum_msat: Some(1_000) }, ..usage
                };
-               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 845);
+               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 1607);
                let usage = ChannelUsage {
                        effective_capacity: EffectiveCapacity::Total { capacity_msat: 6_950_000_000, htlc_maximum_msat: Some(1_000) }, ..usage
                };
-               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 500);
+               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 1262);
                let usage = ChannelUsage {
                        effective_capacity: EffectiveCapacity::Total { capacity_msat: 7_450_000_000, htlc_maximum_msat: Some(1_000) }, ..usage
                };
-               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 500);
+               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 1262);
                let usage = ChannelUsage {
                        effective_capacity: EffectiveCapacity::Total { capacity_msat: 7_950_000_000, htlc_maximum_msat: Some(1_000) }, ..usage
                };
-               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 500);
+               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 1262);
                let usage = ChannelUsage {
                        effective_capacity: EffectiveCapacity::Total { capacity_msat: 8_950_000_000, htlc_maximum_msat: Some(1_000) }, ..usage
                };
-               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 500);
+               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 1262);
                let usage = ChannelUsage {
                        effective_capacity: EffectiveCapacity::Total { capacity_msat: 9_950_000_000, htlc_maximum_msat: Some(1_000) }, ..usage
                };
-               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 500);
+               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 1262);
        }
 
        #[test]
@@ -2116,6 +2160,15 @@ mod tests {
                };
                let scorer = ProbabilisticScorer::new(params, &network_graph, &logger);
                assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 558);
+
+               let params = ProbabilisticScoringParameters {
+                       base_penalty_msat: 500, liquidity_penalty_multiplier_msat: 1_000,
+                       base_penalty_amount_multiplier_msat: (1 << 30),
+                       anti_probing_penalty_msat: 0, ..Default::default()
+               };
+
+               let scorer = ProbabilisticScorer::new(params, &network_graph, &logger);
+               assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 558 + 128);
        }
 
        #[test]
@@ -2132,7 +2185,7 @@ mod tests {
 
                let params = ProbabilisticScoringParameters {
                        liquidity_penalty_multiplier_msat: 1_000,
-                       amount_penalty_multiplier_msat: 0,
+                       liquidity_penalty_amount_multiplier_msat: 0,
                        ..ProbabilisticScoringParameters::zero_penalty()
                };
                let scorer = ProbabilisticScorer::new(params, &network_graph, &logger);
@@ -2140,7 +2193,7 @@ mod tests {
 
                let params = ProbabilisticScoringParameters {
                        liquidity_penalty_multiplier_msat: 1_000,
-                       amount_penalty_multiplier_msat: 256,
+                       liquidity_penalty_amount_multiplier_msat: 256,
                        ..ProbabilisticScoringParameters::zero_penalty()
                };
                let scorer = ProbabilisticScorer::new(params, &network_graph, &logger);
@@ -2171,7 +2224,10 @@ mod tests {
        fn accounts_for_inflight_htlc_usage() {
                let logger = TestLogger::new();
                let network_graph = network_graph(&logger);
-               let params = ProbabilisticScoringParameters::default();
+               let params = ProbabilisticScoringParameters {
+                       considered_impossible_penalty_msat: u64::max_value(),
+                       ..ProbabilisticScoringParameters::zero_penalty()
+               };
                let scorer = ProbabilisticScorer::new(params, &network_graph, &logger);
                let source = source_node_id();
                let target = target_node_id();
index fe99b06c9bcf46e24388c2a8447055bf635a4104..9306fb254f42f8c071d7b6880b22b00bb072fd2d 100644 (file)
@@ -19,7 +19,6 @@ use chain::transaction::OutPoint;
 use chain::keysinterface;
 use ln::features::{ChannelFeatures, InitFeatures};
 use ln::{msgs, wire};
-use ln::msgs::OptionalField;
 use ln::script::ShutdownScript;
 use routing::scoring::FixedPenaltyScorer;
 use util::enforcing_trait_impls::{EnforcingSigner, EnforcementState};
@@ -407,7 +406,7 @@ fn get_dummy_channel_update(short_chan_id: u64) -> msgs::ChannelUpdate {
                        flags: 0,
                        cltv_expiry_delta: 0,
                        htlc_minimum_msat: 0,
-                       htlc_maximum_msat: OptionalField::Absent,
+                       htlc_maximum_msat: msgs::MAX_VALUE_MSAT,
                        fee_base_msat: 0,
                        fee_proportional_millionths: 0,
                        excess_data: vec![],