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

Fix vote for auto excluded stakers #899

Merged
merged 1 commit into from Dec 13, 2021

Conversation

aguycalled
Copy link
Member

This Pull Request fixes a bug where stakers which were auto excluded from voting because they were inactive for more than 10 voting cycles do not commit their votes in the nNonce parameter of the block.

How to test

Verify that staked blocks have its nonce value set to greater that 1 even when the wallet has been auto-excluded from voting

@mxaddict
Copy link
Contributor

mxaddict commented Dec 7, 2021

Away from my main PC right now, will wait for gitian build to test

@aguycalled
Copy link
Member Author

@navbuilder
Copy link

A new build of 0a13ecf has completed succesfully!
Binaries available at https://build.nav.community/binaries/fix-vote-excluded

@mxaddict
Copy link
Contributor

mxaddict commented Dec 9, 2021

Can I see the nonce value for the block in the CLI?

@aguycalled
Copy link
Member Author

aguycalled commented Dec 9, 2021

yep with getblockheader

@mxaddict
Copy link
Contributor

mxaddict commented Dec 9, 2021 via email

@chasingkirkjufell
Copy link
Contributor

im still having issue replicating the issue. so current master the nonce should be 1 when someone is inactive for more than 10 cycles and this PR nonce should be > 1 right?

@aguycalled
Copy link
Member Author

yes the condition is that there must be active votings during those 10 cycles and the voter did not engage in any of those

@mxaddict
Copy link
Contributor

@chasingkirkjufell are you manually testing this? Or adding it to the stressor?

@chasingkirkjufell
Copy link
Contributor

im tseting it manually but i can't exclude vote to happen on devnet unless i just do excludevote=1 @mxaddict

Copy link
Contributor

@chasingkirkjufell chasingkirkjufell left a comment

Choose a reason for hiding this comment

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

i'm not sure why i can't replicate this on devnet but the fix makes sense.

@mxaddict
Copy link
Contributor

I'm trying to figure out how to test this correctly, do you guys have a suggestion?

@chasingkirkjufell
Copy link
Contributor

I'm curious to know if you can get a node to enter exclude vote by being idle for 10 voting cycles with active voting going on. I can't get it to trigger on devent so I didn't verify the bug. I set up two nodes and neither triggered. In the end I just approved it since it makes sense and aguycalled confirmed his nodes are now working properly. @mxaddict

@mxaddict
Copy link
Contributor

I'm curious to know if you can get a node to enter exclude vote by being idle for 10 voting cycles with active voting going on. I can't get it to trigger on devent so I didn't verify the bug. I set up two nodes and neither triggered. In the end I just approved it since it makes sense and aguycalled confirmed his nodes are now working properly. @mxaddict

I'll try that

@mxaddict
Copy link
Contributor

@aguycalled I could not get a devnet node to get auto excluded from the network

I tried doing this:
Node A Staking with votes for a proposal
Node B Staking with no votes selected

I did not notice that node be was excluded after 10 cycles

Am I testing this right?

@aguycalled
Copy link
Member Author

you can use this to debug the auto excluding process

diff --git a/src/miner.cpp b/src/miner.cpp
index dc468dbf..14560afa 100644
--- a/src/miner.cpp
+++ b/src/miner.cpp
@@ -1142,6 +1142,8 @@ bool SignBlock(CBlock *pblock, CWallet& wallet, int64_t nFees, std::string sLog)
                       int lastVote = pVoteList.GetLastVoteHeight();
                       auto nCycleLength = GetConsensusParameter(Consensus::CONSENSUS_PARAM_VOTING_CYCLE_LENGTH, view);
 
+                      LogPrintf("%s: last vote from staker was at height %d (%d voting cycles)\n", __func__, lastVote, (chainActive.Tip()->nHeight - lastVote) / nCycleLength);
+
                       if (chainActive.Tip()->nHeight - lastVote > nCycleLength*10 || chainActive.Tip()->nHeight <= nCycleLength*10)
                       {
                           CBlockIndex* pindex = chainActive.Tip();
@@ -1184,6 +1186,8 @@ bool SignBlock(CBlock *pblock, CWallet& wallet, int64_t nFees, std::string sLog)
 
                               pindex = pindex->pprev;
                           }
+
+                          LogPrintf("%s: from those cycles, %d had an active vote, nonce is set to %d\n", __func__, nCycles, pblock->nNonce);
                       }
                   }

@mxaddict
Copy link
Contributor

I'll try this out, thanks for the snippet

@mxaddict
Copy link
Contributor

I did do some testing with excludevote=1 set, and nonce value was correct, so I guess we should merge this PR

@mxaddict mxaddict merged commit 8f81152 into navcoin:master Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants