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

service/header: deduplicate Header msgs in pubsub #236

Closed
Wondertan opened this issue Nov 23, 2021 · 5 comments · Fixed by #237 or #409
Closed

service/header: deduplicate Header msgs in pubsub #236

Wondertan opened this issue Nov 23, 2021 · 5 comments · Fixed by #237 or #409

Comments

@Wondertan
Copy link
Member

No description provided.

@Wondertan
Copy link
Member Author

Context

Interestingly, it turned out there were two reasons why Header msgs were duplicated on HeaderSub:

  • We didn't put custom MsgId function into PubSub so it couldn't deduplicate them properly according to the hash of msg body.
    • This was fixed in node/p2p: Deduplicate PubSub msgs #237 and the duplication factor decreased from N validators to random of N, but the issue still remains, so this is why it is being reopened.
  • Commit bodies are different on each Core validator.
    • Due to the nature of the Tendermint consensus, validators don't collect all CommitSignatures, but only the minimum required amount of them. Also, signatures are collected asynchronously. Therefore, each validator may have a different set of minimally required signatures causing indeterminism.

The duplication issue is not critical and the network can function normally. The only problem is a waste of bandwidth which ideally should be avoided.

Solutions

  • Make the DA network lag behind one block, by waiting for LastCommit of the next block. The indeterminism is fixed by consensus over the next block which includes the commit of a previous one.
    • It is not trivial to implement, as FullNode will need custom logic to wait for the next block before broadcasting a Header.
    • It degrades responsiveness, as the tx inclusion will take +1 block.
  • MsgId function that ignores Commit from msg id calculation
    • Unfortunately, PubSub has only a global MsgId generation function and there is no way to set per topic one. This is required not to bring semantics of specific topic msgs to all other topics.
    • Fortunately, supporting the above can be a valuable contribution to the PubSub codebase.

Second solution is more correct and desirable.

@Wondertan Wondertan reopened this Nov 24, 2021
@renaynay renaynay added the area:header Extended header label Nov 29, 2021
@Wondertan
Copy link
Member Author

Needed Pubsub feature for the second solution: libp2p/go-libp2p-pubsub#465

@Wondertan
Copy link
Member Author

Another attempt in pubsub: libp2p/go-libp2p-pubsub#468

@Wondertan
Copy link
Member Author

Wondertan commented Jan 23, 2022

libp2p/go-libp2p-pubsub#468 is now merged. The next step would be to wait for a new release and use implement deduplication, but we can not, as the implemented feature goes after Datastore interface update in the PubSub, which requires us to update ourselves to the new interface and subsequently to the new IPLD version, which is laborious(but worth that)(#306). A quick and dirty solution would be to fork PubSub and exclude commit with the interface update and rely on it.

@Wondertan
Copy link
Member Author

Aaahhh, I messed up the link. If you see this and coming from the dependabot PRs then I am forwarding you to #306

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:header Extended header
Projects
No open projects
2 participants