Skip to content

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

Merged
jimjbrettj merged 23 commits intodevelopmentfrom
jimmy/neighborMsgTracking
Oct 31, 2024
Merged

feat(grandpa): implement neighbor tracker and message handling#4230
jimjbrettj merged 23 commits intodevelopmentfrom
jimmy/neighborMsgTracking

Conversation

@jimjbrettj
Copy link
Copy Markdown
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 force-pushed the jimmy/neighborMsgTracking branch from 6293cf3 to 0c0cbb8 Compare October 7, 2024 11:06
@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
@jimjbrettj jimjbrettj force-pushed the jimmy/neighborMsgTracking branch from b51687a to 7c17435 Compare October 8, 2024 07:28
Copy link
Copy Markdown
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.

@jimjbrettj jimjbrettj force-pushed the jimmy/neighborMsgTracking branch from 7c17435 to 38ae1e9 Compare October 15, 2024 06:25
Copy link
Copy Markdown
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.

@jimjbrettj jimjbrettj force-pushed the jimmy/neighborMsgTracking branch from 370e30c to 8104f27 Compare October 15, 2024 11:40
Copy link
Copy Markdown
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.

@jimjbrettj jimjbrettj force-pushed the jimmy/neighborMsgTracking branch from 8104f27 to 9aee52b Compare October 17, 2024 08:27
@jimjbrettj
Copy link
Copy Markdown
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.

@jimjbrettj jimjbrettj requested a review from timwu20 October 17, 2024 10:05
@jimjbrettj jimjbrettj force-pushed the jimmy/neighborMsgTracking branch from 4f39d8f to fa4d931 Compare October 30, 2024 09:01
@jimjbrettj jimjbrettj requested a review from timwu20 October 30, 2024 09:01
@jimjbrettj jimjbrettj force-pushed the jimmy/neighborMsgTracking branch from fa4d931 to 6625430 Compare October 31, 2024 08:25
@jimjbrettj jimjbrettj force-pushed the jimmy/neighborMsgTracking branch from 6625430 to 3dbb1db Compare October 31, 2024 09:59
@jimjbrettj jimjbrettj merged commit 09be343 into development Oct 31, 2024
@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