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

Publish in batch #344

Closed
twoeths opened this issue Sep 28, 2022 · 1 comment · Fixed by #480
Closed

Publish in batch #344

twoeths opened this issue Sep 28, 2022 · 1 comment · Fixed by #480
Assignees

Comments

@twoeths
Copy link
Contributor

twoeths commented Sep 28, 2022

Motivation
There is a missed attestation issue described in ChainSafe/lodestar#4600. When lodestar wants to publish a gossip message, it wants to spread to the network asap

Description
Right now the steps to publish a message are as below:

  • Select peers to publish, this is usually about 20 peers (as in lodestar)
  • For each peer:
    • Form a new rpc for that peer with cached control messages of that peer
    • Encode message to bytes using protobuf
    • Encode to length prefix
    • Send to raw stream

The proposal is to send in batch without checking cached control messages

  • Select peers to publish
  • Encode messages to bytes using protobuf, then encode length prefix
  • Send that bytes data to all raw stream

Something to consider

  • Is it against the spec @wemeetagain
  • Should we add a new api or add a new flag to publish() method? Main thing is not to introduce a breaking change
  • Does it have any side effect to control messages? Since we have to forward messages very frequently, and we also have heartbeat, I suppose "no" optimistically
@wemeetagain
Copy link
Member

This proposal is ok according to the spec.
Lets add a new flag to PublishOpts

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