refactor: add-vote operation by shotonoff · Pull Request #558 · dashpay/tenderdash

shotonoff · 2023-01-19T16:52:05Z

Issue being fixed or feature implemented

Implementation of AddVote operation has a bad design that leads in difficult reading and understanding this process.
In a discussion with @lklimek, we decided that this function must be refactored to make this process clear and maintainable

What was done?

The addVote operation was separated on several independent functions, which are used for prevote and precommit sets
A received vote is processed by the vote-set according to Vote.Type
Key design changes
there are 2 new types:

addVoteFunc func this type must be implemented by each function to be able to use in a add-vote set
addVoteMwFunc this type must be implemented by middleware function

The idea is to use middleware approach (or chain responsibility) where

the process might be interrupted by a specific condition
to follow the single responsibility principle

Voting has been split between these functions:

addVoteToLastPrecommitMw - either add a vote to last-precommit vote-set and break processing or move to the next item
addVoteValidateVoteMw - validate vote before to add and if a vote is valid to move to the next item
addVoteToVoteSet - this is the main function, it only has the logic of adding votes
addVoteUpdateValidBlockMw - updates state-data after successful adding vote and move to the next item
addVoteDispatchPrevoteMw - after successful added vote, executes the transition based on vote state for prevote and move to the next item
addVoteDispatchPrecommitMw - execute another transition based on vote state for precommit
addVoteVerifyVoteExtensionMw - before adding vote verify vote extensions if a vote is precommit
addVoteStatsMw - send msgInfo message to stats-queue
addVoteErrorMw - correct handle error
addVoteLoggingMw - write logs before and after adding the vote

How Has This Been Tested?

Unit-test / E2E test
Added a few new unit-tests for new code, need to add more

Breaking Changes

N/A

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

…e responsibility, use 2 sets (prevote and precommit) to process adding of vote

…htVoteSet component

internal/consensus/context.go

internal/consensus/msg_handlers.go

internal/consensus/state_add_vote_cmd.go

internal/consensus/state_data.go

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

…peration

…o refactor-add-vote-operation # Conflicts: # internal/consensus/state_add_vote_cmd.go

…peration

shotonoff

please check the answers and changes according to the feedback

internal/consensus/msg_handlers.go

lklimek · 2023-01-26T14:04:35Z

internal/consensus/state_add_vote_cmd.go

+			}
+			added, err := stateData.LastPrecommits.AddVote(vote)
+			if !added {
+				logger.Debug("vote not added to last precommits", logKeyValsWithError([]any(nil), err)...)


looks like overkill

Suggested change

logger.Debug("vote not added to last precommits", logKeyValsWithError([]any(nil), err)...)

logger.Debug("vote not added to last precommits", "error", err)

No, the idea is to add an error only if the error is not nil
your patch adds the error with nil value.
the empty (or nil) error leads difficult filtering the logs by error

maybe add sme feature to our log package? to automatically skip error if it's nil. Like logger.Err(err).Debug()? (can be separate PR, to not increase scope of this one).

internal/consensus/state_add_vote_cmd.go

types/vote_set.go

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

…peration

libs/log/context.go

internal/consensus/msg_handlers.go

types/vote_set.go

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

…o refactor-add-vote-operation

shotonoff

applied changes by PR

internal/consensus/msg_handlers.go

shotonoff · 2023-01-27T11:43:23Z

internal/consensus/state_add_vote_cmd.go

+			}
+			added, err := stateData.LastPrecommits.AddVote(vote)
+			if !added {
+				logger.Debug("vote not added to last precommits", logKeyValsWithError([]any(nil), err)...)


No, the idea is to add an error only if the error is not nil
your patch adds the error with nil value.
the empty (or nil) error leads difficult filtering the logs by error

types/vote_set.go

internal/consensus/msg_handlers.go

libs/log/context.go

lklimek · 2023-02-02T11:24:22Z

internal/consensus/state_add_vote_cmd.go

+			}
+			added, err := stateData.LastPrecommits.AddVote(vote)
+			if !added {
+				logger.Debug("vote not added to last precommits", logKeyValsWithError([]any(nil), err)...)


maybe add sme feature to our log package? to automatically skip error if it's nil. Like logger.Err(err).Debug()? (can be separate PR, to not increase scope of this one).

* 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)

refactor: separate TryAddVote.addVote logic on small function with on…

cacbf2a

…e responsibility, use 2 sets (prevote and precommit) to process adding of vote

shotonoff requested review from QuantumExplorer and lklimek as code owners January 19, 2023 16:52

shotonoff changed the title ~~refactor: separate TryAddVote.addVote logic on small function with on…~~ refactor: add-vote operation Jan 19, 2023

shotonoff added 2 commits January 20, 2023 10:08

refactor: replace peerID on proTxHash for peer catchup rounds in Heig…

6f5edb4

…htVoteSet component

refactor: remove peerID from function signature

e4e68f9

lklimek reviewed Jan 24, 2023

View reviewed changes

shotonoff and others added 6 commits January 24, 2023 13:11

Update internal/consensus/state_add_vote_cmd.go

b2904ed

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

Merge branch 'td-115-fix-consensus-deadlock' into refactor-add-vote-o…

584d38f

…peration

Merge remote-tracking branch 'origin/refactor-add-vote-operation' int…

590fca3

…o refactor-add-vote-operation # Conflicts: # internal/consensus/state_add_vote_cmd.go

refactor: changes by PR feedback

db9367c

fix: remove unused errors and StateData.validateVote

d6056d2

Merge branch 'td-115-fix-consensus-deadlock' into refactor-add-vote-o…

e45324e

…peration

shotonoff requested a review from lklimek January 24, 2023 16:47

shotonoff commented Jan 24, 2023

View reviewed changes

lklimek reviewed Jan 26, 2023

View reviewed changes

shotonoff and others added 6 commits January 27, 2023 12:33

Update internal/consensus/msg_handlers.go

4b258a5

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

refactor: changes by PR feedback

1aaaefe

fix: linter issue

60f6a2c

chore: rename AddVoteCommand to AddVoteAction

98b79be

refactor: changes by feedback

2563472

Merge branch 'td-115-fix-consensus-deadlock' into refactor-add-vote-o…

4b3a0f0

…peration

lklimek reviewed Jan 31, 2023

View reviewed changes

shotonoff and others added 4 commits January 31, 2023 10:39

Update types/vote_set.go

a7d10ab

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

refactor: change a way of logging of handling consensus message

11b9695

Merge remote-tracking branch 'origin/refactor-add-vote-operation' int…

7549064

…o refactor-add-vote-operation

chore: fix cs

a0c8766

shotonoff commented Jan 31, 2023

View reviewed changes

lklimek approved these changes Feb 2, 2023

View reviewed changes

shotonoff merged commit a251317 into td-115-fix-consensus-deadlock Feb 2, 2023

lklimek deleted the refactor-add-vote-operation branch February 1, 2024 13:33

	logger.Debug("vote not added to last precommits", logKeyValsWithError([]any(nil), err)...)
	logger.Debug("vote not added to last precommits", "error", err)

Conversation

shotonoff commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shotonoff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lklimek Jan 26, 2023

Choose a reason for hiding this comment

Uh oh!

shotonoff Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

lklimek Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shotonoff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shotonoff Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lklimek Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shotonoff commented Jan 19, 2023 •

edited

Loading