Store a cached NodeId for each Peer
#2022
Conversation
lightning/src/ln/peer_handler.rs
Outdated
| // When setting this field to a `Some(..)` value, use `Peer::set_their_node_id` to also set | ||
| // the following field `their_node_id_serialized`. | ||
| their_node_id: Option<PublicKey>, | ||
| // This field is a cached `NodeId` of `their_node_id` to avoid serializing peers' keys every | ||
| // time we forward gossip messages in `PeerManager`. It should be set anytime `their_node_id` | ||
| // gets set. | ||
| their_node_id_serialized: Option<NodeId>, |
There was a problem hiding this comment.
I'm a bit worried these could go out of sync, but doc comments which would at least appear in most author IDEs could help with that if there's no better way (which I can't think of right now). Unfortunately, single fields cannot be made externally immutable in Rust. I'm sure others might have some experience here. :)
| // When setting this field to a `Some(..)` value, use `Peer::set_their_node_id` to also set | |
| // the following field `their_node_id_serialized`. | |
| their_node_id: Option<PublicKey>, | |
| // This field is a cached `NodeId` of `their_node_id` to avoid serializing peers' keys every | |
| // time we forward gossip messages in `PeerManager`. It should be set anytime `their_node_id` | |
| // gets set. | |
| their_node_id_serialized: Option<NodeId>, | |
| /// NOTE: Do not modify this field directly. Use `Peer::set_their_node_id` instead. | |
| their_node_id: Option<PublicKey>, | |
| /// This field is a cached `NodeId` of `their_node_id` to avoid serializing peers' keys every | |
| /// time we forward gossip messages in `PeerManager`. | |
| /// | |
| /// NOTE: Do not modify this field directly. Use `Peer::set_their_node_id` instead. | |
| their_node_id_serialized: Option<NodeId>, |
There was a problem hiding this comment.
Oh oops I meant to make them doc comments, thanks. And yea those were exactly my thoughts, at the moment I can't think of a way to cache without risking them going out of sync, hoping this would have the least friction for keeping them in sync.
There was a problem hiding this comment.
We could make their_node_id a tuple (PublicKey, NodeId), that way you're forced to update them in tandem.
There was a problem hiding this comment.
Doesn't avoid the possibility of a mismatch but better than two fields.
There was a problem hiding this comment.
Oh nice! Will update
alecchendev
force-pushed
the
2023-02-cache-peer-nodeid
branch
from
b6b52a9 to
08e9bc3
Compare
Codecov ReportBase: 91.04% // Head: 91.06% // Increases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #2022 +/- ##
==========================================
+ Coverage 91.04% 91.06% +0.02%
==========================================
Files 99 99
Lines 51722 51727 +5
Branches 51722 51727 +5
==========================================
+ Hits 47090 47107 +17
+ Misses 4632 4620 -12
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
This is done to avoid redundantly serializing peer node ids when forwarding gossip messages in `PeerManager::forward_broadcast_msg`.
alecchendev
force-pushed
the
2023-02-cache-peer-nodeid
branch
from
08e9bc3 to
4c1055d
Compare
Closes #2021 as a followup on #2016.
Converts
Peerfieldtheir_node_id: Option<PublicKey>toOption<(PublicKey, NodeId)>to avoid redundantly serializing peer public keys when forwarding gossip messages.