Skip to content

Conversation

jking-aus
Copy link

Description

rpc.rs should have been removed previously

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

jxs and others added 3 commits August 20, 2025 22:04
This started with an attempt to solve libp2p#5751 using the previous internal async-channel.
After multiple ideas were discussed off band, replacing the async-channel with an internal more tailored priority queue seemed inevitable.
This priority queue allows us to implement the cancellation of in flight IDONTWANT's very cleanly with the remove_data_messages function.
Clearing the stale messages likewise becomes simpler as we also make use of remove_data_messages
And this has the added advantage of being able to only have a single priority queue and making the code simpler.
If a peer is not making progress we can assume it's not delivering High priority messages and we can penalize it.
@jking-aus jking-aus changed the title remove rpc.rs chore: remove rpc.rs Sep 16, 2025
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

this file is actually required, it's not dangling,
lib.rs requires it

@michaelsproul
Copy link
Member

I don't know what's going on with the diff here*, but rpc.rs is not used on the sigp-gossipsub branch:

mod backoff;
mod behaviour;
mod config;
mod error;
mod gossip_promises;
mod handler;
mod mcache;
#[cfg(feature = "metrics")]
mod metrics;
mod peer_score;
mod protocol;
mod queue;
mod rpc_proto;
mod subscription_filter;
mod time_cache;
mod topic;
mod transform;
mod types;

i.e. commit fabf27f

*Related: I had a commit 5acdf89 checked out locally for sigp-gossipsub which is no longer on that branch, did it get force pushed out of existence? Maybe that's what messed up the diff for this PR.

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.

4 participants