Update gs api 2#16542
Update gs api 2#16542aarshkshah1992 wants to merge 4 commits intorepublish-data-columns-reconstructed-columnsfrom
Conversation
| respCh := make(chan gossipForPeerResponse, 1) | ||
|
|
||
| OnEmitGossip: func(topic string, groupID []byte, gossipPeers []peer.ID, peerStates map[peer.ID]blocks.PartialDataColumnPeerState) { | ||
| select { |
There was a problem hiding this comment.
@MarcoPolo We end up throwing away the "gossipPeers" and "peerStates" here completely. On handing this gossip, we effectively end up calling "PublishPartial" which in turn calls publish actions for ALL peers with partial messages enabled.
So what's the point of having "gossipPeers" here ?
There was a problem hiding this comment.
You could imagine wanting to initialize some state for your gossipPeers in peerStates. For Prysm the zero value works fine so we don't need to do anything special.
| if !existing.Column.Published { | ||
| return | ||
| } | ||
| err := p.publishPartialCol(topic, existing.Column.GroupID(), existing.Column) |
There was a problem hiding this comment.
This is effectively a "Publish Partial" call for ALL peers even though we're saying we're doing gossip here. Is this intentional ?
There was a problem hiding this comment.
yes. Since the call is idempotent for peers we've already published to, the result is only the peers that we gossip to will get updates.
| // ForPeer implements partialmessages.Message. | ||
| func (p *PartialDataColumn) ForPeer(remote peer.ID, requestedMessage bool, peerState partialmessages.PeerState) (partialmessages.PeerState, []byte, | ||
| partialmessages.PartsMetadata, error) { | ||
| func (p *PartialDataColumn) PublishActions(peerStates map[peer.ID]PartialDataColumnPeerState, peerRequestsPartial func(peer.ID) bool) iter.Seq2[peer.ID, partialmessages.PublishAction] { |
There was a problem hiding this comment.
My concern with the "peerRequestsPartial" here is that this is a semantic change compared to earlier Gossipsub.
If you look at how this is set in go-libp2p-gossipsub, it's basically "true" for ALL peers that accept partial messages/have the partial message extension enabled.
But, earlier, this was ONLY set to true for non-gossip publish requests i.e. if "requestedMessage" in the call to "forPeer" below was set to false (which it would be when we were gossiping), we'd only send the parts metadata. But now, this will be set to true for all peers that support the partial message extension and so we will always send cells as well(if applicable) in addition to the parts metadata.
There was a problem hiding this comment.
I don't think it's the end of the world to send cells as part of gossip but want to make sure it's intentional.
There was a problem hiding this comment.
I think there's a misunderstanding in the prior version. The peerRequestsPartial has been consistent since it was introduced in the partial messages extension, the new version of the gs api didn't change this. We don't use it for ethereum, so you could just ignore this parameter.
The purpose of it is to slowly introduce partial messages on a topic. It allows one to roll out support for partial messages before having nodes request those partial messages. We decided to simplify rollout by just having nodes always support and request partial messages and not deal with this transitory state.
aarshkshah1992
force-pushed
the
update-gs-api-2
branch
from
4960826 to
9240158
Compare
2 participants
Update to the latest Gossipsub API and interface.