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

fix(gossipsub): send iwant messages immediately #4875

Closed
wants to merge 1 commit into from

Conversation

zghh
Copy link

@zghh zghh commented Nov 16, 2023

Description

After receiving an IHVE message from another peer, we sometimes need to wait for a heartbeat interval before sending an IWANT message. As a result, the other peer may have cleared the message from the cache, resulting in reducing the other peer's score.

In the implementation of go-libp2p, the IWANT message will be sent in time after receiving the IHAVE message. Therefore this PR will no longer put the message into the control pool but will be sent immediately.

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

Copy link
Contributor

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

This change is good.

However we are currently in the process of simplifying the protocol and improving its efficiency.

The ControlPool was a concept used in go-libp2p which originally was the "specification". It's purpose was to group control messages into a single RPC. The heartbeat was used as a time frame to try and group as many control messages into one RPC within the period.

However as gossipsub has evolved, I agree that we probably want to do immediate IWANT messages and the complexity of the control pool vs anything it saves us if dubious at best.

As we are now preferring simplicity, I would suggest that we remove the control pool entirely, rather than just ad-hoc remove the IWANT's from the control pool. We probably should make a PR that sends all control messages immediately and we no longer bother trying to group messages into a single RPC in the heartbeat.

I'll leave it up to @mxinden and @thomaseizinger to decide whether we merge this PR or a larger one which removes the control pool entirely.

I personally will be back soon working on this code so I can help get this over the line, if needed.

@thomaseizinger
Copy link
Contributor

@jxs What do you prefer? Would you be okay with incorporating the move away from the ControlPool to immediately sending them in your PR?

Alternatively, we could redirect this PR to remove the ControlPool entirely and merge that separately.

@jxs
Copy link
Member

jxs commented Nov 22, 2023

thanks for the ping @thomaseizinger! @zghh with #4811 merged do you want to refactor this one to remove the control_pool?

@jxs
Copy link
Member

jxs commented Nov 27, 2023

friendly ping @zghh, if you want I can go ahead and submit a PR to address this

update:
waiting on #4914 first

@zghh
Copy link
Author

zghh commented Nov 27, 2023

@jxs Great!

@jxs
Copy link
Member

jxs commented Jan 11, 2025

this was addressed with #5595 see more info on #5595 (comment) and

@jxs jxs closed this Jan 11, 2025
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