Avoid querying the chain for outputs for channels we already have #1679
Conversation
TheBlueMatt
force-pushed
the
2022-08-no-double-access
branch
2 times, most recently
from
629b66c to
78c0de1
Compare
TheBlueMatt
force-pushed
the
2022-08-no-double-access
branch
from
78c0de1 to
d5cdf7a
Compare
|
Are we considering the possibility of reorgs, which might necessitate revalidating the same channel announcement? Not really a big edge case though, so I'm ready to ACK either way. |
|
Hmm, that's a good point, I pushed a commit to kinda handle that - we don't care too much if the peers are the same, and it seems kinda weird to bother handling the case where two peers were opening two channels at once, with different capacities, so instead we just re-validate only if the node ids dont match. |
| // We use the Node IDs rather than the bitcoin_keys to check for "equivalence" | ||
| // as we didn't (necessarily) store the bitcoin keys, and we only really care | ||
| // if the peers on the channel changed anyway. | ||
| if NodeId::from_pubkey(&msg.node_id_1) == chan.node_one && NodeId::from_pubkey(&msg.node_id_2) == chan.node_two { |
There was a problem hiding this comment.
At this point, with a reorg, do we care if the capacity might have changed from what we had before for the same SCID?
There was a problem hiding this comment.
Right, I think to have that we'd have to have two nodes which are opening two channels at the same time with different capacities. Its definitely an edge-case, but I'm not sure how much we really care?
There was a problem hiding this comment.
I don't think it's a big deal. We'll definitely figure out how reliable that capacity is anyway.
If we receive a ChannelAnnouncement message but we already have the channel, there's no reason to do a chain lookup. Instead of immediately calling the user-provided `chain::Access` when handling a ChannelAnnouncement, we first check if we have the corresponding channel in the graph. Note that if we do have the corresponding channel but it was not previously checked against the blockchain, we should still check with the `chain::Access` and update if necessary.
TheBlueMatt
force-pushed
the
2022-08-no-double-access
branch
from
16b9281 to
5847abd
Compare
|
Squashed without further change. |
Codecov Report
@@ Coverage Diff @@
## main #1679 +/- ##
==========================================
+ Coverage 90.77% 90.86% +0.09%
==========================================
Files 80 85 +5
Lines 44763 45714 +951
Branches 44763 45714 +951
==========================================
+ Hits 40632 41539 +907
- Misses 4131 4175 +44
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
If we receive a ChannelAnnouncement message but we already have the
channel, there's no reason to do a chain lookup. Instead of
immediately calling the user-provided
chain::Accesswhen handlinga ChannelAnnouncement, we first check if we have the corresponding
channel in the graph.
Note that if we do have the corresponding channel but it was not
previously checked against the blockchain, we should still check
with the
chain::Accessand update if necessary.