Remove NodeAnnouncementInfo addresses

[douglaz]

Hopefully closes #1630

Open questions:

1. Are the concerns regarding storing and relaying NodeAnnouncementInfo with excess data size still valid? Specially given the treatment on

   rust-lightning/lightning/src/routing/gossip.rs

   Line 1366 in c18f705

   announcement_message: if should_relay { full_msg.cloned() } else { None },

2. If update_node_from_unsigned_announcement is called, then the addresses will be empty. Is this behavior acceptable?
   1. Perhaps we could still maintain an ephemeral address field just to store and return something in this situation?
   2. Or perhaps copy to this field on deserialization, as suggested in the issue title?
3. Should pub fn addresses(&self) return Option<Vec<NetAddress>> instead of Vec<NetAddress> if announcement_message is None?

[codecov-commenter]

Codecov Report

Patch coverage: 93.10% and project coverage change: +0.19 🎉

Comparison is base (217c3e0) 91.32% compared to head (6f5e5e3) 91.52%.

📣 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    #2102      +/-   ##
==========================================
+ Coverage   91.32%   91.52%   +0.19%     
==========================================
  Files         101      101              
  Lines       48791    49796    +1005     
  Branches    48791    49796    +1005     
==========================================
+ Hits        44558    45574    +1016     
+ Misses       4233     4222      -11     

Impacted Files	Coverage Δ
lightning/src/routing/gossip.rs	90.34% <93.10%> (+0.40%)	⬆️

... and 6 files with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[wpaulino]

Are the concerns regarding storing and relaying NodeAnnouncementInfo with excess data size still valid?

Absolutely. The protocol allows new address descriptors to be advertised so we wouldn't want to store/relay messages that include a whole lot of junk data by prepending it with a greater address descriptor type than those known. That's why we bound this excess data by a reasonable value (1K).

If update_node_from_unsigned_announcement is called, then the addresses will be empty. Is this behavior acceptable?

Definitely not. We should still maintain a list of reachable addresses for a node even if we don't have the full message (which is always the case when syncing via RGS). It seems like this will require storing either the unsigned variant or just its addresses if the signed one isn't available cc @TheBlueMatt.

Should pub fn addresses(&self) return Option<Vec> instead of Vec if announcement_message is None?

I don't think so. It seems we only return Option<Vec<NetAddress>> from ReadOnlyNetworkGraph::get_addresses to indicate to the caller that the node they requested addresses for is not known. This isn't the case with NodeAnnouncementInfo::addresses since the node is already known to us, otherwise we wouldn't have a NodeAnnouncementInfo in the first place.

[TheBlueMatt]

Definitely not. We should still maintain a list of reachable addresses for a node even if we don't have the full message (which is always the case when syncing via RGS). It seems like this will require storing either the unsigned variant or just its addresses if the signed one isn't available cc @TheBlueMatt.

RGS doesn't provide address information anyway, so there's nothing we can do. I think in the cases where we have address information we store it, so its fine. We should note it in the update_node_from_unsigned_announcement docs, since its public.

Should pub fn addresses(&self) return Option instead of Vec if announcement_message is None?

No strong opinion, I guess its not crazy to say "we dont have data for the given node" if we didn't store the full message, even though technically we do have some data, just not the right data.

[valentinewallace]

We may want to write an empty vec for the old field, since currently old versions will fail to read this

[TheBlueMatt]

Oops, looking back at this this isn't true - vec_type semantics are super unclear - they are allowed to have a missing TLV (just implies an empty vec), but we always write something even if the vec is empty. It doesnt matter much tho.

[TheBlueMatt]

When you go to fix val's last comment, can you add some additional detail to the commit message? Just "remove node info addresses" doesnt explain why we're doing it.

[douglaz]

@TheBlueMatt changed commit message, please take a look