Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify common message handler traits into one trait #3594

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

TheBlueMatt
Copy link
Collaborator

Instead of having each *MessageHandler implement the same
peer_connected, peer_disconnected, provided_init_features,
and provided_node_features methods, here we pull them all out
into one common trait and make all the message handlers extend it.

Since get_and_clear_pending_msg_events was also common we also
move that into the same trait, dropping the
MessageSendEventsProvider trait.

As requested by @tnull.

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Unfortunately currently breaks RUSTFLAGS=--cfg=async_payments

@TheBlueMatt TheBlueMatt force-pushed the 2025-02-peer-connected-trait branch from 7289b80 to 6562669 Compare February 10, 2025 14:10
@TheBlueMatt
Copy link
Collaborator Author

Oops, fixed.

@tnull
Copy link
Contributor

tnull commented Feb 10, 2025

Oops, fixed.

Unfortunately benchmark still failing:

error[E0432]: unresolved import `crate::events::MessageSendEvent`
     --> /home/runner/work/rust-lightning/rust-lightning/lightning/src/ln/channelmanager.rs:15865:29
      |
15865 |     use crate::events::{Event, MessageSendEvent};
      |                                ^^^^^^^^^^^^^^^^ no `MessageSendEvent` in `events`
      |
      = help: consider importing this enum through its public re-export instead:
              crate::ln::channelmanager::MessageSendEvent

warning: unused import: `Mutex`
     --> /home/runner/work/rust-lightning/rust-lightning/lightning/src/ln/channelmanager.rs:15881:25
      |
15881 |     use crate::sync::{Arc, Mutex, RwLock};
      |                            ^^^^^
      |
      = note: `#[warn(unused_imports)]` on by default

warning: unused import: `RouteHop`
    --> /home/runner/work/rust-lightning/rust-lightning/lightning/src/routing/scoring.rs:3678:44
     |
3678 |     use crate::routing::router::{bench_utils, RouteHop};
     |                                               ^^^^^^^^

warning: unused imports: `ChannelFeatures` and `NodeFeatures`
    --> /home/runner/work/rust-lightning/rust-lightning/lightning/src/routing/scoring.rs:3680:31
     |
3680 |     use crate::types::features::{ChannelFeatures, NodeFeatures};
     |                                  ^^^^^^^^^^^^^^^  ^^^^^^^^^^^^

warning: unused variable: `network_graph`
    --> /home/runner/work/rust-lightning/rust-lightning/lightning/src/routing/scoring.rs:3684:8
     |
3684 |         let (network_graph, mut scorer) = bench_utils::read_graph_scorer(&logger).unwrap();
     |              ^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_network_graph`
     |
     = note: `#[warn(unused_variables)]` on by default

@TheBlueMatt TheBlueMatt force-pushed the 2025-02-peer-connected-trait branch from 6562669 to 3fcd949 Compare February 10, 2025 15:49
@TheBlueMatt
Copy link
Collaborator Author

Grr, I really need to fix the CI script so I can run it locally top-to-bottom :/

wpaulino
wpaulino previously approved these changes Feb 10, 2025
@TheBlueMatt
Copy link
Collaborator Author

Gonna at least wait for #3575 and maybe one of the followups for it. Should be pretty straightforward to rebase this.

@wpaulino
Copy link
Contributor

Needs a rebase now that the dependent PR was merged

Instead of having each `*MessageHandler` implement the same
`peer_connected`, `peer_disconnected`, `provided_init_features`,
and `provided_node_features` methods, here we pull them all out
into one common trait and make all the message handlers extend it.

Since `get_and_clear_pending_msg_events` was also common we also
move that into the same trait, dropping the
`MessageSendEventsProvider` trait.

Best reviewed with `--color-moved`
`MessageSendEvent` got placed in the `events` module next to
`Event` but it really doesn't make a whole lot of sense to be
there (despite the superficially similar name). `MessageSendEvent`s
aren't really "events" so much as simply queued messages, handled
by the `PeerManager` rather than the user.

Further, they're used entirely by structs implementing the traits
in `ln::msgs` and basically define an enum over `ln::msgs` structs.
Thus, it only really makes sense to have them in `ln::msgs`, which
we do here.
@TheBlueMatt
Copy link
Collaborator Author

Rebased, I think we can land this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants