lightningdevkit / rust-lightning

A highly modular Bitcoin Lightning library written in Rust. It's rust-lightning, not Rusty's Lightning!
Other
1.11k stars 342 forks source link

(Re-)add handling for `ChannelUpdate::message_flags` #3144

Closed TheBlueMatt closed 16 hours ago

TheBlueMatt commented 1 week ago

When the htlc_maximum_msat field was made mandatory in ChannelUpdate (in https://github.com/lightningdevkit/rust-lightning/commit/b0e8b739b73cc25f3e1ab00695a14d972162a140) we started ignoring the message_flags field entirely and always writing 1. The comment updates indicated that the message_flags field was deprecated, but this is not true - only the htlc_maximum_msat indicator bit was deprecated, requiring it to be 1.

If a node creates a channel_update with message_flags bits set other than the low bit, this will cause us to spuriously reject the message with an invalid signature error as we will check the message against the wrong hash.

With today's current spec this is totally fine - the only other bit defined for message_flags is the dont_forward bit, which when set indicates we shouldn't accept the message into our gossip store anyway (though this may lead to spurious warning messages being sent to peers). However, in the future this may mean we start rejecting valid channel_updates if new bits are defiend.

codecov-commenter commented 1 week ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 97.76358% with 7 lines in your changes missing coverage. Please review.

Project coverage is 90.23%. Comparing base (88e1b56) to head (162b81f). Report is 11 commits behind head on main.

Files Patch % Lines
lightning/src/ln/msgs.rs 46.15% 0 Missing and 7 partials :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3144 +/- ## ========================================== + Coverage 89.83% 90.23% +0.39% ========================================== Files 121 121 Lines 98900 103329 +4429 Branches 98900 103329 +4429 ========================================== + Hits 88847 93237 +4390 - Misses 7457 7462 +5 - Partials 2596 2630 +34 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

TheBlueMatt commented 1 day ago

Address @tnull's nit and squashed:

$ git diff-tree -U1 0fa0c2dc 5b48f3ff
diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs
index 6701a1806..0cc8f1323 100644
--- a/lightning/src/ln/msgs.rs
+++ b/lightning/src/ln/msgs.rs
@@ -3505,3 +3505,3 @@ mod tests {
            timestamp: 20190119,
-           message_flags: 0,
+           message_flags: 1, // Only must_be_one
            channel_flags: if direction { 1 } else { 0 } | if disable { 1 << 1 } else { 0 },
valentinewallace commented 1 day ago

Fuzz is failing

TheBlueMatt commented 1 day ago

Ugh, had to update the log regex:

$ git diff-tree -U1 5b48f3ff 162b81fb
diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs
index a2ae07a21..86f4ef4de 100644
--- a/fuzz/src/full_stack.rs
+++ b/fuzz/src/full_stack.rs
@@ -1781,3 +1781,3 @@ mod tests {
        assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Sending message to all peers except Some(PublicKey(0000000000000000000000000000000000000000000000000000000000000002ff00000000000000000000000000000000000000000000000000000000000002)) or the announced channel's counterparties: ChannelAnnouncement { node_signature_1: 3026020200b202200303030303030303030303030303030303030303030303030303030303030303, node_signature_2: 3026020200b202200202020202020202020202020202020202020202020202020202020202020202, bitcoin_signature_1: 3026020200b202200303030303030303030303030303030303030303030303030303030303030303, bitcoin_signature_2: 3026020200b202200202020202020202020202020202020202020202020202020202020202020202, contents: UnsignedChannelAnnouncement { features: [], chain_hash: 6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000, short_channel_id: 42, node_id_1: NodeId(030303030303030303030303030303030303030303030303030303030303030303), node_id_2: NodeId(020202020202020202020202020202020202020202020202020202020202020202), bitcoin_key_1: NodeId(030303030303030303030303030303030303030303030303030303030303030303), bitcoin_key_2: NodeId(020202020202020202020202020202020202020202020202020202020202020202), excess_data: [] } }".to_string())), Some(&1));
-       assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Sending message to all peers except Some(PublicKey(0000000000000000000000000000000000000000000000000000000000000002ff00000000000000000000000000000000000000000000000000000000000002)): ChannelUpdate { signature: 3026020200a602200303030303030303030303030303030303030303030303030303030303030303, contents: UnsignedChannelUpdate { chain_hash: 6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000, short_channel_id: 42, timestamp: 44, flags: 0, cltv_expiry_delta: 40, htlc_minimum_msat: 0, htlc_maximum_msat: 100000000, fee_base_msat: 0, fee_proportional_millionths: 0, excess_data: [] } }".to_string())), Some(&1));
+       assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Sending message to all peers except Some(PublicKey(0000000000000000000000000000000000000000000000000000000000000002ff00000000000000000000000000000000000000000000000000000000000002)): ChannelUpdate { signature: 3026020200a602200303030303030303030303030303030303030303030303030303030303030303, contents: UnsignedChannelUpdate { chain_hash: 6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000, short_channel_id: 42, timestamp: 44, message_flags: 1, channel_flags: 0, cltv_expiry_delta: 40, htlc_minimum_msat: 0, htlc_maximum_msat: 100000000, fee_base_msat: 0, fee_proportional_millionths: 0, excess_data: [] } }".to_string())), Some(&1));
        assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Sending message to all peers except Some(PublicKey(0000000000000000000000000000000000000000000000000000000000000002ff00000000000000000000000000000000000000000000000000000000000002)) or the announced node: NodeAnnouncement { signature: 302502012802200303030303030303030303030303030303030303030303030303030303030303, contents: UnsignedNodeAnnouncement { features: [], timestamp: 43, node_id: NodeId(030303030303030303030303030303030303030303030303030303030303030303), rgb: [0, 0, 0], alias: NodeAlias([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]), addresses: [], excess_address_data: [], excess_data: [] } }".to_string())), Some(&1));
valentinewallace commented 23 hours ago

FYI mutants is failing

TheBlueMatt commented 16 hours ago

Yea, mutants is gonna be somewhat informational for now - tells us where our coverage is short but we don't really need to hold up a PR on it.