Add node id to remaining RoutingMessageHandler::handle_ methods

[tnull]

Previously, some RoutingMessageHandler::handle_ methods (in particular the ones handling node and channel announcements, as well as channel updates, omitted the their_node_id argument. This didn't allow implementors to discern who sent a particular method.

Here, we add their_node_id: Option<&PublicKey> to have them learn who sent a message, and set None if our own node it the originator of a broadcast operation.

[codecov]

Codecov Report

Attention: Patch coverage is 83.34794% with 190 lines in your changes missing coverage. Please review.

Project coverage is 89.60%. Comparing base (82b3f62) to head (b172942).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/peer_handler.rs	32.23%	82 Missing ⚠️
lightning-net-tokio/src/lib.rs	15.21%	39 Missing ⚠️
lightning/src/util/test_utils.rs	23.25%	33 Missing ⚠️
lightning/src/ln/channelmanager.rs	82.31%	26 Missing and 3 partials ⚠️
lightning/src/ln/functional_test_utils.rs	95.12%	4 Missing ⚠️
lightning/src/routing/gossip.rs	95.65%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3291      +/-   ##
==========================================
- Coverage   89.64%   89.60%   -0.04%     
==========================================
  Files         126      126              
  Lines      102185   102208      +23     
  Branches   102185   102208      +23     
==========================================
- Hits        91599    91586      -13     
- Misses       7863     7892      +29     
- Partials     2723     2730       +7     

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

[TheBlueMatt]

Thanks. Boy that Option really exposes how awkward it is that we loop messages back around to the gossip handler doesn't it...

Anyway, there's some tests that are passing messages to a node with the source being Some(its own pubkey), which seems weird, but I'm not sure it matters that much. We could just as well pas None for all of them.

[tnull]

Thanks. Boy that Option really exposes how awkward it is that we loop messages back around to the gossip handler doesn't it...

Yeah, agree that it's not super pretty. Let me know if you have a better approach in mind, Option seemed to be the simplest choice here.

Anyway, there's some tests that are passing messages to a node with the source being Some(its own pubkey), which seems weird, but I'm not sure it matters that much. We could just as well pas None for all of them.

Yeah, while we currently could give None everywhere since we don't actually use the field, I tried to avoid that as we may start using it in the future and then we'd get unexpected test behavior. In very few cases it was a bit unclear what the 'right' pubkey to use is, for example as in some instances we just use a RoutingMessageHandler that doesn't logically belong to any particular node to check the messages. Let me know if you see any particular examples that should be changed, though.

[TheBlueMatt]

Yeah, agree that it's not super pretty. Let me know if you have a better approach in mind, Option seemed to be the simplest choice here.

I do not have any better ideas.

Yeah, while we currently could give None everywhere since we don't actually use the field, I tried to avoid that as we may start using it in the future and then we'd get unexpected test behavior. In very few cases it was a bit unclear what the 'right' pubkey to use is, for example as in some instances we just use a RoutingMessageHandler that doesn't logically belong to any particular node to check the messages. Let me know if you see any particular examples that should be changed, though.

Nah, it all seems fine, not worth adding a ton of code to calculate the "right" pubkey everywhere.

[tnull]

Now added the discussed refactor to take PublicKey rather than &PublicKey across interfaces, quite a diff, though trivial.

Also rebased to resolve minor conflicts with main.

[valentinewallace]

CI is sad. Happy to ACK otherwise.

[TheBlueMatt]

Just gonna land this cause rebasing it will be a nightmare.

[TheBlueMatt]

Ah, yea, doesnt pass CI...

[tnull]

Force-pushed with minor changes addressing the silent rebase conflicts:

> git diff-tree -U2 d7c458236 3eaa13e17
diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs
index 92b31311c..4bcb8b859 100644
--- a/lightning/src/routing/gossip.rs
+++ b/lightning/src/routing/gossip.rs
@@ -2599,5 +2599,5 @@ pub(crate) mod tests {
                };

-               match gossip_sync.handle_node_announcement(&valid_announcement) {
+               match gossip_sync.handle_node_announcement(Some(node_1_pubkey), &valid_announcement) {
                        Ok(res) => assert!(res),
                        Err(_) => panic!()
@@ -2739,5 +2739,5 @@ pub(crate) mod tests {

                // Don't relay valid channels with excess data
-               match gossip_sync.handle_channel_announcement(&valid_excess_data_announcement) {
+               match gossip_sync.handle_channel_announcement(Some(node_1_pubkey), &valid_excess_data_announcement) {
                        Ok(res) => assert!(!res),
                        _ => panic!()
@@ -3883,5 +3883,4 @@ pub(crate) mod tests {
                gossip_sync.handle_node_announcement(Some(node_1_pubkey), &announcement).unwrap();
                assert!(!network_graph.read_only().node(&node_1_id).unwrap().is_tor_only());
-               assert!(!network_graph.read_only().node(&node_1_id).unwrap().is_tor_only());

                let announcement = get_signed_node_announcement(

[tnull]

I think the linting CI fail is preexisting?

[tnull]

Force pushed again, previously forgot to refactor async_payments_tests.rs and run the other in-progress cfg flags. I think I got them all now.