Skip to content

Fix LastSeenMessage and Validators count issue#1802

Merged
shargon merged 2 commits intoneo-project:master-2.xfrom
ZhangTao1596:fix-exception-validators-count
Aug 7, 2020
Merged

Fix LastSeenMessage and Validators count issue#1802
shargon merged 2 commits intoneo-project:master-2.xfrom
ZhangTao1596:fix-exception-validators-count

Conversation

@ZhangTao1596
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

int count = (int)snapshot.ValidatorsCount.Get().Votes.Select((p, i) => new
{
Count = i,
Count = i + 1,
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

ZhangTao1596 commented Jul 30, 2020

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

@superboyiii
Copy link
Copy Markdown
Member

@erikzhang Shall we merge?

@superboyiii
Copy link
Copy Markdown
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