Avoid parsing PublicKeys when handling RGS updates
#3581
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
arik-so
previously approved these changes
Jan 31, 2025
TheBlueMatt
force-pushed
the
2025-01-rgs-speedups
branch
from
a5810bf to
4df7223
Compare
|
Oh, new commit improves the bench some 25% or so, too. |
arik-so
previously approved these changes
Jan 31, 2025
tnull
reviewed
Jan 31, 2025
|
That said, CI is said as the |
TheBlueMatt
force-pushed
the
2025-01-rgs-speedups
branch
from
4df7223 to
31390b2
Compare
arik-so
previously approved these changes
Jan 31, 2025
|
Feel free to squash. |
shaavan
reviewed
Feb 1, 2025
There was a problem hiding this comment.
Overall, LGTM!
Since there aren’t any direct code references explaining why we switch to using NodeId in RGS, it might be helpful to add a quick note in the commit message (of ddc3326) on why skipping PublicKey verification is safe in this case.
I think adding a little extra context could make it clearer for future readers!
`PublicKey` parsing is relatively expensive as we have to check if the point is actually on the curve. To avoid it, our `NetworkGraph` uses `NodeId`s which don't have the validity requirement. Sadly, we were always parsing the broadcasting node's `PublicKey` from the `node_id` in the network graph whenever we see an update for that channel, whether we have a corresponding signature or not. Here we fix this, only parsing the public key (and hashing the message) if we're going to check a signature.
`PublicKey` parsing is relatively expensive as we have to check if the point is actually on the curve. To avoid it, our `NetworkGraph` uses `NodeId`s which don't have the validity requirement. Here, we take advantage of that in RGS application to avoid parsing `PublicKey`s, improving performance.
When we build a new `NetworkGraph` from empty, we're generally doing an initial startup and will be syncing the graph very soon. Using an initially-empty `IndexedMap` for the `channels` and `nodes` results in quite some memory churn, with the initial RGS application benchmark showing 15% of its time in pagefault handling alone (i.e. allocating new memory from the OS, let alone the 23% of time in `memmove`). Further, when deserializing a `NetworkGraph`, we'd swapped the expected node and channel count constants, leaving the node map too small and causing map doubling as we read entries from disk. Finally, when deserializing, allocating only exactly the amount of map entries we need is likely to lead to at least one doubling, so we're better off just over-estimating the number of nodes and channels and allocating what we want. Here we just always allocate `channels` and `nodes` based on constants, leading to a 20%-ish speedup in the initial RGS application benchmark.
TheBlueMatt
force-pushed
the
2025-01-rgs-speedups
branch
from
31390b2 to
0ff7fba
Compare
|
Squashed with one extra commit at the top that just adds more detail to the |
arik-so
approved these changes
Feb 3, 2025
tnull
approved these changes
Feb 3, 2025
|
Mhh, seems fuzz is failing. Not sure if related, but it blocks merging. |
shaavan
approved these changes
Feb 3, 2025
|
Hmm, its possible this caused performance of some fuzz target to regress, but I think more likely github just decided to run it on a slower machine or so, so gonna disable the fuzz-required and merge. |
|
Backported the first (fixing the perf regression) and third (more correctly reserving the graph structures for a large performance win) commits in #3613 |
TheBlueMatt
added a commit
to TheBlueMatt/rust-lightning
that referenced
this pull request
Apr 3, 2025
v0.1.2 - Apr 02, 2025 - "Foolishly Edgy Cases" API Updates =========== * `lightning-invoice` is now re-exported as `lightning::bolt11_invoice` (lightningdevkit#3671). Performance Improvements ======================== * `rapid-gossip-sync` graph parsing is substantially faster, resolving a regression in 0.1 (lightningdevkit#3581). * `NetworkGraph` loading is now substantially faster and does fewer allocations, resulting in a 20% further improvement in `rapid-gossip-sync` loading when initializing from scratch (lightningdevkit#3581). * `ChannelMonitor`s for closed channels are no longer always re-persisted immediately after startup, reducing on-startup I/O burden (lightningdevkit#3619). Bug Fixes ========= * BOLT 11 invoices longer than 1023 bytes long (and up to 7089 bytes) now properly parse (lightningdevkit#3665). * In some cases, when using synchronous persistence with higher latency than the latency to communicate with peers, when receiving an MPP payment with multiple parts received over the same channel, a channel could hang and not make progress, eventually leading to a force-closure due to timed-out HTLCs. This has now been fixed (lightningdevkit#3680). * Some rare cases with multi-hop BOLT 11 route hints or multiple redundant blinded paths could have led to the router creating invalid `Route`s were fixed (lightningdevkit#3586). * Corrected the decay logic in `ProbabilisticScorer`'s historical buckets model. Note that by default historical buckets are only decayed if no new datapoints have been added for a channel for two weeks (lightningdevkit#3562). * `{Channel,Onion}MessageHandler::peer_disconnected` will now be called if a different message handler refused connection by returning an `Err` from its `peer_connected` method (lightningdevkit#3580). * If the counterparty broadcasts a revoked state with pending HTLCs, those will now be claimed with other outputs which we consider to not be vulnerable to pinning attacks if they are not yet claimable by our counterparty, potentially reducing our exposure to pinning attacks (lightningdevkit#3564).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The first commit fixes a regression in 0.1 and should be backported, the second is just an optimization but shouldn't be backported as it changes the public API.