navcoin / navcoin-core

bitcoin-core 0.13 fork ported for NavCoin
MIT License
123 stars 92 forks source link

Fix vote for auto excluded stakers #899

Closed aguycalled closed 2 years ago

aguycalled commented 2 years ago

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 commented 2 years ago

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

aguycalled commented 2 years ago

https://build.nav.community/binaries/aguycalled%3Afix-vote-excluded/

navbuilder commented 2 years ago

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

mxaddict commented 2 years ago

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

aguycalled commented 2 years ago

yep with getblockheader

mxaddict commented 2 years ago

Thanks

On Thu, Dec 9, 2021, 19:50 alex v. @.***> wrote:

yep with getblockheader

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/navcoin/navcoin-core/pull/899#issuecomment-989780708, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIDAKKQFZ4XUN67CPPWXBTUQCJWVANCNFSM5JRP7A4A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

chasingkirkjufell commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

chasingkirkjufell commented 2 years ago

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

mxaddict commented 2 years ago

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

chasingkirkjufell commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

I'll try this out, thanks for the snippet

mxaddict commented 2 years ago

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