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/core_listener: Fix bug where old block headers are spammed to headersub by bridge nodes #507

Closed
wants to merge 5 commits into from

Conversation

renaynay
Copy link
Member

@renaynay renaynay commented Mar 9, 2022

Resolves #499.

@renaynay renaynay added area:header Extended header area:core_and_app Relationship with Core node and Celestia-App labels Mar 9, 2022
@adlerjohn
Copy link
Member

If there are still TODOs, can we convert this to draft?

@renaynay
Copy link
Member Author

renaynay commented Mar 9, 2022

How do you turn it back to draft mode @adlerjohn ?

@adlerjohn
Copy link
Member

draft

@renaynay renaynay marked this pull request as draft March 9, 2022 14:50
@renaynay renaynay marked this pull request as ready for review March 9, 2022 15:51
@renaynay renaynay marked this pull request as draft March 9, 2022 15:51
@renaynay renaynay marked this pull request as ready for review March 10, 2022 11:30
@renaynay
Copy link
Member Author

TestSyncStartStopLightWithBridge is flakey on main branch as well.

core/fetcher.go Outdated Show resolved Hide resolved
core/fetcher.go Outdated Show resolved Hide resolved
core/fetcher.go Outdated Show resolved Hide resolved
…e.Sleep for second before requesting sync state again
service/header/core_listener.go Outdated Show resolved Hide resolved
service/header/core_listener.go Outdated Show resolved Hide resolved
service/header/core_listener.go Outdated Show resolved Hide resolved
core/fetcher.go Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member

Appendix to my review. The assumption about fixing the issue has to be verified somehow. Manual verification is also fine and shouldn't be hard to setup

@renaynay
Copy link
Member Author

Core_listener should still listen for new blocks while bridge node syncing and erasure code them but should not broadcast them yet.

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.

This is a straightforward change and PR. Thanks!

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.

Only thing that could be improved is related to:

Core_listener should still listen for new blocks while bridge node syncing and erasure code them but should not broadcast them yet.

Ideally this was captured by a test somehow (unit/swamp).

@Wondertan
Copy link
Member

We found an issue with the approach there and after discussing plenty of solutions decided to change the solution here. Specifically, we decided to contribute to the PubSub for a specific feature that would allow us to solve the problem: libp2p/go-libp2p-pubsub#481
We won't wait for it to be accepted as we maintain our own PubSub fork due to #306, so I am cherry-picking the solution now and making an alternative PR. If something is change on main pubsub branch, I will do changes here acrrodingly.

@Wondertan
Copy link
Member

Closed in favour of #549

@Wondertan Wondertan closed this Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core_and_app Relationship with Core node and Celestia-App area:header Extended header
Projects
Archived in project
4 participants