Skip to content

refactor: consensus peer gossip mechanism#551

Merged
shotonoff merged 27 commits intotd-115-fix-consensus-deadlockfrom
refactor-consensus-peer-gossip
Mar 2, 2023
Merged

refactor: consensus peer gossip mechanism#551
shotonoff merged 27 commits intotd-115-fix-consensus-deadlockfrom
refactor-consensus-peer-gossip

Conversation

@shotonoff
Copy link
Collaborator

@shotonoff shotonoff commented Jan 9, 2023

Issue being fixed or feature implemented

The gossip logic implemented as a part of the consensus reactor, that isn't easy to read, support and understand.
The objective of this refactoring is to decouple gossip logic from the consensus reactor into separate components, to address implementation flaws (mentioned above) and more detailed coverage of the gossip protocol.

What was done?

  1. Decouple gossip handlers from the consensus reactor into 3 handler functions
  • queryMaj23GossipHandler
  • votesAndCommitGossipHandler
  • dataGossipHandler

These handlers are responsible for checking the conditions for gossip of a particular data type, similar to the initial implementation of the protocol.

If the condition is satisfied then a handler calls one of the method of Gossiper interface

The idea of this refactoring is to separate the hander logic and the logic of creation and sending data to a peer. It allows us to cover with a test of each use case.

  1. Each handler (mentioned above) must be registered with peerGossipWorker, similar to adding http handler to builtin web server. This component is responsible for starting all registered handlers and can gracefully stop them.

  2. For the sending message is responsible for p2pMsgSender. Instead of using certain p2p channel, this component has all possible p2p channels (channelBundle) and route the message when we send it to a peer

  3. Added a new GO dependency github.com/benbjohnson/clock.
    This package helps to mock time using a mock of clock.Clock interface

How Has This Been Tested?

Added a new unit tests for

  1. msgGossiper is the implementation of Gossiper interface
  • TestGossipVoteSetMaj23 is the test for msgGossiper.GossipVoteSetMaj23
  • TestGossipProposalBlockParts is the test for msgGossiper.GossipProposalBlockParts
  • TestGossipProposal is the test for msgGossiper.GossipProposal

not covered methods yet:

  • msgGossiper.GossipBlockPartsAndCommitForCatchup
  • msgGossiper.GossipCommit
  • msgGossiper.GossipVote
  1. peerGossipWorker is fully covered
  2. p2pMsgSender is fully covered
  3. gossip handlers:
  • queryMaj23GossipHandler is covered

not covered handlers yet:

  • votesAndCommitGossipHandler
  • dataGossipHandler

Breaking Changes

The gossip protocol in almost not modified (removed only redundant sleep calls)

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale label Jan 21, 2023
@github-actions github-actions bot closed this Jan 26, 2023
@shotonoff shotonoff reopened this Jan 26, 2023
@github-actions github-actions bot removed the Stale label Jan 27, 2023
Copy link
Collaborator

@lklimek lklimek left a comment

Choose a reason for hiding this comment

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

Reviewed. I didn't check the logic in details, assuming it's ported correctly.

Copy link
Collaborator Author

@shotonoff shotonoff left a comment

Choose a reason for hiding this comment

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

please have a look at the applied changes

Copy link
Collaborator Author

@shotonoff shotonoff left a comment

Choose a reason for hiding this comment

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

As requested in the comments, peerGossipWorker implements service.Service interface

@shotonoff shotonoff requested a review from lklimek February 28, 2023 16:04
if tc.mockFn != nil {
tc.mockFn(tc.stateData.RoundState, tc.peerState.GetRoundState())
}
hd(ctx, tc.stateData)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see any assertions here. Please fix or add a comment explaining expected result.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

since the method doesn't return a value that we can check on correctness, the assertions/expectations are defined in mockFn as mocking methods.

tc.mockFn()
}
hd(ctx, tc.appState)
hd(ctx, tc.stateData)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see any assertions here. Please fix or add a comment explaining expected result.

Copy link
Collaborator Author

@shotonoff shotonoff left a comment

Choose a reason for hiding this comment

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

answered

@shotonoff shotonoff merged commit 8dcc03d into td-115-fix-consensus-deadlock Mar 2, 2023
@shotonoff shotonoff deleted the refactor-consensus-peer-gossip branch March 2, 2023 09:36
shotonoff added a commit that referenced this pull request Mar 17, 2023
* refactor: add sendMessage method to consensus.State

* refactor: modify sendMessage

* refactor: decompose WAL interface on a few small

* refactor: refactor an approach of message consuming for consensus state component

* refactor: add graceful stop for chanMsgReader

* refactor: consensus state priv-validator

* refactor: huge refactoring of consensus state, separate each state change task as a separate command, similar to state machine implementation

* refactor: add a stop function to a consensus to be able to stop receiveRoutine by a condition

* fix: remove block validating in finalizeCommit

* refactor: define Executor and VoteExtender interfaces, add executor mock, refactor return statement of Executor.ExtendVote

* test: add TestChanQueue, TestChanMsgSender, TestChanMsgReader

* test: generate PrivValidator mock

* refactor: move sending stats data from message handler

* refactor: add-vote operation (#558)

* refactor: separate TryAddVote.addVote logic on small function with one responsibility, use 2 sets (prevote and precommit) to process adding of vote
* refactor: replace peerID on proTxHash for peer catchup rounds in HeightVoteSet component
* fix: remove unused errors and StateData.validateVote

* refactor: EventSwitch implementation (#569)

* refactor: package lib/events rename on eventemitter and EventSwitch rename on EventEmitter, change EventEmitter api

* fix: add PeerManager.Unsubscribe method (#594)

* refactor: set and decide proposal operations (#565)

* refactor: set and decide proposal operations move to the separated component proposaler

* refactor: consensus peer gossip mechanism (#551)

* refactor: separate a gossip logic on implementation and a standard way of iterative handling a gossip condition

* chore: remove unless commented line

* refactor: remove custom "peerSyncErr" error

* refactor: add Gossiper interface, generate Gossiper mock struct, add unit test to queryMaj23GossipHandler

* refactor: peerGossipWorker implements service.Service

* refactor: blocksync module is refactored on using worker-pool

* test: replace custom assertError on tmrequire.Error

* refactor: add Now function into flowrate package, to obtain the current time

* fix: TestHandshakeErrorsIfAppReturnsWrongAppHash in replay_test.go

* refactor: move the logic wait-for blockchain sync from reactor to BlockPool

* fix: TestReactor_SyncTime

* test: change timing for test TestBlockPoolBasic

* chore: replace clock on clockwork

* refactor: replace reactor's consuming on separate consumer function in Channel, rename SetPeerRange on AddPeer

* test: add TestConsumeError

* chore: remove unused mustHexToBytes

* chore: add examples in promise/example_test.go and docs comments

* chore: change a package for MockValidatorSet from types on factory

* Update internal/blocksync/channel.go

* feat: create p2p/client package (#602)

* refactor: add internal/p2p/client implementation

* refactor: change blocksync module to support p2p client

* test: fix TestMakeHTTPDialerURL (#605)

* refactor: rename RandomNode on AnyNode as more specific name

* refactor: introduce p2p client in mempool module (#606)
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.

2 participants