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 #1834 #1838

Merged
merged 4 commits into from
Aug 13, 2020
Merged

Fix #1834 #1838

merged 4 commits into from
Aug 13, 2020

Conversation

shargon
Copy link
Member

@shargon shargon commented Aug 12, 2020

We need to ensure that all the indexes can fit in LastSeenMessage, if we use a integer type, we have the half available chain.

Also, if heigh it's 0, LastSeenMessage?.Count(p => Block.Index > 0 && p.Value < (Block.Index - 1)) ?? 0; it will produce an integer overflow, and the condition always will be true.

@shargon shargon requested a review from erikzhang August 12, 2020 08:59
@erikzhang
Copy link
Member

uint is better?

@shargon
Copy link
Member Author

shargon commented Aug 12, 2020

Yes, it's better, but the consensus UT use -1, we can fix the UT and change to uint

@shargon
Copy link
Member Author

shargon commented Aug 12, 2020

The problem is that ConsensusService use Blockchain.Singleton and it has 1 Height. So it can't meet this UT

mockContext.Object.ChangeViewPayloads[mockContext.Object.MyIndex] = null;
Console.WriteLine("Forcing Failed nodes for recovery request... ");
mockContext.Object.CountFailed.Should().Be(0);
mockContext.Object.LastSeenMessage.Clear();
foreach (var validator in mockContext.Object.Validators)
{
mockContext.Object.LastSeenMessage[validator] = -1;
}
mockContext.Object.CountFailed.Should().Be(7);
Console.WriteLine("\nWaiting for recovery due to failed nodes... ");
var backupOnRecoveryDueToFailedNodes = subscriber.ExpectMsg<LocalNode.SendDirectly>();
var recoveryPayload = (ConsensusPayload)backupOnRecoveryDueToFailedNodes.Inventory;

@shargon
Copy link
Member Author

shargon commented Aug 12, 2020

I think that now it's better, because if LastSeen doesn't contain the pubKey, it will be counted as failed.

erikzhang
erikzhang previously approved these changes Aug 13, 2020
@erikzhang
Copy link
Member

Test need to be fixed.

@shargon shargon merged commit e6d68dc into neo-project:master Aug 13, 2020
@shargon shargon deleted the fix-1834 branch August 13, 2020 11:20
cloud8little pushed a commit to cloud8little/neo that referenced this pull request Jan 24, 2021
* Fix 1834

* Fix UT

* Remove useless condition

Co-authored-by: Erik Zhang <erik@neo.org>
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.

None yet

2 participants