Resolve long time consensus stuck by network and simple refactorings #915

Charleslee522 · 2019-01-21T08:18:43Z

Github Issue

fixes #900

Solution

When receiving the old ballot, a node broadcasts expired ballot.
If the ballot what a node has is old, a node removes it from the voting results and broadcasts an expired ballot
Because of expired ballot, the consensus stuck is resolved and the next round of voting begins.

ps. As usual, please check each commits

@spikeekips Thanks for your awesome design idea :)

spikeekips · 2019-01-28T02:46:09Z

lib/node/runner/checker.go

+				Round:       blt.VotingBasis().Round,
+				BallotState: blt.State(),
+			},
+			false,


In this part, nr.SetSendRecord should be true? The expired Ballot is sent, right?

@spikeekips Oh, good question 👍
This false setting is for initializing SendRecord.
Because if SendRecord is true, then this ballot will be discarded in nr.BroadcastBallot().
If it is confusing, I can make this to another method like InitializeSendRecord.

@Charleslee522 Renaming will be good :)

Charleslee522 · 2019-01-29T05:44:37Z

@anarcher @kfangw @Geod24 Do you need any help? More description or something? :)

Geod24

Just a few comments, but looks nice

Geod24 · 2019-01-29T08:49:00Z

lib/consensus/isaac.go

+	config := is.Conf
+	opc, _ := ballot.NewCollectTxFeeFromBallot(*newExpiredBallot, config.CommonAccountAddress)
+	opi, _ := ballot.NewInflationFromBallot(*newExpiredBallot, config.CommonAccountAddress, config.InitialBalance)
+	ptx, _ := ballot.NewProposerTransactionFromBallot(*newExpiredBallot, opc, opi)


Do not avoid error checking here

Geod24 · 2019-01-29T08:49:06Z

lib/consensus/isaac.go

@@ -85,6 +85,25 @@ func (is *ISAAC) SelectProposer(blockHeight uint64, round uint64) string {
 	return is.proposerSelector.Select(blockHeight, round)
 }

+func (is *ISAAC) GenerateExpiredBallot(basis voting.Basis, state ballot.State) ballot.Ballot {


Documentation?

Geod24 · 2019-01-29T08:51:16Z

lib/consensus/isaac.go

+
+	newExpiredBallot.SetProposerTransaction(ptx)
+	newExpiredBallot.SignByProposer(is.Node.Keypair(), config.NetworkID)
+	newExpiredBallot.Sign(is.Node.Keypair(), config.NetworkID)


Since we always need to call Sign after SignByProposer, SignByProposer should probably do it itself.
That will also avoid the double hashing / signature which is expensive.

hm.. already SignByProposer is called in Sign.
I need more time to think about handing and signing ExpiredBallot.

@Geod24 How about handling it in the next pr?
I think it is not related with this commit :)

@Geod24 #923

Geod24 · 2019-01-29T09:15:30Z

lib/ballot/ballot.go

@@ -2,7 +2,6 @@ package ballot

 import (
 	"encoding/json"
-	"time"


I love this commit

Geod24 · 2019-01-29T09:17:40Z

lib/consensus/isaac.go

@@ -225,14 +225,14 @@ func (is *ISAAC) Vote(b ballot.Ballot) (isNew bool, err error) {
 	return
 }

-func (is *ISAAC) CanGetVotingResult(b ballot.Ballot) (rv RoundVoteResult, vh voting.Hole, finished bool) {
+func (is *ISAAC) CanGetVotingResult(blt ballot.Ballot) (rv RoundVoteResult, vh voting.Hole, finished bool) {


👍 for using at least 3 char identifiers. Makes the code much more readable IMO.

Geod24 · 2019-01-29T09:21:56Z

lib/consensus/ballot_send_record.go

@@ -24,11 +24,11 @@ func NewBallotSendRecord(nodeAlias string) *BallotSendRecord {
 	return p
 }

-func (r *BallotSendRecord) SetSent(state ISAACState) {
+func (r *BallotSendRecord) SetSent(state ISAACState, sent bool) {


Good time to add documentation?

Geod24 · 2019-01-29T09:29:28Z

lib/consensus/isaac.go

@@ -225,6 +225,52 @@ func (is *ISAAC) Vote(b ballot.Ballot) (isNew bool, err error) {
 	return
 }

+// RemoveOldBallots checkes that `blt` has valid confirmed and proposed time.


Whyyyyyy do all golang documentation start with the function name ? Presumable having the doc attached to the symbol should be enough ? Especially given the extra long names of Go functions...

Anyway, you don't have to change it, since it's consistent with the rest. However, "checkes" => "checks"

Geod24 · 2019-01-29T09:31:49Z

lib/consensus/isaac.go

+	}
+	var roundVote *RoundVote
+	var err error
+	roundVote, err = runningRound.RoundVote(blt.Proposer())


Why not := ?

Geod24 · 2019-01-29T09:34:47Z

lib/consensus/isaac.go

+			}
+		}
+	case ballot.StateACCEPT:
+		for nodeAddr, blt := range roundVote.ACCEPT {


No way to pull this code together ? They have the same logic, just a different variables (roundVote.{SIGN,ACCEPT})

From voting.Hole to ballot.Ballot Because we need ballot proposed time and ballot hash

And documentation related methods

Geod24 · 2019-01-31T05:55:12Z

lib/consensus/ballot_send_record.go

@@ -24,6 +24,8 @@ func NewBallotSendRecord(nodeAlias string) *BallotSendRecord {
 	return p
 }

+// SetSent sets that the ballot of this ISAACState has already been sent.
+// This is to prevent one node from retransmitting another result.


Big big 👍 for documenting what the function is for. It's much more important than what the function do (which one can understand from the code).

Geod24

LGTM

Charleslee522 · 2019-01-31T08:29:25Z

@spikeekips can i merge it? :)

spikeekips

@spikeekips can i merge it? :)

Sorry Too late :)

Charleslee522 added enhancement New feature or request Refactoring labels Jan 21, 2019

Charleslee522 self-assigned this Jan 21, 2019

Charleslee522 requested review from anarcher, spikeekips, kfangw and Geod24 January 21, 2019 08:18

Charleslee522 force-pushed the consensus-stuck-missing-ballot branch 2 times, most recently from f23ce85 to 1a88da6 Compare January 22, 2019 01:00

spikeekips reviewed Jan 28, 2019

View reviewed changes

Charleslee522 force-pushed the consensus-stuck-missing-ballot branch 2 times, most recently from 7923616 to 5620c9a Compare January 29, 2019 02:19

Geod24 reviewed Jan 29, 2019

View reviewed changes

Charleslee522 added 6 commits January 31, 2019 11:23

Broadcast EXP ballot when receiving old ballot

377d4bc

Modify the value of RoundVoteResult

cd3c9f7

From voting.Hole to ballot.Ballot Because we need ballot proposed time and ballot hash

Refactor checking ballot confirmed time

1f631c5

Rename Ballot from b to blt in ISAAC.CanGetVotingResult

f8ee4e8

Add InitSent method in BallotSendRecord

61ecda7

And documentation related methods

Add removing old ballots and broadcast EXP ballots

2d15378

Charleslee522 force-pushed the consensus-stuck-missing-ballot branch from 5620c9a to 2d15378 Compare January 31, 2019 02:24

Geod24 reviewed Jan 31, 2019

View reviewed changes

Geod24 approved these changes Jan 31, 2019

View reviewed changes

spikeekips approved these changes Feb 1, 2019

View reviewed changes

Charleslee522 mentioned this pull request Feb 1, 2019

Check the process of signing an expired ballot in detail #923

Open

Charleslee522 merged commit 073713c into bosnet:master Feb 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve long time consensus stuck by network and simple refactorings #915

Resolve long time consensus stuck by network and simple refactorings #915

Charleslee522 commented Jan 21, 2019 •

edited

Loading

spikeekips Jan 28, 2019

Charleslee522 Jan 29, 2019

spikeekips Jan 29, 2019

Charleslee522 commented Jan 29, 2019

Geod24 left a comment

Geod24 Jan 29, 2019

Geod24 Jan 29, 2019

Geod24 Jan 29, 2019

Charleslee522 Jan 31, 2019

Charleslee522 Jan 31, 2019

Geod24 Jan 31, 2019

Charleslee522 Feb 1, 2019

Geod24 Jan 29, 2019

Geod24 Jan 29, 2019

Geod24 Jan 29, 2019

Geod24 Jan 29, 2019

Geod24 Jan 29, 2019

Geod24 Jan 29, 2019

Geod24 Jan 31, 2019

Geod24 left a comment

Charleslee522 commented Jan 31, 2019

spikeekips left a comment

Resolve long time consensus stuck by network and simple refactorings #915

Resolve long time consensus stuck by network and simple refactorings #915

Conversation

Charleslee522 commented Jan 21, 2019 • edited Loading

Github Issue

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Charleslee522 commented Jan 29, 2019

Geod24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geod24 left a comment

Choose a reason for hiding this comment

Charleslee522 commented Jan 31, 2019

spikeekips left a comment

Choose a reason for hiding this comment

Charleslee522 commented Jan 21, 2019 •

edited

Loading