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 LastSeenMessage and Validators count issue #1802

Merged

Conversation

ZhangTao1596
Copy link
Contributor

@ZhangTao1596 ZhangTao1596 commented Jul 29, 2020

Close #1799

Changes:

  • Use dictionary to store LastSeenMessage in order to cover scenario like this:
    • Validators replaced by another account without changing validators count
    • Both validators and validators count changed
  • Fix bug in method of counting the number of validators based on votes

How to test and verify the impact of these changes?

  • Voting to change validators count or validators to check whether it works right
  • Stop some consensus nodes to check whether failed count is right
  • Don't start validator node you want to add in consensus to check whether failed count is right
  • Switch old version to this fix version and resync blocks to check whether it works well

Where in the software does this update applies to?

  • Consensus
  • Persistence

@erikzhang @shargon How about fix like this?

@superboyiii
Copy link
Member

superboyiii commented Jul 29, 2020

Tested. PASS

Vote 4 CN to 7 CN, switch smoothly and no Index was outside the bounds of the array.

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

The code looks good but I see the following points:

  • We save the lastSeen by publicKey instead of by index, but it's needed? this requires comparing by public key, which is slower than an array, but is more accurate.
  • It does not contain a cleaning logic, if we change the CN each block will increase the memory of the node until break it.

@ZhangTao1596
Copy link
Contributor Author

  • It does not contain a cleaning logic, if we change the CN each block will increase the memory of the node until break it.

If consensus changed, LastSeenMesage will be re-new.

@@ -245,7 +245,7 @@ public IEnumerable<ECPoint> GetValidators(IEnumerable<Transaction> others)
}
int count = (int)snapshot.ValidatorsCount.Get().Votes.Select((p, i) => new
{
Count = i,
Count = i + 1,
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we use Votes.Length - 1 as index when add votes.

snapshot.ValidatorsCount.GetAndChange().Votes[account.Votes.Length - 1] += output.Value;

snapshot.ValidatorsCount.GetAndChange().Votes[account.Votes.Length - 1] -= out_prev.Value;

And as @superboyiii described in #1799. Only 6 nodes became validators after vote 7 validators.

@ZhangTao1596
Copy link
Contributor Author

ZhangTao1596 commented Jul 30, 2020

@igormcoelho @vncoelho @erikzhang @Tommo-L Please have a look?

@superboyiii
Copy link
Member

@erikzhang Shall we merge?

@superboyiii
Copy link
Member

superboyiii commented Aug 7, 2020

Retest it based on my local repository (merged this with neox-2.x and master-2.x)
Result: PASS
Test steps:

  1. Create 7 nodes private net via 2.10.3, vote for 7 address (one is taked placed by a new address).
  2. Sync the latest version and take place the old consensus.
  3. Revote for these new consensus (vote for original candidates)
  4. Ensure no error and block generated normally.

@shargon Please help us merge this.

@shargon shargon merged commit 28420f1 into neo-project:master-2.x Aug 7, 2020
@ZhangTao1596 ZhangTao1596 deleted the fix-exception-validators-count branch August 7, 2020 08:56
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.

4 participants