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

feat(grandpa): implement neighbor tracker and message handling #4230

Merged
merged 23 commits into from
Oct 31, 2024

Conversation

jimjbrettj
Copy link
Contributor

@jimjbrettj jimjbrettj commented Oct 7, 2024

Changes

This PR adds the neighbor messaging protocol. This includes the ability to handle received neighbor messages, the ability to correctly send neighbor messages, as well as tracking the state of our neighbors (that we use when deciding who to send messages too),

Tests

go test -tags integration github.com/ChainSafe/gossamer

Issues

#2931

@jimjbrettj jimjbrettj marked this pull request as ready for review October 7, 2024 10:41
@jimjbrettj jimjbrettj requested a review from P1sar as a code owner October 7, 2024 11:06
@jimjbrettj jimjbrettj changed the base branch from jimmy/paseoBlock1789153 to development October 7, 2024 11:07
lib/grandpa/message_handler.go Outdated Show resolved Hide resolved
lib/grandpa/neighbor_tracker.go Outdated Show resolved Hide resolved
lib/grandpa/neighbor_tracker.go Outdated Show resolved Hide resolved
lib/grandpa/neighbor_tracker.go Outdated Show resolved Hide resolved
lib/grandpa/neighbor_tracker.go Outdated Show resolved Hide resolved
lib/grandpa/neighbor_tracker.go Outdated Show resolved Hide resolved
lib/grandpa/neighbor_tracker.go Outdated Show resolved Hide resolved
Copy link
Member

@EclesioMeloJunior EclesioMeloJunior left a comment

Choose a reason for hiding this comment

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

LGTM, just a small comment over the usage of a constant.

lib/grandpa/neighbor_tracker.go Outdated Show resolved Hide resolved
Copy link
Contributor

@haikoschol haikoschol left a comment

Choose a reason for hiding this comment

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

Just a few nits/questions.

lib/grandpa/neighbor_tracker.go Outdated Show resolved Hide resolved
lib/grandpa/neighbor_tracker.go Outdated Show resolved Hide resolved
lib/grandpa/neighbor_tracker.go Show resolved Hide resolved
Copy link
Contributor

@timwu20 timwu20 left a comment

Choose a reason for hiding this comment

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

There should be more tests regarding running the NeighborTracker and passing in messages via channel. NeighborTracker.BroadcastNeighborMsg should be tested.

I think one of the integration tests are broken as well based on the changes.

lib/grandpa/neighbor_tracker.go Outdated Show resolved Hide resolved
lib/grandpa/neighbor_tracker.go Outdated Show resolved Hide resolved
@jimjbrettj
Copy link
Contributor Author

@timwu20 Responded to feedback, fixed broken integration tests, and added tests for 1) start/stop 2) updating peer state via channel.

I did not add testing for the finalization channel because no state is actually updated based on it so I didnt know what to assert. Since this channel is used/tested in other parts of the code I thought this is okay, but let me know if/how this should be updated.

lib/grandpa/grandpa.go Outdated Show resolved Hide resolved
lib/grandpa/neighbor_tracker.go Outdated Show resolved Hide resolved
lib/grandpa/neighbor_tracker.go Show resolved Hide resolved
lib/grandpa/network_integration_test.go Outdated Show resolved Hide resolved
@jimjbrettj jimjbrettj merged commit 09be343 into development Oct 31, 2024
24 checks passed
@jimjbrettj jimjbrettj deleted the jimmy/neighborMsgTracking branch October 31, 2024 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants