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

[consensus][viewchange] fix total power of signers in IsQuorumAchieved for view change #2758

Conversation

gupadhyaya
Copy link
Contributor

fixes #2741

didn't want to completely get rid of quorum.ViewChange, as signers total voting power information could be useful. Further, removing the option quorum.ViewChange may lead to regression.

@gupadhyaya gupadhyaya requested a review from rlan35 April 6, 2020 19:53
@fxfactorial
Copy link
Contributor

This is a change of original view change logic. Dig deeper into the git commit history of initial usage of submitvote to see this was original value used. I also was confused by this initially but didn't want to deviate from original code.

@gupadhyaya
Copy link
Contributor Author

gupadhyaya commented Apr 6, 2020

Went back in history... looks like in the original OnViewChange method, the the vote was still added to commitSigs, but never used, instead viewIDSigs was used. See below:

consensus.viewIDSigs[senderKey.SerializeToHexStr()] = recvMsg.ViewidSig
...
if len(consensus.viewIDSigs) >= consensus.Quorum() {
    ...
    consensus.commitSigs[consensus.PubKey.SerializeToHexStr()] = consensus.priKey.SignHash(commitPayload)

History:

commit where view change was working: ddb2076cee19294b14a8b814acbff5f48d5bfc2e (without staking refactor)
commit that broke view change: 9f00923ac344d297f5347958f38b909e1656e95a (staking refactor)
commit that fixed it by changing the decider to mask: 925afa59eea2a1b1f7e98d8fcf4df0d217aeb9fa

when @fxfactorial refactored to IsQuorumAchieved I believe it broke view change, hence @chaosma changed the logic to use IsQuorumAchievedByMask. This explains why view change was working correctly.

Right now the code still uses IsQuorumAchievedByMask and not IsQuorumAchieved, hence view change works as expected.

@fxfactorial fxfactorial merged commit 054c3cb into harmony-one:master Apr 6, 2020
@rlan35
Copy link
Contributor

rlan35 commented Apr 6, 2020

if consensus.Decider.IsQuorumAchieved(quorum.ViewChange) {
    consensus.getLogger().Debug().
        Int64("have", consensus.Decider.SignersCount(quorum.ViewChange)).
        Int64("need", consensus.Decider.TwoThirdsSignersCount()).
        Str("validatorPubKey", recvMsg.SenderPubkey.SerializeToHexStr()).
        Msg("[onViewChange] Received Enough View Change Messages")
    return
}

this part was a no-op, because it never reach quorum. But it wasn't a problem because after that it's checked by Mask.
So to me, there wasn't a problem maybe. Just in the last time view change failed (maybe due to not enough votes). And when I look at the log, I saw the 0 voting power thing in IsQuorumAchieved. But it isn't the cause of view change failure.

rlan35 added a commit to rlan35/harmony that referenced this pull request Apr 7, 2020
commit 70a4272
Author: Rongjian Lan <rongjian.lan@gmail.com>
Date:   Mon Apr 6 17:06:33 2020 -0700

    fix block proposal ordering; other offchain commits change (harmony-one#2761)

    * fix block proposal ordering; other offchain commits change

    * remove div by 0 checks

    * Fix imports

commit 054c3cb
Author: Ganesha Upadhyaya <ganeshrvce@gmail.com>
Date:   Mon Apr 6 13:42:07 2020 -0700

    [consensus][viewchange] fix total power of signers in IsQuorumAchieved for view change (harmony-one#2758)

commit 094a613
Author: Edgar Aroutiounian <edgar.factorial@gmail.com>
Date:   Mon Apr 6 11:45:50 2020 -0700

    [project] Unused yaml files, staticcheck fixes, dead tests that only delay build  (harmony-one#2755)

    * [project] Unused yaml files

    * [core] Remove dead tx_cacher.go

    * [project] Further fixes from staticcheck

commit a7b7961
Author: Edgar Aroutiounian <edgar.factorial@gmail.com>
Date:   Mon Apr 6 10:04:54 2020 -0700

    [project] More Spring Cleaning  (harmony-one#2751)

    * [project] Remove hmyclient

    * [drand] Remove dead drand

    * [drand] Remove dead code in proto, more dead drand things

    * [project] Remove dead demo tool

    * [project] Remove dead Lottery solidity

    * [project] Remove dead hmy client main

    * [node] Remove unused variable in test code

    * [project] Remove keygen

    * [project] Remove not useful test

commit ba1c0cd
Author: Edgar Aroutiounian <edgar.factorial@gmail.com>
Date:   Sun Apr 5 23:53:03 2020 -0700

    [blockchain] Create fresh big int, remove more dead code from static … (harmony-one#2749)

    * [blockchain] Create fresh big int, remove more dead code from static analysis

    * [measure] Remove IzZero/IsNil checks

commit 191f1d5
Author: Rongjian Lan <rongjian.lan@gmail.com>
Date:   Sun Apr 5 21:11:21 2020 -0700

    Final attempt: Don't div by 0; print out debug info (harmony-one#2746)

    * Revert "Revert "fix earned-reward by writing the stats only once, remove from UpdateValdiatorVotingPower (harmony-one#2737)" (harmony-one#2742)"

    This reverts commit 832b01d.

    * Don't div by 0; print out debug info

commit 832b01d
Author: Edgar Aroutiounian <edgar.factorial@gmail.com>
Date:   Sun Apr 5 19:40:37 2020 -0700

    Revert "fix earned-reward by writing the stats only once, remove from UpdateValdiatorVotingPower (harmony-one#2737)" (harmony-one#2742)

    This reverts commit dc036e6.

commit dc036e6
Author: Ganesha Upadhyaya <ganeshrvce@gmail.com>
Date:   Sun Apr 5 17:18:36 2020 -0700

    fix earned-reward by writing the stats only once, remove from UpdateValdiatorVotingPower (harmony-one#2737)

commit 66f26e8
Author: Rongjian Lan <rongjian.lan@gmail.com>
Date:   Sun Apr 5 17:06:50 2020 -0700

    do snapshot for validators at last block of epoch (harmony-one#2736)

commit f2524a8
Author: Rongjian Lan <rongjian.lan@gmail.com>
Date:   Sun Apr 5 12:33:35 2020 -0700

    Make validator snapshot on new validator too (harmony-one#2733)

commit 350b7a0
Author: Rongjian Lan <rongjian.lan@gmail.com>
Date:   Sat Apr 4 23:41:19 2020 -0700

    fix validator snapshot cache

commit 8f5f287
Author: Leo Chen <leo@harmony.one>
Date:   Sat Apr 4 19:09:38 2020 -0700

    [node.sh] use rclone to fast sync harmony_db_0 (harmony-one#2709)

    Signed-off-by: Leo Chen <leo@harmony.one>

commit 05f2e20
Author: Rongjian Lan <rongjian.lan@gmail.com>
Date:   Sat Apr 4 19:00:28 2020 -0700

    make validator snapshot consistent with election (harmony-one#2726)

    * make validator snapshot consistent with election

    * fix condition with only beacon chain

    * Fix epoch number

    * Fix stats update nil epoch issue

commit 24a17fe
Author: Edgar Aroutiounian <edgar.factorial@gmail.com>
Date:   Sat Apr 4 16:38:12 2020 -0700

    [state] ValidatorWrapper div zero panic fix -  (harmony-one#2724)

    * [state] Error on div zero

    * [state] Do not pay out commit when commit is 0%

    * [state] Put whole commission in guarded block

    * [core] Reward cannot be zero

    * [state] return nil, not hard error

commit 1f83ebf
Author: Minh Doan <40258599+mikedoan@users.noreply.github.com>
Date:   Sat Apr 4 14:38:55 2020 -0700

    change explorer node storage folder (harmony-one#2720)

commit 3aa08e9
Author: Edgar Aroutiounian <edgar.factorial@gmail.com>
Date:   Sat Apr 4 10:21:51 2020 -0700

    [rpc] Show both latest header of beacon chain and shard chain (harmony-one#2714)

commit 1a2c23a
Author: Edgar Aroutiounian <edgar.factorial@gmail.com>
Date:   Fri Apr 3 17:49:24 2020 -0700

    [validator] Hide one field from JSON (harmony-one#2705)

# Conflicts:
#	core/tx_cacher.go
rlan35 added a commit that referenced this pull request Apr 7, 2020
* Squashed commit of the following:

commit 70a4272
Author: Rongjian Lan <rongjian.lan@gmail.com>
Date:   Mon Apr 6 17:06:33 2020 -0700

    fix block proposal ordering; other offchain commits change (#2761)

    * fix block proposal ordering; other offchain commits change

    * remove div by 0 checks

    * Fix imports

commit 054c3cb
Author: Ganesha Upadhyaya <ganeshrvce@gmail.com>
Date:   Mon Apr 6 13:42:07 2020 -0700

    [consensus][viewchange] fix total power of signers in IsQuorumAchieved for view change (#2758)

commit 094a613
Author: Edgar Aroutiounian <edgar.factorial@gmail.com>
Date:   Mon Apr 6 11:45:50 2020 -0700

    [project] Unused yaml files, staticcheck fixes, dead tests that only delay build  (#2755)

    * [project] Unused yaml files

    * [core] Remove dead tx_cacher.go

    * [project] Further fixes from staticcheck

commit a7b7961
Author: Edgar Aroutiounian <edgar.factorial@gmail.com>
Date:   Mon Apr 6 10:04:54 2020 -0700

    [project] More Spring Cleaning  (#2751)

    * [project] Remove hmyclient

    * [drand] Remove dead drand

    * [drand] Remove dead code in proto, more dead drand things

    * [project] Remove dead demo tool

    * [project] Remove dead Lottery solidity

    * [project] Remove dead hmy client main

    * [node] Remove unused variable in test code

    * [project] Remove keygen

    * [project] Remove not useful test

commit ba1c0cd
Author: Edgar Aroutiounian <edgar.factorial@gmail.com>
Date:   Sun Apr 5 23:53:03 2020 -0700

    [blockchain] Create fresh big int, remove more dead code from static … (#2749)

    * [blockchain] Create fresh big int, remove more dead code from static analysis

    * [measure] Remove IzZero/IsNil checks

commit 191f1d5
Author: Rongjian Lan <rongjian.lan@gmail.com>
Date:   Sun Apr 5 21:11:21 2020 -0700

    Final attempt: Don't div by 0; print out debug info (#2746)

    * Revert "Revert "fix earned-reward by writing the stats only once, remove from UpdateValdiatorVotingPower (#2737)" (#2742)"

    This reverts commit 832b01d.

    * Don't div by 0; print out debug info

commit 832b01d
Author: Edgar Aroutiounian <edgar.factorial@gmail.com>
Date:   Sun Apr 5 19:40:37 2020 -0700

    Revert "fix earned-reward by writing the stats only once, remove from UpdateValdiatorVotingPower (#2737)" (#2742)

    This reverts commit dc036e6.

commit dc036e6
Author: Ganesha Upadhyaya <ganeshrvce@gmail.com>
Date:   Sun Apr 5 17:18:36 2020 -0700

    fix earned-reward by writing the stats only once, remove from UpdateValdiatorVotingPower (#2737)

commit 66f26e8
Author: Rongjian Lan <rongjian.lan@gmail.com>
Date:   Sun Apr 5 17:06:50 2020 -0700

    do snapshot for validators at last block of epoch (#2736)

commit f2524a8
Author: Rongjian Lan <rongjian.lan@gmail.com>
Date:   Sun Apr 5 12:33:35 2020 -0700

    Make validator snapshot on new validator too (#2733)

commit 350b7a0
Author: Rongjian Lan <rongjian.lan@gmail.com>
Date:   Sat Apr 4 23:41:19 2020 -0700

    fix validator snapshot cache

commit 8f5f287
Author: Leo Chen <leo@harmony.one>
Date:   Sat Apr 4 19:09:38 2020 -0700

    [node.sh] use rclone to fast sync harmony_db_0 (#2709)

    Signed-off-by: Leo Chen <leo@harmony.one>

commit 05f2e20
Author: Rongjian Lan <rongjian.lan@gmail.com>
Date:   Sat Apr 4 19:00:28 2020 -0700

    make validator snapshot consistent with election (#2726)

    * make validator snapshot consistent with election

    * fix condition with only beacon chain

    * Fix epoch number

    * Fix stats update nil epoch issue

commit 24a17fe
Author: Edgar Aroutiounian <edgar.factorial@gmail.com>
Date:   Sat Apr 4 16:38:12 2020 -0700

    [state] ValidatorWrapper div zero panic fix -  (#2724)

    * [state] Error on div zero

    * [state] Do not pay out commit when commit is 0%

    * [state] Put whole commission in guarded block

    * [core] Reward cannot be zero

    * [state] return nil, not hard error

commit 1f83ebf
Author: Minh Doan <40258599+mikedoan@users.noreply.github.com>
Date:   Sat Apr 4 14:38:55 2020 -0700

    change explorer node storage folder (#2720)

commit 3aa08e9
Author: Edgar Aroutiounian <edgar.factorial@gmail.com>
Date:   Sat Apr 4 10:21:51 2020 -0700

    [rpc] Show both latest header of beacon chain and shard chain (#2714)

commit 1a2c23a
Author: Edgar Aroutiounian <edgar.factorial@gmail.com>
Date:   Fri Apr 3 17:49:24 2020 -0700

    [validator] Hide one field from JSON (#2705)

# Conflicts:
#	core/tx_cacher.go

* fix merge conflict
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.

View change fail due to voting power always being 0
3 participants