Validate `channel_update` signatures without holding a graph lock #3310

TheBlueMatt · 2024-09-11T23:56:02Z

We often process many gossip messages in parallel across different
peer connections, making the `NetworkGraph` mutexes fairly
contention-sensitive (not to mention the potential that we want to
send a payment and need to find a path to do so).

Because we need to look up a node's public key to validate a
signature on `channel_update` messages, we always need to take a
`NetworkGraph::channels` lock before we can validate the message.

For simplicity, and to avoid taking a lock twice, we'd always
validated the `channel_update` signature while holding the same
lock, but here we address the contention issues by doing a
`channel_update` validation in three stages.

First we take a read lock on `NetworkGraph::channels` and check if
the `channel_update` is new, then release the lock and validate the
message signature, and finally take a write lock, (re-check if the
`channel_update` is new) and update the graph.

In the next commit we'll move to checking `channel_update`s in three steps - first we check if the `channel_update` is new and the latest for a channel, then we check the signature, and finally we update our local state. This allows us to avoid holding a lock on `NetworkGraph::channels` while validating the message signature. Here we do a quick prefactor to make that simpler - moving the validation logic of `channel_update` that we'll do in step one (and repeat in step three) into a local function. We also take this opportunity to do one static check unlocked which we had been doing while holding a `channel` lock.

We often process many gossip messages in parallel across different peer connections, making the `NetworkGraph` mutexes fairly contention-sensitive (not to mention the potential that we want to send a payment and need to find a path to do so). Because we need to look up a node's public key to validate a signature on `channel_update` messages, we always need to take a `NetworkGraph::channels` lock before we can validate the message. For simplicity, and to avoid taking a lock twice, we'd always validated the `channel_update` signature while holding the same lock, but here we address the contention issues by doing a `channel_update` validation in three stages. First we take a read lock on `NetworkGraph::channels` and check if the `channel_update` is new, then release the lock and validate the message signature, and finally take a write lock, (re-check if the `channel_update` is new) and update the graph.

codecov · 2024-09-12T04:42:26Z

Codecov Report

Attention: Patch coverage is 94.28571% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.98%. Comparing base (db905e8) to head (0325c14).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/routing/gossip.rs	94.28%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3310      +/-   ##
==========================================
+ Coverage   89.64%   89.98%   +0.34%     
==========================================
  Files         126      126              
  Lines      102251   105314    +3063     
  Branches   102251   105314    +3063     
==========================================
+ Hits        91666    94771    +3105     
  Misses       7863     7863              
+ Partials     2722     2680      -42

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jkczyz · 2024-09-12T17:58:18Z

lightning/src/routing/gossip.rs

-				if msg.channel_flags & 1 == 1 {
-					if let Some(sig) = sig {
-						secp_verify_sig!(self.secp_ctx, &msg_hash, &sig, &PublicKey::from_slice(channel.node_two.as_slice()).map_err(|_| LightningError{
+		let node_pubkey;


nit:

let node_pubkey = { // ... };

I just find that hard to read across a long block...I know we do it a lot of places, was just trying to avoid it and try something new 🤷‍♂️

Yeah, YMMV. Just seemed shorter after your change.

At least I didn't suggest a long chaining expression. 😉

I don't think it affects legibility much, though consistency would favor @jkczyz's suggestion.

lightning/src/routing/gossip.rs

arik-so · 2024-09-19T02:19:03Z

Looks good, feel free to squash the fixup.

TheBlueMatt added 2 commits September 11, 2024 23:37

tnull self-requested a review September 12, 2024 10:14

jkczyz reviewed Sep 12, 2024

View reviewed changes

f one less closure

0325c14

tnull removed their request for review September 19, 2024 09:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate `channel_update` signatures without holding a graph lock #3310

Validate `channel_update` signatures without holding a graph lock #3310

TheBlueMatt commented Sep 11, 2024

codecov bot commented Sep 12, 2024 •

edited

Loading

jkczyz Sep 12, 2024

TheBlueMatt Sep 12, 2024

jkczyz Sep 12, 2024

jkczyz Sep 12, 2024

arik-so Sep 19, 2024

arik-so commented Sep 19, 2024

Validate channel_update signatures without holding a graph lock #3310

Are you sure you want to change the base?

Validate channel_update signatures without holding a graph lock #3310

Conversation

TheBlueMatt commented Sep 11, 2024

codecov bot commented Sep 12, 2024 • edited Loading

Codecov Report

jkczyz Sep 12, 2024

Choose a reason for hiding this comment

TheBlueMatt Sep 12, 2024

Choose a reason for hiding this comment

jkczyz Sep 12, 2024

Choose a reason for hiding this comment

jkczyz Sep 12, 2024

Choose a reason for hiding this comment

arik-so Sep 19, 2024

Choose a reason for hiding this comment

arik-so commented Sep 19, 2024

Validate `channel_update` signatures without holding a graph lock #3310

Validate `channel_update` signatures without holding a graph lock #3310

codecov bot commented Sep 12, 2024 •

edited

Loading