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: fix ExtendedHeader message duplicates on the network #409

Merged
merged 8 commits into from Feb 14, 2022

Conversation

Wondertan
Copy link
Member

@Wondertan Wondertan commented Feb 2, 2022

What

Finally, a fix to solve this longstanding issue.

How

By using a new feature in libp2p/go-libp2p-pubsub#468 we can now implement custom per-topic message id generation where for the 'HeaderSub' we avoid Signatures from being hashed.

TODO

- [ ] An integration test to assert this is exactly true
We don't have a way to fully simulate this case, as we don't have a test suite to run multiple core validating nodes per test, which we need to rcv multiple headers with different commit set.

  • Manually d assert that light nodes do not receive duplicates occasionally.

Closes #236

@Wondertan Wondertan self-assigned this Feb 2, 2022
go.mod Show resolved Hide resolved
@adlerjohn
Copy link
Member

#237 seems like a PR, did you link to the right thing?

Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

Nice fix! Left a comment nit.

service/header/p2p_subscriber.go Outdated Show resolved Hide resolved
service/header/p2p_subscriber.go Outdated Show resolved Hide resolved
service/header/p2p_subscriber.go Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member Author

Thank you for catching this. No, I linked the wrong thing.

#237 seems like a PR, did you link to the right thing?

Wondertan and others added 2 commits February 8, 2022 19:03
Co-authored-by: John Adler <adlerjohn@users.noreply.github.com>
Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>
@Wondertan
Copy link
Member Author

Just tested manually on my node that this works as expected and there are no duplicate header logs anymore!
YAY!!! 🎉

renaynay
renaynay previously approved these changes Feb 9, 2022
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

LGTM

adlerjohn
adlerjohn previously approved these changes Feb 12, 2022
@Wondertan
Copy link
Member Author

A quick note that we are now maintaining our own fork of pubsub due to #409 (comment)

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Looks good! No objections from my side. I'd give the inner msgID function a different name though.

go.mod Show resolved Hide resolved
service/header/p2p_subscriber.go Show resolved Hide resolved
service/header/p2p_subscriber.go Outdated Show resolved Hide resolved
@Wondertan Wondertan dismissed stale reviews from adlerjohn and renaynay via c1960d8 February 14, 2022 10:17
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

LGTM
+1 to @liamsi comment re: issue for dep

@Wondertan Wondertan merged commit 5b0c2c7 into main Feb 14, 2022
@Wondertan Wondertan deleted the hlib/dedup-headers branch February 14, 2022 10:26
Bidon15 pushed a commit to Bidon15/celestia-node that referenced this pull request Feb 14, 2022
Bidon15 pushed a commit to Bidon15/celestia-node that referenced this pull request Feb 15, 2022
Bidon15 pushed a commit to Bidon15/celestia-node that referenced this pull request Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

service/header: deduplicate Header msgs in pubsub
4 participants